Skip to content
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.

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

Check origin header for websocket connection #1626

Merged
merged 3 commits into from
Mar 22, 2019

Conversation

timfjord
Copy link

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

No

Motivation / Use-Case

This is a port of #1603 fix but for v2 branch.
Basically, it addresses CVE vulnerability https://www.npmjs.com/advisories/725

I know that v2 is deprecated but it is not an easy task to upgrade this lib to v3 in rails apps, because some dependencies are still in release candidate state or even in beta.

Breaking Changes

Websocket is now checked for origin

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Jan 14, 2019

CLA assistant check
All committers have signed the CLA.

@hiroppy
Copy link
Member

hiroppy commented Feb 14, 2019

@Timsly please sign 👆

@alexander-akait
Copy link
Member

Tests failed, need fix

@timfjord
Copy link
Author

@hiroppy Done, signed
@evilebottnawi here is what I see in the build
14 48 47

@alexander-akait
Copy link
Member

Yep, expected, we can merge this and release, don't have access for computer right now

@timfjord
Copy link
Author

That would be great

@hiroppy
Copy link
Member

hiroppy commented Mar 5, 2019

rebuild https://ci.appveyor.com/project/sokra/webpack-dev-server

@Timsly Could you rebase from master?

@timfjord
Copy link
Author

timfjord commented Mar 6, 2019

@hiroppy I cannot rebase from master because this PR is exclusively for v2 and master contains v3

@alexander-akait
Copy link
Member

/cc @Timsly due we don't have tests for v2 so we should test this manually, do you check you solution is work as expected? Because we merge 8bb3ca8 and 1dfd4fb after original PR to avoid some problems

@timfjord
Copy link
Author

timfjord commented Mar 6, 2019

Hm, in this case, I will cherry-pick them here

@timfjord
Copy link
Author

timfjord commented Mar 13, 2019

@evilebottnawi I've ported both fixes, see f6c6af6 and ff8b19c
Could you please run the build to make sure it works as expected?

@alexander-akait
Copy link
Member

@Timsly we don't have tests for v2 so i can't, can you test this manually? Maybe ping something else for testing too

@timfjord
Copy link
Author

@evilebottnawi I've tested current version and it works fine.

@alexander-akait alexander-akait merged commit c42d0da into webpack:v2 Mar 22, 2019
@timfjord timfjord deleted the CVE-2018-14732-v2 branch March 22, 2019 14:15
@timfjord
Copy link
Author

@evilebottnawi is there a chance that this will be released as 2.11.2?

@alexander-akait
Copy link
Member

@Timsly today

@timfjord
Copy link
Author

Thanks

// if hostHeader doesn't have scheme, add // for parsing.
/^(.+:)?\/\//.test(hostHeader) ? hostHeader : `//${hostHeader}`,
false,
true,

Choose a reason for hiding this comment

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

@Timsly, would you mind to delete this comma with another PR? It causes issues for node.js <= 7.x
See issue #1607 and the PR that fixed it #1609

Copy link
Author

Choose a reason for hiding this comment

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

@ifnoword Done - #1736

Choose a reason for hiding this comment

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

@Timsly I appreciate it. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants