[Topics] add Topics header for iframe navigation request [chromium/src : main]
Yao Xiao (Gerrit)
Attention is currently required from: Josh Karlin.
Yao Xiao would like Josh Karlin to review this change.
[Topics] add Topics header for iframe navigation request
Implement topics navigation header:
- Before a request or redirect is handled (i.e. NavigationRequest's
constructor, and OnRedirectChecksComplete()), calculate and append
the header.
- After the corresponding response is received (i.e.
OnRedirectChecksComplete() and CommitNavigation()), check and handle
the topics response header.
Also, fixed the checks in HTMLIFrameElement::ParseAttribute() and
HTMLIFrameElement::DidChangeAttributes(): currently, the RuntimeFeature
and SecureContext are not properly checked when the iframe attribute is
set via setAttribute. The annotation in the IDL wasn't sufficient.
Bug: 1352345
Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
---
M chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
M components/browsing_topics/browsing_topics_service_impl.cc
M components/browsing_topics/common/common_types.h
M content/browser/browsing_topics/header_util.cc
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/navigation_request.h
M third_party/blink/renderer/core/html/html_iframe_element.cc
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-default.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-none.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-self.tentative.https.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute.tentative.https.html
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/check-topics-request-header-notify-parent.py
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/navigation-header-util.sub.js
M tools/metrics/histograms/enums.xml
15 files changed, 544 insertions(+), 5 deletions(-)
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Yao Xiao (Gerrit)
Attention is currently required from: Josh Karlin.
1 comment:
Patchset:
jkarlin@: PTAL. Thanks!
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Blink W3C Test Autoroller (Gerrit)
Attention is currently required from: Josh Karlin, Yao Xiao.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/39097.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Josh Karlin (Gerrit)
Attention is currently required from: Yao Xiao.
Patch set 1:Code-Review +1
3 comments:
Patchset:
lgtm with nits
File chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc:
Patch Set #1, Line 338:
iframe.browsingTopics = $1;Please also test via the attribute in markup, e.g,. <iframe browsingTopics> vs <iframe>
File content/browser/renderer_host/navigation_request.cc:
Patch Set #1, Line 1135:
topics_eligibleWould prefer returning an optional<string> to avoid the output param.
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Yao Xiao (Gerrit)
Attention is currently required from: Alex Moshchuk.
Yao Xiao would like Alex Moshchuk to review this change.
[Topics] add Topics header for iframe navigation request
Implement topics navigation header:
- Before a request or redirect is handled (i.e. NavigationRequest's
constructor, and OnRedirectChecksComplete()), calculate and append
the header.
- After the corresponding response is received (i.e.
OnRedirectChecksComplete() and CommitNavigation()), check and handle
the topics response header.
Also, fixed the checks in HTMLIFrameElement::ParseAttribute() and
HTMLIFrameElement::DidChangeAttributes(): currently, the RuntimeFeature
and SecureContext are not properly checked when the iframe attribute is
set via setAttribute. The annotation in the IDL wasn't sufficient.
Bug: 1352345
Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
---
M chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
A chrome/test/data/browsing_topics/page_with_custom_attribute_iframe.html
M components/browsing_topics/browsing_topics_service_impl.cc
M components/browsing_topics/common/common_types.h
M content/browser/browsing_topics/header_util.cc
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/navigation_request.h
M third_party/blink/renderer/core/html/html_iframe_element.cc
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-default.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-none.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-self.tentative.https.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute.tentative.https.html
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/check-topics-request-header-notify-parent.py
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/navigation-header-util.sub.js
M tools/metrics/histograms/enums.xml
16 files changed, 645 insertions(+), 5 deletions(-)
Yao Xiao (Gerrit)
Attention is currently required from: Alex Moshchuk.
3 comments:
Patchset:
alexmos@: PTAL at navigation_request.h/.cc. Thanks!
File chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc:
Patch Set #1, Line 338:
iframe.browsingTopics = $1;
Please also test via the attribute in markup, e.g,. […]
Done
File content/browser/renderer_host/navigation_request.cc:
Patch Set #1, Line 1135:
topics_eligibleWould prefer returning an optional<string> to avoid the output param.
Done
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Attention is currently required from: Yao Xiao.
8 comments:
Patchset:
Thanks!
File content/browser/renderer_host/navigation_request.cc:
Patch Set #2, Line 1068:
DCHECK(rfh);I think this DCHECK isn't really needed, the fact that current_frame_host() isn't null is pretty fundamental.
Patch Set #2, Line 1074:
rfh == rfh->GetMainFrame()Can we just check is_main_frame() instead?
Patch Set #2, Line 1095:
url::Origin::Create(url);This won't account for things like sandbox flags, which could also make the final origin opaque. Is that ok?
Patch Set #2, Line 1111:
parent_policy, container_policy, origin);Why do we need to recompute the permissions policy, rather than just using the one we already have in rfh->GetPermissionsPolicy()? Because we need to account for the target origin, and so you're trying to estimate what the future permissions policy would be when it commits? Can you please add a comment?
More generally, I'm a bit worried this computation might diverge from how the permission policy is set in RFHI::ResetPermissionsPolicy(). Perhaps NavigationRequest should have a GetPermissionsPolicyToCommit(), similar to GetOriginToCommit() which we could reference both here and in ResetPermissionsPolicy()? This is fine to explore in a followup, though, as there's at least one other place that does this already and would need to change too (https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=671;drc=b4bc946c63b2b95e1f05dec4e84adcadd10499c6)
Patch Set #2, Line 5016:
if (topics_eligible_) {Can you add a high-level comment about what the redirect handling is achieving? I'm guessing the eligibility for sending the topics header might've changed based on the redirected URL, so we need to recompute it?
Patch Set #2, Line 5512:
topics_eligible_ = false;Instead of CommitNavigation, we could end up going through CommitErrorPage in case of an error page. Should we reset this value in that case? (Not sure if it ever matters for anything, but it's probably best not to leave this as true for error pages.)
Patch Set #2, Line 5516:
url::Origin::Create(common_params_->url)I'm curious whether you'd want to use GetOriginToCommit() here. That would also take into account sandbox flags and other weird cases, though on the other hand maybe you actually want to ignore all that for handling this.
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Yao Xiao (Gerrit)
Attention is currently required from: Alex Moshchuk.
7 comments:
File content/browser/renderer_host/navigation_request.cc:
Patch Set #2, Line 1068:
DCHECK(rfh);
I think this DCHECK isn't really needed, the fact that current_frame_host() isn't null is pretty fun […]
Done
Patch Set #2, Line 1074:
rfh == rfh->GetMainFrame()Can we just check is_main_frame() instead?
Done
Patch Set #2, Line 1095:
url::Origin::Create(url);
This won't account for things like sandbox flags, which could also make the final origin opaque. […]
I think this is okay. We need to check the <iframe browsingtopics> opt-in attribute anyway, so there's no real security/privacy issue by ignoring other iframe attributes. So I'll align with the handling for the analogous `fetch(<url>, {browsingTopics: true})` request, implemented in https://source.chromium.org/chromium/chromium/src/+/main:content/browser/browsing_topics/browsing_topics_url_loader.cc?q=GetTopicsHeaderValueForSubresourceRequest
Patch Set #2, Line 1111:
parent_policy, container_policy, origin);
Why do we need to recompute the permissions policy, rather than just using the one we already have i […]
I switched to `parent_policy->IsFeatureEnabledForOrigin()`. There's no general consensus or spec on how navigation request should be checked for permissions policy. All features are doing something special (ClientHint, TrustToken). I still did the switch, to align with the existing permissions policy checks for topics fetch() request.
Patch Set #2, Line 5016:
if (topics_eligible_) {
Can you add a high-level comment about what the redirect handling is achieving? I'm guessing the el […]
Done
Patch Set #2, Line 5512:
topics_eligible_ = false;
Instead of CommitNavigation, we could end up going through CommitErrorPage in case of an error page. […]
Done
Patch Set #2, Line 5516:
url::Origin::Create(common_params_->url)
I'm curious whether you'd want to use GetOriginToCommit() here. […]
See the other comment. I think it's *okay* to use the URL.
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Attention is currently required from: Yao Xiao.
Patch set 3:Code-Review +1
4 comments:
Patchset:
navigation_request LGTM, thanks!
File content/browser/renderer_host/navigation_request.cc:
Patch Set #2, Line 1095:
url::Origin::Create(url);I think this is okay. […]
Ack. Could you please add a comment that this should align with the handling in GetTopicsHeaderValueForSubresourceRequest() that you pointed to, so that someone doesn't forget to update it also if this ever changes in the future?
Patch Set #2, Line 1111:
parent_policy, container_policy, origin);
I switched to `parent_policy->IsFeatureEnabledForOrigin()`. […]
Ack. Might also be nice to file a bug and include permission policy folks to get agreement on how to properly check permissions policy in NavigationRequest.
Patch Set #2, Line 5516:
url::Origin::Create(common_params_->url)
See the other comment. I think it's *okay* to use the URL.
Acknowledged
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Josh Karlin (Gerrit)
1 comment:
Patchset:
Looks like my +1 got lost, adding it back.
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Yao Xiao (Gerrit)
Attention is currently required from: Dominic Farolino.
Yao Xiao would like Dominic Farolino to review this change.
16 files changed, 649 insertions(+), 5 deletions(-)
Yao Xiao (Gerrit)
Attention is currently required from: Dominic Farolino.
3 comments:
Patchset:
dom@: PTAL at html_iframe_element.cc. Thanks!
File content/browser/renderer_host/navigation_request.cc:
Patch Set #2, Line 1095:
url::Origin::Create(url);
Ack. […]
Done. Added a function-level comment.
Patch Set #2, Line 1111:
parent_policy, container_policy, origin);
Ack. […]
Ack. Created an issue in the spec repo: https://github.com/w3c/webappsec-permissions-policy/issues/510
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Farolino (Gerrit)
Attention is currently required from: Yao Xiao.
1 comment:
Commit Message:
Patch Set #4, Line 20:
set via setAttribute. The annotation in the IDL wasn't sufficient.I'm observing the current behavior working correctly in Chromium on HTTP sites. Can you clarify what you're observing, because this should not be necessary.
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Yao Xiao (Gerrit)
Attention is currently required from: Dominic Farolino.
1 comment:
Commit Message:
Patch Set #4, Line 20:
set via setAttribute. The annotation in the IDL wasn't sufficient.
I'm observing the current behavior working correctly in Chromium on HTTP sites. […]
Without the change, the second test case in `third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html` would fail (which sets the attribute via `frame.setAttribute("browsingtopics", "123");` / doesn't go through the IDL attribute).
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Farolino (Gerrit)
Attention is currently required from: Yao Xiao.
Patch set 4:Code-Review +1
1 comment:
Commit Message:
Patch Set #4, Line 20:
set via setAttribute. The annotation in the IDL wasn't sufficient.
Without the change, the second test case in `third_party/blink/web_tests/external/wpt/browsing-topic […]
Ah that makes sense. In that case, please also update the spec accordingly to cater to this.
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Yao Xiao (Gerrit)
1 comment:
Commit Message:
Patch Set #4, Line 20:
set via setAttribute. The annotation in the IDL wasn't sufficient.
Ah that makes sense. In that case, please also update the spec accordingly to cater to this.
Ack. Will update the spec.
To view, visit change 4356935. To unsubscribe, or for help writing mail filters, visit settings.
Yao Xiao (Gerrit)
Patch set 4:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Topics] add Topics header for iframe navigation request
Implement topics navigation header:
- Before a request or redirect is handled (i.e. NavigationRequest's
constructor, and OnRedirectChecksComplete()), calculate and append
the header.
- After the corresponding response is received (i.e.
OnRedirectChecksComplete() and CommitNavigation()), check and handle
the topics response header.
Also, fixed the checks in HTMLIFrameElement::ParseAttribute() and
HTMLIFrameElement::DidChangeAttributes(): currently, the RuntimeFeature
and SecureContext are not properly checked when the iframe attribute is
set via setAttribute. The annotation in the IDL wasn't sufficient.
Bug: 1352345
Change-Id: Ia84c67e8f9858a98d7161668bc82fa688c563497
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4356935
Reviewed-by: Alex Moshchuk <ale...@chromium.org>
Reviewed-by: Josh Karlin <jka...@chromium.org>
Commit-Queue: Yao Xiao <yao...@chromium.org>
Reviewed-by: Dominic Farolino <d...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122570}
---
M chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
A chrome/test/data/browsing_topics/page_with_custom_attribute_iframe.html
M components/browsing_topics/browsing_topics_service_impl.cc
M components/browsing_topics/common/common_types.h
M content/browser/browsing_topics/header_util.cc
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/navigation_request.h
M third_party/blink/renderer/core/html/html_iframe_element.cc
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-default.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-none.tentative.https.sub.html
M third_party/blink/web_tests/external/wpt/browsing-topics/browsing-topics-permissions-policy-self.tentative.https.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute-insecure-context.tentative.http.sub.html
A third_party/blink/web_tests/external/wpt/browsing-topics/iframe-topics-attribute.tentative.https.html
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/check-topics-request-header-notify-parent.py
A third_party/blink/web_tests/external/wpt/browsing-topics/resources/navigation-header-util.sub.js
M tools/metrics/histograms/enums.xml
16 files changed, 649 insertions(+), 5 deletions(-)
Blink W3C Test Autoroller (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/39097