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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use chrome.tabs.executeScript when encountering javascript bookmarks #3167

Closed
wants to merge 1 commit into from
Closed

Conversation

jonasws
Copy link

@jonasws jonasws commented Oct 18, 2018

This PR changes the behavior of openUrlInCurrentTab to use chrome.tabs.executeScript whenever it encounters a JavaScript bookmark. chrome.tabs.update does not support this anymore, resulting in an error whenever trying this.

Tested in Both Chrome and Firefox 馃槃

@gdh1995
Copy link
Contributor

gdh1995 commented Oct 25, 2018

chrome.tabs.update does not support this anymore

Why do you think so?

In my tests, I could use both chrome.tabs.update(tabId, {url: 'javascript:alert(1)'}) and chrome.tabs.executeScript(tabId, {'code': 'alert(1)'}) on http and https pages. My environment is Chrome stable 70.0.3538.77 (Official Build) (64-bit) on Windows 10.

@gdh1995
Copy link
Contributor

gdh1995 commented Oct 25, 2018

And, chrome.tabs.executeScript runs scripts on Vimium's isolated context, rather than the page's main context. So this change will make some JavaScript bookmarks not work as they want, and cause a risk of leaking some Vimium's privileges to third-party scripts.

@jonasws
Copy link
Author

jonasws commented Oct 26, 2018

Thanks for getting back to me!

The reason for me believing initially that JavaScript bookmarks would no longer be supported in chrome.tabs.update, stems from the docs for said method. However, the source of my troubles may be that I have whitespace in wrong positions, or (I really hope I'm wrong here) that bookmarklets don't properly support ES2015 syntax.

I receive an error in the browser console when trying to open a JS bookmark through the omnibar:

Unchecked lastError value: Error: Illegal URL: javascript: (function() { const finncodeRegex = /finnkode=(\d+)/; const finncode = finncodeRegex.exec(window.location.search)[1]; if (finncode) { const TOKEN = 'tokentoknentoken'; window.location.assign( `https://www.finn.no/some/url/${finncode}?token=${encodeURIComponent( TOKEN )}` ); } })();

I also agree that we want to avoid exposing the users to all sorts of malicious scripts. I'll look into if formatting th JS bookmark properly will help solve the problem.

@gdh1995
Copy link
Contributor

gdh1995 commented Oct 27, 2018

Could you paste real JS URL again? The url in the lastError message does work on my Chrome.

@gdh1995
Copy link
Contributor

gdh1995 commented Dec 26, 2018

According to my tests, Chrome 71 does not allow "javascript:" urls in chrome.tabs.update, but older versions can do so, so we may try chrome.tabs.executeScript first and if any error then switch back to tab.update.

Update: document about changes of Chrome 71 is https://developer.chrome.com/extensions/tabs#property-updateProperties-url .

@smblott-github
Copy link
Collaborator

Let me see if I know where we stand.

chrome.tabs.update no longer supports JS URLs.

The fix is to use chrome.tabs.executeScript but that creates two issues:

  1. The JS executes in the Vimum context, which may lead to unexpected results.
  2. The JS executes in the Vimium context with elevated permissions, so users may inadvertently shoot themselves in the foot.

We might have to just live with 1, if there's nothing else we can do.

Re. 2... this wouldn't be a problem if we were starting from scratch and users were aware of the issue. However, if we suddenly start executing existing JS bookmarks in an environment with elevated permissions, then that's potentially a very serious matter. I'm not sure we can do that.

smblott-github added a commit to smblott-github/vimium that referenced this pull request Dec 27, 2018
Since `chrome.tabs.update()` no longer supports `javascript:` URLs, here
we inject them into the page itself.

Replaces philc#3167.
Replaces philc#3209.
Fixes philc#3178.
@philc
Copy link
Owner

philc commented Feb 21, 2020

Great summary @smblott-github. This was superseded by #3437, so closing.

@philc philc closed this Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants