Localhost access permission #17321

ShivanKaul · 2023-02-22T05:05:30Z

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

I've created a test website at https://shivankaul.com/brave/localhost/ with testing instructions.
A few other things to test:

A localhost connection from a localhost website should Just Work without a prompt.
There should be a new site setting called Localhost access at brave://settings/content/localhostAccess

brave-builds · 2023-02-22T06:31:30Z

A Storybook has been deployed to preview UI for the latest push

browser/net/brave_localhost_permission_network_delegate_helper.cc

iefremov · 2023-02-22T17:14:34Z

browser/net/brave_localhost_permission_network_delegate_helper.cc

+  const auto& request_initiator_url = ctx->initiator_url;
+  const auto& request_url = ctx->request_url;
+
+  if (!IsLocalhostRequest(request_url, request_initiator_url)) {


in case of websocket initiator would be empty, is it okay?

it would come from BraveProxyingWebSocket

Interesting! Is there a way to detect that? We wouldn't want to get the embedding/initiating URL from contents->GetLastCommittedURL() because we want to only capture the cases where a website is making the localhost request.

Hm I seem to get the right initiator and request URLs with a WebSocket actually. My test client is https://shivankaul.com/brave/localhost/ws_client.html (check the console for messages) and test server is just a simple WebSocket server running on port 8080 locally: https://shivankaul.com/brave/localhost/ws_server.js

browser/net/brave_localhost_permission_network_delegate_helper.cc

ShivanKaul · 2023-02-22T20:42:12Z

@goodov The presubmit CI fails on

browser/net/brave_localhost_permission_network_delegate_helper.cc:47:
std::shared_ptr should not be used. Use scoped_refptr instead.

But OnBeforeURLRequest_LocalhostPermissionWork is just adhering to

OnBeforeURLRequestCallback =
    base::RepeatingCallback<int(const ResponseCallback& next_callback,
                                std::shared_ptr<BraveRequestInfo> ctx)>;

goodov · 2023-02-24T07:27:52Z

@goodov The presubmit CI fails on

browser/net/brave_localhost_permission_network_delegate_helper.cc:47:
std::shared_ptr should not be used. Use scoped_refptr instead.

But OnBeforeURLRequest_LocalhostPermissionWork is just adhering to

OnBeforeURLRequestCallback =
    base::RepeatingCallback<int(const ResponseCallback& next_callback,
                                std::shared_ptr<BraveRequestInfo> ctx)>;

add your file to this list:

brave-core/chromium_presubmit_config.json5

Line 83 in 9c36ad8

"CheckNoBannedFunctions": [

app/brave_settings_strings.grdp

browser/about_flags.cc

browser/permissions/localhost_access_permission_browsertest.cc

fmarier · 2023-03-04T00:40:55Z

browser/permissions/localhost_access_permission_browsertest.cc

+  CheckAskAndDenyFlow(kButtonHtmlId, target_url, 1);
+}
+
+IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest, OneTwoSeven) {


Not a big deal, but for completeness' sake, we could also test the IPv6 version ([::1]).

components/permissions/contexts/brave_localhost_permission_context.cc

brave-builds · 2023-03-17T22:55:47Z

A Storybook has been deployed to preview UI for the latest push

browser/net/brave_localhost_permission_network_delegate_helper.cc

app/brave_settings_strings.grdp

mkarolin

Strings ++

browser/permissions/localhost_access_permission_browsertest.cc

browser/net/brave_localhost_permission_network_delegate_helper.h

Only throttle for subresources No throttling for adblocked resources Add tests for adblock

Instead of WebContents pointer

PermissionControllerDelegate => PermissionController RequestPermissionsForOrigin => RequestPermissionsFromCurrentDocument

TGrade · 2024-03-21T20:04:31Z

Hey. Sorry I'm a bit curious about this. Does this check your local routes? Will it prevent websites from accessing your local by accessing addresses like 192.168.0.123 for example?

ShivanKaul requested a review from goodov February 22, 2023 05:05

ShivanKaul requested review from iefremov and a team as code owners February 22, 2023 05:05

github-actions bot added CI/run-network-audit Run network-audit CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels Feb 22, 2023

github-actions bot assigned ShivanKaul Feb 22, 2023

goodov reviewed Feb 22, 2023

View reviewed changes

browser/net/brave_localhost_permission_network_delegate_helper.cc Show resolved Hide resolved

iefremov reviewed Feb 22, 2023

View reviewed changes

browser/net/brave_localhost_permission_network_delegate_helper.cc Outdated Show resolved Hide resolved

fmarier reviewed Mar 4, 2023

View reviewed changes

ShivanKaul force-pushed the feature/localhost-permission branch from ce472f2 to cba92ad Compare March 20, 2023 21:50

ShivanKaul removed the CI/storybook-url Deploy storybook and provide a unique URL for each build label Mar 20, 2023

ShivanKaul force-pushed the feature/localhost-permission branch from b6e3c2e to 9db4b4b Compare March 20, 2023 23:51

goodov reviewed Mar 21, 2023

View reviewed changes

browser/net/brave_localhost_permission_network_delegate_helper.cc Show resolved Hide resolved

ShivanKaul force-pushed the feature/localhost-permission branch 2 times, most recently from f3b0d4e to 62bc1a0 Compare March 21, 2023 23:22

ShivanKaul force-pushed the feature/localhost-permission branch from 62bc1a0 to c45e59c Compare April 12, 2023 18:16

ShivanKaul requested a review from a team as a code owner April 12, 2023 18:16

mkarolin reviewed Apr 12, 2023

View reviewed changes

app/brave_settings_strings.grdp Show resolved Hide resolved

ShivanKaul force-pushed the feature/localhost-permission branch from c45e59c to fdf8b55 Compare April 13, 2023 00:49

mkarolin approved these changes Apr 13, 2023

View reviewed changes

goodov approved these changes Apr 14, 2023

View reviewed changes

browser/permissions/localhost_access_permission_browsertest.cc Outdated Show resolved Hide resolved

ShivanKaul force-pushed the feature/localhost-permission branch from 6d9e822 to dd086b1 Compare April 14, 2023 13:51

iefremov reviewed Apr 17, 2023

View reviewed changes

browser/net/brave_localhost_permission_network_delegate_helper.h Outdated Show resolved Hide resolved

iefremov approved these changes Apr 21, 2023

View reviewed changes

ShivanKaul added 16 commits April 22, 2023 10:43

Add network delegate and permission for localhost requests

c97eb45

Add strings and settings UI

c0d9217

Add browsertest

3661687

Various fixes

01dc788

Only throttle for subresources No throttling for adblocked resources Add tests for adblock

Build fixes

15820eb

Add permission string for Android

435827f

Presubmit fixes

3b8f404

Add service worker logic

9ae629d

Presubmit fixes

c504445

Fix settings UI issues

98f654b

Use base::MakeRefCounted in test

d153662

Cleanup unused includes

1f09e1a

Add comment explaining why reload

ac46d2d

Pass frame tree node id to OnPermissionRequestStatus

1c92b4c

Instead of WebContents pointer

Address review feedback

c1f8d15

PermissionControllerDelegate => PermissionController RequestPermissionsForOrigin => RequestPermissionsFromCurrentDocument

Fix merge conflict related to patch

6d79b02

ShivanKaul force-pushed the feature/localhost-permission branch from 96b03e3 to 6d79b02 Compare April 24, 2023 07:16

mihaiplesa merged commit 11330da into master Apr 24, 2023

mihaiplesa deleted the feature/localhost-permission branch April 24, 2023 15:04

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Apr 24, 2023

github-actions bot added this to the 1.52.x - Nightly milestone Apr 24, 2023

ShivanKaul mentioned this pull request Apr 25, 2023

Android setting for localhost permission #18222

Merged

25 tasks

MicahZoltu mentioned this pull request May 6, 2023

Allow specific localhost resources to be accessible by all websites. brave/brave-browser#30181

Open

ShivanKaul mentioned this pull request May 10, 2023

Secure WebSocket connection to the loopback adapter fails to connect brave/brave-browser#26940

Closed

Uni-verse mentioned this pull request May 25, 2023

[Android] Localhost permission brave/brave-browser#29730

Closed

ShivanKaul mentioned this pull request Jun 1, 2023

Use component update list for localhost permission prompting #18354

Merged

25 tasks

ShivanKaul mentioned this pull request Jun 21, 2023

Add default filter provider manager #18876

Merged

25 tasks

uazo mentioned this pull request Jun 22, 2023

Add UI for INSECURE_PRIVATE_NETWORK site setting (default disabled) uazo/cromite#344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Localhost access permission #17321

Localhost access permission #17321

ShivanKaul commented Feb 22, 2023 •

edited

Loading

brave-builds commented Feb 22, 2023

iefremov Feb 22, 2023

iefremov Feb 22, 2023

ShivanKaul Feb 22, 2023

ShivanKaul Feb 26, 2023 •

edited

Loading

ShivanKaul commented Feb 22, 2023

goodov commented Feb 24, 2023

fmarier Mar 4, 2023

brave-builds commented Mar 17, 2023

mkarolin left a comment

TGrade commented Mar 21, 2024

Localhost access permission #17321

Localhost access permission #17321

Conversation

ShivanKaul commented Feb 22, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Feb 22, 2023

iefremov Feb 22, 2023

Choose a reason for hiding this comment

iefremov Feb 22, 2023

Choose a reason for hiding this comment

ShivanKaul Feb 22, 2023

Choose a reason for hiding this comment

ShivanKaul Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

ShivanKaul commented Feb 22, 2023

goodov commented Feb 24, 2023

fmarier Mar 4, 2023

Choose a reason for hiding this comment

brave-builds commented Mar 17, 2023

mkarolin left a comment

Choose a reason for hiding this comment

TGrade commented Mar 21, 2024

ShivanKaul commented Feb 22, 2023 •

edited

Loading

ShivanKaul Feb 26, 2023 •

edited

Loading