Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690933003:
Don't sanitize errors if the protocol is "data:" (Closed)
|
Created:
3 years, 10 months ago by andypaicu2 Modified:
3 years, 10 months ago Reviewers:
Mike West CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't sanitize errors if the protocol is "data:"
Added a special check for 'data:' URLs to not sanitize error messages.
BUG=691518, 508734
Review-Url: https://codereview.chromium.org/2690933003
Cr-Commit-Position: refs/heads/master@{#450342}
Committed: https://chromium.googlesource.com/chromium/src/+/5f11cf4c938565ded9c4268af70e7e578169fa3e
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved test to the external folder #Patch Set 3 : Updated incorrect test result expectations for data URLs #
Total comments: 1
Patch Set 4 : Correct expected results for external/wpt/html/webappapis/scripting/processing-model-2/runtime-erro… #Patch Set 5 : Removed the runtime-error-data-url-expected.txt file since both tests in it are now passing #
Messages
Total messages: 30 (19 generated)
Description was changed from ========== Don't sanitize errors if the protocols is "data:" BUG=691518 ========== to ========== Don't sanitize errors if the protocol is "data:" BUG=691518 ==========
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
LGTM % the test. Thanks! https://codereview.chromium.org/2690933003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/security/script-dataurl-not-sanitized.html (right): https://codereview.chromium.org/2690933003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/security/script-dataurl-not-sanitized.html:1: <!DOCTYPE html> It looks like there's an existing test in https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/.... Why don't we update that test a bit to test more of the error event (and to change the misleading "same-origin" comment) and unskip it in TestExpectations rather than adding a new test?
The CQ bit was checked by andypaicu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't sanitize errors if the protocol is "data:" BUG=691518 ========== to ========== Don't sanitize errors if the protocol is "data:" Added an special check for 'data:' URLs to not sanitize error messages. BUG=691518 ==========
Description was changed from ========== Don't sanitize errors if the protocol is "data:" Added an special check for 'data:' URLs to not sanitize error messages. BUG=691518 ========== to ========== Don't sanitize errors if the protocol is "data:" Added an special check for 'data:' URLs to not sanitize error messages. BUG=691518,508734 ==========
Patch that moved test to external https://codereview.chromium.org/2690933003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/security/script-dataurl-not-sanitized.html (right): https://codereview.chromium.org/2690933003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/security/script-dataurl-not-sanitized.html:1: <!DOCTYPE html> On 2017/02/14 at 07:19:07, Mike West (sloooooow) wrote: > It looks like there's an existing test in https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/.... Why don't we update that test a bit to test more of the error event (and to change the misleading "same-origin" comment) and unskip it in TestExpectations rather than adding a new test? Done
The CQ bit was checked by andypaicu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't sanitize errors if the protocol is "data:" Added an special check for 'data:' URLs to not sanitize error messages. BUG=691518,508734 ========== to ========== Don't sanitize errors if the protocol is "data:" Added a special check for 'data:' URLs to not sanitize error messages. BUG=691518,508734 ==========
The CQ bit was unchecked by andypaicu@google.com
The CQ bit was checked by andypaicu@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2690933003/#ps20001 (title: "Moved test to the external folder")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for moving to the WPT test! Changes still LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by andypaicu@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2690933003/#ps40001 (title: "Updated incorrect test result expectations for data URLs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2690933003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/processing-model-2/runtime-error-data-url-expected.txt (right): https://codereview.chromium.org/2690933003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/processing-model-2/runtime-error-data-url-expected.txt:3: PASS window.onerror - runtime error in <script src=data:...> If we're now passing this test (and it looks like we are), then please just delete the `-expected.txt` file.
On 2017/02/14 at 12:17:13, mkwst wrote: > https://codereview.chromium.org/2690933003/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/processing-model-2/runtime-error-data-url-expected.txt (right): > > https://codereview.chromium.org/2690933003/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/processing-model-2/runtime-error-data-url-expected.txt:3: PASS window.onerror - runtime error in <script src=data:...> > If we're now passing this test (and it looks like we are), then please just delete the `-expected.txt` file. Oh is this a similar mechanism to TestExpectations for tests that we are not passing yet?
On 2017/02/14 at 12:25:56, andypaicu wrote: > On 2017/02/14 at 12:17:13, mkwst wrote: > > https://codereview.chromium.org/2690933003/diff/40001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/processing-model-2/runtime-error-data-url-expected.txt (right): > > > > https://codereview.chromium.org/2690933003/diff/40001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/external/wpt/html/webappapis/scripting/processing-model-2/runtime-error-data-url-expected.txt:3: PASS window.onerror - runtime error in <script src=data:...> > > If we're now passing this test (and it looks like we are), then please just delete the `-expected.txt` file. > > Oh is this a similar mechanism to TestExpectations for tests that we are not passing yet? Since the repository from which these tests are drawn is shared between browsers, it's totally possible for a test to pass in Edge and match the spec, but for us to need to do some work to catch up. We check in `-expected.txt` files to capture expected failures, and to make sure we don't regress in new and exciting ways. Once we pass the test, there's no need for the `-expected.txt` file, so we can throw it away. :)
The CQ bit was checked by andypaicu@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2690933003/#ps80001 (title: "Removed the runtime-error-data-url-expected.txt file since both tests in it are now passing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487075471832670,
"parent_rev": "edebc1fbd29714dce0151bfa57a08b49eea5293d", "commit_rev":
"5f11cf4c938565ded9c4268af70e7e578169fa3e"}
Message was sent while issue was closed.
Description was changed from ========== Don't sanitize errors if the protocol is "data:" Added a special check for 'data:' URLs to not sanitize error messages. BUG=691518,508734 ========== to ========== Don't sanitize errors if the protocol is "data:" Added a special check for 'data:' URLs to not sanitize error messages. BUG=691518,508734 Review-Url: https://codereview.chromium.org/2690933003 Cr-Commit-Position: refs/heads/master@{#450342} Committed: https://chromium.googlesource.com/chromium/src/+/5f11cf4c938565ded9c4268af70e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/5f11cf4c938565ded9c4268af70e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
