Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
412643003:
Fix the origin access whitelist for extensions (Closed)
|
Created:
6 years, 5 months ago by Devlin Modified:
6 years, 5 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix the origin access whitelist for extensions
BUG=393686
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285095
Patch Set 1 #
Total comments: 11
Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #Messages
Total messages: 18 (0 generated)
Well... that was dumb of me. https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (left): https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:777: WebSecurityPolicy::resetOriginAccessWhitelists(); This was introduced in https://codereview.chromium.org/348313003, when I for some inexplicable reason thought this would only clear the extension's origin whitelist. Clearly, this isn't the case, and results in us clearing the whitelist for extensions obscenely frequently.
looks fine but a suggestion of how to make this change less invasive (and avoid changing the messages, which is always a pain) https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (left): https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:777: WebSecurityPolicy::resetOriginAccessWhitelists(); On 2014/07/22 21:56:47, Devlin wrote: > This was introduced in https://codereview.chromium.org/348313003, when I for > some inexplicable reason thought this would only clear the extension's origin > whitelist. Clearly, this isn't the case, and results in us clearing the > whitelist for extensions obscenely frequently. right... good point. This would be such a simpler change if we just added a clearOriginAccessWhitelist method which takes the origin. doesn't look very hard, but blink changes... eh. though a only-slightly-less-complicated change in the vein of what you had before would be to remove the existing permissions then add the new ones. https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:744: // If webkit isn't initialized, this will be done when it finishes starting s/webkit/blink/ though I think the comment isn't really necessary https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:747: bool was_added = params.was_added; this alias seems like overkill. https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:806: static const char* schemes[] = { kSchemes?
https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (left): https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:777: WebSecurityPolicy::resetOriginAccessWhitelists(); On 2014/07/23 01:18:54, kalman wrote: > On 2014/07/22 21:56:47, Devlin wrote: > > This was introduced in https://codereview.chromium.org/348313003, when I for > > some inexplicable reason thought this would only clear the extension's origin > > whitelist. Clearly, this isn't the case, and results in us clearing the > > whitelist for extensions obscenely frequently. > > right... good point. > > This would be such a simpler change if we just added a > clearOriginAccessWhitelist method which takes the origin. doesn't look very > hard, but blink changes... eh. > > though a only-slightly-less-complicated change in the vein of what you had > before would be to remove the existing permissions then add the new ones. But to add the new ones, we add the new ones for _each existing extension_ which seems bad. Also, this would definitely mean updating callers of this to make sure we aren't doing what we're doing now - which is calling this for each extension when a new renderer is created. Overall, to me, that sounds like a slightly more invasive change... WDYT? https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:744: // If webkit isn't initialized, this will be done when it finishes starting On 2014/07/23 01:18:54, kalman wrote: > s/webkit/blink/ > > though I think the comment isn't really necessary Done. https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:747: bool was_added = params.was_added; On 2014/07/23 01:18:54, kalman wrote: > this alias seems like overkill. Done. https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:806: static const char* schemes[] = { On 2014/07/23 01:18:55, kalman wrote: > kSchemes? Done.
https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (left): https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:777: WebSecurityPolicy::resetOriginAccessWhitelists(); > But to add the new ones, we add the new ones for _each existing extension_ which > seems bad. I don't think so? add/remove take the origin which is unique for every extension (takes extension->url()). Maybe I'm misunderstanding. > Also, this would definitely mean updating callers of this to make > sure we aren't doing what we're doing now - which is calling this for each > extension when a new renderer is created. Overall, to me, that sounds like a > slightly more invasive change... WDYT? I was imagining a method like SetOriginPermissions instead of UpdateOriginPermissions (or still call it UpdateOriginPermissions... whatever). instead of taking a was_added boolean it'd take the old permissions. in the case of InitOriginPermissions the old permissions would be an empty set.
https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (left): https://codereview.chromium.org/412643003/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:777: WebSecurityPolicy::resetOriginAccessWhitelists(); On 2014/07/23 15:45:35, kalman wrote: > > But to add the new ones, we add the new ones for _each existing extension_ > which > > seems bad. > > I don't think so? add/remove take the origin which is unique for every extension > (takes extension->url()). Maybe I'm misunderstanding. > > > Also, this would definitely mean updating callers of this to make > > sure we aren't doing what we're doing now - which is calling this for each > > extension when a new renderer is created. Overall, to me, that sounds like a > > slightly more invasive change... WDYT? > > I was imagining a method like SetOriginPermissions instead of > UpdateOriginPermissions (or still call it UpdateOriginPermissions... whatever). > instead of taking a was_added boolean it'd take the old permissions. in the case > of InitOriginPermissions the old permissions would be an empty set. Ahhh, I see, I misunderstood (thought you meant still use resetOriginAccessWhitelists(), and then re-add everything). Yes, that will work. :)
lgtm https://codereview.chromium.org/412643003/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/412643003/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:791: const URLPatternSet& new_patterns, nit: could you swap new_patterns and old_patterns? this order (new then old) is opposite to what I expect. https://codereview.chromium.org/412643003/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:816: // Add any new patterns that weren't in the old patterns. nit: this comment should be up a couple of lines to match the placement of the other one
The CQ bit was checked by rdevlin.cronin@chromium.org
https://codereview.chromium.org/412643003/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/412643003/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:791: const URLPatternSet& new_patterns, On 2014/07/23 17:01:17, kalman wrote: > nit: could you swap new_patterns and old_patterns? this order (new then old) is > opposite to what I expect. Done. https://codereview.chromium.org/412643003/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:816: // Add any new patterns that weren't in the old patterns. On 2014/07/23 17:01:17, kalman wrote: > nit: this comment should be up a couple of lines to match the placement of the > other one Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/412643003/...
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/412643003/...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/412643003/...
Message was sent while issue was closed.
Change committed as 285095 |
