Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1645313002:
Don't apply the SandboxPlugins flag until we know a plugin will be used (Closed)
DescriptionDon't apply the SandboxPlugins flag until we know a plugin will be used
Move the check of the SandboxPlugins flag out of the pluginIsLoadable
function and to just before the actual load/instantiation of the plugin
is initiated. This means the URL and MIME-type is still subject to
SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will
not block resources if they will not use a plugin.
Split pluginIsLoadable into one part that checks if the URL/MIME-type is
allowed, and one part that checks if the plugin itself can be
loaded/instantiated. The former is allowedToLoadObject while the latter
is allowedToLoadPlugin. Only call the latter if we determine that a
plugin will be used to view the content (URL or not). Sink the
allowedToLoadPlugin check into loadPlugin, which in turn means it will
apply to the code-path through createPluginWithoutLayoutObject() as well,
while adding a call to allowedToLoadObject there as well.
Also make sure that shouldUsePlugin() sets the |useFallback| out-
variable before all returns. (Could previously be used uninitialized.
Found by code inspection.)
BUG=578916
Committed: https://crrev.com/4acd739260230f59c08858a31e795c2dcfdc54d4
Cr-Commit-Position: refs/heads/master@{#377559}
Patch Set 1 #
Total comments: 6
Patch Set 2 : pluginIsLoadable returns #
Total comments: 6
Patch Set 3 : Refactor, update names and add basic positive test #Patch Set 4 : ASSERTs #Patch Set 5 : Allow empty URLs to be rejected #Patch Set 6 : Hoisted that compiles #Patch Set 7 : (╯°□°）╯︵ ┻━┻ #Patch Set 8 : Side-effecting assert == bad #
Total comments: 4
Patch Set 9 : Testharness-based test #
Messages
Total messages: 29 (11 generated)
pdr@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) It seems odd to me that sandboxing and the plugin csp checks below result in different behavior. Is that intended? https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:488: return loadPlugin(completedURL, mimeType, paramNames, paramValues, useFallback, true); There is a scary side-codepath (requestPluginCreationWithoutLayoutObjectIfPossible). Can this code be structured so loadPlugin can ASSERT_WITH_SECURITY_IMPLICATION(objectIsLoadable && shouldUsePlugin)?
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) On 2016/01/29 at 22:14:54, pdr wrote: > It seems odd to me that sandboxing and the plugin csp checks below result in different behavior. Is that intended? It's intended (for now at least) - as in better to tread with caution. AIUI, this means you can still block an image/svg+xml object via a CSP directive. Arguably it should move to within shouldUsePlugin though. https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:488: return loadPlugin(completedURL, mimeType, paramNames, paramValues, useFallback, true); On 2016/01/29 at 22:14:54, pdr wrote: > There is a scary side-codepath (requestPluginCreationWithoutLayoutObjectIfPossible). Can this code be structured so loadPlugin can ASSERT_WITH_SECURITY_IMPLICATION(objectIsLoadable && shouldUsePlugin)? Yeah, I noticed that - and was equally frightened. Looking closer though it appeared to be a (poorly marked) escape-hatch for tests (test_runner embedder impl. is the only thing that doesn't block it/return false.) I guess that doesn't necessarily stop us from pushing the sandbox-check down into loadPlugin - hopefully no test actually relies on the escape-hatch. Alternatively just push the requireLayoutObject bool through requestObject too and then call that in requestPluginCreationWithoutLayoutObjectIfPossible.
Description was changed from ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Rename pluginIsLoadable to objectIsLoadable (consistent with its caller requestObject) to indicate that the "object" is consider in wider scope and will not necessarily be treated as a plugin. Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ========== to ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Rename pluginIsLoadable to objectIsLoadable (consistent with its caller requestObject) to indicate that the "object" is consider in wider scope and will not necessarily be treated as a plugin. Use the pluginIsLoadable name for the new method checking if we're allowed to instantiate the plugin. Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ==========
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) On 2016/01/30 at 00:03:09, fs wrote: > On 2016/01/29 at 22:14:54, pdr wrote: > > It seems odd to me that sandboxing and the plugin csp checks below result in different behavior. Is that intended? > > It's intended (for now at least) - as in better to tread with caution. AIUI, this means you can still block an image/svg+xml object via a CSP directive. Arguably it should move to within shouldUsePlugin though. I shuffled the code a bit and the plugin-CSP check is now grouped with the sandbox check.
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) On 2016/02/01 at 09:21:06, fs wrote: > On 2016/01/30 at 00:03:09, fs wrote: > > On 2016/01/29 at 22:14:54, pdr wrote: > > > It seems odd to me that sandboxing and the plugin csp checks below result in different behavior. Is that intended? > > > > It's intended (for now at least) - as in better to tread with caution. AIUI, this means you can still block an image/svg+xml object via a CSP directive. Arguably it should move to within shouldUsePlugin though. > > I shuffled the code a bit and the plugin-CSP check is now grouped with the sandbox check. Doing that added a new failure: http/tests/security/contentSecurityPolicy/1.1/plugintypes-notype-url.html I guess this makes us take the mimeType.isEmpty() path in FrameLoaderClientImpl::objectContentType and end up returning ObjectContentFrame.
pdr@chromium.org changed reviewers: + mkwst@chromium.org
+cc mkwst, could you take a look at this too? The more securiteyes the merrier. https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:496: { Can you assert both objectIsLoadable and pluginIsLoadable here? https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:597: { Can you add an assert here so future refactorings don't try calling pluginIsLoadable by itself? ASSERT(objectIsLoadable(url, mimetype)); https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right): https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.h:132: bool pluginIsLoadable(const KURL&, const String& mimeType); Can you add a short comment here that mentions the difference between pluginIsLoadable and objectIsLoadable? I.e., they check different things and both need to be checked for plugins.
On 2016/02/02 at 04:32:42, pdr wrote: > +cc mkwst, could you take a look at this too? The more securiteyes the merrier. I think this change looks reasonable, but I don't understand the new test results. Why do we end up with platform-specific breakage?
On 2016/02/04 at 18:20:08, Mike West (OOO until 2nd) wrote: > On 2016/02/02 at 04:32:42, pdr wrote: > > +cc mkwst, could you take a look at this too? The more securiteyes the merrier. > > I think this change looks reasonable, but I don't understand the new test results. Why do we end up with platform-specific breakage? (Or is that the question you're asking me? :) )
On 2016/02/04 at 18:20:20, mkwst wrote: > On 2016/02/04 at 18:20:08, Mike West (OOO until 2nd) wrote: > > On 2016/02/02 at 04:32:42, pdr wrote: > > > +cc mkwst, could you take a look at this too? The more securiteyes the merrier. > > > > I think this change looks reasonable, but I don't understand the new test results. Why do we end up with platform-specific breakage? > > (Or is that the question you're asking me? :) ) The tests in question (and all plugin tests in general I think) are skipped on Linux (ref: crbug.com/318978)
Description was changed from ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Rename pluginIsLoadable to objectIsLoadable (consistent with its caller requestObject) to indicate that the "object" is consider in wider scope and will not necessarily be treated as a plugin. Use the pluginIsLoadable name for the new method checking if we're allowed to instantiate the plugin. Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ========== to ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadURL while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well. Hoist the rejection of Java applet MIME-types out into requestObject(). Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ==========
Description was changed from ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadURL while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well. Hoist the rejection of Java applet MIME-types out into requestObject(). Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ========== to ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadURL while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well. Hoist the rejection of Java applet MIME-types out into requestObject(). Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ==========
Patchset #6 (id:100001) has been deleted
Description was changed from ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadURL while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well. Hoist the rejection of Java applet MIME-types out into requestObject(). Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ========== to ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL/MIME-type is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadObject while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well, while adding a call to allowedToLoadObject there as well. Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ==========
Patchset #8 (id:160001) has been deleted
Prodding this a bit again, but am a bit puzzled by the following error: https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... (two mixed content console warnings rather than one) which I'm not able to reproduce locally. Something any of you've seen before? I guess it could be a race with creating the fallback content or something like that... https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:496: { On 2016/02/02 at 04:32:42, pdr wrote: > Can you assert both objectIsLoadable and pluginIsLoadable here? In the latest PS I've sunk the latter (or its renamed equiv.) into here instead, which should hopefully address your concern as well. https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:597: { On 2016/02/02 at 04:32:42, pdr wrote: > Can you add an assert here so future refactorings don't try calling pluginIsLoadable by itself? > ASSERT(objectIsLoadable(url, mimetype)); Done. https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right): https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.h:132: bool pluginIsLoadable(const KURL&, const String& mimeType); On 2016/02/02 at 04:32:42, pdr wrote: > Can you add a short comment here that mentions the difference between pluginIsLoadable and objectIsLoadable? I.e., they check different things and both need to be checked for plugins. I renamed these two a bit in the latest PS, so hopefully the names are more descriptive - I did add comments too though (belt _and_ suspenders).
On 2016/02/22 at 18:36:56, fs wrote: > Prodding this a bit again, but am a bit puzzled by the following error: > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > (two mixed content console warnings rather than one) > > which I'm not able to reproduce locally. Something any of you've seen before? I guess it could be a race with creating the fallback content or something like that... > > https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): > > https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:496: { > On 2016/02/02 at 04:32:42, pdr wrote: > > Can you assert both objectIsLoadable and pluginIsLoadable here? > > In the latest PS I've sunk the latter (or its renamed equiv.) into here instead, which should hopefully address your concern as well. > > https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:597: { > On 2016/02/02 at 04:32:42, pdr wrote: > > Can you add an assert here so future refactorings don't try calling pluginIsLoadable by itself? > > ASSERT(objectIsLoadable(url, mimetype)); > > Done. > > https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLPlugInElement.h (right): > > https://codereview.chromium.org/1645313002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLPlugInElement.h:132: bool pluginIsLoadable(const KURL&, const String& mimeType); > On 2016/02/02 at 04:32:42, pdr wrote: > > Can you add a short comment here that mentions the difference between pluginIsLoadable and objectIsLoadable? I.e., they check different things and both need to be checked for plugins. > > I renamed these two a bit in the latest PS, so hopefully the names are more descriptive - I did add comments too though (belt _and_ suspenders). This looks good from my perspective. I think we need to understand those two console messages before landing though. @mkwst, can you give this a review and give it the magical word?
On 2016/02/22 at 23:18:35, pdr wrote: > On 2016/02/22 at 18:36:56, fs wrote: > > Prodding this a bit again, but am a bit puzzled by the following error: > > > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > > > (two mixed content console warnings rather than one) > > > > which I'm not able to reproduce locally. Something any of you've seen before? I guess it could be a race with creating the fallback content or something like that... ... > This looks good from my perspective. I think we need to understand those two console messages before landing though. Ok, this turned out to be pretty silly - ASSERTing something which has side-effects can be bad for your console warning emittal, mkay? (This means you no longer get your assert in allowedToLoadPlugin BTW...) > @mkwst, can you give this a review and give it the magical word? Yes please!
On 2016/02/23 at 16:05:13, fs wrote: > On 2016/02/22 at 23:18:35, pdr wrote: > > On 2016/02/22 at 18:36:56, fs wrote: > > > Prodding this a bit again, but am a bit puzzled by the following error: > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > > > > > (two mixed content console warnings rather than one) > > > > > > which I'm not able to reproduce locally. Something any of you've seen before? I guess it could be a race with creating the fallback content or something like that... > ... > > This looks good from my perspective. I think we need to understand those two console messages before landing though. > > Ok, this turned out to be pretty silly - ASSERTing something which has side-effects can be bad for your console warning emittal, mkay? (This means you no longer get your assert in allowedToLoadPlugin BTW...) > > > @mkwst, can you give this a review and give it the magical word? > > Yes please! LGTM Please wait for mkwst's final comment though.
On 2016/02/23 at 21:29:16, pdr wrote: > On 2016/02/23 at 16:05:13, fs wrote: > > On 2016/02/22 at 23:18:35, pdr wrote: > > > On 2016/02/22 at 18:36:56, fs wrote: > > > > Prodding this a bit again, but am a bit puzzled by the following error: > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel... > > > > > > > > (two mixed content console warnings rather than one) > > > > > > > > which I'm not able to reproduce locally. Something any of you've seen before? I guess it could be a race with creating the fallback content or something like that... > > ... > > > This looks good from my perspective. I think we need to understand those two console messages before landing though. > > > > Ok, this turned out to be pretty silly - ASSERTing something which has side-effects can be bad for your console warning emittal, mkay? (This means you no longer get your assert in allowedToLoadPlugin BTW...) > > > > > @mkwst, can you give this a review and give it the magical word? > > > > Yes please! > > LGTM > > Please wait for mkwst's final comment though. mkwst, ping
LGTM with nits about the test. Sorry this completely fell off my radar! Thanks for pinging it back up, and for doing the work. https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html (right): https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html:8: } You could turn this into a `testharness`-style test that would also work in Firefox, et al. if you listened for a message from `object-svg.html`, and then reached into that frame to verify the resulting DOM (and `error`/`load` handlers on the `object`). https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html:10: <iframe sandbox src="resources/object-svg.html" You'd need to add `allow-same-origin` to do so.
https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html (right): https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html:8: } On 2016/02/24 at 13:30:55, Mike West wrote: > You could turn this into a `testharness`-style test that would also work in Firefox, et al. if you listened for a message from `object-svg.html`, and then reached into that frame to verify the resulting DOM (and `error`/`load` handlers on the `object`). Done. https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html:10: <iframe sandbox src="resources/object-svg.html" On 2016/02/24 at 13:30:55, Mike West wrote: > You'd need to add `allow-same-origin` to do so. And 'allow-scripts' too I guess.
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1645313002/#ps200001 (title: "Testharness-based test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1645313002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645313002/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL/MIME-type is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadObject while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well, while adding a call to allowedToLoadObject there as well. Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 ========== to ========== Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL/MIME-type is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadObject while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well, while adding a call to allowedToLoadObject there as well. Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 Committed: https://crrev.com/4acd739260230f59c08858a31e795c2dcfdc54d4 Cr-Commit-Position: refs/heads/master@{#377559} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4acd739260230f59c08858a31e795c2dcfdc54d4 Cr-Commit-Position: refs/heads/master@{#377559} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
