Skip to content

fix(core): Simplify Websocket origin security checks #15761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(core): Simplify Websocket origin security checks #15761

wants to merge 2 commits into from
+66 −134

Conversation

netroy
Copy link
Member

@netroy netroy commented May 27, 2025 •
edited by despairblue
Loading

Summary

We recently updated cross-origin checks on websocket requests, but the implementation is dependent on the reverse-proxy sending a non-standard X-Forwarded-Proto header, which the proxy might not send or send an incorrect value of.
This PR simplifies those checks to only check the host instead.

Related Linear tickets, Github issues, and Community forum posts

Fixes #15706 #15744

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@netroy netroy requested a review from despairblue May 27, 2025 14:49
cubic-dev-ai[bot]
cubic-dev-ai bot reviewed May 27, 2025
View reviewed changes
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cubic reviewed 2 files and found no issues. Review PR in cubic.dev.

@codecov Codecov
Copy link

codecov bot commented May 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels May 27, 2025
despairblue
despairblue approved these changes May 27, 2025
View reviewed changes
@despairblue
Copy link
Contributor

I pushed a commit updating the tests. The names were not describing the failure reasons and a test for this was missing:

				typeof headers['x-forwarded-host'] === 'string'
					? headers['x-forwarded-host']
					: headers.host;

E.g. ignoring x-forwarded-host and always picking host would have passed all tests.

@shortstacked
Copy link
Contributor

Workflow Test Results 📊 🔴 2 Failed, ⚠️ 3 Warnings, 👍 78 Successful out of 83 total workflows.

Detail: Workflows failing: 243: Workflow contains 2 deleted data. View full workflow run

Tested Ref: 95a8e5c8b11ce0da5373dfdbae9c0d88f7005897 by @despairblue

❌ Failed Tests (2)

Workflow ID Workflow Name Reason
243 Agent:ReAct Workflow contains 2 deleted data.
258 Agent:auto-fix:openai Workflow contains 2 deleted data.

⚠️ Warnings (3)

Workflow ID Workflow Name Reason
237 BasicLLMChain:AzureChat Workflow contains new data that previously did not exist.
35 Slack:User:getPresence info:UserProfile:get update... Workflow contains new data that previously did not exist.
257 Agent:auto-fix:anthropic Workflow contains new data that previously did not exist.

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@despairblue despairblue self-requested a review May 28, 2025 08:49
@shortstacked
Copy link
Contributor

Workflow Test Results 📊 🔴 1 Failed, ⚠️ 4 Warnings, 👍 78 Successful out of 83 total workflows.

Detail: Workflows failing: 243: Workflow contains 2 deleted data. View full workflow run

Tested Ref: 95a8e5c8b11ce0da5373dfdbae9c0d88f7005897 by @despairblue

❌ Failed Tests (1)

Workflow ID Workflow Name Reason
243 Agent:ReAct Workflow contains 2 deleted data.

⚠️ Warnings (4)

Workflow ID Workflow Name Reason
237 BasicLLMChain:AzureChat Workflow contains new data that previously did not exist.
35 Slack:User:getPresence info:UserProfile:get update... Workflow contains new data that previously did not exist.
257 Agent:auto-fix:anthropic Workflow contains new data that previously did not exist.
53 ConvertKit:CustomField:create getAll update delete... Workflow contains new data that previously did not exist.

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

2 similar comments
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cubic-dev-ai cubic-dev-ai[bot] cubic-dev-ai[bot] left review comments

@despairblue despairblue despairblue approved these changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Web Socket breaks when upgrading to 1.95.0
3 participants
@netroy @despairblue @shortstacked