Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413193010:
Add counters for various ways of loading scripts with bad mimetypes (Closed)
|
Created:
5 years, 1 month ago by Dan Ehrenberg Modified:
5 years ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd counters for various ways of loading scripts with bad mimetypes
Unchecked use of proxies could enable cross-origin reading of some CSV
files which contain sensitive non-numerical information. Tightening
down mimetype checking could provide a mitigating strategy. This patch
adds UseCounters to see how often scripts are used with different
mimetypes to determine what could be prohibited.
BUG=chromium:399951
R=adamk
Committed: https://crrev.com/6f9d55e0e902b20bcb8a38be6721f498a2a973ab
Cr-Commit-Position: refs/heads/master@{#364013}
Patch Set 1 #Patch Set 2 : Fix compilation #Patch Set 3 : Test which doesn't work #Patch Set 4 : Tests which do work #
Total comments: 2
Patch Set 5 : Small changes for review; adding application counter #Patch Set 6 : Fix code and tests, passing again #
Total comments: 4
Patch Set 7 : Changes from review #Patch Set 8 : renumber #Patch Set 9 : Rebase #Patch Set 10 : #Patch Set 11 : rebase #
Messages
Total messages: 48 (23 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
littledan@chromium.org changed reviewers: + japhet@chromium.org
PTAL
Seems fine to me, but Nate is the right reviewer for this.
Why is the test in http/tests/security/? Is this security related? https://codereview.chromium.org/1413193010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/1413193010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:374: bool text = mimetype.lower().startsWith("text/"); Maybe pull this new logging into a helper?
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Moved the tests. PTAL. https://codereview.chromium.org/1413193010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/1413193010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:374: bool text = mimetype.lower().startsWith("text/"); On 2015/11/10 at 18:02:49, Nate Chapin wrote: > Maybe pull this new logging into a helper? Done
lgtm, just some formatting nitpicks. https://codereview.chromium.org/1413193010/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/1413193010/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:342: if (!expectedJs) { Early exit is probably more readable than the remainder of the function wrapped in an if(). https://codereview.chromium.org/1413193010/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:343: UseCounter::Feature feature = sameOrigin ? (text ? UseCounter::SameOriginTextScript : application ? UseCounter::SameOriginApplicationScript : UseCounter::SameOriginOtherScript) : (text ? UseCounter::CrossOriginTextScript : application ? UseCounter::CrossOriginApplicationScript : UseCounter::CrossOriginOtherScript); This is a bit much for 1 line, even by WebKit-style standards :)
https://codereview.chromium.org/1413193010/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/1413193010/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:342: if (!expectedJs) { On 2015/12/03 at 17:53:32, Nate Chapin wrote: > Early exit is probably more readable than the remainder of the function wrapped in an if(). Done https://codereview.chromium.org/1413193010/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:343: UseCounter::Feature feature = sameOrigin ? (text ? UseCounter::SameOriginTextScript : application ? UseCounter::SameOriginApplicationScript : UseCounter::SameOriginOtherScript) : (text ? UseCounter::CrossOriginTextScript : application ? UseCounter::CrossOriginApplicationScript : UseCounter::CrossOriginOtherScript); On 2015/12/03 at 17:53:32, Nate Chapin wrote: > This is a bit much for 1 line, even by WebKit-style standards :) Well, I'd love to, but this is what git cl format seems to force me to do.
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1413193010/#ps120001 (title: "Changes from review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1413193010/#ps140001 (title: "renumber")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1413193010/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1413193010/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413193010/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413193010/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add counters for various ways of loading scripts with bad mimetypes Unchecked use of proxies could enable cross-origin reading of some CSV files which contain sensitive non-numerical information. Tightening down mimetype checking could provide a mitigating strategy. This patch adds UseCounters to see how often scripts are used with different mimetypes to determine what could be prohibited. BUG=chromium:399951 R=adamk ========== to ========== Add counters for various ways of loading scripts with bad mimetypes Unchecked use of proxies could enable cross-origin reading of some CSV files which contain sensitive non-numerical information. Tightening down mimetype checking could provide a mitigating strategy. This patch adds UseCounters to see how often scripts are used with different mimetypes to determine what could be prohibited. BUG=chromium:399951 R=adamk Committed: https://crrev.com/6f9d55e0e902b20bcb8a38be6721f498a2a973ab Cr-Commit-Position: refs/heads/master@{#364013} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6f9d55e0e902b20bcb8a38be6721f498a2a973ab Cr-Commit-Position: refs/heads/master@{#364013} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
