[APC][iOS] Implement same-site iframe extraction and cross-site redaction on iOS [chromium/src : main]
Fikre Mengistu (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle added 6 comments![]( "Open in Gerrit")
I feel this change works different from the parent change , is it possible ?
here the redaction is done native side whereas in the parent CL the redaction is done from the iframe metadata coming from the renderer
net::SchemefulSite main_frame_site(_webState->GetLastCommittedURL());I feel we should capture that when creating the wrapper to avoid toctou issues if navigation occurs during the async extraction
annotatedPageContentBarrier.Run();note: so we drop extracting cross site frames?, this is what I was proposing in the parent cl of this cl
node->mutable_content_attributes()->set_attribute_type(not 100% sure we want to move this here
this is usually a fix proposed by the AI when xframe registration fails
if (IsRefactored()) {note: I think we can clean up the refactored VS non-refactored versions
but let's keep the test variant here until the cleanup
// Subdomain (same-site) text SHOULD be extracted.I think it is the other way around
Related details
- Fikre Mengistu
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. |
Fikre Mengistu (Gerrit)
Fikre Mengistu added 6 comments![]( "Open in Gerrit")
I feel this change works different from the parent change , is it possible ?
here the redaction is done native side whereas in the parent CL the redaction is done from the iframe metadata coming from the renderer
This CL introduces native C++ pre-extraction cross origin gating in PageContextWrapper. This enables redactions and avoids executing heavy JS extraction on cross-site subframes that would ultimately be discarded. When blocked, their placeholder pins flow into Andrew's sweep where they are redacted with REASON_CROSS_SITE if the same_site_only flag is enabled.
net::SchemefulSite main_frame_site(_webState->GetLastCommittedURL());I feel we should capture that when creating the wrapper to avoid toctou issues if navigation occurs during the async extraction
Done
annotatedPageContentBarrier.Run();Fikre Mengistunote: so we drop extracting cross site frames?, this is what I was proposing in the parent cl of this cl
I believe it's fulfils what you are proposing the parent cl. This approach enables redactions while skipping unnecessary cross-site frame js extractions.
node->mutable_content_attributes()->set_attribute_type(not 100% sure we want to move this here
this is usually a fix proposed by the AI when xframe registration fails
Setting attribute_type = CONTENT_ATTRIBUTE_IFRAME prior to registration here was intended for apc v1. Even in V1 mode, cross-origin iframes emit placeholder tokens, so setting the attribute type before returning ensures those placeholder pins identify as iframes when Andrew's sweep checks them. There was some talk about keeping apc v1 around for feature that may want a more lightweight extraction, so I still think this is worth keeping. wyt?
if (IsRefactored()) {note: I think we can clean up the refactored VS non-refactored versions
but let's keep the test variant here until the cleanup
Ack.
// Subdomain (same-site) text SHOULD be extracted.I think it is the other way around
I believe this is correctly written. Because include_same_site_only allows same-site frames, the subdomain iframe shares the main frame's net::SchemefulSite and should have its text extracted. Conversely, the third-party cross-site iframe has a different site, so gating blocks it and its text should not be extracted.
Related details
- Nicolas MacBeth
- Vincent Boisselle
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. |
Vincent Boisselle (Gerrit)
Vincent Boisselle added 7 comments![]( "Open in Gerrit")
Fikre MengistuI feel this change works different from the parent change , is it possible ?
here the redaction is done native side whereas in the parent CL the redaction is done from the iframe metadata coming from the renderer
This CL introduces native C++ pre-extraction cross origin gating in PageContextWrapper. This enables redactions and avoids executing heavy JS extraction on cross-site subframes that would ultimately be discarded. When blocked, their placeholder pins flow into Andrew's sweep where they are redacted with REASON_CROSS_SITE if the same_site_only flag is enabled.
Ok so they sort of work in tandem ? we redact before extraction and also feed the redaction data from the renderer extraction but without actually extracting the content of that frame where we just want the "metadata"
// post-processing can determine if it is cross-site.add also a note that this data will be replaced with the full metadata when resolving the placeholders if the frame data isn't redacted
*iframe_data, destination_node, origin,C++ Style Improvement:
In Chromium C++, it is much more idiomatic and readable to use `base::NullCallback()` to generate empty callbacks:
```cpp
PopulateIframeData(*iframe_data, destination_node, origin, base::NullCallback());
```
net::SchemefulSite main_frame_site(main_frame_url);Missing Header Include:
You use `net::SchemefulSite` here, but this file is missing the explicit include for it. Relying on transitive includes can be fragile and might lead to future (or current) compilation errors.
Please add `#import "net/base/schemeful_site.h"` at the top of the file.
}TOCTOU & Potential Null Pointer Crash:
By querying `_webState->GetLastCommittedURL()` again here, you defeat the purpose of caching `_mainFrameSite` in the constructor (which was added specifically to prevent TOCTOU race conditions). Also, `_webState` is a `base::WeakPtr`, so dereferencing it using `->` inside this loop without first checking its validity can lead to a crash if the web state was destroyed during the asynchronous operations.
Use the cached `_mainFrameSite` instance variable instead, just as you successfully did in the refactored path loop earlier in the file:
```objc
if (_config->include_same_site_only()) {
net::SchemefulSite child_site(webFrame->GetSecurityOrigin());
if (_mainFrameSite != child_site) {
annotatedPageContentBarrier.Run();
continue;
}
}
```
node->mutable_content_attributes()->set_attribute_type(Fikre Mengistunot 100% sure we want to move this here
this is usually a fix proposed by the AI when xframe registration fails
Setting attribute_type = CONTENT_ATTRIBUTE_IFRAME prior to registration here was intended for apc v1. Even in V1 mode, cross-origin iframes emit placeholder tokens, so setting the attribute type before returning ensures those placeholder pins identify as iframes when Andrew's sweep checks them. There was some talk about keeping apc v1 around for feature that may want a more lightweight extraction, so I still think this is worth keeping. wyt?
ack, I think I see in the changes that was moved it back to after ?
I think setting the content attribute type too early may mess up with
```
void MergeContent(optimization_guide::proto::ContentNode* placeholder,
FrameGrafter::FrameContent&& frame_content,
autofill::RemoteFrameToken document_id) {
if (placeholder->content_attributes().attribute_type() ==
optimization_guide::proto::CONTENT_ATTRIBUTE_IFRAME) {
// Rich Extraction: The placeholder is already assigned as an
// iframe, this means that the data in the `placeholder` is already
// partially set so do a partial merge in this case. The iframe content tree
// needs to be added as a child of the attributed iframe ContentNode.
// Content starts from the page root (like for the main frame).
optimization_guide::proto::FrameData* frame_data =
placeholder->mutable_content_attributes()
->mutable_iframe_data()
->mutable_frame_data();
frame_data->Swap(&frame_content.frame_data);
// Set the document identifier here because it is not available in the
// frame data extracted for the entire page which is the case for
// cross-origin frames that require grafting.
frame_data->mutable_document_identifier()->set_serialized_token(
document_id.ToString());
*placeholder->add_children_nodes() = std::move(frame_content.content);
} else {
// Light Extraction: The placeholder doesn't hold any partial data,
// it means that the whole ContentNode for the iframe is provided from the
// content stored in `unregistered_content_.
*placeholder = std::move(frame_content.content);
}
}
```discerning between "legacy" and rich extraction
Also
One caveat of putting it before was that CONTENT_ATTRIBUTE_IFRAME would be set and some tests only checked that attribute to verify whether the iframe content was grafted , but if the tests are complete enough they would look at more stuff so they can catch regressions where the content stops being grafted
// Subdomain (same-site) text SHOULD be extracted.Fikre MengistuI think it is the other way around
I believe this is correctly written. Because include_same_site_only allows same-site frames, the subdomain iframe shares the main frame's net::SchemefulSite and should have its text extracted. Conversely, the third-party cross-site iframe has a different site, so gating blocks it and its text should not be extracted.
right, sorry I think I read "redacted" instead of "extracted"
Related details
- Fikre Mengistu
- Nicolas MacBeth
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. |
Fikre Mengistu (Gerrit)
Fikre Mengistu added 6 comments![]( "Open in Gerrit")
Fikre MengistuI feel this change works different from the parent change , is it possible ?
here the redaction is done native side whereas in the parent CL the redaction is done from the iframe metadata coming from the renderer
Vincent BoisselleThis CL introduces native C++ pre-extraction cross origin gating in PageContextWrapper. This enables redactions and avoids executing heavy JS extraction on cross-site subframes that would ultimately be discarded. When blocked, their placeholder pins flow into Andrew's sweep where they are redacted with REASON_CROSS_SITE if the same_site_only flag is enabled.
Ok so they sort of work in tandem ? we redact before extraction and also feed the redaction data from the renderer extraction but without actually extracting the content of that frame where we just want the "metadata"
Yes, if same_site_only is enabled, we prevent the extraction of cross site subframe from happening to prevent unnecessary work. In the main frame's extraction, we put a placeholder in place of the cross site iframe that inlcudes site metada, and the frame grafter in the end will look at the placeholder and replace it with a redaction node.
// post-processing can determine if it is cross-site.add also a note that this data will be replaced with the full metadata when resolving the placeholders if the frame data isn't redacted
Done
C++ Style Improvement:
In Chromium C++, it is much more idiomatic and readable to use `base::NullCallback()` to generate empty callbacks:
```cpp
PopulateIframeData(*iframe_data, destination_node, origin, base::NullCallback());
```
Done
net::SchemefulSite main_frame_site(main_frame_url);Missing Header Include:
You use `net::SchemefulSite` here, but this file is missing the explicit include for it. Relying on transitive includes can be fragile and might lead to future (or current) compilation errors.
Please add `#import "net/base/schemeful_site.h"` at the top of the file.
that import statement is already exists at the top of this file.
TOCTOU & Potential Null Pointer Crash:
By querying `_webState->GetLastCommittedURL()` again here, you defeat the purpose of caching `_mainFrameSite` in the constructor (which was added specifically to prevent TOCTOU race conditions). Also, `_webState` is a `base::WeakPtr`, so dereferencing it using `->` inside this loop without first checking its validity can lead to a crash if the web state was destroyed during the asynchronous operations.
Use the cached `_mainFrameSite` instance variable instead, just as you successfully did in the refactored path loop earlier in the file:
```objc
if (_config->include_same_site_only()) {
net::SchemefulSite child_site(webFrame->GetSecurityOrigin());
if (_mainFrameSite != child_site) {
annotatedPageContentBarrier.Run();
continue;
}
}
```
Done. Thanks for the catch.
node->mutable_content_attributes()->set_attribute_type(Acknowledged. I removed the change from this CL, and will follow up with you about possibly making this change in conjunction with the changes in mergecontent and tests.
Related details
- Nicolas MacBeth
- Vincent Boisselle
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. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vincent Boisselle
| Code-Review | +1 |
Fikre MengistuI feel this change works different from the parent change , is it possible ?
here the redaction is done native side whereas in the parent CL the redaction is done from the iframe metadata coming from the renderer
Vincent BoisselleThis CL introduces native C++ pre-extraction cross origin gating in PageContextWrapper. This enables redactions and avoids executing heavy JS extraction on cross-site subframes that would ultimately be discarded. When blocked, their placeholder pins flow into Andrew's sweep where they are redacted with REASON_CROSS_SITE if the same_site_only flag is enabled.
Fikre MengistuOk so they sort of work in tandem ? we redact before extraction and also feed the redaction data from the renderer extraction but without actually extracting the content of that frame where we just want the "metadata"
Yes, if same_site_only is enabled, we prevent the extraction of cross site subframe from happening to prevent unnecessary work. In the main frame's extraction, we put a placeholder in place of the cross site iframe that inlcudes site metada, and the frame grafter in the end will look at the placeholder and replace it with a redaction node.
- Fikre Mengistu
- Nicolas MacBeth
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 5 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
| Commit-Queue | +1 |
5 comments
thanks Fikre! looks great! see some comments and one ask here:
can you look at the actor_service's page context extraction and add the flag there to be used please? (correct me if i'm wrong but we want this for actuation stuff only, correct?)
bool include_same_site_only,nit: update the comment to briefly explain `include_same_site_only`
if (!include_same_site_only) {can you add a comment here why we need this?
placeholder->content_attributes().iframe_data().frame_data().url());This is an AI recommendation, and I'm a little outside the context right now so you'll know better whether this is actionable or not: we are doing security-related things with this information, but it comes from untrusted JS-based sources. should we be validating this URL or checking it somehow? sorry if this is very ambiguous
GURL url = _webState->GetVisibleURL();I think `GetLastCommitedURL` is better security-wise, can you confirm? If so, can you update this existing code to make sure we're consistent?
Related details
- Fikre Mengistu
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. |
Fikre Mengistu (Gerrit)
Fikre Mengistu added 5 comments![]( "Open in Gerrit")
thanks Fikre! looks great! see some comments and one ask here:
can you look at the actor_service's page context extraction and add the flag there to be used please? (correct me if i'm wrong but we want this for actuation stuff only, correct?)
Yes, redactions will only be enabled for agentic tasks. I was going to enable the flag in a separate CL, but it can also be done here. Done.
bool include_same_site_only,nit: update the comment to briefly explain `include_same_site_only`
Done
can you add a comment here why we need this?
Done
placeholder->content_attributes().iframe_data().frame_data().url());This is an AI recommendation, and I'm a little outside the context right now so you'll know better whether this is actionable or not: we are doing security-related things with this information, but it comes from untrusted JS-based sources. should we be validating this URL or checking it somehow? sorry if this is very ambiguous
Subframe extraction gating in PageContextWrapper uses C++ browser state (`webFrame->GetSecurityOrigin()`) rather than the JS-reported placeholder URL, so spoofed JS URLs cannot trick the crawler into extracting cross-site content. Even if the URL extracted in js is misrepresented here, the subframe's DOM/text was already blocked from being extracted, leaving an empty node with zero data here.
I think `GetLastCommitedURL` is better security-wise, can you confirm? If so, can you update this existing code to make sure we're consistent?
- Nicolas MacBeth
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. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted and added 3 comments![]( "Open in Gerrit")
Votes added by Vincent Boisselle
| Code-Review | +1 |
3 comments
GURL main_frame_url(apc->main_frame_data().url());we have changed how this works in the parent cl
net::SchemefulSite child_site(webFrame->GetSecurityOrigin());childSite
EXPECT_FALSE(subdomain_node->content_attributes()can u verify that there are children as well ?
Related details
- Fikre Mengistu
- Nicolas MacBeth
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. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
2 comments
placeholder->content_attributes().iframe_data().frame_data().url());Fikre MengistuThis is an AI recommendation, and I'm a little outside the context right now so you'll know better whether this is actionable or not: we are doing security-related things with this information, but it comes from untrusted JS-based sources. should we be validating this URL or checking it somehow? sorry if this is very ambiguous
Subframe extraction gating in PageContextWrapper uses C++ browser state (`webFrame->GetSecurityOrigin()`) rather than the JS-reported placeholder URL, so spoofed JS URLs cannot trick the crawler into extracting cross-site content. Even if the URL extracted in js is misrepresented here, the subframe's DOM/text was already blocked from being extracted, leaving an empty node with zero data here.
Acknowledged
Related details
- Fikre Mengistu
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. |