Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect gateway-like urls to ipfs:// #15204

Merged
merged 4 commits into from
Oct 18, 2022
Merged

Redirect gateway-like urls to ipfs:// #15204

merged 4 commits into from
Oct 18, 2022

Conversation

cypt4
Copy link
Collaborator

@cypt4 cypt4 commented Sep 23, 2022

Resolves brave/brave-browser#21454 Urls in format of https://bafy.ipfs.gateway.io or https://gateway.io/ipfs/bafy are now redirected to ipfs:// scheme if x-ipfs-path header is received
Sec review https://github.com/brave/security/issues/1063

Resolves

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, npm run lint, 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:

"Redirect IPFS resources to the configured IPFS gateway" setting should open ipfs:// urls and, so, should work with IPFS resolve method "ASK"

@cypt4 cypt4 requested a review from iefremov as a code owner September 23, 2022 22:00
@github-actions github-actions bot added the CI/run-network-audit Run network-audit label Sep 23, 2022
@cypt4 cypt4 force-pushed the brave_21454_1 branch 2 times, most recently from 0434c69 to a160422 Compare September 25, 2022 11:32
@cypt4 cypt4 requested a review from yrliou October 6, 2022 10:12
std::vector<std::string> host_parts = base::SplitStringUsingSubstr(
url.host(), ".", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);

if (host_parts.size() > 2 && IsValidCID(host_parts.at(0)) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having an example of what original url transforms to which final url would be handy

@iefremov
Copy link
Contributor

iefremov commented Oct 6, 2022

I'd suggest to ask someone from the sec team to look at the parsing code

@cypt4
Copy link
Collaborator Author

cypt4 commented Oct 12, 2022

I'd suggest to ask someone from the sec team to look at the parsing code

Opened sec review https://github.com/brave/security/issues/1063

components/ipfs/ipfs_utils_unittest.cc Show resolved Hide resolved
browser/net/ipfs_redirect_network_delegate_helper.cc Outdated Show resolved Hide resolved
components/ipfs/ipfs_utils_unittest.cc Outdated Show resolved Hide resolved
components/ipfs/ipfs_utils.cc Show resolved Hide resolved
@@ -104,25 +109,19 @@ int OnHeadersReceived_IPFSRedirectWork(
if (ctx->ipfs_auto_fallback && !api_gateway && response_headers &&
response_headers->GetNormalizedHeader("x-ipfs-path", &ipfs_path) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does ipfs_path still get used anywhere?

Copy link
Collaborator Author

@cypt4 cypt4 Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used as the marker in this case, content isn't used

@cypt4
Copy link
Collaborator Author

cypt4 commented Oct 13, 2022

/build linux

@cypt4
Copy link
Collaborator Author

cypt4 commented Oct 13, 2022

/build macos

@cypt4 cypt4 requested review from yrliou and diracdeltas October 13, 2022 14:05
cypt4 added 3 commits October 14, 2022 15:35
Resolves brave/brave-browser#21454
Urls in format of https://bafy.ipfs.gateway.io or https://gateway.io/ipfs/bafy
are now redirected to ipfs:// scheme if x-ipfs-path header is received
@@ -94,6 +95,13 @@ int OnHeadersReceived_IPFSRedirectWork(
std::shared_ptr<brave::BraveRequestInfo> ctx) {
if (!ctx->browser_context)
return net::OK;

// Auto-redirect gateway-like urls is enabled only for top-level frames
// To avoid mixed content corner cases.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/To/to

@cypt4 cypt4 merged commit d4865d8 into master Oct 18, 2022
@cypt4 cypt4 deleted the brave_21454_1 branch October 18, 2022 17:41
@github-actions github-actions bot added this to the 1.46.x - Nightly milestone Oct 18, 2022

if (host_parts.size() > 2 && IsValidCID(host_parts.at(0)) &&
host_parts.at(1) == "ipfs") {
GURL final_url = GURL("ipfs://" + host_parts.at(0) + url.path());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're redirecting http/https to an ipfs url? I don't think that is the correct way to handle this, you should be using url rewriting to display ipfs as the virtual url

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the reverse of what we do when someone enters ipfs:// in the urlbar

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are checking the response header at the moment

Copy link
Collaborator

@bridiver bridiver Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this redirects any gateway url? I don't think we should be doing that. If I explicitly enter an ipfs url that uses a different gateway than what I have configured, it should not redirect to my configured gateway. I thought this was only to display ipfs for any urls that matched the configured gateway which can be done with the rewriter.

Copy link
Collaborator Author

@cypt4 cypt4 Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this redirects any gateway url? I don't think we should be doing that. If I explicitly enter an ipfs url that uses a different gateway than what I have configured, it should not redirect to my configured gateway. I thought this was only to display ipfs for any urls that matched the configured gateway which can be done with the rewriter.

Nope, this is about reusing configured gateway to load IPFS resources.

Automatically uses the configured gateway for IPFS resolutions when an IPFS gateway resource is encountered.

As for the manual url input i'll discuss it, maybe this should be exclusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-network-audit Run network-audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic redirects to IPFS via gateway don't always work
5 participants