Strictly UTF-8 decode the fallback URL #346

jyasskin · 2018-11-30T23:19:28Z

Bikeshed warned that I'd never defined |fallbackUrl|, but I missed it
before going on parental leave. Do y'all see any problems with picking this conversion from bytes to a string?

@irori, we ought to test this with a couple bad fallback URLs, say one starting with a BOM, and one with an invalid sequence that the UTF-8 decoder might convert to U+FFFD. Would you be willing to add those tests?

Preview | Diff

Bikeshed warned that I'd never defined |fallbackUrl|, but I missed it before going on parental leave.

irori

Sure, I will add the tests.

jyasskin · 2018-12-04T03:12:49Z

I should add that I found https://cs.chromium.org/chromium/src/net/http/http_response_headers.cc?l=920&rcl=6aa20f013414d789e8d72752a6885ef3d990d274 after I wrote this change, indicating that maybe we'd rather Location headers be all-ASCII. We can enforce that for SXG URLs if y'all think it's a good idea.

This is also related to whatwg/fetch#843.

annevk · 2018-12-04T10:37:48Z

UTF-8 is generally preferable I think, but it depends a bit on what kind of field this is. Happy to help if you could give some context.

jyasskin · 2018-12-04T15:00:06Z

@annevk This is the field at the beginning of the Signed Exchange format that gives both the URL of the inner resource and a target to redirect to if the Signed Exchange is broken (doesn't parse, the signature doesn't validate, maybe other things).

annevk · 2018-12-05T09:03:43Z

I'd make those UTF-8. The URL parser will turn it all into ASCII, but it seems preferable to be able to serialize in more ways.

jyasskin · 2018-12-05T15:38:41Z

Thanks, @annevk!

GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222

GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#617772}

irori · 2018-12-20T08:44:07Z

For the record, tests for this are web-platform-tests/wpt@2e19cbe and web-platform-tests/wpt@e663fa0.

…RL has invalid UTF-8 sequence, a=testonly Automatic update from web-platform-tests SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#617772} -- wpt-commits: e663fa084dcdac43fea5cdd6c69b059b5ad0743f wpt-pr: 14599

…RL has invalid UTF-8 sequence, a=testonly Automatic update from web-platform-tests SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamotochromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#617772} -- wpt-commits: e663fa084dcdac43fea5cdd6c69b059b5ad0743f wpt-pr: 14599 UltraBlame original commit: 812e25f081670195d2e97402e27d1fa54a8860e7

…RL has invalid UTF-8 sequence, a=testonly Automatic update from web-platform-tests SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamotochromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#617772} -- wpt-commits: e663fa084dcdac43fea5cdd6c69b059b5ad0743f wpt-pr: 14599 UltraBlame original commit: c680ff16a9c7a3ff68ac3372ddf0c50263e55a24

…RL has invalid UTF-8 sequence, a=testonly Automatic update from web-platform-tests SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamotochromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#617772} -- wpt-commits: e663fa084dcdac43fea5cdd6c69b059b5ad0743f wpt-pr: 14599 UltraBlame original commit: 812e25f081670195d2e97402e27d1fa54a8860e7

…RL has invalid UTF-8 sequence, a=testonly Automatic update from web-platform-tests SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamotochromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#617772} -- wpt-commits: e663fa084dcdac43fea5cdd6c69b059b5ad0743f wpt-pr: 14599 UltraBlame original commit: c680ff16a9c7a3ff68ac3372ddf0c50263e55a24

…RL has invalid UTF-8 sequence, a=testonly Automatic update from web-platform-tests SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamotochromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#617772} -- wpt-commits: e663fa084dcdac43fea5cdd6c69b059b5ad0743f wpt-pr: 14599 UltraBlame original commit: 812e25f081670195d2e97402e27d1fa54a8860e7

…RL has invalid UTF-8 sequence, a=testonly Automatic update from web-platform-tests SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence GURL parser happily accepts invalid UTF-8 path, so we need to validate the fallback URL string before parsing. Spec: WICG/webpackage#346 Bug: 916390 Change-Id: Ife25621e2a41beef01cbf36a5ab523eaee1ea222 Reviewed-on: https://chromium-review.googlesource.com/c/1382724 Commit-Queue: Kunihiko Sakamoto <ksakamotochromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#617772} -- wpt-commits: e663fa084dcdac43fea5cdd6c69b059b5ad0743f wpt-pr: 14599 UltraBlame original commit: c680ff16a9c7a3ff68ac3372ddf0c50263e55a24

jyasskin requested review from nyaxt and irori November 30, 2018 23:19

Strictly UTF-8 decode the fallback URL

121ecb1

Bikeshed warned that I'd never defined |fallbackUrl|, but I missed it before going on parental leave.

jyasskin force-pushed the utf-8-decode-fallback-url branch from 876b3c9 to 121ecb1 Compare December 1, 2018 00:31

nyaxt approved these changes Dec 3, 2018

View reviewed changes

irori approved these changes Dec 4, 2018

View reviewed changes

jyasskin merged commit f603e91 into WICG:master Dec 5, 2018

jyasskin deleted the utf-8-decode-fallback-url branch December 5, 2018 15:38

nyaxt mentioned this pull request Dec 13, 2018

Implementation snapshot b3 #350

Closed

10 tasks

chromium-wpt-export-bot mentioned this pull request Dec 19, 2018

SignedExchange: Reject SXG if fallback URL has invalid UTF-8 sequence web-platform-tests/wpt#14599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strictly UTF-8 decode the fallback URL #346

Strictly UTF-8 decode the fallback URL #346

jyasskin commented Nov 30, 2018 •

edited by pr-preview bot

Loading

irori left a comment

jyasskin commented Dec 4, 2018

annevk commented Dec 4, 2018

jyasskin commented Dec 4, 2018

annevk commented Dec 5, 2018

jyasskin commented Dec 5, 2018

irori commented Dec 20, 2018

Strictly UTF-8 decode the fallback URL #346

Strictly UTF-8 decode the fallback URL #346

Conversation

jyasskin commented Nov 30, 2018 • edited by pr-preview bot Loading

irori left a comment

Choose a reason for hiding this comment

jyasskin commented Dec 4, 2018

annevk commented Dec 4, 2018

jyasskin commented Dec 4, 2018

annevk commented Dec 5, 2018

jyasskin commented Dec 5, 2018

irori commented Dec 20, 2018

jyasskin commented Nov 30, 2018 •

edited by pr-preview bot

Loading