Make `allow_universal_access_from_file_urls` verification Android only. [chromium/src : main]

Illia Klimov (Gerrit)

unread,

Jun 23, 2022, 11:03:21 AM6/23/22

to Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Richard Coles.

View Change

1 comment:

Patchset:
- Patch Set #3:
  Hi Richard,
  Permissions team is verifying if `allow_universal_access_from_file_urls` is enabled before fetching an origin. But I believe there is no reason to check it for all platforms, hence I've added the verification only for Android.
  Please check if it make sense?

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Richard Coles (Gerrit)

unread,

Jun 23, 2022, 2:37:08 PM6/23/22

to Illia Klimov, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Illia Klimov.

Patch set 4:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #3:
  Hi Richard, […]
  Yes - only Android WebView is allowed to enable this, and I believe there are already other places that check this which are also only enabled for OS_ANDROID.

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Illia Klimov (Gerrit)

unread,

Jun 23, 2022, 3:18:40 PM6/23/22

to Andy Paicu, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andy Paicu.

View Change

1 comment:

Patchset:
- Patch Set #4:
  Hi Andy,
  It makes sense to check `allow_universal_access_from_file_urls` flag only if it is Android, hence I'm adding it under IS_ANDROID. PTAL.

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Andy Paicu (Gerrit)

unread,

Jun 23, 2022, 4:39:07 PM6/23/22

to Illia Klimov, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Illia Klimov.

View Change

1 comment:

File content/browser/permissions/permission_util.cc:
- Patch Set #4, Line 25: #if BUILDFLAG(IS_ANDROID)
  Should the code at https://source.chromium.org/chromium/chromium/src/+/main:content/web_test/browser/web_test_permission_manager.cc;l=28?q=%60allow_universal_access_from_file_urls%60 also be updated?

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Illia Klimov (Gerrit)

unread,

Jul 2, 2022, 2:34:50 AM7/2/22

to Arthur Sonzogni, Andy Paicu, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Andy Paicu, Arthur Sonzogni.

View Change

2 comments:

Patchset:
- Patch Set #6:
  Hi Andy,
  thank you for the comment, PTAL.
  @arthursonzogni:
  content/web_test/browser/web_test_permission_manager.cc
File content/browser/permissions/permission_util.cc:

- Patch Set #4, Line 25: #if BUILDFLAG(IS_ANDROID)

- Should the code at https://source.chromium. […]
  Thank you, updated!

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Andy Paicu (Gerrit)

unread,

Jul 4, 2022, 6:01:23 AM7/4/22

to Illia Klimov, Arthur Sonzogni, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, Illia Klimov.

Patch set 6:Code-Review +1

View Change

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Arthur Sonzogni (Gerrit)

unread,

Jul 4, 2022, 6:06:34 AM7/4/22

to Illia Klimov, Andy Paicu, Arthur Sonzogni, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, Illia Klimov.

View Change

1 comment:

File content/web_test/browser/web_test_permission_manager.cc:
- Patch Set #6, Line 24: GURL GetLastCommittedOriginAsURL(content::RenderFrameHost* render_frame_host) {
  This is duplicated from two other location. Should you try to deduplicate? Or at least add a comment explaining this function must be kept in sync, similar to the comment in the previous file?

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Arthur Sonzogni (Gerrit)

unread,

Jul 4, 2022, 6:35:54 AM7/4/22

to Illia Klimov, Arthur Sonzogni, Andy Paicu, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Illia Klimov.

View Change

1 comment:

File chrome/browser/permissions/permissions_security_model_interactive_uitest.cc:
- Patch Set #6, Line 913: /*expect_granted*/ false
  nit: The clang-format recognized format for this is:
```
/*expect_granted=*/false
```
  clang-format will remove spaces in between the comment and the value. We might also have some kind of checks the comment is matching the argument it is referring too.

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Illia Klimov (Gerrit)

unread,

Jul 7, 2022, 9:06:24 AM7/7/22

to Arthur Sonzogni, Andy Paicu, Arthur Sonzogni, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni.

View Change

3 comments:

Patchset:
- Patch Set #7:
  Hi Arthur, thanks for the comments, PTAL
File chrome/browser/permissions/permissions_security_model_interactive_uitest.cc:

- Patch Set #6, Line 913: /*expect_granted*/ false

- nit: The clang-format recognized format for this is: […]
  Thank you!
File content/web_test/browser/web_test_permission_manager.cc:

- Patch Set #6, Line 24: GURL GetLastCommittedOriginAsURL(content::RenderFrameHost* render_frame_host) {

- This is duplicated from two other location. […]
  I would really like to deduplicate and I've tried it in the previous patchset [1]. But due to dependencies issue, using //content/browser/* inside //content/web_test/* I wasn't able to do it.
  Maybe you know how can I add such dependency?
  [1] https://chromium-review.googlesource.com/c/chromium/src/+/3720955/5

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Arthur Sonzogni (Gerrit)

unread,

Jul 7, 2022, 11:16:24 AM7/7/22

to Illia Klimov, danakj, Arthur Sonzogni, Andy Paicu, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, Illia Klimov.

View Change

3 comments:

Patchset:
- Patch Set #7:
  +Dana for the question about `web_test_browser` depending on `content/browser:browser`
File content/web_test/browser/web_test_permission_manager.cc:

- Patch Set #6, Line 24: GURL GetLastCommittedOriginAsURL(content::RenderFrameHost* render_frame_host) {

