Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix URL parsing consistency issue #10346

Merged
merged 1 commit into from
Aug 10, 2017
Merged

Fix URL parsing consistency issue #10346

merged 1 commit into from
Aug 10, 2017

Conversation

diracdeltas
Copy link
Member

Fix #10270 by preferring muon.url.parse over Node's legacy (and non-standards compliant) URL parser. This is subobtimal because unit tests are running a different URL parser from the actual browser, but seems like the best trade off for now.

Also fixes #6098

Test Plan:

  1. go to brave.com and disable shields
  2. go to http://brave.com%60x.code-fu.org/. shields should not be disabled.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

Fix #10270 by preferring muon.url.parse over Node's legacy (and non-standards compliant) URL parser. This is subobtimal because unit tests are running a different URL parser from the actual browser, but seems like the best trade off for now.

Test Plan:
1. go to brave.com and disable shields
2. go to http://brave.com%60x.code-fu.org/. shields should not be disabled.
@bbondy bbondy merged commit 90931e9 into master Aug 10, 2017
@bbondy bbondy modified the milestones: 0.18.x Hotfix, 0.19.x (Beta Channel) Aug 10, 2017
bbondy added a commit that referenced this pull request Aug 10, 2017
Fix URL parsing consistency issue
bbondy added a commit that referenced this pull request Aug 10, 2017
Fix URL parsing consistency issue
bbondy added a commit that referenced this pull request Aug 10, 2017
Fix URL parsing consistency issue
@srirambv
Copy link
Collaborator

@diracdeltas

go to http://brave.com%60x.code-fu.org/. shields should not be disabled.

The lion head should not be disabled?

@diracdeltas
Copy link
Member Author

@srirambv right

@luixxiul
Copy link
Contributor

luixxiul commented Aug 15, 2017

CC @diracdeltas I'm not sure if this is fixed. The lion icon is orange, while the toggle is disabled.

clipboard01

Brave: 0.18.22
rev: d850e28
Muon: 4.3.9
libchromiumcontent: 60.0.3112.90
V8: 6.0.286.52
Node.js: 7.9.0
Update Channel: dev
OS Platform: Linux
OS Release: 4.9.0-3-amd64
OS Architecture: x64

@diracdeltas
Copy link
Member Author

@luixxiul did you start with a clean session store? seems to work for me on 0.18.x

@srirambv
Copy link
Collaborator

srirambv commented Aug 15, 2017

Works as per str. But I still see the shields show as brave.com as per @luixxiul screenshot above.

@luixxiul
Copy link
Contributor

maybe I have turned off the shield on brave.com. Let me check again.

@srirambv
Copy link
Collaborator

I think it works as expected, if I visit http://brave.com%60x.code-fu.org/ in a new tab, shields is disabled by default, so shields showing brave.com sounds correct to me.

@bsclifton bsclifton deleted the fix/10270 branch August 21, 2017 18:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hackerone] https://hackerone.com/reports/255991 refactor js/lib/urlutil.js for safer url parsing
4 participants