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

wiki: clear window.opener [solved: see top of OP] #401

Closed
Thorin-Oakenpants opened this Issue Apr 18, 2018 · 21 comments

Comments

Projects
None yet
3 participants
@Thorin-Oakenpants
Copy link
Member

Thorin-Oakenpants commented Apr 18, 2018

Edit

  • Legacy GM & script: no issues
  • Web Extensions & Script: disable it and use window.opener be gone
    • this is the same script but injected by an extension to solve the issues

OP as follows

issue to deal with clear window.opener.
quick wiki link
quick test link

note:

  • when I say 57+ I am using that as the marker since that is based on our extensions recommendations (we recommend VM in 57+) and is a clean line in the sand and legacy extensions vs web ext. Tests were only done on 59.0.2 (VM is 2.9.1)

summary:

  • all three scripts work in legacy GM and ESR52.x which we still support
  • in web ext GM and 57+, all three scripts fail (due to document-start?)
  • in web ext VM and 57+, only clear window.opener "fails"
    • earthlng tested webext GM (correct me if wrong)
  • the script noopener_by_default in VM and 57+ works
  • edit: diff in the two scripts: noopener_by_default is heavy handed and modifies the original/source page, while clear window.opener modifies the new pages
  • edit: diffs in how web extensions GM+VM handle document-start
  • edit: GM almost has a document start fix in place (different to VM)

conclusion:

  • clear window.opener if modified might work in VM (see the new script), but see diffs in what the script actually does.

done:

  • wiki: I have already edited the top few lines of the wiki to clearly say that the scripts will not work with the web ext version of GM.

to do:

  • decide if earthlng wishes to provide a second script for 57+
  • clearly label the current script as legacy extension only
  • otherwise point to the new script OR point to an extension
  • wait for GM to land the document-start fix and switch to GM
  • talk to VM and see what can be done

Awaiting earthlng's decision, insight, knowledge

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 18, 2018

earthlng said:

apparently, without looking at the code, just based on your test results, VM uses a different way to inject the scripts and that's why the user-scripts work in their current form. However that method comes with a different set of problems. For example I'm pretty sure the window.opener script doesn't work because the demo page is hosted on a domain with strict CSP rules. That means you can't rely on your user-scripts to work on potentially malicious pages, at all.
I can think of at least 1 other problem but won't bother you with the details.

re: the 2 different scripts: noopener_by_default works by modifying the links on the original page, while "my" user-script works on the "new" page.

noopener_by_default has several issues (1) it's less efficient because it modifies all links targeting a new tab, (2) can easily be tricked to not modify a malicious link, (3) and maybe most importantly, it doesn't clear window.opener when a page uses window.open(). There might be other problems as well, fe. a form can also have target=_blank and it's possible that a page opened that way could also have access to window.opener. If it does then noopener_by_default does nothing against that, either. It can also break a page because adding rel=noopener to a link apparently opens the page without the referer header. EDIT: rel=noopener on its own doesn't remove the referrer header. There's rel="noreferrer" for that.

The script in the wiki has none of these problems because it works on the "new" page.

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 18, 2018

VM uses a different way to inject the scripts and that's why the user-scripts work in their current form. However that method comes with a different set of problems.

does that mean the other 2 scripts also suffer from the same issue? Or are they OK because they are not modifying content? A bit over my head.

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 18, 2018

The script in the wiki has none of these problems because it works on the "new" page

So it works then? i.e in VM on 57+? We just get a miscellaneous result because the test expected something else?

@earthlng

This comment has been minimized.

Copy link
Member

earthlng commented Apr 18, 2018

decide if earthlng wishes to provide a second script for 57+

the script works in its current form with VM BUT with certain limitations and problems due to the way VM works.
It will also work in its current form with GM (and probably without those limitations and problems that VM has) once run-at document-start works in the webextension version. I think the GM guys already have a patch ready that fixes @ document-start but they aren't quite satisfied with some other things in that patch.

@earthlng

This comment has been minimized.

Copy link
Member

earthlng commented Apr 18, 2018

does that mean the other 2 scripts also suffer from the same issue?

you tell me. You're the author of Conceal history.length, aren't you? :trollface:

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 18, 2018

Question: can an extension overcome these limitations: eg strict CSP, run-at doc-start, modifying every link

https://addons.mozilla.org/en-US/firefox/addon/dont-touch-my-tabs/?src=search - looks like it modifies every link

"But won't this 'break my internet'?"
Nope! Because when a hyperlink points to a web page hosted on the same domain name as the one you're on, it will NOT add the rel=noopener attribute. Neat huh?

So maybe not every link then. Also looks like he will, in time, add a whitelisting feature

@crssi

This comment has been minimized.

Copy link
Collaborator

crssi commented Apr 18, 2018

^^ Nah... not every link.

Description says this (pay attention to bolded text):

