Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541843002:
Part 5.1: Is policy list subsumed under subsuming policy? (Closed)
DescriptionPart 5.1: Is policy list subsumed under subsuming policy?
In this CL we start considering subsumtpion on MediaListDirective level.
The logic is straightforward: we required exact match for the specified
plugin types.
Two steps:
- Normalize: find effective list of allowed plugin types
- Check for subsumption.
BUG=647588
Committed: https://crrev.com/7abaa81c2d0044a70d45fbadf0f00729a5647f5a
Cr-Commit-Position: refs/heads/master@{#437066}
Patch Set 1 #
Total comments: 7
Patch Set 2 : More test cases #
Total comments: 1
Patch Set 3 : Adding a comment #Patch Set 4 : Fixing c++ vector initialization in the test #
Messages
Total messages: 29 (20 generated)
amalika@google.com changed reviewers: + jochen@chromium.org, mkwst@chromium.org
Based on specifications and examples I found, it seems that: - Subtypes have to always be specified, i.e. "application/" would not subsumes "application/x-valid-type" because the former is not a valid media-plugin-list. - If the list m_pluginTypes is empty, then the effective policy does not allow any plugins to be loaded.
LGTM % minor extensions to tests. Thanks! https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/MediaListDirective.cpp (right): https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/MediaListDirective.cpp:91: for (uint32_t i = 1; i < other.size(); i++) s/uint32_t/size_t/ https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (right): https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:23: "application/x-shockwave-flash application/pdf text/plain", csp.get()); Can you check that we have a sane behavior when |A| is empty? https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:29: {"text/plain", {"text/plain"}}, Can you add an empty list? https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:32: {"application/x-shockwave-flash text/plain", Nit: Can you add tests that reverse the order, just to make sure? https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:36: {"application/x-shockwave-flash", "application/pdf", "text/plain"}}, Can you add some things like `text/`, `text/*`, and `*/plain`? https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:53: "application/x-shockwave-flash application/pdf text/plain", csp.get()); Same general comments. Empty lists, wildcards, ordering.
https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (right): https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:36: {"application/x-shockwave-flash", "application/pdf", "text/plain"}}, On 2016/12/06 at 13:39:07, Mike West (slow) wrote: > Can you add some things like `text/`, `text/*`, and `*/plain`? Just to make sure: None of these will be valid, right? From my understanding, `media-type = type "/" subtype` which does not allow any wildcards and requires `subtype`. (if for example, I include `text/` it does not even get stored as a value on MediaListDirective).
On 2016/12/06 at 13:54:19, amalika wrote: > https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (right): > > https://codereview.chromium.org/2541843002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:36: {"application/x-shockwave-flash", "application/pdf", "text/plain"}}, > On 2016/12/06 at 13:39:07, Mike West (slow) wrote: > > Can you add some things like `text/`, `text/*`, and `*/plain`? > > Just to make sure: None of these will be valid, right? > From my understanding, `media-type = type "/" subtype` which does not allow any wildcards and requires `subtype`. > (if for example, I include `text/` it does not even get stored as a value on MediaListDirective). I think we talked about this just before I left today, but you're correct: I don't expect these to pass. Just add tests that show they don't influence the result in any meaningful way. :)
The CQ bit was checked by amalika@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...
The CQ bit was unchecked by amalika@google.com
Added more test cases https://codereview.chromium.org/2541843002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (right): https://codereview.chromium.org/2541843002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:83: {{"application/*"}, false, false}, Parsing does not actually validate the `subtype` so anything after a `type`, including a wildcard, gets stored in the internal variable m_pluginTypes, even if later it won't be a valid MIME type. Only if subtype is missing, the value is not saved such as "text/". So "application/*" won't be subsumed by `A` since it is not allowed in `A`, but "text/*" (case just above) will be subsumed.
The CQ bit was checked by amalika@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/12/07 at 10:13:08, amalika wrote: > Added more test cases > > https://codereview.chromium.org/2541843002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp (right): > > https://codereview.chromium.org/2541843002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/csp/MediaListDirectiveTest.cpp:83: {{"application/*"}, false, false}, > Parsing does not actually validate the `subtype` so anything after a `type`, including a wildcard, gets stored in the internal variable m_pluginTypes, even if later it won't be a valid MIME type. Only if subtype is missing, the value is not saved such as "text/". Hrm. That's a bug: `*` doesn't match the grammar at: https://tools.ietf.org/html/rfc2045#section-5.1. Would you mind putting a TODO somewhere reasonable? > So "application/*" won't be subsumed by `A` since it is not allowed in `A`, but "text/*" (case just above) will be subsumed. That's pretty strange. But justifiable, I suppose: the two policies will have identically meaningless impact on the page. I'm ok landing this as-is, but we should fix the parser in a future CL. Would you be interested in making that change?
The CQ bit was checked by amalika@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by amalika@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...
The CQ bit was unchecked by amalika@google.com
The CQ bit was checked by amalika@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/2541843002/#ps80001 (title: "Fixing c++ vector initialization in the test")
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": 1481137583682520,
"parent_rev": "9523075ad46ecb6f42c7a51a39cbe3434cc5db2f", "commit_rev":
"41cd555f7439e11f04e66db34f9a2e63cf0cc3e1"}
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Part 5.1: Is policy list subsumed under subsuming policy? In this CL we start considering subsumtpion on MediaListDirective level. The logic is straightforward: we required exact match for the specified plugin types. Two steps: - Normalize: find effective list of allowed plugin types - Check for subsumption. BUG=647588 ========== to ========== Part 5.1: Is policy list subsumed under subsuming policy? In this CL we start considering subsumtpion on MediaListDirective level. The logic is straightforward: we required exact match for the specified plugin types. Two steps: - Normalize: find effective list of allowed plugin types - Check for subsumption. BUG=647588 Committed: https://crrev.com/7abaa81c2d0044a70d45fbadf0f00729a5647f5a Cr-Commit-Position: refs/heads/master@{#437066} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7abaa81c2d0044a70d45fbadf0f00729a5647f5a Cr-Commit-Position: refs/heads/master@{#437066} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
