Issue 29451568: Issue 4494 - Avoid causing some sandbox related warnings (Closed)
|
Created:
May 30, 2017, 9:22 a.m. by kzar Modified:
May 31, 2017, 11:06 a.m. Reviewers:
Sebastian Noack CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 4494 - Avoid causing some sandbox related warnings
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix indentation #Patch Set 3 : Use regexp, ignore case #MessagesTotal messages: 10
Patch Set 1 This obviously won't solve the problem completely but it should reduce the number of warnings shown.
https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js#n... inject.preload.js:391: window.frameElement.getAttribute("sandbox"); Nit: The indentation looks a bit off here. https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js#n... inject.preload.js:392: if (typeof sandbox != "string" || sandbox.includes("allow-scripts")) What if allow-scripts is misspelled like "allow-scriptss"?
For reference I was initially interested in this issue since I figured there might be another way to inject our wrappers for these frames. One way I could see to do it was like this comment[0] describes... remove the iframe from the document, add the allow-scripts string, add the iframe back to the document, remove the allow-scripts string. But that's not workable since: 1. Scripts will still be able to run in the iframe, we'd have to remove and add it back to the document again to avoid that. So we've broken sandboxes. 2. Website scripts running in the parent window could spot those DOM mutations. Another way would be to add an event listener for "abp-RANDOMEVENT-inject" in the injected code in the parent window, fire that with the frameElement when the sandbox is detected and have the parent window inject the scripts using eval like we do in our contentWindow wrapper. This seems kinda complicated though and also wouldn't work unless allow-same-origin is enabled. [0] - https://github.com/brave/browser-laptop/issues/1791#issuecomment-223850632
On 2017/05/30 09:48:51, kzar wrote: > For reference I was initially interested in this issue since I figured there > might be another way to inject our wrappers for these frames. > > One way I could see to do it was like this comment[0] describes... remove the > iframe from the document, add the allow-scripts string, add the iframe back to > the document, remove the allow-scripts string. But that's not workable since: > 1. Scripts will still be able to run in the iframe, we'd have to remove and add > it back to the document again to avoid that. So we've broken sandboxes. > 2. Website scripts running in the parent window could spot those DOM mutations. > > Another way would be to add an event listener for "abp-RANDOMEVENT-inject" in > the injected code in the parent window, fire that with the frameElement when the > sandbox is detected and have the parent window inject the scripts using eval > like we do in our contentWindow wrapper. This seems kinda complicated though and > also wouldn't work unless allow-same-origin is enabled. > > [0] - https://github.com/brave/browser-laptop/issues/1791#issuecomment-223850632 Do we even have to bother about frames in which JavaScript cannot run? If no script can run there, they also cannot use WebSocket and RTCPeerConnection, hence our wrappers would be redundant anyway, right?
On 2017/05/30 10:03:00, Sebastian Noack wrote: > Do we even have to bother about frames in which JavaScript cannot run? If no > script can run there, they also cannot use WebSocket and RTCPeerConnection, > hence our wrappers would be redundant anyway, right? Well it's less of a concern now that we wrap contentWindow, before we did that it definitely was a problem, see #4586. I can't see any way it's a problem in practice now that we do, but maybe I've missed something.
Patch Set 2 : Fix indentation https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js#n... inject.preload.js:391: window.frameElement.getAttribute("sandbox"); On 2017/05/30 09:41:16, Sebastian Noack wrote: > Nit: The indentation looks a bit off here. Done. https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js#n... inject.preload.js:392: if (typeof sandbox != "string" || sandbox.includes("allow-scripts")) On 2017/05/30 09:41:16, Sebastian Noack wrote: > What if allow-scripts is misspelled like "allow-scriptss"? Well a false-positive doesn't matter here, it just results in the warning being displayed.
https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js#n... inject.preload.js:392: if (typeof sandbox != "string" || sandbox.includes("allow-scripts")) On 2017/05/30 10:20:31, kzar wrote: > On 2017/05/30 09:41:16, Sebastian Noack wrote: > > What if allow-scripts is misspelled like "allow-scriptss"? > > Well a false-positive doesn't matter here, it just results in the warning being > displayed. I wonder whether we even need this check. If scripts are not allowed this code won't run anyway, right?
https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js#n... inject.preload.js:392: if (typeof sandbox != "string" || sandbox.includes("allow-scripts")) On 2017/05/30 10:36:46, Sebastian Noack wrote: > On 2017/05/30 10:20:31, kzar wrote: > > On 2017/05/30 09:41:16, Sebastian Noack wrote: > > > What if allow-scripts is misspelled like "allow-scriptss"? > > > > Well a false-positive doesn't matter here, it just results in the warning > being > > displayed. > > I wonder whether we even need this check. If scripts are not allowed this code > won't run anyway, right? Never mind. We are in the extension's content script here, I mistakenly assumed that this is part of the injected code. Anyway, what I was initially suggesting here, was to use /(^|\s)allow-scripts($|\s)/.test(sandbox), which would be a little more accurrate. But it's not too important. I leave it up to you. LGTM either way.
Patch Set 3 : Use regexp, ignore case https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29451568/diff/29451569/inject.preload.js#n... inject.preload.js:392: if (typeof sandbox != "string" || sandbox.includes("allow-scripts")) On 2017/05/30 10:43:34, Sebastian Noack wrote: > Anyway, what I was initially suggesting here, was to use > /(^|\s)allow-scripts($|\s)/.test(sandbox), which would be a little more > accurrate. But it's not too important. I leave it up to you. LGTM either way. Doing some further reading I realised that window.frameElement.sandbox.contains("allow-scripts") would be even better. But then I realised DOMTokenList is case sensitive, so while "ALLOW-SCRIPTS" would work, we wouldn't detect it! This was really bad, giving websites a way to circumvent our wrappers. So instead I've gone with your approach, adding the case insensitive flag. Done.
LGTM 
|
