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

fix(client): don't override protocol for socket connection to 127.0.0.1 #2303

Merged
merged 2 commits into from Nov 29, 2019

Conversation

nougad
Copy link
Contributor

@nougad nougad commented Nov 5, 2019

Chrome and Firefox accept http:///ws:// mixed-content connection to
127.0.0.1 even when the actual website is loaded via https://.

Fixes #2302

  • 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?

I'm not sure if the current tests actually contain correct assertions.

Motivation / Use-Case

described in #2302

Breaking Changes

nope

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Nov 5, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@hiroppy hiroppy left a comment

Need to add tests.

@codecov
Copy link

codecov bot commented Nov 6, 2019

Codecov Report

Merging #2303 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2303      +/-   ##
==========================================
+ Coverage   93.83%   93.84%   +<.01%     
==========================================
  Files          34       34              
  Lines        1282     1283       +1     
  Branches      365      366       +1     
==========================================
+ Hits         1203     1204       +1     
  Misses         78       78              
  Partials        1        1
Impacted Files Coverage Δ
client-src/default/utils/createSocketUrl.js 96.96% <100%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f501e83...6cc5853. Read the comment docs.

@nougad
Copy link
Contributor Author

nougad commented Nov 6, 2019

Thank you @hiroppy for your fast comment. Looking at the current tests it seams like the serialization of the URL object generated wrong snapshots:

https://github.com/webpack/webpack-dev-server/blob/master/test/client/utils/__snapshots__/createSocketUrl.test.js.snap#L13

That makes adding new tests hard since the assertions are arbitrary. Is my assumption correct? Is there a ticket for replacing the snapshot with actual tests?

@hiroppy
Copy link
Member

hiroppy commented Nov 6, 2019

I think this line is incorrect.

urlParts = url.parse(resourceQuery.substr(1));

Is there a ticket for replacing the snapshot with actual tests?

Currently no. I'll check this. thanks!

@nougad
Copy link
Contributor Author

nougad commented Nov 6, 2019

Awesome! let me know when you found the bug. I'm happy to add tests for this PR 😄

@odbayar
Copy link

odbayar commented Nov 9, 2019

Will it work for "localhost" too?

@nougad
Copy link
Contributor Author

nougad commented Nov 10, 2019

@odbayar AFAIK it only works with 127.0.0.1.

@hiroppy That line is actually correct. The method expects only a query like ?http://0.0.0.0&asdf=32 as parameter but the tests provide a full URL. If I remove the substr other tests fail.

I updated the tests to pass only the querystring as parameter but that lines are fishy as well:

if (scriptHost) {
// eslint-disable-next-line no-useless-escape
scriptHost = scriptHost.replace(/\/[^\/]+$/, '');
}

which leads to wrong tests like that one:

https://github.com/webpack/webpack-dev-server/blob/master/test/client/utils/__snapshots__/createSocketUrl.test.js.snap#L23-L25

I removed the block since it looks unused and I could not make any sense out of it.

Lastly I cleaned some unrelated unit tests to use the correct assertions and refactored the createSocketUrl file to make it easier testable. I made everything a separate commit. Let me know if you want separate PRs instead.

@nougad
Copy link
Contributor Author

nougad commented Nov 14, 2019

@hiroppy could you please have a second look if the improvements for unit testing make sense? Thanks

@nougad nougad requested a review from hiroppy Nov 20, 2019
Copy link
Member

@alexander-akait alexander-akait left a comment

Looks like big breaking change, why a lot of lines was rewritten? Please avoid unnecessary changes in this PR

@nougad
Copy link
Contributor Author

nougad commented Nov 21, 2019

@evilebottnawi I understand this change was growing a bit more than expected over time. Please have a look at the individual commits:

  • 75e620a - thats the actual change. But as @hiroppy pointed out above, the unit tests for that file are incorrect
  • 0df1f0c fixes then the unit tests for the file
  • d1a86d7 is a general improvement of unit test assertions and I'm happy to discard/move into a different PR
  • 8b0ef4a is the actual big diff because it introduces some refactoring into the file to make it easier to test. Again, I'm happy of not adding those additional tests/refactoring or move it into a second PR to unblock the actual change.

Please let me know which change you want in a separate PR or if it's ok to review it as part of this since everything is kind of related.

@nougad nougad requested a review from alexander-akait Nov 21, 2019
@alexander-akait
Copy link
Member

alexander-akait commented Nov 21, 2019

@nougad let's do refactor and fix tests in other PRs for better git history and good changelog and then we will return to this PR

@nougad
Copy link
Contributor Author

nougad commented Nov 21, 2019

I created two separate PRs with fixing tests and improving assertions:

I will update this PR here one the test fix is merged since it's a prerequisite to have working unit tests.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 25, 2019

/cc @nougad need rebase 👍

Chrome and Firefox accept `http://`/`ws://` mixed-content connection to
`127.0.0.1` even when the actual website is loaded via `https://`.

Fixes webpack#2302
@nougad
Copy link
Contributor Author

nougad commented Nov 25, 2019

@evilebottnawi rebased and updated to only include the one relevant commit.

Once merged I will create another PR with the refactoring on top.

Copy link
Member

@alexander-akait alexander-akait left a comment

@nougad
Copy link
Contributor Author

nougad commented Nov 28, 2019

Is there anything left for this PR? I promise even more extensive testing with the next PR once this one is merged :) nougad@acd46c8

@hiroppy hiroppy merged commit 3a31917 into webpack:master Nov 29, 2019
17 checks passed
@hiroppy
Copy link
Member

hiroppy commented Nov 29, 2019

thanks

@nougad nougad deleted the fix-1270001-protocol branch Nov 29, 2019
@BinarySpike
Copy link

BinarySpike commented Aug 17, 2021

Could this be expanded to localhost as well?

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.

6 participants