Prevent tabs opened by a hyperlink from hijacking the previous tab by adding the rel=noopener attribute to all hyperlinks (excluding same-domain hyperlinks).

So somehow interfering when same-domain is undisired also for author.
Thats something I was asking at #75 (comment) but being ignored/forgoten.

Or I might be wrong again. 😄

Cheers

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 18, 2018

but being ignored/forgoten.

You're always on my mind .. so at least one person still loves you :) PS: I skipped that because I thought earthlng had answered it

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 20, 2018

I moved GM in the wiki from legacy to non-legacy, it's had three+ releases to mature.

I'm clearly not a developer (did you guess?), and would like to ask @gera2ld about this, but not waste his time. eg: bypassing strict CSP, how it handles run-at document-start (excuse my ignorance if terms are incorrect)

I don't wish to rehash what already exists ( https://github.com/violentmonkey/violentmonkey/search?utf8=✓&q=document-start&type=Issues = 9 issues with document-start) but it's all over my head 😭 and I can't seem to find a dedicated topic on it.

@gera2ld : our wiki has three scripts of which this one Clear window.opener doesn't seem to work in VM (FF59+, VM 2.9.1). Can you give us any heads up or link to an existing topic on this document-start business? TIA

Edit: I see 2.9.2 is out but have not updated yet

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 20, 2018

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 20, 2018

I moved GM in the wiki from legacy to non-legacy, it's had three+ releases to mature.

Actually, I moved it back. Doesn't seem to me like any progress on document-start. They no longer have a privacy policy, and the reviews on AMO are not that great. Maybe when it can run our scripts we can re-look at it.

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 28, 2018

@earthlng Is an extension a feasible solution? i.e none of the document-start, strict CSP hindrances - and one that works the way you described, by modifying the linked page, not the source page.

@earthlng

This comment has been minimized.

Copy link
Member

earthlng commented Apr 29, 2018

Probably.

https://anonfile.com/ef3fm5eab4/windowopener_be_gone-1.0-an_fx.xpi

Very basic webextension that injects the user-script listed in the wiki. No whitelist support or anything like that. Not listed on AMO because I don't want to deal with people asking for new features or bitching about how it breaks some sites. It is what it is, take it or leave it. I might improve it when I move away from ESR but hopefully until then GM will be able to correctly run scripts like this again.
The extension also works on ESR52 but it's probably more convenient to use the user-script with legacy GM for an easy way to exclude certain sites.

@crssi it's only my opinion but yes I think it's possible that an attacker can abuse window.opener on same-domain as well.

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 29, 2018

the xpi - works on the test site perfectly, thank you. Should we link to it in the wiki (with your disclaimers)? eg add it to the extensions wiki page?

Got to say I like the icon
shady

@earthlng

This comment has been minimized.

Copy link
Member

earthlng commented Apr 29, 2018

Should we link to it in the wiki (with your disclaimers)? eg add it to the extensions wiki page?

up to you

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 29, 2018

I'd like to. As you said, the other script can be fooled, we have issues with document run start in VM & GM, and not sure about the other extension (which modifies the links) - whereas your modifies the new page (which is better? right?).

tl;dr: AFAIK, windows.opener be gone (covers same-domain right as per crssi's question and your reply?), while it can be improved in future (whitelisting) is the BEST solution - right?

Just decide where it's going to live forever, and I'll add it

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 29, 2018

@earthlng

This comment has been minimized.

Copy link
Member

earthlng commented Apr 29, 2018

FWIW I think you should store the xpi here: https://github.com/earthlng/testpages with the other two

download link: https://github.com/earthlng/testpages/raw/master/windowopener_be_gone-1.0-an%2Bfx.xpi

@Thorin-Oakenpants Thorin-Oakenpants changed the title wiki: clear window.opener wiki: clear window.opener [solved: see top of OP] Apr 29, 2018

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 29, 2018

  • added to wiki extensions (just the file and link back to this issue)
  • wiki scripts, conceal window.opener mentioned at the top in that slight mess of explanations, AND mentioned just above the script itself

Feel free to fix anything I did to the two wiki pages

@earthlng

This comment has been minimized.

Copy link
Member

earthlng commented Apr 29, 2018

whereas your modifies the new page (which is better? right?).

it clears window.opener whenever it is not already null. It's better in the sense that it doesn't matter how window.opener was set. There are several tags in HTML that can use target=_blank of which at least <a> and <area> support the rel=noopener attribute, according to mozilla. But there might be others besides these 2 that could also result in window.opener being set, like form or whatnot, IDK. And window.open() also sets window.opener. don't touch my tabs only modifies <a> tags.

window.opener be gone covers same-domain

yes

@Thorin-Oakenpants

This comment has been minimized.

Copy link
Member Author

Thorin-Oakenpants commented Apr 29, 2018

^^ awesome. You're so patient with my for-dummies mentality. All I'm hearing (and I do understand most of it) is that yours is a superior solution, that's good enough for me. I trust your expertise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.