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

URL: stripping of tab and newlines in setters #23265

Merged
merged 4 commits into from
Apr 28, 2020

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 27, 2020

@domenic
Copy link
Member

domenic commented Apr 27, 2020

This does not match jsdom/whatwg-url in several ways. I'll collate them here, then we can dig into whether they're implementation issues or test issues...

Setting hash with leading U+0000

    Expected value to strictly be equal to:
      "#%00test"
    Received:
      "#test"
Setting username with leading U+0009

    Expected value to strictly be equal to:
      "test"
    Received:
      "%09test"

and then the last one again for U+000A, U+000D, and all three cases again for password

@domenic
Copy link
Member

domenic commented Apr 27, 2020

Setting hash with leading U+0000

This test is incorrect. https://url.spec.whatwg.org/#fragment-state does a validation error, and then does nothing, for U+0000

@domenic
Copy link
Member

domenic commented Apr 27, 2020

Setting username with leading U+0009

These also appear to be incorrect. E.g. https://url.spec.whatwg.org/#dom-url-username goes to https://url.spec.whatwg.org/#set-the-username which percent-encodes using https://url.spec.whatwg.org/#userinfo-percent-encode-set which includes U+0009, U+000A, U+000D.

@domenic
Copy link
Member

domenic commented Apr 27, 2020

Setting hash with leading U+0000

This test is incorrect. https://url.spec.whatwg.org/#fragment-state does a validation error, and then does nothing, for U+0000

Well, OK, I think this is correct now that the latest spec changes have landed :).

@annevk
Copy link
Member Author

annevk commented Apr 28, 2020

I could have sworn I made an intermediate commit for trailing stuff before adding non-special scheme support. Oh well.

@domenic want to verify again? This now is about as exhaustive as I imagined it. Trailing caught new failures, non-special scheme only caught failures in implementations that don't have good support for those anyway.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23265 April 28, 2020 07:31 Inactive
@domenic
Copy link
Member

domenic commented Apr 28, 2020

I'm now getting a lot of failures in the "Setting protocol with trailing X" tests. The setters change the result in whatwg-url but the test expects them to stay unchanged.

I don't see any reason why the protocol would stay unchanged. Note that step 3 of basic URL parse removes all ASCII tab and newline from the input, not just trailing/leading ones.

Also, if you wouldn't mind rebasing this on master, that would help me testing a bit.

@annevk annevk force-pushed the annevk/url-setters-stripping branch from c972dc3 to c0d8a84 Compare April 28, 2020 16:06
@annevk
Copy link
Member Author

annevk commented Apr 28, 2020

Thanks for highlighting that! I should also add a "middle" variant then.

@domenic
Copy link
Member

domenic commented Apr 28, 2020

The latest commit (c0d8a84) passes entirely with whatwg-url. Happy to test again if you add a middle variant.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23265 April 28, 2020 16:24 Inactive
@annevk
Copy link
Member Author

annevk commented Apr 28, 2020

The middle variant found port setter bugs in Safari, provided I did it correctly.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Still passes

@annevk annevk merged commit 04aec1f into master Apr 28, 2020
@annevk annevk deleted the annevk/url-setters-stripping branch April 28, 2020 17:17
watilde added a commit to watilde/node that referenced this pull request Sep 4, 2020
Following the specification that allows using `\x00` in the hash.

Related: web-platform-tests/wpt#23265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants