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

add encodeURIComponent for WebUI data URLs in FE code #27636

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

kdenhartog
Copy link
Member

@kdenhartog kdenhartog commented Feb 13, 2025

Resolves brave/brave-browser#43367

This is meant to prevent query params that are being used to pass data like URLs and images around to avoid an external URL being able to inject parameters to chrome://image. The majority of the instances were in brave_wallet_ui, but I did also spot some instances in brave_new_tab_ui. This is primarily a find/replace fix, so it would be good to make sure this isn't going to accidentally break images from being rendered.

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • 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 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:

@kdenhartog kdenhartog requested a review from a team as a code owner February 13, 2025 01:47
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet labels Feb 13, 2025
@brave-builds
Copy link
Collaborator

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

Copy link
Contributor

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

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

Hey so I think we don't want to use encodeURIComponent on any chrome://favicon urls (as it doesn't support URL encoded origins) or on any brave://wallet urls (it looks like it will break routing).

chrome://image looks fine to change though!

(also, if we migrate to chrome://favicon2 it does support url encoding the host name, so we could do that here too, if you're feeling keen!)

Copy link
Contributor

Chromium major version is behind target branch (133.0.6943.54 vs 134.0.6998.15). Please rebase.

@github-actions github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Feb 18, 2025
@fallaciousreasoning
Copy link
Contributor

Probably needs a rebase on cr134

@kdenhartog kdenhartog requested review from fallaciousreasoning and removed request for goodov February 19, 2025 21:39
@kdenhartog
Copy link
Member Author

kdenhartog commented Feb 19, 2025

Ok, all the feedback should be addressed at this point.

When I messed up the rebase I had to cherry-pick my old commits off, and I think I messed up the order of the cherry-pick. So all the feedback has been collapsed back into the single commit rather than separating it out.

At this point, no chrome://favicon URIs or brave://wallet URIs should be changed based on the feedback.

I think the last things that still need to be looked at by @Douglashdaniel are the following comments and to double-check I didn't accidentally break any images with this change.

Here's the comments:
#27636 (comment)
#27636 (comment)
#27636 (comment)

@kdenhartog
Copy link
Member Author

@Douglashdaniel mentioned to me that this is breaking some of the images currently. I spent some time doing some debugging yesterday in storybook (which seems to behave different from local build) and I found out I could get things working if I did a decodeURIComponent in stripERC20Images. I don't think this is the correct fix for this, but it does suggest that we're probably doing some form of dual encoding as called out here: #27636 (comment)

Additionally, I noticed we've got some NFT images that are calling out directly to cdn.simplehash.com from brave://wallet. These should be handled via chrome://images instead to make sure the content gets sanitized first.

@fallaciousreasoning
Copy link
Contributor

Oh interesting - I wouldn't have thought we could access Web urls from an internal WebUI page 😨

@diracdeltas
Copy link
Member

@fallaciousreasoning we are not supposed to, but for some reason img-src https: is in the chrome-untrusted://nft-display content security policy. we should remove this allowance @Douglashdaniel .

Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

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

security approving

It seems we shouldn't be updating chrome://favicon or brave://wallet URIs, so
this is an updated commit that addresses just the limited set of URIs that
require encodedURIComponents usage.
@Douglashdaniel Douglashdaniel merged commit 0e23cf8 into master Feb 27, 2025
18 checks passed
@Douglashdaniel Douglashdaniel deleted the kdh/issue-43367 branch February 27, 2025 19:24
@github-actions github-actions bot added this to the 1.78.x - Nightly milestone Feb 27, 2025
@brave-builds
Copy link
Collaborator

Released in v1.78.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/run-network-audit Run network-audit CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chrome://image calls should be URI-encoded
8 participants