New issue
Advanced search Search tips

Issue 829932 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Signed Exchanges: Check that certUrl and validityUrl are absolute

Project Member Reported by jyasskin@chromium.org, Apr 6 2018

Issue description

Comment 1 by horo@chromium.org, Apr 9 2018

Status: Started (was: Assigned)
We already have the GURL.is_valid() check for certUrl and validityUrl.
But we don't have the check for request url.
https://chromium.googlesource.com/chromium/src/+/8ba3e153b566e20156093dd96fc1134ff86b9640/content/browser/web_package/signed_exchange_header.cc#84

And also there is no check of the reference fragment (#foobar) for all of them.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da1e9ae5f918c66f3f35f56b00190f5ed9773405

commit da1e9ae5f918c66f3f35f56b00190f5ed9773405
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Tue Apr 10 07:43:22 2018

Check the validity of URLs of SignedHTTPExchange

CertUrl and validityUrl and request URL must be absolute.
https://github.com/WICG/webpackage/pull/170

Currently the validity of certUrl and validityUrl is checked in ParseSignature().
But there is no validity check of the request url.
And also there is no has_ref() check of all of them.

So this CL adds these checks:
 - is_valid() and !has_ref() for request URL.
 - !has_ref() check for certUrl and validityUrl.

Bug:  829932 
Change-Id: I4502cb7cbb381e631292c8fd7a1ca998614f21b3
Reviewed-on: https://chromium-review.googlesource.com/1002338
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549446}
[modify] https://crrev.com/da1e9ae5f918c66f3f35f56b00190f5ed9773405/content/browser/web_package/signed_exchange_header.cc
[modify] https://crrev.com/da1e9ae5f918c66f3f35f56b00190f5ed9773405/content/browser/web_package/signed_exchange_header_parser.cc
[modify] https://crrev.com/da1e9ae5f918c66f3f35f56b00190f5ed9773405/content/browser/web_package/signed_exchange_header_parser_unittest.cc
[modify] https://crrev.com/da1e9ae5f918c66f3f35f56b00190f5ed9773405/content/browser/web_package/signed_exchange_header_unittest.cc

Thanks! Could you add a test that a relative URL is rejected? I don't see documentation that GURL::is_valid() returns false for relative URLs, which was the original question in https://github.com/WICG/webpackage/issues/147.

Comment 4 by horo@chromium.org, Apr 11 2018

Sure. Created http://crrev.com/c/1004836
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd9526165a87f22c2ef6bc26d1b671684e7315a2

commit bd9526165a87f22c2ef6bc26d1b671684e7315a2
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Wed Apr 11 04:54:15 2018

Add tests to check that relative URL is rejected in SignedHTTPExchange

Bug:  829932 
Change-Id: Ic437dd2cc43ccbfbf83a100d1c6a383261cddc6b
Reviewed-on: https://chromium-review.googlesource.com/1004836
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549766}
[modify] https://crrev.com/bd9526165a87f22c2ef6bc26d1b671684e7315a2/content/browser/web_package/signed_exchange_header_parser_unittest.cc
[modify] https://crrev.com/bd9526165a87f22c2ef6bc26d1b671684e7315a2/content/browser/web_package/signed_exchange_header_unittest.cc

Comment 6 by horo@chromium.org, Apr 11 2018

Status: Fixed (was: Started)

Sign in to add a comment