Bug 232243 – Form navigations with target=_blank should not have an opener

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Chris Dumez]

Form navigations with target=_blank should not have an opener, similarly to link navigations.

[Chris Dumez]

Created attachment 442372 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Anne van Kesteren]

*** Bug 194398 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 442377 [details]
Patch

[Sam Weinig]

Comment on attachment 442377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442377&action=review

> Source/WebCore/ChangeLog:10
> +        on the <form>), similarly to link navigations. This is better for security and allows us to
> +        process-swap in more cases.

Would be good to explain in a bit more detail the security benefits.

[Darin Adler]

Comment on attachment 442377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442377&action=review

> Source/WebCore/html/HTMLFormElement.cpp:431
> +    if (relAttributes.noopener || relAttributes.noreferrer || (!relAttributes.opener && document().settings().blankAnchorTargetImpliesNoOpenerEnabled() && equalIgnoringASCIICase(formSubmission->target(), "_blank")))

Why is blankAnchorTargetImpliesNoOpenerEnabled a setting? Change log doesn’t mention this.

If "_blank" is a special value for target, I am surprised that we have the constant here. Is there anywhere else we treat it specially? Can we share a constant or a function to check it?

[Chris Dumez]

Comment on attachment 442377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442377&action=review

>> Source/WebCore/html/HTMLFormElement.cpp:431
>> +    if (relAttributes.noopener || relAttributes.noreferrer || (!relAttributes.opener && document().settings().blankAnchorTargetImpliesNoOpenerEnabled() && equalIgnoringASCIICase(formSubmission->target(), "_blank")))
> 
> Why is blankAnchorTargetImpliesNoOpenerEnabled a setting? Change log doesn’t mention this.
> 
> If "_blank" is a special value for target, I am surprised that we have the constant here. Is there anywhere else we treat it specially? Can we share a constant or a function to check it?

Even though this is now part of the specification, WebKit was the first one to imply "noopener" when using target=_blank. As a result, this was implemented behind an experimental feature flag. Now that this is in the specification and has shipped in Safari a while back, I guess we could drop the feature flag too.

I will check if we have a _blank constant somewhere.

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 442377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442377&action=review
> 
> >> Source/WebCore/html/HTMLFormElement.cpp:431
> >> +    if (relAttributes.noopener || relAttributes.noreferrer || (!relAttributes.opener && document().settings().blankAnchorTargetImpliesNoOpenerEnabled() && equalIgnoringASCIICase(formSubmission->target(), "_blank")))
> > 
> > Why is blankAnchorTargetImpliesNoOpenerEnabled a setting? Change log doesn’t mention this.
> > 
> > If "_blank" is a special value for target, I am surprised that we have the constant here. Is there anywhere else we treat it specially? Can we share a constant or a function to check it?
> 
> Even though this is now part of the specification, WebKit was the first one
> to imply "noopener" when using target=_blank. As a result, this was
> implemented behind an experimental feature flag. Now that this is in the
> specification and has shipped in Safari a while back, I guess we could drop
> the feature flag too.
> 
> I will check if we have a _blank constant somewhere.

It doesn't seem we have a _blank constant somewhere currently. We could add one but I am not sure where would be the best place to add it.

[Chris Dumez]

Created attachment 442403 [details]
Patch

[Darin Adler]

(In reply to Chris Dumez from comment #8)
> It doesn't seem we have a _blank constant somewhere currently. We could add
> one but I am not sure where would be the best place to add it.

I will take care of this next time I have a chance; I’ll look at the different uses of "_blank" and try to figure out how to make them share a constant. Or maybe share a function. Presumably the different places that process target frame names have some shared patterns.

[EWS]

Committed r284821 (243513@main): <https://commits.webkit.org/243513@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442403 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84630994>