Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
239143009:
Disallow top-level navigation in gaia iframe (Closed)
|
Created:
6 years, 8 months ago by guohui Modified:
6 years, 8 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDisallow top-level navigation in gaia iframe
BUG=364100
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264672
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264758
Patch Set 1 #Patch Set 2 : allow-same-origin #Patch Set 3 : adds browsertest #
Total comments: 6
Patch Set 4 : real test #Patch Set 5 : fix broken cros bot? #Patch Set 6 : undo cros change #
Total comments: 3
Patch Set 7 : fix racing #Patch Set 8 : allows popup #Patch Set 9 : allows pointer lock api #Patch Set 10 : disable flaky test #
Messages
Total messages: 32 (0 generated)
Lgtm Thanks, Roger On Apr 16, 2014 6:02 PM, <guohui@chromium.org> wrote: > Reviewers: Charlie Reis, dcheng, Roger Tawa, > > Description: > Disallow top-level navigation in gaia iframe > > BUG=364100 > > Please review this at https://codereview.chromium.org/239143009/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files (+2, -1 lines): > M chrome/browser/resources/gaia_auth/main.html > > > Index: chrome/browser/resources/gaia_auth/main.html > diff --git a/chrome/browser/resources/gaia_auth/main.html > b/chrome/browser/resources/gaia_auth/main.html > index 57e14f3b273f2d2846f2b5366d09a63606b8ea45.. > 0fa9d7398331777ae185904dc7674716d4f78390 100644 > --- a/chrome/browser/resources/gaia_auth/main.html > +++ b/chrome/browser/resources/gaia_auth/main.html > @@ -8,6 +8,7 @@ > <script src="main.js"></script> > </head> > <body> > - <iframe id="gaia-frame" name="gaia-frame" src="about:blank" > frameborder="0"></iframe> > + <iframe id="gaia-frame" name="gaia-frame" src="about:blank" > frameborder="0" > + sandbox="allow-scripts allow-forms"></iframe> > </body> > </html> > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM for preventing framebusting, though please also wait for Daniel to confirm.
This looks right, but how hard would it be to add a browser test for this?
On 2014/04/16 22:23:17, dcheng wrote: > This looks right, but how hard would it be to add a browser test for this? +1
Seems that "allow-same-origin" is needed for Cros login. Uncaught SecurityError: Failed to set the 'cookie' property on 'Document': The document is sandboxed and lacks the 'allow-same-origin' flag. accounts.youtube.com/accounts/CheckConnection?pmpo=https%3A%2F%2Faccounts.google.com&v=2058555526×tamp=1397687539752:9 X
thanks a lot xiyuan, fixed in the patch 2 as suggested. will add test tmrw morning.
lgtm
Added browsertest. Please take another look. Thanks!
https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:178: ui_test_utils::NavigateToURL(browser(), url); NavigateToURL waits for one LOAD_STOP notification. Would it reflect the loading state in iframe? If yes, would it be good enough for the deframe script to run and finish?
Hmm, this test doesn't fail when the fix is removed, at least on my local machine. https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:181: EXPECT_EQ(url, contents->GetURL()); GetLastCommittedURL() https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:184: EXPECT_TRUE(controller.GetPendingEntry() == NULL); This doesn't seem like a sufficient check to me. We could have committed the deframe navigation and this would pass. Can you also check that GetLastCommittedURL() is what we expect?
thanks charlie for catching it. It did fail without fix on my machine once, but i could not repro it, guess some racing condition... Them problem was chrome signin page has two layers of iframe, a gaia auth extension iframe, which in turn embeds a gaia web page, both iframes are initially set to the blank page. As xiyuan said, NavigateToURL waits for one loadstop, thus at the end of it, the gaia auth extension iframe is still blank, and the gaia web page iframe does not exist yet, thus the test trivially passes without my fix. The same issue applies to another existing test, InlineLoginUISafeIframeBrowserTest.NoWebUIInIframe. Fixed in a new patch by blocking until the web auth UI is ready. Verified w/o the patch that both EXPECT* checks would fail. Please take another look, thanks! https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:178: ui_test_utils::NavigateToURL(browser(), url); On 2014/04/17 01:15:29, xiyuan (OOO Apr 17-18) wrote: > NavigateToURL waits for one LOAD_STOP notification. Would it reflect the loading > state in iframe? If yes, would it be good enough for the deframe script to run > and finish? it does, but the iframe is initially set to blank page, thus the test passes without the fix in this CL... Corrected in a new patch. https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:181: EXPECT_EQ(url, contents->GetURL()); On 2014/04/17 01:20:20, Charlie Reis wrote: > GetLastCommittedURL() GetLastCommittedURL returns the committed url, but in this case, even if top frame navigation is allowed, the navigation would be pending, thus GetLastCommittedURL would always return the old URL. Verified without the fix in a local build. Changed to GetVisibleURL. https://codereview.chromium.org/239143009/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:184: EXPECT_TRUE(controller.GetPendingEntry() == NULL); On 2014/04/17 01:20:20, Charlie Reis wrote: > This doesn't seem like a sufficient check to me. We could have committed the > deframe navigation and this would pass. Can you also check that > GetLastCommittedURL() is what we expect? After i added the WaitForUIReady call above, both the GetVisibleURL check and GetPendingEntry check fail without my fix.
in patch 5, i made a fix for the broken cros bot which requires slight changes to cros logic. Since xiyuan is OOO, and I have no cros box to test the fix, and the CL would be merged into stable, thus to minimize the risk, i reverted the cros change and disabled the WaitForUIReady function on cros. Please take another look, i would like to submit this CL asap.
https://codereview.chromium.org/239143009/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/239143009/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:142: ASSERT_TRUE(message_queue.WaitForMessage(&message)); This is could timeout when 'ready' event is already fired. We probably need to keep the 'ready' state in GaiaAuthHost and check the state also in addition to the event listener. This might be the case on cros.
https://codereview.chromium.org/239143009/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/239143009/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:142: ASSERT_TRUE(message_queue.WaitForMessage(&message)); On 2014/04/17 15:04:15, xiyuan (OOO Apr 17-18) wrote: > This is could timeout when 'ready' event is already fired. We probably need to > keep the 'ready' state in GaiaAuthHost and check the state also in addition to > the event listener. > > This might be the case on cros. it seems on cros ready event is fired upon receiving clearOldAttempts message from the gaia page, while on desktop ready event if fired upon load complete of the gaia page. To fix the test for cros, i need to change cros code. As mentioned in an earlier comment, to minimize the risk, i prefer to disable WaitUntilUIReady for cros, and fix it in a separate CL.
https://codereview.chromium.org/239143009/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc (right): https://codereview.chromium.org/239143009/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc:142: ASSERT_TRUE(message_queue.WaitForMessage(&message)); On 2014/04/17 15:08:31, guohui wrote: > On 2014/04/17 15:04:15, xiyuan (OOO Apr 17-18) wrote: > > This is could timeout when 'ready' event is already fired. We probably need to > > keep the 'ready' state in GaiaAuthHost and check the state also in addition to > > the event listener. > > > > This might be the case on cros. > > it seems on cros ready event is fired upon receiving clearOldAttempts message > from the gaia page, while on desktop ready event if fired upon load complete of > the gaia page. To fix the test for cros, i need to change cros code. As > mentioned in an earlier comment, to minimize the risk, i prefer to disable > WaitUntilUIReady for cros, and fix it in a separate CL. > as clarified over chat, xiyuan is right, on desktop this may timeout if ready event is already fired. Fixed in a new patch.
LGTM!
Thanks for updating the test. LGTM
The CQ bit was checked by guohui@chromium.org
The CQ bit was unchecked by guohui@chromium.org
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/239143009/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_x64_rel
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/239143009/220001
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/239143009/240001
Message was sent while issue was closed.
Change committed as 264672
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/239143009/280001
Message was sent while issue was closed.
Change committed as 264758 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
