Summary: | Form navigations with target=_blank should not have an opener | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | Forms | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cdumez, changseok, clopez, darin, ehsan, esprehn+autocc, ews-watchlist, ggaren, gyuyoung.kim, mifenton, sam, webkit-bug-importer, wenson_hsieh, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 232170 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Chris Dumez
2021-10-25 08:11:05 PDT
Created attachment 442372 [details]
Patch
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 *** Bug 194398 has been marked as a duplicate of this bug. *** Created attachment 442377 [details]
Patch
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. 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? 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. (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. Created attachment 442403 [details]
Patch
(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. 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]. |