Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix decoding internal blocked page #22138

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Conversation

cuba
Copy link
Contributor

@cuba cuba commented Feb 17, 2024

Resolves brave/brave-browser#36378

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:

See issue for STR

@cuba cuba requested a review from a team as a code owner February 17, 2024 22:59
Copy link
Collaborator

@ShivanKaul ShivanKaul left a comment

Choose a reason for hiding this comment

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

It's a bit hard to read @cuba with the formatting changes, but did we add a test for the URL/case that @bridiver found?

@cuba
Copy link
Contributor Author

cuba commented Feb 18, 2024

It's a bit hard to read @cuba with the formatting changes, but did we add a test for the URL/case that @bridiver found?

you can see just the code changes (without formatting) here:
90fa3a9

And yes I did add a test. I didn't add the specific url but I made sure there are query params used in my test which is what the problem was.

@cuba cuba added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 labels Feb 20, 2024
@cuba cuba force-pushed the js/fix-decoding-internal-url branch 2 times, most recently from 90fa3a9 to dfd484a Compare February 21, 2024 12:20
@iccub iccub requested a review from Brandon-T February 26, 2024 21:27
@cuba cuba force-pushed the js/fix-decoding-internal-url branch from dfd484a to eda8cb5 Compare February 29, 2024 14:22
@cuba cuba merged commit 5db2abb into master Mar 1, 2024
19 checks passed
@cuba cuba deleted the js/fix-decoding-internal-url branch March 1, 2024 16:12
@github-actions github-actions bot added this to the 1.65.x - Nightly milestone Mar 1, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] Proceed on an iOS blocked page leads to a 404 page
3 participants