- I would really like to deduplicate and I've tried it in the previous patchset [1]. […]
  So, you would like to make the target `web_test_browser` to depend on `content/browser`
  I see the file:
  `content/web_test/browser/web_test_permission_manager.cc`
  is for the target:
  `web_test_browser`
  Then, it seems Chrome is doing something very special about this target depending on content/browser depending on
```
In the `content/web_test/BUILD.gn`, I see 
```
    # This is to support our dependency on //content/browser.
    # See comment at the top of //content/BUILD.gn for why this is disabled in
    # component builds.
    if (is_component_build) {
      check_includes = false
    }
```
```
```
and the
```
    deps = [
      "//content/browser:for_content_tests",  # For non-component builds.
      "//content/public/browser",  # For component builds.
    ]
```
```
  I am not sure to understand everything. @danakj probably knows. I guess you can't because this is not defined from content/public/browser and not visible for the web_test_browser target.
File content/web_test/browser/web_test_permission_manager.cc:
- Patch Set #7, Line 24: GetLastCommittedOriginAsURL
  At least, could you add:
```
// Due to dependency issues, this method is duplicated in
// content/browser/permissions/permission_util.cc.
```
  similar to what is done in:
  components/permissions/permission_util.cc

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Jul 7, 2022, 12:03:02 PM7/7/22

to Illia Klimov, Arthur Sonzogni, Andy Paicu, Arthur Sonzogni, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, Arthur Sonzogni, Illia Klimov.

View Change

1 comment:

File content/web_test/browser/web_test_permission_manager.cc:

- Patch Set #6, Line 24: GURL GetLastCommittedOriginAsURL(content::RenderFrameHost* render_frame_host) {

- So, you would like to make the target `web_test_browser` to depend on `content/browser` […]
  Web test code can depend on content/browser/ internals, since it's not production.
  "//content/browser:for_content_tests" does exactly that.
  The error in PS5 is a linking error because the permission util method is not exported, so it's not available for linking in the component build. It should be exported with content/common/content_export.h
  https://source.chromium.org/chromium/chromium/src/+/main:content/browser/permissions/permission_util.h;drc=a5fe6465633bad4f31c33f658f2721fa44078ece;l=18
  That's why it is failing on _debug_ bots, they are component builds.

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Arthur Sonzogni (Gerrit)

unread,

Jul 8, 2022, 5:20:59 AM7/8/22

to Illia Klimov, danakj, Arthur Sonzogni, Andy Paicu, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, Illia Klimov.

View Change

1 comment:

File content/web_test/browser/web_test_permission_manager.cc:

- Patch Set #6, Line 24: GURL GetLastCommittedOriginAsURL(content::RenderFrameHost* render_frame_host) {

- Web test code can depend on content/browser/ internals, since it's not production. […]
  Thanks Dana!
  So let's do this and remove this duplicated function.

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Illia Klimov (Gerrit)

unread,

Jul 19, 2022, 7:20:43 AM7/19/22

to danakj, Arthur Sonzogni, Andy Paicu, Arthur Sonzogni, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, danakj.

View Change

3 comments:

Patchset:
- Patch Set #9:
  Hi Dana,
  thank you for the suggestion, exporting API fixed the linking issue. Arthur is OOO, could you please stamp `content/web_test/browser/web_test_permission_manager.cc`?
File content/web_test/browser/web_test_permission_manager.cc:

- Patch Set #6, Line 24: GURL GetLastCommittedOriginAsURL(content::RenderFrameHost* render_frame_host) {

- Thanks Dana! […]
  Thank you!
File content/web_test/browser/web_test_permission_manager.cc:

- Patch Set #7, Line 24: GetLastCommittedOriginAsURL

- At least, could you add: […]
  Removed duplication.

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

danakj (Gerrit)

unread,

Jul 19, 2022, 9:23:15 AM7/19/22

to Illia Klimov, Arthur Sonzogni, Andy Paicu, Arthur Sonzogni, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, Illia Klimov.

Patch set 9:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #9:
  LGTM

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Illia Klimov (Gerrit)

unread,

Jul 19, 2022, 9:32:22 AM7/19/22

to danakj, Arthur Sonzogni, Andy Paicu, Arthur Sonzogni, Richard Coles, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Arthur Sonzogni, Illia Klimov.

Patch set 9:Commit-Queue +2

View Change

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Jul 19, 2022, 10:30:39 AM7/19/22

to Illia Klimov, danakj, Arthur Sonzogni, Andy Paicu, Arthur Sonzogni, Richard Coles, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Illia Klimov: Commit
  Andy Paicu: Looks good to me
  danakj: Looks good to me
  Richard Coles: Looks good to me

Make `allow_universal_access_from_file_urls` verification Android only.

An origin for permissions request and verification is calculated as RFH::GetLastCommittedOrigin(). On Android it is possible to change visible URL if `allow_universal_access_from_file_urls` flag is enabled. To meet users' expectation, we use RFH::GetLastCommittedURL().

This CL restricts `allow_universal_access_from_file_urls` flag verification only for Android.

Bug: 1327384
Change-Id: Ibd59f59e2d12500f717af3a32fc1c089bd01094a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3720955
Commit-Queue: Illia Klimov <el...@google.com>
Reviewed-by: Richard Coles <to...@chromium.org>
Reviewed-by: Andy Paicu <andy...@chromium.org>
Reviewed-by: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1025707}
---
M chrome/browser/permissions/permissions_security_model_interactive_uitest.cc
M components/permissions/permission_util.cc
M content/browser/permissions/permission_util.cc
M content/browser/permissions/permission_util.h
M content/web_test/browser/web_test_permission_manager.cc
5 files changed, 63 insertions(+), 48 deletions(-)

To view, visit change 3720955. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward