New issue
Advanced search Search tips

Issue 845845 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Audit the use of ExtensionFunction::include_incognito().

Project Member Reported by rhalavati@chromium.org, May 23 2018

Issue description

ExtensionFunction::include_incognito() is sometimes used instead of extensions::util::IsIncognitoEnabled().

All usages of both functions should be checked to ensure the correct function is used.

Also we can rename include_incognito() to reduce the chance of future errors.
 

Comment 1 Deleted

Comment 2 Deleted

Project Member

Comment 3 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9732ad00f8dd2564a6283a67156697cfe85dbac

commit d9732ad00f8dd2564a6283a67156697cfe85dbac
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Wed May 30 14:49:01 2018

Update ContentSettingsService access to incognito profile.

ContentSettingsService handles incognito mode requests internally, so
a flag is set to redirect incognito mode requests to the original
profile.
Also, in Set Content Settings API, incorrect use of include_incognito()
function instead of IsIncognitoEnabled() is corrected.
Two tests are added to check that all settings written in incognito
are applied and do not affect regular mode, and incognito settings
cannot be modified from regular mode.

Bug:  832697 
Bug:  845845 
Change-Id: I6d65f824e3e3ca2fbd218ed0bf5af1ddc15c87ef
Reviewed-on: https://chromium-review.googlesource.com/1061853
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#561811}
Reviewed-on: https://chromium-review.googlesource.com/1074651
Cr-Commit-Position: refs/heads/master@{#562817}
[modify] https://crrev.com/d9732ad00f8dd2564a6283a67156697cfe85dbac/chrome/browser/extensions/api/content_settings/content_settings_api.cc
[modify] https://crrev.com/d9732ad00f8dd2564a6283a67156697cfe85dbac/chrome/browser/extensions/api/content_settings/content_settings_apitest.cc
[modify] https://crrev.com/d9732ad00f8dd2564a6283a67156697cfe85dbac/chrome/browser/extensions/api/content_settings/content_settings_service.h
[modify] https://crrev.com/d9732ad00f8dd2564a6283a67156697cfe85dbac/chrome/browser/extensions/extension_apitest.h
[add] https://crrev.com/d9732ad00f8dd2564a6283a67156697cfe85dbac/chrome/test/data/extensions/api_test/content_settings/incognitoisolation/manifest.json
[add] https://crrev.com/d9732ad00f8dd2564a6283a67156697cfe85dbac/chrome/test/data/extensions/api_test/content_settings/incognitoisolation/test.html
[add] https://crrev.com/d9732ad00f8dd2564a6283a67156697cfe85dbac/chrome/test/data/extensions/api_test/content_settings/incognitoisolation/test.js

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992

commit acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Wed Jun 06 07:05:12 2018

Rename include_incognito function to include_incognito_information.

ExtensionFunction::include_incognito() is renamed to
ExtensionFunction::include_incognito_information() to provide more
clarity on the use cases.

Bug:  845845 
Change-Id: I0ba7bf58faf763a2ca8a50bf15f4c82e75ea82a0
Reviewed-on: https://chromium-review.googlesource.com/1078810
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564812}
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/content_settings/content_settings_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/cookies/cookies_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/debugger/debugger_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/downloads/downloads_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/downloads_internal/downloads_internal_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/extension_action/extension_action_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/page_capture/page_capture_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/preference/preference_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/processes/processes_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/tab_capture/tab_capture_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/tabs/tabs_test.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/tabs/windows_util.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/chrome_extension_function_details.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/chrome/browser/extensions/extension_tab_util.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/extensions/browser/api_test_utils.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/extensions/browser/extension_function.cc
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/extensions/browser/extension_function.h
[modify] https://crrev.com/acdd17f1b73cb9d1d2ec849afbd9fb4af78f0992/extensions/browser/extension_function_dispatcher.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8755fde253dc0377efd54981c04d1bb7426c98e1

commit 8755fde253dc0377efd54981c04d1bb7426c98e1
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Mon Jun 11 07:40:01 2018

Update extensions downloads API incognito profile access.

Download function of Extensions Downloads API used incognito downnload
manager, if available, even if the request was from regular profile.

This is changed to using the download manager of current profile.

Bug:  845845 
Change-Id: I751a06850f0c9d6ac288ec9ff83bc592012fc3f5
Reviewed-on: https://chromium-review.googlesource.com/1087963
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Ben Hayden <benjhayden@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565941}
[modify] https://crrev.com/8755fde253dc0377efd54981c04d1bb7426c98e1/chrome/browser/extensions/api/downloads/OWNERS
[modify] https://crrev.com/8755fde253dc0377efd54981c04d1bb7426c98e1/chrome/browser/extensions/api/downloads/downloads_api.cc
[modify] https://crrev.com/8755fde253dc0377efd54981c04d1bb7426c98e1/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eceb5bb5272886c9c2da7b957aae15cee8010008

commit eceb5bb5272886c9c2da7b957aae15cee8010008
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Mon Jun 11 07:59:24 2018

Update access to Incognito profile in Extension Function Dispatcher.

Extension Function dispatcher is updated to allow access to incogntio
information, if incognito mode is enabled and extension can cross
incognito mode (is running in spanning mode).

Bug:  845845 
Change-Id: I922ae4e5a8b6c08797ae71be464d2d6aa01b6aff
Reviewed-on: https://chromium-review.googlesource.com/1088609
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565943}
[modify] https://crrev.com/eceb5bb5272886c9c2da7b957aae15cee8010008/extensions/browser/extension_function_dispatcher.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9013e33822af593c3746edd7fafbc5672b2a8ab4

commit 9013e33822af593c3746edd7fafbc5672b2a8ab4
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Tue Jun 12 05:20:42 2018

Update access to Incognito extension preferences.

Access to extension preferences in Incognito mode is updated to allow
only if extension is enabled in incognito, and either called in
incognito or its running in spanning mode.

Bug:  845845 
Change-Id: I8581c572ea63989db64778b5c87130cd40fa0053
Reviewed-on: https://chromium-review.googlesource.com/1087966
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566322}
[modify] https://crrev.com/9013e33822af593c3746edd7fafbc5672b2a8ab4/chrome/browser/extensions/api/preference/preference_api.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4600b63b4a22418c2d311e40732500afdfb60845

commit 4600b63b4a22418c2d311e40732500afdfb60845
Author: Ramin Halavati <rhalavati@chromium.org>
Date: Tue Jun 12 05:21:22 2018

Update Extensions Download API tests.

Some misuses of incognito/regular browser contexts are corrected and
some tests are updated.

Bug:  845845 
Change-Id: I4398b3fb5fef95696b007036983c7508fb6d4aeb
Reviewed-on: https://chromium-review.googlesource.com/1095216
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566323}
[modify] https://crrev.com/4600b63b4a22418c2d311e40732500afdfb60845/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc

Devlin,
Do you think we can close this bug or there is any other issue that you think of?

It was best if we could add proper tests to avoid this sort of bugs in future. I will prepare a separate doc to discuss this and create a new bug for it if we reach a conclusion.
I think it's probably safe to close this bug out.  If you wanted to draft a doc (or even just a separate bug that can be marked available) for followup work, that could certainly be helpful, but I don't think it need block this bug.

Thank you for your work here, rhalavati@!
Status: Fixed (was: Assigned)
Thanks Devlin. I sent you the initial draft, closing this one.

Sign in to add a comment