FrameLoader::StartNavigation should not allow emulated Ctrl+click from sandboxed iframes [chromium/src : main]
Nate Chapin (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
I have, thus far, not gotten a unit test to actually tickle this case. NavigationPolicy is surprisingly hard to test programmatically.
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
I have, thus far, not gotten a unit test to actually tickle this case. NavigationPolicy is surprisingly hard to test programmatically.
Would a WPT work for this? I guess it'd be kind of hard to detect that it was blocked though.
Related details
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
Daniel ChengI have, thus far, not gotten a unit test to actually tickle this case. NavigationPolicy is surprisingly hard to test programmatically.
Would a WPT work for this? I guess it'd be kind of hard to detect that it was blocked though.
I got a (weird and invasive) unit test working that shows that BeginNavigation() doesn't get called in the problematic case. Is that worth landing?
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
network::mojom::blink::WebSandboxFlags::kPopups);Why do we need to set this separately? It seems a bit weird to set sandbox flags after the page is already loaded.
To<HTMLElement>(element)->click();Since the test is split up between this and the html file, would it make sense to add a comment here that this shouldn't result in an additional navigation due to the iframe being sandboxed without allow-poups?
EXPECT_EQ(web_frame_client.iframe_client()->BeginNavigationCallCount(), 1);This is just from the initial load right, or something else as well? Would it make sense to EXPECT this after line 14883 as well?
Related details
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
network::mojom::blink::WebSandboxFlags::kPopups);Why do we need to set this separately? It seems a bit weird to set sandbox flags after the page is already loaded.
For some reason the sandbox flags aren't getting propagated to the about:srcdoc document (but they are in a full chrome build). I think there's something I'm not stubbing out, but I haven't been able to figure out what it is. How hard should I try? (I was several hours in when I just set the sandbox flags directly)
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 2 comments![]( "Open in Gerrit")
Since the test is split up between this and the html file, would it make sense to add a comment here that this shouldn't result in an additional navigation due to the iframe being sandboxed without allow-poups?
Done
EXPECT_EQ(web_frame_client.iframe_client()->BeginNavigationCallCount(), 1);This is just from the initial load right, or something else as well? Would it make sense to EXPECT this after line 14883 as well?
Done
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
network::mojom::blink::WebSandboxFlags::kPopups);Nate ChapinWhy do we need to set this separately? It seems a bit weird to set sandbox flags after the page is already loaded.
For some reason the sandbox flags aren't getting propagated to the about:srcdoc document (but they are in a full chrome build). I think there's something I'm not stubbing out, but I haven't been able to figure out what it is. How hard should I try? (I was several hours in when I just set the sandbox flags directly)
Maybe just throw a `// TODO` here for now? It'd be nice to fix... but I can't also claim that this is the best use of anyone's time at this point either.
Related details
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
network::mojom::blink::WebSandboxFlags::kPopups);Nate ChapinWhy do we need to set this separately? It seems a bit weird to set sandbox flags after the page is already loaded.
Daniel ChengFor some reason the sandbox flags aren't getting propagated to the about:srcdoc document (but they are in a full chrome build). I think there's something I'm not stubbing out, but I haven't been able to figure out what it is. How hard should I try? (I was several hours in when I just set the sandbox flags directly)
Maybe just throw a `// TODO` here for now? It'd be nice to fix... but I can't also claim that this is the best use of anyone's time at this point either.
You can attach my name I suppose :)