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

noopener window.open feature seems pretty broken as implemented in browsers #1902

Open
bzbarsky opened this Issue Oct 14, 2016 · 16 comments

Comments

5 participants
@bzbarsky
Collaborator

bzbarsky commented Oct 14, 2016

Trying to use it gives a very broken window because it turns off all the other window features, in both Firefox and Chrome.

Furthermore, it forces the new window into a separate window, not a tab, because it's turning off all the other features.

I don't see how it could possibly be usable in the state it's in right now, unfortunately. Of course it doesn't help that per spec the only supported features are left/right/width/height, which doesn't match browser behavior....

@mikewest

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Oct 20, 2016

Member

We should move the handling of features argument from CSSOM View to HTML, and add whatever more features browsers need to support for Web compat...

Member

zcorpan commented Oct 20, 2016

We should move the handling of features argument from CSSOM View to HTML, and add whatever more features browsers need to support for Web compat...

@AliceWonderMiscreations

This comment has been minimized.

Show comment
Hide comment
@AliceWonderMiscreations

AliceWonderMiscreations Nov 30, 2016

Off-topic This absolutely needs to be changed.

The proper solution, when Page A is going to allow another window to modify it, Page A needs to send a header with a whitelist of domains that can access it via window.opener().

This must be done as a header because headers can not be modified once content is sent. A rel tag can be modified after content is sent.

Yes this will break the few sites that make legitimate use of window.opener() but all those sites will need to do is send a header with the whitelist. They don't even need to modify their application if they don't want to, that could be done via .htaccess or their front-end proxy.

By requiring a rel tag to secure a page from this flaw, not only will a larger number of current sites need to be modified to be secure but a bunch of archived web pages will need to be modified to be secure.

It makes a lot more sense to break backwards compatibility and require sites that use the feature to add a header than to keep backwards compatibility, use an attribute that can be modified after page load, and leave a large number of websites and archived web pages vulnerable.

I don't know who to bug with this to get the right thing done, but rel="noopener" is the wrong way to solve the problem.

AliceWonderMiscreations commented Nov 30, 2016

Off-topic This absolutely needs to be changed.

The proper solution, when Page A is going to allow another window to modify it, Page A needs to send a header with a whitelist of domains that can access it via window.opener().

This must be done as a header because headers can not be modified once content is sent. A rel tag can be modified after content is sent.

Yes this will break the few sites that make legitimate use of window.opener() but all those sites will need to do is send a header with the whitelist. They don't even need to modify their application if they don't want to, that could be done via .htaccess or their front-end proxy.

By requiring a rel tag to secure a page from this flaw, not only will a larger number of current sites need to be modified to be secure but a bunch of archived web pages will need to be modified to be secure.

It makes a lot more sense to break backwards compatibility and require sites that use the feature to add a header than to keep backwards compatibility, use an attribute that can be modified after page load, and leave a large number of websites and archived web pages vulnerable.

I don't know who to bug with this to get the right thing done, but rel="noopener" is the wrong way to solve the problem.

@AliceWonderMiscreations

This comment has been minimized.

Show comment
Hide comment
@AliceWonderMiscreations

AliceWonderMiscreations Nov 30, 2016

Off-topic I should ad that archived web pages are a real danger because if any of them link to a domain that has expired, the attacker only needs to register that domain and they can attack the archived page.

AliceWonderMiscreations commented Nov 30, 2016

Off-topic I should ad that archived web pages are a real danger because if any of them link to a domain that has expired, the attacker only needs to register that domain and they can attack the archived page.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Nov 30, 2016

Member
Off-topic @AliceWonderMiscreations can you please take your comments on changing rel=noopener in general to another issue? This is about a specific technical bug with window.open, and derailing it with a general dislike for an almost-unrelated feature is not appropriate.
Member

domenic commented Nov 30, 2016

Off-topic @AliceWonderMiscreations can you please take your comments on changing rel=noopener in general to another issue? This is about a specific technical bug with window.open, and derailing it with a general dislike for an almost-unrelated feature is not appropriate.
@AliceWonderMiscreations

This comment has been minimized.

Show comment
Hide comment
@AliceWonderMiscreations

AliceWonderMiscreations Nov 30, 2016

Off-topic Yes, I will start a new thread and I apologize. I generally stay out of standards discussions because I do not understand how to "play the game" so to speak, I posted here because it was an existing issue related to window.opener so I don't see it as off topic, my mind just doesn't work that way, I did not mean to derail, I apologize.

AliceWonderMiscreations commented Nov 30, 2016

Off-topic Yes, I will start a new thread and I apologize. I generally stay out of standards discussions because I do not understand how to "play the game" so to speak, I posted here because it was an existing issue related to window.opener so I don't see it as off topic, my mind just doesn't work that way, I did not mean to derail, I apologize.
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 9, 2017

Member

Did this get fixed together with #2474? The processing model should be much more clear since @zcorpan defined a parser.

Member

annevk commented Oct 9, 2017

Did this get fixed together with #2474? The processing model should be much more clear since @zcorpan defined a parser.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 9, 2017

Collaborator

This didn't really get fixed, because per spec this is not really a problem, in theory.

The key part is "per spec the only supported features are left/right/width/height, which doesn't match browser behavior". Though the spec has changed even on this and doesn't really support anything at all now.

In particular, per spec as currently written, the open steps tokenize the features (modulo #3107) and then ignore all the tokens except "noopener" (thus raising the question of what the point of https://html.spec.whatwg.org/multipage/window-object.html#normalizing-the-feature-name is for, of course). That's not what browsers actually do though; they pay attention to other things in that feature string and will turn off various features if they are not present in a nonempty feature string. So simply specifying "noopener" will turn off large chunks of the browser UI.

Collaborator

bzbarsky commented Oct 9, 2017

This didn't really get fixed, because per spec this is not really a problem, in theory.

The key part is "per spec the only supported features are left/right/width/height, which doesn't match browser behavior". Though the spec has changed even on this and doesn't really support anything at all now.

In particular, per spec as currently written, the open steps tokenize the features (modulo #3107) and then ignore all the tokens except "noopener" (thus raising the question of what the point of https://html.spec.whatwg.org/multipage/window-object.html#normalizing-the-feature-name is for, of course). That's not what browsers actually do though; they pay attention to other things in that feature string and will turn off various features if they are not present in a nonempty feature string. So simply specifying "noopener" will turn off large chunks of the browser UI.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 9, 2017

Member

Hmm, the point was that we'd defer to CSSOM for the remaining features. Ah, #2464 is still open and about that. Seems @zcorpan wasn't able to drive that to a conclusion yet.

Member

annevk commented Oct 9, 2017

Hmm, the point was that we'd defer to CSSOM for the remaining features. Ah, #2464 is still open and about that. Seems @zcorpan wasn't able to drive that to a conclusion yet.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 9, 2017

Collaborator

OK. We do need to actually pass the tokenized features to somewhere where CSSOM will see it...

But fwiw, on the HTML spec front just removing "noopener" from the tokenized features before passing on to the lower layer would presumably fix this problem, in that it would make it clear that noopener has no effect on the appearance.... Then we could at least file bugs on implementations to fix their broken stuff.

Collaborator

bzbarsky commented Oct 9, 2017

OK. We do need to actually pass the tokenized features to somewhere where CSSOM will see it...

But fwiw, on the HTML spec front just removing "noopener" from the tokenized features before passing on to the lower layer would presumably fix this problem, in that it would make it clear that noopener has no effect on the appearance.... Then we could at least file bugs on implementations to fix their broken stuff.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 9, 2017

Member

Wouldn't CSSOM ignore unknown tokens anyway?

It seems not passing it to CSSOM regressed somewhere after b649435. Sigh.

Member

annevk commented Oct 9, 2017

Wouldn't CSSOM ignore unknown tokens anyway?

It seems not passing it to CSSOM regressed somewhere after b649435. Sigh.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 9, 2017

Collaborator

Wouldn't CSSOM ignore unknown tokens anyway?

That doesn't match actual browser behavior (again in at least Firefox and Chrome). If I do:

window.open("", "", "yousaywhat?");

I get a browser window missing UI pieces in both.

Collaborator

bzbarsky commented Oct 9, 2017

Wouldn't CSSOM ignore unknown tokens anyway?

That doesn't match actual browser behavior (again in at least Firefox and Chrome). If I do:

window.open("", "", "yousaywhat?");

I get a browser window missing UI pieces in both.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 9, 2017

Member

Okay, I'd love @zcorpan to comment here with whether he took that into account when figuring all this out.

Member

annevk commented Oct 9, 2017

Okay, I'd love @zcorpan to comment here with whether he took that into account when figuring all this out.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 9, 2017

Member

I regressed not invoking CSSOM in 29afad4.

Member

annevk commented Oct 9, 2017

I regressed not invoking CSSOM in 29afad4.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 9, 2017

Member

I pushed a regression fix to #3108.

Member

annevk commented Oct 9, 2017

I pushed a regression fix to #3108.

jplaisted added a commit to google/closure-library that referenced this issue Oct 19, 2017

Open URL in new tab instead of new window to fix the URL flow.
RELNOTES: Introduces a workaround for window.open(..., 'noopener') which will cause the the URL to open in a new browser window with no bookmark bar or ability to add tabs. This is a known issue tracked here: whatwg/html#1902

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=172163219

shicks added a commit to google/closure-library that referenced this issue Oct 27, 2017

Automated g4 rollback of changelist 172163219.
*** Reason for rollback ***

Broke tests.

*** Original change description ***

Open URL in new tab instead of new window to fix the URL flow.

RELNOTES: Introduces a workaround for window.open(..., 'noopener') which will cause the the URL to open in a new browser window with no bookmark bar or ability to add tabs. This is a known issue tracked here: whatwg/html#1902

***

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=172165281

shicks added a commit to google/closure-library that referenced this issue Oct 27, 2017

Roll forward of CL 172163219: Open URL in new tab instead of new wind…
…ow to fix the URL flow.

NEW: Had to add if-check. Updating yt-navigation-manager in separate cl/172167161.

RELNOTES: Introduces a workaround for window.open(..., 'noopener') which will cause the the URL to open in a new browser window with no bookmark bar or ability to add tabs. This is a known issue tracked here: whatwg/html#1902

Automated g4 rollback of changelist 172234702.

*** Reason for rollback ***

Do not rollback this cl, instead, cr/172222691 would fix the breakage test.

*** Original change description ***

Automated g4 rollback of changelist 172221695.

*** Reason for rollback ***

Break youtube.

*** Original change description ***

Automated g4 rollback of changelist 172165281.

*** Reason for rollback ***

Had to add if-check. Updating yt-navigation-manager in separate cl/172167161.

*** Original change description ***

Automated g4 rollback...

***

ROLLBACK_OF=172234702
RELNOTES: rollback

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=172281082
@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Dec 4, 2017

Member

I was aware that unknown features are different from empty string, that was reported in https://www.w3.org/Bugs/Public/show_bug.cgi?id=25685#c3 and not yet fixed.

#2464 is indeed also about this. But a good first step would be to specify special treatment to the empty string (or maybe empty set of features) in cssom.

@bzbarsky

But fwiw, on the HTML spec front just removing "noopener" from the tokenized features before passing on to the lower layer would presumably fix this problem, in that it would make it clear that noopener has no effect on the appearance....

That's a good idea.

(Edit: fixed 2474 -> 2464 above.)

Member

zcorpan commented Dec 4, 2017

I was aware that unknown features are different from empty string, that was reported in https://www.w3.org/Bugs/Public/show_bug.cgi?id=25685#c3 and not yet fixed.

#2464 is indeed also about this. But a good first step would be to specify special treatment to the empty string (or maybe empty set of features) in cssom.

@bzbarsky

But fwiw, on the HTML spec front just removing "noopener" from the tokenized features before passing on to the lower layer would presumably fix this problem, in that it would make it clear that noopener has no effect on the appearance....

That's a good idea.

(Edit: fixed 2474 -> 2464 above.)

annevk added a commit that referenced this issue Dec 16, 2017

Remove noopener from tokenizedFeatures
Apparently CSSOM needs to react differently if it's there.

Helps with #1902.
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Dec 16, 2017

Member

Okay, I created #3297 to remove noopener before passing it on to CSSOM.

Member

annevk commented Dec 16, 2017

Okay, I created #3297 to remove noopener before passing it on to CSSOM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment