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

do not generally allow file:// to be opened in new tab #14973

Merged
merged 1 commit into from
Aug 13, 2018
Merged

do not generally allow file:// to be opened in new tab #14973

merged 1 commit into from
Aug 13, 2018

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Aug 8, 2018 •
edited by bsclifton
Loading

fix #14522

Test Plan:

  1. create an HTML file called 'file-load.html' somewhere on your
    computer
  2. go to https://jsfiddle.net/L9t0je3h/3/ and replace
    'file:///Users/yan/Downloads/file-load.html' with the path of this
    file
  3. in the jsfiddle page, right click on the link that says 'right click
    to open in new tab' and try clicking any of the context menu items to
    open in a new tab/window. they should all load about:blank instead of
    the real file.
  4. go to Menu > File > Open files. pick some files to open and make sure
    these open correctly.

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.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

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

@diracdeltas
do not generally allow file:// to be opened in new tab
a115240 
fix #14522

Test Plan:
1. create an HTML file called 'file-load.html' somewhere on your
   computer
2. go to https://jsfiddle.net/L9t0je3h/3/ and replace
   'file:///Users/yan/Downloads/file-load.html' with the path of this
   file
3. in the jsfiddle page, right click on the link that says 'right click
   to open in new tab' and try clicking any of the context menu items to
   open in a new tab/window. they should all load about:blank instead of
   the real file.
4. go to Menu > File > Open files. pick some files to open and make sure
   these open correctly.
@diracdeltas diracdeltas self-assigned this Aug 8, 2018
@diracdeltas diracdeltas requested a review from jumde August 8, 2018 23:32
@jumde
Copy link
Contributor

jumde commented Aug 12, 2018

Chrome/Safari/Firefox allow file URLs to be opened from file URLs in new tab. This patch disables that as well.

@diracdeltas
Copy link
Member Author

@jumde good catch. can we merge this and then do a lower-priority fix for opening file from file via context menu? (i think file should still be openable from context-menu via cmd+click)

@bsclifton bsclifton self-requested a review August 13, 2018 17:57
bsclifton
bsclifton reviewed Aug 13, 2018
View reviewed changes
js/actions/appActions.js
*/
createTabRequested: function (createProperties, activateIfOpen = false, isRestore = false, focusWindow = false) {
createTabRequested: function (createProperties, activateIfOpen = false, isRestore = false, focusWindow = false, allowFile = false) {
Copy link
Member

Choose a reason for hiding this comment

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

style comment (not blocking by any means 😛 I know it was already like this) - when functions start to have this many params, it might be better to accept a map with named params

bsclifton
bsclifton approved these changes Aug 13, 2018
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Test plan works - code changes look good (one note left about style, but fixing is optional IMO). Functionality is also covered by test 👍

@jumde jumde mentioned this pull request Aug 13, 2018
Closed
@jumde
Copy link
Contributor

jumde commented Aug 13, 2018

Follow up issue: #15006

@bsclifton bsclifton merged commit 2c964fd into master Aug 13, 2018
@bsclifton bsclifton deleted the fix/14522 branch August 13, 2018 18:24
bsclifton added a commit that referenced this pull request Aug 13, 2018
@bsclifton
Merge pull request #14973 from brave/fix/14522
3b080d6 
do not generally allow file:// to be opened in new tab
bsclifton added a commit that referenced this pull request Aug 13, 2018
@bsclifton
Merge pull request #14973 from brave/fix/14522
2d611aa 
do not generally allow file:// to be opened in new tab
@bsclifton
Copy link
Member

master 2c964fd
0.24.x 3b080d6
0.23.x 2d611aa

@jumde jumde mentioned this pull request Aug 13, 2018
Closed
@Metnew
Copy link

Metnew commented Aug 15, 2018

Could you close the report on H1, please? @diracdeltas

@diracdeltas diracdeltas mentioned this pull request Sep 12, 2018
Closed
diracdeltas added a commit that referenced this pull request Sep 17, 2018
@diracdeltas
Block privileged URL loads in context menu only
df790e9 
Partial revert of #14973.

Fix #15134
Fix #15203

Test Plan:
1. Repeat test plan in
   #14973 and verify it
   still passes.
2. Drag and drop an HTML or image file into Brave and verify that it
   opens
3. With Brave set to the default browser, double click an HTML file and
   verifies that it opens correctly.
@diracdeltas diracdeltas mentioned this pull request Sep 17, 2018
Merged
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jumde jumde jumde approved these changes

@bsclifton bsclifton bsclifton approved these changes

Assignees

@diracdeltas diracdeltas

Labels
None yet
Projects
None yet
Milestone
0.23.x Release 5 (Release Channel)
Development

Successfully merging this pull request may close these issues.

[hackerone] file open in new tab issue
4 participants
@diracdeltas @jumde @bsclifton @Metnew