Attention is currently required from: Arthur Sonzogni.
Yifan Luo would like Arthur Sonzogni to review this change.
[Anonymous iframe] Disable autofill
Bug: 1233858
Change-Id: Ibd5fa303b93b88092a0f335b64b7bd41395a4d94
---
M components/autofill/content/renderer/autofill_agent.cc
M third_party/blink/public/web/web_node.h
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/exported/web_node.cc
M third_party/blink/renderer/core/html/html_iframe_element.h
5 files changed, 9 insertions(+), 1 deletion(-)
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Arthur Sonzogni.
1 comment:
Patchset:
Would you mind have a look?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Yifan Luo.
Yifan Luo has uploaded this change for review.
[Anonymous iframe] Disable autofill
Bug: 1233858
Change-Id: Ibd5fa303b93b88092a0f335b64b7bd41395a4d94
---
M components/autofill/content/renderer/autofill_agent.cc
M third_party/blink/public/web/web_node.h
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/exported/web_node.cc
M third_party/blink/renderer/core/html/html_iframe_element.h
5 files changed, 9 insertions(+), 1 deletion(-)
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Yifan Luo.
1 comment:
Patchset:
Thanks!
The current prototype is made from the renderer process. From there you can't easily know if a document is anonymous of not. Indeed, it depends on the whole chain of ancestor (anonymous iframe propagate to every nested iframe) and timing (anonymous attribute it taken into account at the beginning of the navigation that created the document). Currently the document's anonymousness isn't known to the renderer process.
For instance with:
```
<iframe anonymous=true>
#document (1)
<iframe>
#document (2)
</iframe>
</iframe>
```
This patch won't disable autofill for (2) wrongly.
I was hoping we could hook into the the locations in the browser process where we provide the data and propose a design doc to the Autofill team.
The AutofillDriver looked promising, especially everywhere where they look for Incognito mode.
```
// Interface that allows Autofill core code to interact with its driver (i.e.,
// obtain information from it and give information to it). A concrete
// implementation must be provided by the driver.
class AutofillDriver {
```
I am going to send an email to Folks working on Autofill in Munich. I am going to ask them what they believe would be the best way to achieve this, so that we can more easily write a design doc about this.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Arthur Sonzogni.
1 comment:
Patchset:
Thanks! […]
Have you sent the Email? Would you mind cc me on the thread?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Yifan Luo.
1 comment:
Patchset:
Thanks!
(Removing this from my visibility list), until this got updated. I would like to use the "anonymous" flags from the document, not the one from iframe attribute.
This would require propagating to the renderer process the RenderFrameHost::anonymous_ flags toward RenderFrame::anonymous_, via the CommitNavigation IPC.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
2 comments:
Patchset:
Hey Arthur! I got a problem with the test. Could you please help?
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc:
Patch Set #18, Line 4028: ->FirstChild()
The FirstChild here seems to be nullptr. Will you happened to know why?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc:
Patch Set #18, Line 4028: ->FirstChild()
The FirstChild here seems to be nullptr. […]
I think "LoadHTML" simulate a navigation, by creating a URLLoaderClient able to retrieve the resource and pass it to the document by manually calling CommitNavigation.
However, after that, blink needs to retrieve the content, parse the HTML, parse the script, execute the script. This takes time and the tasks are posted into a queue, but not yet executed when you call GetMainFrame()->FirstFrame().
So, this would require to wait for those tasks to happen first.
However there is a second problem. This test is running inside the renderer process and "simulate" what the browser process would have done. However it is very incomplete and the logic about anonymous iframe isn't execute. It means in your test, the document inside the anonymous iframe won't appear as anonymous.
I think it would be preferable to run a test with both the browser process and the renderer process running.
Maybe you could find an example to adapt from:
chrome/browser/password_manager/password_manager_browsertest.cc
?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
1 comment:
Patchset:
@arthur You might want to have a look when you got free time?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
File content/browser/renderer_host/render_frame_host_impl.cc:
Patch Set #21, Line 7431: commit_params->anonymous = anonymous_;
I wonder how this can work, since `anonymous_` knows whether the *current* document is anonymous, while `commit_params->anonymous` wants to know whether the document to commit should be anonymous. I think you should use NavigationRequest::anonumous() to populate this field instead.
Also, the fact that the CQ passes with this means that we probably need more tests for this.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.cc:
Patch Set #21, Line 7431: commit_params->anonymous = anonymous_;
I wonder how this can work, since `anonymous_` knows whether the *current* document is anonymous, wh […]
I thought the document's anonymous or not is base on the status of current frame which is the document commit to?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Yifan Luo.
1 comment:
File content/browser/renderer_host/render_frame_host_impl.cc:
Patch Set #21, Line 7431: commit_params->anonymous = anonymous_;
I thought the document's anonymous or not is base on the status of current frame which is the docume […]
The RenderFrameHost is currently rendering the previous document. It might be that the previous document was not anonymous. But if someone set the anonymous flag on the iframe element since then, the document-to-commit should become anonymous.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Yifan Luo.
10 comments:
Patchset:
Thanks!
I let some comments below:
File chrome/browser/password_manager/password_manager_browsertest.cc:
IN_PROC_BROWSER_TEST_F(PasswordManagerPrerenderBrowserTest,
DisableOnAnonymousIframe
Maybe we should create the iframe without anonymous first, check credentials are populate. Then flip iframe.anonymous and check they aren't populate this time.
// Create an iframe and navigate cross-site.
NavigationObserver iframe_observer(WebContents());
iframe_observer.SetPathToWaitFor("/password/crossite_iframe_content.html");
GURL iframe_url = embedded_test_server()->GetURL(
"abc.foo.com", "/password/crossite_iframe_content.html");
std::string create_iframe =
base::StringPrintf("create_iframe('%s');", iframe_url.spec().c_str());
ASSERT_TRUE(content::ExecuteScriptWithoutUserGesture(RenderFrameHost(),
create_iframe));
iframe_observer.Wait();
Is this step needed to store credentials, or it can be removed?
Patch Set #21, Line 4728: ExecuteScriptWithoutUserGesture
Deprecated (see below)
Patch Set #21, Line 4739: ExecuteScriptWithoutUserGestureAndExtractString
This is deprecated:
https://source.chromium.org/chromium/chromium/src/+/main:content/public/test/browser_test_utils.h;l=513
Noawadays, we should use content::EvalJs / ExecJs / JsReplace / etc...
File chrome/test/data/password/password_form_in_anonymous_iframe.html:
Patch Set #21, Line 22: window.addEventListener("message", receiveMessage, false);
This test you copy-pasted is very old. It is doing complicated logic with postMessage / domAutomationController, because it doesn't know how to get access to the RenderFrameHost for the iframe outside of content.
That's actually quite easy:
```
ASSERT_EQUALS(2u, WebContent()->GetAllFrames().size();
RenderFrameHost top_level_document = WebContents()->GetAllFrames()[0];
RenderFrameHost anonymous_document = WebContents()->GetAllFrames()[1];
```
And then execute directly anything you want like querying the "username" value:
```
EXPECT_EQ("temp", content::EvalJs(anonymous_document, "document.getElementById("username").value"));
```
<html>
<body id="inParent">
<script>
function receiveMessage(event) {
window.domAutomationController.send(event.data);
}
function sendMessage(msg) {
document.getElementById("iframe").contentWindow.postMessage(msg,"*");
}
function create_iframe(src) {
var ifrm = document.createElement("IFRAME");
ifrm.anonymous = true;
ifrm.setAttribute("id", "iframe");
ifrm.setAttribute("name", "iframe");
ifrm.setAttribute("src", src);
document.body.appendChild(ifrm);
}
window.addEventListener("message", receiveMessage, false);
</script>
</body>
</html>
Instead of creating a new page. What about loading "/empty.html" and inject any script you want like:
```
content::EvalJs(render_frame_host, JsReplace(R"(
const iframe = document.createElement("iframe");
iframe.anonymous = true;
iframe.src = $1;
document.body.appendChild(iframe);
)", iframe_url);
```
File content/browser/renderer_host/render_frame_host_impl.cc:
Patch Set #21, Line 7431: commit_params->anonymous = anonymous_;
The RenderFrameHost is currently rendering the previous document. […]
+1
Yes, this should be instead:
```
commit_params->anonymous = navigation_request->anonymous()
```
File content/public/browser/render_frame_host.h:
// Returns whether this is an anonymous iframe or not.
virtual bool anonymous() = 0;
I don't think this is used outside of content/. So this shouldn't be exposed to embedders.
https://source.chromium.org/chromium/chromium/src/+/main:content/public/README.md
```
Only expose methods in the public API that embedders need. If a method is only used by other code in content, it belongs in foo_impl.h and not foo.h.
```
File third_party/blink/renderer/core/dom/document.h:
Patch Set #21, Line 2301: bool anonymous_ = false;
Can this be:
```
const bool anonymous_;
```
instead, to ensure this is a constant property of the document?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
6 comments:
File chrome/browser/password_manager/password_manager_browsertest.cc:
// Create an iframe and navigate cross-site.
NavigationObserver iframe_observer(WebContents());
iframe_observer.SetPathToWaitFor("/password/crossite_iframe_content.html");
GURL iframe_url = embedded_test_server()->GetURL(
"abc.foo.com", "/password/crossite_iframe_content.html");
std::string create_iframe =
base::StringPrintf("create_iframe('%s');", iframe_url.spec().c_str());
ASSERT_TRUE(content::ExecuteScriptWithoutUserGesture(RenderFrameHost(),
create_iframe));
iframe_observer.Wait();
Is this step needed to store credentials, or it can be removed?
I fined it one way previous tests used to add credentials. There might be easier way, let me see.
Patch Set #21, Line 4728: ExecuteScriptWithoutUserGesture
Deprecated (see below)
Done
Patch Set #21, Line 4739: ExecuteScriptWithoutUserGestureAndExtractString
This is deprecated: […]
Done
File content/browser/renderer_host/render_frame_host_impl.cc:
Patch Set #21, Line 7431: commit_params->anonymous = anonymous_;
+1 […]
Done
File content/public/browser/render_frame_host.h:
// Returns whether this is an anonymous iframe or not.
virtual bool anonymous() = 0;
I don't think this is used outside of content/. So this shouldn't be exposed to embedders. […]
Done
File third_party/blink/renderer/core/dom/document.h:
Patch Set #21, Line 2301: bool anonymous_ = false;
Can this be: […]
Done
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Arthur Sonzogni, Yifan Luo.
7 comments:
Patchset:
I would suggest to split this CL into two parts:
1) sending the `anonymous` bit to the renderer
2) the password autofill logic
Since we want anyway to expose to the web platform whether the document is anonymous (with document::anonymous), I would implement that in 1), too (behind the feature flag). That would allow us to test that the anonymous flag in blink and in the browser always agree - which I fear is not that easy and is not yet the case with this change. For doing that, I would go through all browsertests for anonymous iframes and add checks like
EXPECT_EQ(rfh->anonymous(), EvalJs(rfh, "document.anonymous()"))
File chrome/browser/password_manager/password_manager_browsertest.cc:
Patch Set #23, Line 4732: EXPECT_TRUE(content::EvalJs(RenderFrameHost(),
why EvalJs and not ExecJs? What value is this returning?
Patch Set #23, Line 4738: )",iframe_url),content::EXECUTE_SCRIPT_NO_USER_GESTURE)
please run `git cl format` before uploading
Patch Set #23, Line 4762: have not been
"have been"
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #23, Line 1786: bool anonymous();
could you revert this change since this is not `override` anymore?
File content/renderer/render_frame_impl.cc:
Patch Set #23, Line 2935: navigation_params->anonymous = commit_params->anonymous;
I fear there are more places where NavigationParams are created. See this CL https://chromium-review.googlesource.com/c/chromium/src/+/3003365 and in particular this line https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/web_local_frame_impl.cc;l=2363;drc=3095f0ce2cadab0ffa2d0ae922ad7dffd3eeda30.
File third_party/blink/public/mojom/navigation/navigation_params.mojom:
Patch Set #23, Line 484: // Whether this is an anonymous iframe or not.
I think "Whether this navigation will commit an anonymous document" is more clear
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
Patch set 23:Code-Review +1
3 comments:
File chrome/browser/password_manager/password_manager_browsertest.cc:
Patch Set #23, Line 4762: have not been
"have been"
Done
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #23, Line 1786: bool anonymous();
could you revert this change since this is not `override` anymore?
The diff tool is truly confused. I actually removed `override` in this patch other than added it.
File third_party/blink/public/mojom/navigation/navigation_params.mojom:
Patch Set #23, Line 484: // Whether this is an anonymous iframe or not.
I think "Whether this navigation will commit an anonymous document" is more clear
Done
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori.
5 comments:
File chrome/browser/password_manager/password_manager_browsertest.cc:
Patch Set #23, Line 4701: EvalJs
ExecJs? Since you don't rely on the result?
You can also remove
```
,content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractBool());
```
// Create an normal iframe and navigate cross-site.
NavigationObserver iframe_observer(WebContents());
iframe_observer.SetPathToWaitFor("/password/crossite_iframe_content.html");
GURL iframe_url = embedded_test_server()->GetURL(
"www.bar.com", "/password/crossite_iframe_content.html");
EXPECT_TRUE(content::EvalJs(RenderFrameHost(),
content::JsReplace(R"(
const iframe = document.createElement("iframe");
iframe.id = "normal";
iframe.src = $1;
document.body.appendChild(iframe);
)",iframe_url),content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractBool());
iframe_observer.Wait();
Why do you need to make one navigation in the iframe exactly?
Can this step be removed, so that we can proceed directly by "Store a password for autofill later"?
EXPECT_TRUE(content::ExecJs(
normal_document,
"var usernameRect = "
"document.getElementById('username').getBoundingClientRect();",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
int normal_y = content::EvalJs(RenderFrameHost(),
"usernameRect.top + usernameRect.bottom;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
int normal_x = content::EvalJs(RenderFrameHost(),
"usernameRect.left + usernameRect.right;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
content::SimulateMouseClickAt(WebContents(), 0,
blink::WebMouseEvent::Button::kLeft,
gfx::Point(normal_x / 2, normal_y / 2));
Maybe:
```
content::SimulateMouseClickOrTapElementWithId(WebContents(),"username");
```
EXPECT_TRUE(content::ExecJs(
anonymous_document,
"var usernameRect = "
"document.getElementById('username').getBoundingClientRect();",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
int y = content::EvalJs(RenderFrameHost(),
"usernameRect.top + usernameRect.bottom;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
int x = content::EvalJs(RenderFrameHost(),
"usernameRect.left + usernameRect.right;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
content::SimulateMouseClickAt(WebContents(), 0,
blink::WebMouseEvent::Button::kLeft,
gfx::Point(x / 2, y / 2));
Same here.
File third_party/blink/renderer/core/dom/document_init.h:
"loaded"
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Yifan Luo.
2 comments:
Patchset:
I would suggest to split this CL into two parts: […]
Unresolving to make this visible.
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #23, Line 1786: bool anonymous();
The diff tool is truly confused. I actually removed `override` in this patch other than added it.
Yes. I meant, can you inline again this getter?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
Patch set 24:-Commit-Queue
3 comments:
File chrome/browser/password_manager/password_manager_browsertest.cc:
EXPECT_TRUE(content::ExecJs(
normal_document,
"var usernameRect = "
"document.getElementById('username').getBoundingClientRect();",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
int normal_y = content::EvalJs(RenderFrameHost(),
"usernameRect.top + usernameRect.bottom;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
int normal_x = content::EvalJs(RenderFrameHost(),
"usernameRect.left + usernameRect.right;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
content::SimulateMouseClickAt(WebContents(), 0,
blink::WebMouseEvent::Button::kLeft,
gfx::Point(normal_x / 2, normal_y / 2));
Maybe: […]
Looks like we this function cannot successfully get the element from iframes.
File content/browser/renderer_host/render_frame_host_impl.h:
Patch Set #23, Line 1786: bool anonymous();
Yes. […]
Done
File third_party/blink/renderer/core/dom/document_init.h:
"loaded"
Done
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
Yifan Luo removed a vote from this change.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
Yifan Luo removed a vote from this change.
Attention is currently required from: Antonio Sartori, Yifan Luo.
15 comments:
Commit Message:
Could you put a brief description of your patch and reference your design doc?
https://docs.google.com/document/d/1_EqoEtozhMjsgfB3AWVRV4jAhTqD79JF6KH5AvGvIKo
Since your doc was created with @google.com, you should probably create a copy from your @chromium.org address and use this link instead.
Patchset:
Thanks!
File chrome/browser/password_manager/password_manager_browsertest.cc:
IN_PROC_BROWSER_TEST_F(PasswordManagerPrerenderBrowserTest,
DisableOnAnonymousIframe
Maybe we should create the iframe without anonymous first, check credentials are populate. […]
(Addressed)
// Create an iframe and navigate cross-site.
NavigationObserver iframe_observer(WebContents());
iframe_observer.SetPathToWaitFor("/password/crossite_iframe_content.html");
GURL iframe_url = embedded_test_server()->GetURL(
"abc.foo.com", "/password/crossite_iframe_content.html");
std::string create_iframe =
base::StringPrintf("create_iframe('%s');", iframe_url.spec().c_str());
ASSERT_TRUE(content::ExecuteScriptWithoutUserGesture(RenderFrameHost(),
create_iframe));
iframe_observer.Wait();
I fined it one way previous tests used to add credentials. There might be easier way, let me see.
(waiting for this comment to be resolved, one way or another)
File chrome/browser/password_manager/password_manager_browsertest.cc:
EXPECT_TRUE(content::ExecJs(
normal_document,
"var usernameRect = "
"document.getElementById('username').getBoundingClientRect();",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
int normal_y = content::EvalJs(RenderFrameHost(),
"usernameRect.top + usernameRect.bottom;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
int normal_x = content::EvalJs(RenderFrameHost(),
"usernameRect.left + usernameRect.right;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
content::SimulateMouseClickAt(WebContents(), 0,
blink::WebMouseEvent::Button::kLeft,
gfx::Point(normal_x / 2, normal_y / 2));
Looks like we this function cannot successfully get the element from iframes.
Indeed! I don't see any API taking a RFH instead of a WebContent.
I guess you can keep your code.
File chrome/browser/password_manager/password_manager_browsertest.cc:
Patch Set #24, Line 4691: www.foo.com
Sleevi@ tolkd me using the .test top-level domain is preferable.
What about replacing "www.foo.com" and "www.abc.com", with:
Patch Set #24, Line 4702: create_iframe
If you address one of my previous comment about not adding:
file password_form_in_anonymous_iframe.html
Then, you won't have access to "create_iframe".
So you can add iframe using:
```
EXPECT_TRUE(content::ExecJs(RenderFrameHost(), content::JsReplace(R"(
const iframe = document.createElement("iframe");
iframe.src = $1;
document.body.appendChild(iframe);
)", iframe_url)));
```
Patch Set #24, Line 4703: .spec().c_str()
This can be omitted.
std::string submit =
"document.getElementById('username').value = 'temp';"
"document.getElementById('password').value = 'pa55w0rd';"
"document.getElementById('submit_button').click();";
ASSERT_TRUE(content::ExecJs(iframe_document, submit));
Maybe submit can be inlined?
```
ASSERT_TRUE(content::ExecJs(iframe_document, R"(
document.getElementById('username').value = 'temp';
document.getElementById('password').value = 'pa55w0rd';
document.getElementById('submit_button').click();
)"));
```
Previously, we were using "*Wait*ForElementValue".
This means, password auto filling may take some time before writing the value.
Here, this test check they are empty immediately. They may be empty because it did not have time to fill. So it means this may not break if your implementation is removed.
I think you should wait some time before checking the value to ensure the test is not passing because of this.
```
// Let autofill some time to potentially fill the element's values.
EXPECT_TRUE(content::ExecJs(
WebContents(), "new Promise(resolve => setTimeout(resolve, 1000)"));
```
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc:
Patch Set #18, Line 4028: ->FirstChild()
I think "LoadHTML" simulate a navigation, by creating a URLLoaderClient able to retrieve the resourc […]
(Addressed)
File chrome/test/data/password/password_form_in_anonymous_iframe.html:
Patch Set #21, Line 22: window.addEventListener("message", receiveMessage, false);
This test you copy-pasted is very old. […]
(Addressed)
<html>
<body id="inParent">
<script>
function receiveMessage(event) {
window.domAutomationController.send(event.data);
}
function sendMessage(msg) {
document.getElementById("iframe").contentWindow.postMessage(msg,"*");
}
function create_iframe(src) {
var ifrm = document.createElement("IFRAME");
ifrm.anonymous = true;
ifrm.setAttribute("id", "iframe");
ifrm.setAttribute("name", "iframe");
ifrm.setAttribute("src", src);
document.body.appendChild(ifrm);
}
window.addEventListener("message", receiveMessage, false);
</script>
</body>
</html>
Instead of creating a new page. What about loading "/empty. […]
Ack
<html>
<body id="inParent">
<script>
function receiveMessage(event) {
window.domAutomationController.send(event.data);
}
function sendMessage(msg) {
document.getElementById("iframe").contentWindow.postMessage(msg,"*");
}
function create_iframe(src) {
var ifrm = document.createElement("IFRAME");
ifrm.anonymous = true;
ifrm.setAttribute("id", "iframe");
ifrm.setAttribute("name", "iframe");
ifrm.setAttribute("src", src);
document.body.appendChild(ifrm);
}
window.addEventListener("message", receiveMessage, false);
</script>
</body>
</html>
Instead of creating a new page. What about loading "/empty. […]
(waiting for this to be addressed)
File third_party/blink/renderer/core/loader/document_loader.h:
Patch Set #24, Line 664: bool anonymous_ = false;
I am wondering why we do not trigger the ASSERT:
```
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/document_loader.cc;l=325;bpv=0;bpt=0
```
Which should be a warning for develloper:
```
// Asserts size of DocumentLoader, so that whenever a new attribute is added to
// DocumentLoader, the assert will fail. When hitting this assert failure,
// please ensure that the attribute is copied correctly (if appropriate) in
// DocumentLoader::CreateWebNavigationParamsToCloneDocument().
ASSERT_SIZE(DocumentLoader, SameSizeAsDocumentLoader);
```
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Yifan Luo.
Yifan Luo uploaded patch set #26 to this change.
[Anonymous iframe] Disable autofill
This CL aimed to disable username and password autofill for
anonymous iframe. For further info, please follow the design
doc:
https://docs.google.com/document/d/1UHEC10kjxfgNmt-ejGkuxgvv63_-dsEH-DhVF_Sthak/edit?usp=sharing
Bug: 1233858
Change-Id: Ibd5fa303b93b88092a0f335b64b7bd41395a4d94
---
M chrome/browser/password_manager/password_manager_browsertest.cc
A chrome/test/data/password/password_form_in_anonymous_iframe.html
M components/autofill/content/renderer/password_autofill_agent.cc
M content/browser/renderer_host/navigation_controller_impl.cc
M content/browser/renderer_host/navigation_entry_impl.cc
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/render_frame_host_impl.cc
M content/browser/renderer_host/render_frame_host_impl.h
M content/renderer/render_frame_impl.cc
M third_party/blink/public/mojom/navigation/navigation_params.mojom
M third_party/blink/public/web/web_document.h
M third_party/blink/public/web/web_navigation_params.h
M third_party/blink/renderer/core/dom/document.h
M third_party/blink/renderer/core/dom/document_init.cc
M third_party/blink/renderer/core/dom/document_init.h
M third_party/blink/renderer/core/exported/web_document.cc
M third_party/blink/renderer/core/loader/document_loader.cc
M third_party/blink/renderer/core/loader/document_loader.h
18 files changed, 180 insertions(+), 9 deletions(-)
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
Patch set 27:Commit-Queue +1
6 comments:
Commit Message:
Could you put a brief description of your patch and reference your design doc? […]
Done
File chrome/browser/password_manager/password_manager_browsertest.cc:
EXPECT_TRUE(content::ExecJs(
normal_document,
"var usernameRect = "
"document.getElementById('username').getBoundingClientRect();",
content::EXECUTE_SCRIPT_NO_USER_GESTURE));
int normal_y = content::EvalJs(RenderFrameHost(),
"usernameRect.top + usernameRect.bottom;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
int normal_x = content::EvalJs(RenderFrameHost(),
"usernameRect.left + usernameRect.right;",
content::EXECUTE_SCRIPT_NO_USER_GESTURE)
.ExtractInt();
content::SimulateMouseClickAt(WebContents(), 0,
blink::WebMouseEvent::Button::kLeft,
gfx::Point(normal_x / 2, normal_y / 2));
Indeed! I don't see any API taking a RFH instead of a WebContent. […]
Done
File chrome/browser/password_manager/password_manager_browsertest.cc:
Sleevi@ tolkd me using the .test top-level domain is preferable. […]
Done
Patch Set #24, Line 4702: create_iframe
If you address one of my previous comment about not adding: […]
I would argue for a html file is better and more understandable than write the js three times in the .cc test file.
Patch Set #24, Line 4703: .spec().c_str()
This can be omitted.
Done
File third_party/blink/renderer/core/loader/document_loader.h:
Patch Set #24, Line 664: bool anonymous_ = false;
I am wondering why we do not trigger the ASSERT: […]
No idea. Added anonymous_ to SameSizeAsDocumentLoader
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Yifan Luo.
1 comment:
File third_party/blink/renderer/core/loader/document_loader.h:
Patch Set #24, Line 664: bool anonymous_ = false;
No idea. […]
This was added to force developers to think about propagating state from one Document to the other when cloned, because of XSLT or javascript-url document commit.
It looks like you ignored the warning above. Could you not, and instead propagate "anonymous" flag in DocumentLoader::CreateWebNavigationParamsToCloneDocument()
?
It would be good adding a test. However, we can do it in a followup. I opened a bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=1249063
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori.
Patch set 27:-Commit-Queue
3 comments:
Patchset:
Unresolving to make this visible.
Sorry, I've missed this comment. Sounds a right direction to seperate the CL and put it behind the flag with few questions: 1) Do we already have the flag in browser or I need to create a new one? 2) How many browser tests involved in it? Sounds like it would be better to have another seperate cl for the checking anonymous in the exist tests?
File chrome/browser/password_manager/password_manager_browsertest.cc:
Patch Set #23, Line 4732: EXPECT_TRUE(content::EvalJs(RenderFrameHost(),
why EvalJs and not ExecJs? What value is this returning?
Done
Patch Set #23, Line 4738: )",iframe_url),content::EXECUTE_SCRIPT_NO_USER_GESTURE)
please run `git cl format` before uploading
Done
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
1 comment:
File content/renderer/render_frame_impl.cc:
Patch Set #23, Line 2935: navigation_params->anonymous = commit_params->anonymous;
I fear there are more places where NavigationParams are created. […]
This particular code looks to be called by render_frame_impl.cc, btw anonymous in commit_params is default to be false which I do not see worries here
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Arthur Sonzogni, Yifan Luo.
1 comment:
Patchset:
Sorry, I've missed this comment. […]
1) We already have a flag https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=179;drc=1871bd3ba37b12c317f2088ad114236993af6e7d
2) We do have a few browser tests: https://source.chromium.org/search?q=anonymousiframe%20file:browsertest&sq=&ss=chromium. I am not sure about the separate CL.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Yifan Luo.
1 comment:
File content/renderer/render_frame_impl.cc:
Patch Set #23, Line 2935: navigation_params->anonymous = commit_params->anonymous;
This particular code looks to be called by render_frame_impl. […]
The autofill code is called only when the Document is associated with a RenderFrame.
IMO, the missing places where anonymous isn't properly plumbed are for the initial empty document and XSLT/Javascript cloning of the document.
So in:
1. FrameLoader::Init()
2. DocumentLoader::CreateWebNavigationParamsToCloneDocument()
I think you can let a TODO for (1), until we resolve the initial empty document case. However, you can already fix (2)
There are other document not associated with a RenderFrame. They don't matter for autofill. However, they matter for the future `document.credentialless`. We will have to find something in the future and keep this in mind.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori.
1 comment:
Patchset:
1) We already have a flag https://source.chromium. […]
I mean we might need a new flag in browser context in this case?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Yifan Luo.
1 comment:
Patchset:
I mean we might need a new flag in browser context in this case?
Why? The 'anonymous' attribute is parsed only if that flag is enabled. If we don't parse 'anonymous', we never set the 'anonymous' bit, so the browser code does nothing.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori.
1 comment:
Patchset:
Why? The 'anonymous' attribute is parsed only if that flag is enabled. […]
So what do you mean by "test that the anonymous flag in blink and in the browser always agree" here?
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Antonio Sartori, Arthur Sonzogni.
2 comments:
File chrome/browser/password_manager/password_manager_browsertest.cc:
Previously, we were using "*Wait*ForElementValue". […]
Done
File content/renderer/render_frame_impl.cc:
Patch Set #23, Line 2935: navigation_params->anonymous = commit_params->anonymous;
The autofill code is called only when the Document is associated with a RenderFrame. […]
I am already fixed (2) in the cl. I will add a TODO for FrameLoader::Init()
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Arthur Sonzogni, Yifan Luo.
1 comment:
Patchset:
So what do you mean by "test that the anonymous flag in blink and in the browser always agree" here?
Sorry, the "anonymous flag" should have been the "anonymous bit" or "anonymous attribute". I'd like to test that we always have `RenderFrameHostImpl::anonymous() == Document::anonymous()`
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Arthur Sonzogni, Yifan Luo.
2 comments:
Patchset:
I separated the CL. Would you mind move to https://chromium-review.googlesource.com/c/chromium/src/+/3161670?
File third_party/blink/renderer/core/loader/document_loader.h:
Patch Set #24, Line 664: bool anonymous_ = false;
This was added to force developers to think about propagating state from one Document to the other w […]
Done
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.
Yifan Luo abandoned this change.
To view, visit change 3093355. To unsubscribe, or for help writing mail filters, visit settings.