|
|
Created:
5 years, 1 month ago by svaldez Modified:
4 years, 11 months ago Reviewers:
Bernhard Bauer, jochen (gone - plz use gerrit), Ryan Sleevi, esprehn, raymes, Will Harris CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, jam, Nate Chapin, jochen+watch_chromium.org, loading-reviews_chromium.org, mlamouri+watch-test-runner_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@keygen_core Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding <keygen> Content Setting (Blink)
Adding the KEYGEN hooks into Blink to detect <keygen> usage.
BUG=514767
Committed: https://crrev.com/d5d97d05af70d8a8003769e22c46dbee17a3251c
Cr-Commit-Position: refs/heads/master@{#367650}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Using AllowKeygen IPC. #
Total comments: 2
Patch Set 3 : Fixing comment. #Patch Set 4 : Fixing profile_io_data use in chrome_render_message_filter #
Total comments: 9
Patch Set 5 : Clean up code. #
Total comments: 2
Patch Set 6 : Simplify Keygen check. #Patch Set 7 : Rebase? #
Total comments: 7
Patch Set 8 : Simplifying messages. #
Total comments: 8
Patch Set 9 : More fixes. #Patch Set 10 : Remove CRMF #Patch Set 11 : Adding origin URL. #Patch Set 12 : Fixing missing frame. #Patch Set 13 : Fixing ContentBlocked calls. #Patch Set 14 : Rebase. #Patch Set 15 : Fix failures. #Patch Set 16 : Fix typo. #Patch Set 17 : Fixing nullptr. #Patch Set 18 : Rebase. #Dependent Patchsets: Messages
Total messages: 62 (16 generated)
svaldez@chromium.org changed reviewers: + jochen@chromium.org, raymes@chromium.org, rsleevi@chromium.org
Not sure who is the best person to review this part (it's not me:). jochen do you have any ideas?
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... chrome/renderer/content_settings_observer.cc:366: // allowing all keygen, it's quicker this way. This comment made _some_ sense before you copy-and-pasted it from below and replaced "scripts" with "keygen", but now it doesn't really anymore. https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... chrome/renderer/content_settings_observer.cc:367: bool allow = true; You can probably just send an IPC to the browser to call ContentBrowserClient::AllowKeygen() instead of pushing the rules to each renderer. Or see my other comment below -- I'm not sure we even need this check at all in the renderer. https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... chrome/renderer/content_settings_observer.h:158: // Caches the result of |AllowKeygen|. Nit: method names don't go into pipe symbols. https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_message_filter.cc:583: if (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { If you have this check anyway, why do you need the one in the renderer? Just fail here and show the blockage icon. https://codereview.chromium.org/1417033010/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/1417033010/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:177: virtual bool allowKeygen(bool enabledPerSettings) { return enabledPerSettings; } What is |enabledPerSettings| for? It seems you only ever pass true for it, and there is no flag that controls keygen in WebSettings.
https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... chrome/renderer/content_settings_observer.cc:366: // allowing all keygen, it's quicker this way. On 2015/11/05 11:12:13, Bernhard Bauer wrote: > This comment made _some_ sense before you copy-and-pasted it from below and > replaced "scripts" with "keygen", but now it doesn't really anymore. Done. https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... chrome/renderer/content_settings_observer.cc:367: bool allow = true; On 2015/11/05 11:12:13, Bernhard Bauer wrote: > You can probably just send an IPC to the browser to call > ContentBrowserClient::AllowKeygen() instead of pushing the rules to each > renderer. Or see my other comment below -- I'm not sure we even need this check > at all in the renderer. Done. https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_set... chrome/renderer/content_settings_observer.h:158: // Caches the result of |AllowKeygen|. On 2015/11/05 11:12:13, Bernhard Bauer wrote: > Nit: method names don't go into pipe symbols. Done. https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_message_filter.cc:583: if (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { On 2015/11/05 11:12:13, Bernhard Bauer wrote: > If you have this check anyway, why do you need the one in the renderer? Just > fail here and show the blockage icon. The problem is that we want to have the Page Action show up when the page is rendered, instead of having UI show up when the form is being submitted. https://codereview.chromium.org/1417033010/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/1417033010/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:177: virtual bool allowKeygen(bool enabledPerSettings) { return enabledPerSettings; } On 2015/11/05 11:12:13, Bernhard Bauer wrote: > What is |enabledPerSettings| for? It seems you only ever pass true for it, and > there is no flag that controls keygen in WebSettings. Removed, I was trying to match the other allow methods.
https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_message_filter.cc:583: if (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { On 2015/11/05 16:35:20, svaldez wrote: > On 2015/11/05 11:12:13, Bernhard Bauer wrote: > > If you have this check anyway, why do you need the one in the renderer? Just > > fail here and show the blockage icon. > > The problem is that we want to have the Page Action show up when the page is > rendered, instead of having UI show up when the form is being submitted. Wait, so you want to show something in the omnibox anytime any webpage has a <keygen> element, even if it's never used? Isn't that a bit overkill?
On 2015/11/05 16:45:19, Bernhard Bauer wrote: > https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_message_filter.cc (right): > > https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_message_filter.cc:583: if > (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { > On 2015/11/05 16:35:20, svaldez wrote: > > On 2015/11/05 11:12:13, Bernhard Bauer wrote: > > > If you have this check anyway, why do you need the one in the renderer? Just > > > fail here and show the blockage icon. > > > > The problem is that we want to have the Page Action show up when the page is > > rendered, instead of having UI show up when the form is being submitted. > > Wait, so you want to show something in the omnibox anytime any webpage has a > <keygen> element, even if it's never used? Isn't that a bit overkill? What we really need is to be able to show UI when <keygen> is being used in a form, but waiting until form submission makes the UX complicated since then we need to wait for the user to respond to the UI in the middle of a form submission. Displaying the page action on render seemed to be the most straightforward way of presenting UI. The usage of <keygen> is low enough that there shouldn't be many false positives in displaying the page action on a page that might not actually have a form submission using keygen.
LGTM w/ a nit, but I don't own much (if anything?) here. On 2015/11/05 16:59:29, svaldez wrote: > On 2015/11/05 16:45:19, Bernhard Bauer wrote: > > > https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... > > File content/browser/renderer_host/render_message_filter.cc (right): > > > > > https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_ho... > > content/browser/renderer_host/render_message_filter.cc:583: if > > (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { > > On 2015/11/05 16:35:20, svaldez wrote: > > > On 2015/11/05 11:12:13, Bernhard Bauer wrote: > > > > If you have this check anyway, why do you need the one in the renderer? > Just > > > > fail here and show the blockage icon. > > > > > > The problem is that we want to have the Page Action show up when the page is > > > rendered, instead of having UI show up when the form is being submitted. > > > > Wait, so you want to show something in the omnibox anytime any webpage has a > > <keygen> element, even if it's never used? Isn't that a bit overkill? > > What we really need is to be able to show UI when <keygen> is being used in a > form, but waiting until form submission makes the UX complicated since then we > need to wait for the user to respond to the UI in the middle of a form > submission. Displaying the page action on render seemed to be the most > straightforward way of presenting UI. > > The usage of <keygen> is low enough that there shouldn't be many false positives > in displaying the page action on a page that might not actually have a form > submission using keygen. OK, makes sense. https://codereview.chromium.org/1417033010/diff/20001/chrome/renderer/content... File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/1417033010/diff/20001/chrome/renderer/content... chrome/renderer/content_settings_observer.h:45: // |AllowScriptFromSource()|, and |AllowKeygen()|. |content_setting_rules| Revert this?
https://codereview.chromium.org/1417033010/diff/20001/chrome/renderer/content... File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/1417033010/diff/20001/chrome/renderer/content... chrome/renderer/content_settings_observer.h:45: // |AllowScriptFromSource()|, and |AllowKeygen()|. |content_setting_rules| On 2015/11/05 17:17:33, Bernhard Bauer wrote: > Revert this? Done.
with the test runner changes, it should be possible to add a layout test, no? https://codereview.chromium.org/1417033010/diff/60001/chrome/renderer/content... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/60001/chrome/renderer/content... chrome/renderer/content_settings_observer.cc:359: if (it != cached_keygen_permissions_.end()) do you expect that this happens so often that it's worthwhile to cache this? https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: document().frame()->loader().client()->didNotAllowKeygen(); why did you put this check here? Also, it seems like nothing happens? https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:229: no empty line here https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:283: no empty line here
https://codereview.chromium.org/1417033010/diff/60001/chrome/renderer/content... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/60001/chrome/renderer/content... chrome/renderer/content_settings_observer.cc:359: if (it != cached_keygen_permissions_.end()) On 2015/11/07 05:53:29, jochen (slow - traveling) wrote: > do you expect that this happens so often that it's worthwhile to cache this? Done. https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: document().frame()->loader().client()->didNotAllowKeygen(); On 2015/11/07 05:53:29, jochen (slow - traveling) wrote: > why did you put this check here? Also, it seems like nothing happens? Isn't this required to tell the browser that the renderer created a keygen element, therefore we should display the page action to block/allow keygen? https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:229: On 2015/11/07 05:53:29, jochen (slow - traveling) wrote: > no empty line here Done. https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:283: On 2015/11/07 05:53:29, jochen (slow - traveling) wrote: > no empty line here Done.
https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: document().frame()->loader().client()->didNotAllowKeygen(); On 2015/11/10 at 15:23:41, svaldez wrote: > On 2015/11/07 05:53:29, jochen (slow - traveling) wrote: > > why did you put this check here? Also, it seems like nothing happens? > > Isn't this required to tell the browser that the renderer created a keygen element, therefore we should display the page action to block/allow keygen? yes, but I don't understand why you've put this into createLayoutObject(), and I also would expect that you do actually block something if you don't have permission.
On 2015/11/10 19:54:45, jochen (slow - traveling) wrote: > yes, but I don't understand why you've put this into createLayoutObject(), and I > also would expect that you do actually block something if you don't have > permission. The permission only comes in to play with form submission, which is weird in Blink for many reasons: - Form building here is synchronous. Unlike slapping file data (an asynchronous op), <keygen> form gathering flows through the normal input types, as it's expected that the Blink/renderer has all the data. This is an artifact from the WebKit days and Apple's security model, but three attempts at refactoring this (and the fact that we're deprecating it) have resulted in... well, not much progress. - It's in a navigational action, for the most part. While a form.submit() doesn't *have* to be a navigational request (e.g. if you're building a FormData for a fetch()-like event), in practice and in the real world, it is - so there's this weird aspect of load-in-progress due to how the form submission part works that is super hawkward. That's why we trigger the UI state on the presence of the <keygen> (e.g. before the form submission/navigation start), the user can allow, and then when Blink begins assembling the form data, the allow/deny action can determine whether or not the key should be generate then. [This awfulness is one of the many complexities of having <keygen> around at all; if we were keeping it, fixing it would be one of those much needed and much hairy tasks] For cases where the form submission may be automatic (e.g. as soon as onload happens, script invokes the form.submit action), enterprise-level policy whitelisting can be used to pre-seed the permission to the 'allow' phase. This seems consistent with the actual mainstream deployed solutions (e.g. those being offered as managed CA solutions for device enrollment scenarios). Longer-term, our answer to this is "Use the device management capabilities as appropriate" (active directory, android MDM, the ChromeOS enterprise APIs, etc), but the short-term solution for that case is to also support whitelisting during the deprecation phase while such future-compat solutions are adopted by enterprises.
so you're saying that the actual blocking happens somewhere else (outside of blink?)
On 2015/11/10 21:54:00, jochen (slow - traveling) wrote: > so you're saying that the actual blocking happens somewhere else (outside of > blink?) Yes. The actual mechanism-to-be-blocked happens by the Renderer process making a call to the Browser process to do the needful, dangerous operation (generate a key). The browser process does the ACL check to see whether or not the renderer-origin should be permitted. However, at that time of generation, the renderer is in all sorts of weird state (quasi-navigational status, blocking IPC to the browser), and so the only option at that time would be to do a blocking prompt (ala window.alert). Working with enamelites, the desire was to avoid modal-blocking dialogs, and so the solution put forth is to soft-badge as a site setting that the site 'will want' to generate a key (as observed through the presence of a <keygen> element in the DOM tree), and then when the renderer IPCs to the browser, the browser just checks the ACL and either does it or denies it (where deny == sends back the empty string, which is the spec-compliant means of saying GTFO)
jochen@chromium.org changed reviewers: + esprehn@chromium.org
+esprehn for some advice how we'd best ping chrome about the fact that a given frame contains a <keygen> element https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: document().frame()->loader().client()->didNotAllowKeygen(); ok, so this should not be a content setting like thing at all: it's not a check whether we allow keygen, and the didNotAllowKeygen callback is also lying about what it does. instead, the HTMLKeygenElement ctor should just ping the embedder about the fact that it's there.
On 2015/11/11 22:14:52, jochen wrote: > +esprehn for some advice how we'd best ping chrome about the fact that a given > frame contains a <keygen> element > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: > document().frame()->loader().client()->didNotAllowKeygen(); > ok, so this should not be a content setting like thing at all: > > it's not a check whether we allow keygen, and the didNotAllowKeygen callback is > also lying about what it does. > > instead, the HTMLKeygenElement ctor should just ping the embedder about the fact > that it's there. Is it reasonable to be pinging the embedder everytime we render keygen, instead of only when its being blocked? Though I guess the usage is low enough there shouldn't be a noticeable difference.
On 2015/11/17 at 21:54:04, svaldez wrote: > On 2015/11/11 22:14:52, jochen wrote: > > +esprehn for some advice how we'd best ping chrome about the fact that a given > > frame contains a <keygen> element > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: > > document().frame()->loader().client()->didNotAllowKeygen(); > > ok, so this should not be a content setting like thing at all: > > > > it's not a check whether we allow keygen, and the didNotAllowKeygen callback is > > also lying about what it does. > > > > instead, the HTMLKeygenElement ctor should just ping the embedder about the fact > > that it's there. > > Is it reasonable to be pinging the embedder everytime we render keygen, instead of only when its being blocked? Though I guess the usage is low enough there shouldn't be a noticeable difference. isn't that the whole point of this CL? Once keygen is blocked, it's too late to show UI
On 2015/11/18 16:16:09, jochen wrote: > On 2015/11/17 at 21:54:04, svaldez wrote: > > On 2015/11/11 22:14:52, jochen wrote: > > > +esprehn for some advice how we'd best ping chrome about the fact that a > given > > > frame contains a <keygen> element > > > > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): > > > > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: > > > document().frame()->loader().client()->didNotAllowKeygen(); > > > ok, so this should not be a content setting like thing at all: > > > > > > it's not a check whether we allow keygen, and the didNotAllowKeygen callback > is > > > also lying about what it does. > > > > > > instead, the HTMLKeygenElement ctor should just ping the embedder about the > fact > > > that it's there. > > > > Is it reasonable to be pinging the embedder everytime we render keygen, > instead of only when its being blocked? Though I guess the usage is low enough > there shouldn't be a noticeable difference. > > isn't that the whole point of this CL? Once keygen is blocked, it's too late to > show UI Sorry, wasn't clear. The second part was about only pinging the embedder when the content setting says that keygen should be blocked, not when keygen itself is actually run. But I think I agree that since we default to a content setting to block keygen unless allowed, pinging the embedder every time keygen is used is probably better than plumbing the setting all the way through.
On 2015/11/18 16:23:45, svaldez wrote: > On 2015/11/18 16:16:09, jochen wrote: > > On 2015/11/17 at 21:54:04, svaldez wrote: > > > On 2015/11/11 22:14:52, jochen wrote: > > > > +esprehn for some advice how we'd best ping chrome about the fact that a > > given > > > > frame contains a <keygen> element > > > > > > > > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): > > > > > > > > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: > > > > document().frame()->loader().client()->didNotAllowKeygen(); > > > > ok, so this should not be a content setting like thing at all: > > > > > > > > it's not a check whether we allow keygen, and the didNotAllowKeygen > callback > > is > > > > also lying about what it does. > > > > > > > > instead, the HTMLKeygenElement ctor should just ping the embedder about > the > > fact > > > > that it's there. > > > > > > Is it reasonable to be pinging the embedder everytime we render keygen, > > instead of only when its being blocked? Though I guess the usage is low enough > > there shouldn't be a noticeable difference. > > > > isn't that the whole point of this CL? Once keygen is blocked, it's too late > to > > show UI > > Sorry, wasn't clear. The second part was about only pinging the embedder when > the content setting says that keygen should be blocked, not when keygen itself > is actually run. But I think I agree that since we default to a content setting > to block keygen unless allowed, pinging the embedder every time keygen is used > is probably better than plumbing the setting all the way through. It probably doesn't make a big difference in any case, because right now we do an IPC to get the content setting, so we could just make that an async IPC that only notifies the browser.
On 2015/11/18 at 16:44:18, bauerb wrote: > On 2015/11/18 16:23:45, svaldez wrote: > > On 2015/11/18 16:16:09, jochen wrote: > > > On 2015/11/17 at 21:54:04, svaldez wrote: > > > > On 2015/11/11 22:14:52, jochen wrote: > > > > > +esprehn for some advice how we'd best ping chrome about the fact that a > > > given > > > > > frame contains a <keygen> element > > > > > > > > > > > > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): > > > > > > > > > > > > > > > https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: > > > > > document().frame()->loader().client()->didNotAllowKeygen(); > > > > > ok, so this should not be a content setting like thing at all: > > > > > > > > > > it's not a check whether we allow keygen, and the didNotAllowKeygen > > callback > > > is > > > > > also lying about what it does. > > > > > > > > > > instead, the HTMLKeygenElement ctor should just ping the embedder about > > the > > > fact > > > > > that it's there. > > > > > > > > Is it reasonable to be pinging the embedder everytime we render keygen, > > > instead of only when its being blocked? Though I guess the usage is low enough > > > there shouldn't be a noticeable difference. > > > > > > isn't that the whole point of this CL? Once keygen is blocked, it's too late > > to > > > show UI > > > > Sorry, wasn't clear. The second part was about only pinging the embedder when > > the content setting says that keygen should be blocked, not when keygen itself > > is actually run. But I think I agree that since we default to a content setting > > to block keygen unless allowed, pinging the embedder every time keygen is used > > is probably better than plumbing the setting all the way through. > > It probably doesn't make a big difference in any case, because right now we do an IPC to get the content setting, so we could just make that an async IPC that only notifies the browser. Right
Rebasing made the patchsets weird, but the diff from 5 to 7 should show the new changes. https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): https://codereview.chromium.org/1417033010/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: document().frame()->loader().client()->didNotAllowKeygen(); On 2015/11/11 22:14:51, jochen wrote: > ok, so this should not be a content setting like thing at all: > > it's not a check whether we allow keygen, and the didNotAllowKeygen callback is > also lying about what it does. > > instead, the HTMLKeygenElement ctor should just ping the embedder about the fact > that it's there. Done.
https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:185: *allowed = content_settings->GetContentSetting( instead of getting the content settings you probably want to notify the tab specific content settings of the fact that a keygen element is on the page, no? https://codereview.chromium.org/1417033010/diff/120001/components/content_set... File components/content_settings/content/common/content_settings_messages.h (right): https://codereview.chromium.org/1417033010/diff/120001/components/content_set... components/content_settings/content/common/content_settings_messages.h:78: IPC_SYNC_MESSAGE_CONTROL2_1(ChromeViewHostMsg_AllowKeygen, update the name of this ipc also, can be async now and doesn't need a return value anymore https://codereview.chromium.org/1417033010/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): https://codereview.chromium.org/1417033010/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:65: document().frame()->loader().client()->usedKeygen(); why not in the ctor? https://codereview.chromium.org/1417033010/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.h (right): https://codereview.chromium.org/1417033010/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.h:145: void usedKeygen() override; didUseKeygen
https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:185: *allowed = content_settings->GetContentSetting( On 2015/11/24 13:05:08, jochen wrote: > instead of getting the content settings you probably want to notify the tab > specific content settings of the fact that a keygen element is on the page, no? Done. https://codereview.chromium.org/1417033010/diff/120001/components/content_set... File components/content_settings/content/common/content_settings_messages.h (right): https://codereview.chromium.org/1417033010/diff/120001/components/content_set... components/content_settings/content/common/content_settings_messages.h:78: IPC_SYNC_MESSAGE_CONTROL2_1(ChromeViewHostMsg_AllowKeygen, On 2015/11/24 13:05:08, jochen wrote: > update the name of this ipc > > also, can be async now and doesn't need a return value anymore Done. https://codereview.chromium.org/1417033010/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.h (right): https://codereview.chromium.org/1417033010/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.h:145: void usedKeygen() override; On 2015/11/24 13:05:08, jochen wrote: > didUseKeygen Done.
https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.h:18: class ProfileIOData; changes to this file are no longer required, right? https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... chrome/renderer/content_settings_observer.cc:597: Send(new ChromeViewHostMsg_DidUseKeygen(routing_id())); i'd include the frame url here, so you can show what sites want to use keygen in the UI. I'd also store it somewhere in tab specific content settings https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... chrome/renderer/content_settings_observer.h:147: // Stores if images, scripts, keygen, and plugins have actually been blocked. revert this change https://codereview.chromium.org/1417033010/diff/140001/content/browser/render... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/140001/content/browser/render... content/browser/renderer_host/render_message_filter.cc:593: if (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { unrelated to this CL?
https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.h:18: class ProfileIOData; On 2015/11/25 08:54:34, jochen wrote: > changes to this file are no longer required, right? Done. https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... chrome/renderer/content_settings_observer.cc:597: Send(new ChromeViewHostMsg_DidUseKeygen(routing_id())); On 2015/11/25 08:54:34, jochen wrote: > i'd include the frame url here, so you can show what sites want to use keygen in > the UI. I'd also store it somewhere in tab specific content settings We're currently using the top-level origin for the content-setting, and we already display that in the UI through ContentSettingImage/Bubble. https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/renderer/conten... chrome/renderer/content_settings_observer.h:147: // Stores if images, scripts, keygen, and plugins have actually been blocked. On 2015/11/25 08:54:34, jochen wrote: > revert this change Done. https://codereview.chromium.org/1417033010/diff/140001/content/browser/render... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/140001/content/browser/render... content/browser/renderer_host/render_message_filter.cc:593: if (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { On 2015/11/25 08:54:35, jochen wrote: > unrelated to this CL? This is the callback which Blink uses to actually generate the keygen data for use in the form submission. However it should probably be separated into another CL since otherwise we'd be blocking keygen before we have the CL for the keygen UI committed.
lgtm if rsleevi and/or felt confirms that we actually want to show the top-level URL in the UI, even if an unrelated third-party uses keygen
On 2015/11/25 15:01:00, jochen wrote: > lgtm if rsleevi and/or felt confirms that we actually want to show the top-level > URL in the UI, even if an unrelated third-party uses keygen Yeah, thinking about it, I'm not sure which is the best way of handling this. If we do it per frame, than we have issues with pages that have multiple iframes that each use the keygen element. On the other hand, doing it based on the top-level origin complicates the cases where we've explicitly denied keygen use in the iframe domains.
On 2015/11/25 at 15:39:05, svaldez wrote: > On 2015/11/25 15:01:00, jochen wrote: > > lgtm if rsleevi and/or felt confirms that we actually want to show the top-level > > URL in the UI, even if an unrelated third-party uses keygen > > Yeah, thinking about it, I'm not sure which is the best way of handling this. If we do it per frame, than we have issues with pages that have multiple iframes that each use the keygen element. On the other hand, doing it based on the top-level origin complicates the cases where we've explicitly denied keygen use in the iframe domains. i'd just include the url in the IPC macro and collect it on the browser side, then you can still decide what to do about it
On 2015/11/25 at 15:39:05, svaldez wrote: > On 2015/11/25 15:01:00, jochen wrote: > > lgtm if rsleevi and/or felt confirms that we actually want to show the top-level > > URL in the UI, even if an unrelated third-party uses keygen > > Yeah, thinking about it, I'm not sure which is the best way of handling this. If we do it per frame, than we have issues with pages that have multiple iframes that each use the keygen element. On the other hand, doing it based on the top-level origin complicates the cases where we've explicitly denied keygen use in the iframe domains. i'd just include the url in the IPC macro and collect it on the browser side, then you can still decide what to do about it
On 2015/11/26 13:25:30, jochen wrote: > On 2015/11/25 at 15:39:05, svaldez wrote: > > On 2015/11/25 15:01:00, jochen wrote: > > > lgtm if rsleevi and/or felt confirms that we actually want to show the > top-level > > > URL in the UI, even if an unrelated third-party uses keygen > > > > Yeah, thinking about it, I'm not sure which is the best way of handling this. > If we do it per frame, than we have issues with pages that have multiple iframes > that each use the keygen element. On the other hand, doing it based on the > top-level origin complicates the cases where we've explicitly denied keygen use > in the iframe domains. > > i'd just include the url in the IPC macro and collect it on the browser side, > then you can still decide what to do about it Added. I think this is probably going to require additional plumbing on the content setting side, but in either case adding the origin URL should be sufficient for the Blink plumbing.
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1417033010/#ps220001 (title: "Fixing missing frame.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
svaldez@chromium.org changed reviewers: + wfh@chromium.org
Modification to content_settings_messages needs a security review.
On 2015/11/30 22:00:27, svaldez wrote: > Modification to content_settings_messages needs a security review. Updates to Keygen for review: - Setting OnContentBlocked to only be called when Keygen is actually blocked to fix UI issues. - Adding the frame URL in case we end up wanting to do permissions based on frame origin. I believe other than a IPC message review, this should be ready to commit?
security lgtm for the messages you could consider adding a RAPPOR in TabSpecificContentSettings::OnDidUseKeygen with the frame origin then you would get potentially useful stats about where keygen is being used.
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1417033010/#ps240001 (title: "Fixing ContentBlocked calls.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/240001
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_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by svaldez@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/1417033010/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
So I ended up having to move the didUseKeygen call back out of the constructor due to tests failing when it was in the ctor. I'm investigating but is it reasonable to have didUseKeygen called in createLayout?
On 2015/12/11 at 16:20:37, svaldez wrote: > So I ended up having to move the didUseKeygen call back out of the constructor due to tests failing when it was in the ctor. I'm investigating but is it reasonable to have didUseKeygen called in createLayout? why did the tests fail?
On 2015/12/14 12:08:41, jochen wrote: > On 2015/12/11 at 16:20:37, svaldez wrote: > > So I ended up having to move the didUseKeygen call back out of the constructor > due to tests failing when it was in the ctor. I'm investigating but is it > reasonable to have didUseKeygen called in createLayout? > > why did the tests fail? Actually I think I found the issue. It turns out Document doesn't necessarily have a Frame object when constructed through some of the WebKit tests.
The CQ bit was checked by svaldez@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/1417033010/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/320001
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jochen@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/1417033010/#ps340001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Adding <keygen> Content Setting (Blink) Adding the KEYGEN hooks into Blink to detect <keygen> usage. BUG=514767 ========== to ========== Adding <keygen> Content Setting (Blink) Adding the KEYGEN hooks into Blink to detect <keygen> usage. BUG=514767 Committed: https://crrev.com/d5d97d05af70d8a8003769e22c46dbee17a3251c Cr-Commit-Position: refs/heads/master@{#367650} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/d5d97d05af70d8a8003769e22c46dbee17a3251c Cr-Commit-Position: refs/heads/master@{#367650} |