Skip to content

Commit 17638e7

Browse files
author
[email protected]
committed
Experiment: target=_blank on anchors should imply rel=noopener
https://bugs.webkit.org/show_bug.cgi?id=190481 Reviewed by Alex Christensen. Source/WebCore: As an experiment, try and make it so that target=_blank on anchors implies `rel=noopener` for improved security. WebContent can then request an opener relationship by using `rel=opener` instead. This change was discussed at: - whatwg/html#4078 We want to attempt this change is STP to see if it is Web-compatible. Preliminary testing seems to indicate that OAuth workflows still work. * html/HTMLAnchorElement.cpp: (WebCore::HTMLAnchorElement::parseAttribute): (WebCore::HTMLAnchorElement::handleClick): (WebCore::HTMLAnchorElement::effectiveTarget const): * html/HTMLAnchorElement.h: * page/RuntimeEnabledFeatures.h: (WebCore::RuntimeEnabledFeatures::setBlankAnchorTargetImpliesNoOpenerEnabled): (WebCore::RuntimeEnabledFeatures::blankAnchorTargetImpliesNoOpenerEnabled const): Source/WebKit: * Shared/WebPreferences.yaml: Tools: Add API test coverage to make sure we can now swap process when target=_blank is specified on an anchor but rel=noopener is not. * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm: LayoutTests: Update existing tests to reflect behavior change. * TestExpectations: * http/tests/navigation/no-referrer-reset.html: * http/tests/security/resources/referrer-policy-redirect-link.html: * http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html: * http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html: * http/tests/security/xssAuditor/link-opens-new-window.html: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237144 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 0c0d330 commit 17638e7

18 files changed

+199
-7
lines changed

‎LayoutTests/ChangeLog

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
2018-10-15 Chris Dumez <[email protected]>
2+
3+
Experiment: target=_blank on anchors should imply rel=noopener
4+
https://bugs.webkit.org/show_bug.cgi?id=190481
5+
6+
Reviewed by Alex Christensen.
7+
8+
Update existing tests to reflect behavior change.
9+
10+
* TestExpectations:
11+
* http/tests/navigation/no-referrer-reset.html:
12+
* http/tests/security/resources/referrer-policy-redirect-link.html:
13+
* http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html:
14+
* http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html:
15+
* http/tests/security/xssAuditor/link-opens-new-window.html:
16+
117
2018-10-15 Andy Estes <[email protected]>
218

319
[Apple Pay] New shipping methods are ignored when updating after the shippingaddresschange event

‎LayoutTests/TestExpectations

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2865,6 +2865,13 @@ webkit.org/b/185308 legacy-animation-engine/animations/combo-transform-translate
28652865
# This newly imported test crashes in debug and flakily times out.
28662866
webkit.org/b/189917 imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/contentType.window.html [ Skip ]
28672867

2868+
# These tests started to time out or fail because of our experiment to have target=_blank on anchors imply rel=noopener (https://bugs.webkit.org/show_bug.cgi?id=190481).
2869+
imported/w3c/web-platform-tests/html/browsers/windows/auxiliary-browsing-contexts/opener-closed.html [ Skip ]
2870+
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/002.html [ Failure ]
2871+
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html [ Skip ]
2872+
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html [ Skip ]
2873+
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_blank-003.html [ Failure ]
2874+
28682875
fast/gradients/conic-repeating.html [ Skip ]
28692876
fast/gradients/conic.html [ Skip ]
28702877
fast/gradients/conic-off-center.html [ Skip ]

‎LayoutTests/http/tests/navigation/anchor-blank-target-implies-rel-noopener-expected.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
CONSOLE MESSAGE: line 6: PASS: New window should not have an opener
2+
Tests that a new window opened via target=_blank does not have an opener
3+
4+

‎LayoutTests/http/tests/navigation/anchor-blank-target-implies-rel-noopener.html

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<p>Tests that a new window opened via target=_blank does not have an opener</p>
5+
<a id="testAnchor" href="resources/anchor-blank-target-implies-rel-noopener-win.html" target="_blank"></a>
6+
<script>
7+
if (window.testRunner) {
8+
testRunner.dumpAsText();
9+
testRunner.waitUntilDone();
10+
testRunner.setCanOpenWindows();
11+
}
12+
13+
onload = function() {
14+
setTimeout(() => {
15+
testAnchor.click();
16+
}, 0);
17+
}
18+
</script>
19+
</body>
20+
</html>

‎LayoutTests/http/tests/navigation/no-referrer-reset.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
2. Click a rel="noreferrer" link: referrer is null, but window.opener remains set since the link was not opened with target="_blank".<br/>
55
3. Click a link without rel="noreferrer": referrer is sent, but window.opener is still set.
66
<br/>
7-
<a id="link" href="resources/no-referrer-reset-helper.php" target="_blank">Start reset test</a>
7+
<a id="link" href="resources/no-referrer-reset-helper.php" target="_blank" rel="opener">Start reset test</a>
88
<script>
99
window.name = "consoleWindow";
1010
window.noreferrerStepDone = false;

‎LayoutTests/http/tests/navigation/resources/anchor-blank-target-implies-rel-noopener-win.html

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<script>
5+
if (!window.opener)
6+
console.log("PASS: New window should not have an opener");
7+
else
8+
console.log("FAIL: New window should not have an opener");
9+
10+
if (window.testRunner)
11+
testRunner.notifyDone();
12+
</script>
13+
</body>
14+
</html>

‎LayoutTests/http/tests/security/resources/referrer-policy-redirect-link.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
</script>
2222
</head>
2323
<body>
24-
<a id="link" target="_blank" href="https://127.0.0.1:8443/resources/redirect.php?url=http://127.0.0.1:8000/security/resources/referrer-policy-postmessage.php">If not running in DumpRenderTree, click this link</a>
24+
<a id="link" target="_blank" href="https://127.0.0.1:8443/resources/redirect.php?url=http://127.0.0.1:8000/security/resources/referrer-policy-postmessage.php" rel="opener">If not running in DumpRenderTree, click this link</a>
2525
<div id="log"></div>
2626
</body>
2727
</html>

‎LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
// Case: Initial load
3636
var link = document.createElement("a");
3737
link.target = "_blank";
38+
link.rel = "opener";
3839
link.href = "?actually-attack";
3940
link.click(); // Open a new window.
4041
}

‎LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
// Case: Initial load
3636
var link = document.createElement("a");
3737
link.target = "_blank";
38+
link.rel = "opener";
3839
link.href = "?actually-attack";
3940
link.click(); // Open a new window.
4041
}

‎LayoutTests/http/tests/security/xssAuditor/link-opens-new-window.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,6 @@
1919
</script>
2020
</head>
2121
<body>
22-
<a id="anchorLink" href="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?test=/security/xssAuditor/link-opens-new-window.html&notifyDone=1&q=<script>alert(String.fromCharCode(0x58,0x53,0x53))</script>" target="_blank">Click me</a>
22+
<a id="anchorLink" href="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?test=/security/xssAuditor/link-opens-new-window.html&notifyDone=1&q=<script>alert(String.fromCharCode(0x58,0x53,0x53))</script>" target="_blank" rel="opener">Click me</a>
2323
</body>
2424
</html>

‎Source/WebCore/ChangeLog

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,28 @@
1+
2018-10-15 Chris Dumez <[email protected]>
2+
3+
Experiment: target=_blank on anchors should imply rel=noopener
4+
https://bugs.webkit.org/show_bug.cgi?id=190481
5+
6+
Reviewed by Alex Christensen.
7+
8+
As an experiment, try and make it so that target=_blank on anchors implies `rel=noopener` for improved security.
9+
WebContent can then request an opener relationship by using `rel=opener` instead.
10+
11+
This change was discussed at:
12+
- https://github.com/whatwg/html/issues/4078
13+
14+
We want to attempt this change is STP to see if it is Web-compatible. Preliminary testing seems to indicate
15+
that OAuth workflows still work.
16+
17+
* html/HTMLAnchorElement.cpp:
18+
(WebCore::HTMLAnchorElement::parseAttribute):
19+
(WebCore::HTMLAnchorElement::handleClick):
20+
(WebCore::HTMLAnchorElement::effectiveTarget const):
21+
* html/HTMLAnchorElement.h:
22+
* page/RuntimeEnabledFeatures.h:
23+
(WebCore::RuntimeEnabledFeatures::setBlankAnchorTargetImpliesNoOpenerEnabled):
24+
(WebCore::RuntimeEnabledFeatures::blankAnchorTargetImpliesNoOpenerEnabled const):
25+
126
2018-10-15 Andy Estes <[email protected]>
227

328
[Apple Pay] New shipping methods are ignored when updating after the shippingaddresschange event

‎Source/WebCore/html/HTMLAnchorElement.cpp

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,12 +250,15 @@ void HTMLAnchorElement::parseAttribute(const QualifiedName& name, const AtomicSt
250250
// Update HTMLAnchorElement::relList() if more rel attributes values are supported.
251251
static NeverDestroyed<AtomicString> noReferrer("noreferrer", AtomicString::ConstructFromLiteral);
252252
static NeverDestroyed<AtomicString> noOpener("noopener", AtomicString::ConstructFromLiteral);
253+
static NeverDestroyed<AtomicString> opener("opener", AtomicString::ConstructFromLiteral);
253254
const bool shouldFoldCase = true;
254255
SpaceSplitString relValue(value, shouldFoldCase);
255256
if (relValue.contains(noReferrer))
256257
m_linkRelations.add(Relation::NoReferrer);
257258
if (relValue.contains(noOpener))
258259
m_linkRelations.add(Relation::NoOpener);
260+
if (relValue.contains(opener))
261+
m_linkRelations.add(Relation::Opener);
259262
if (m_relList)
260263
m_relList->associatedAttributeValueChanged(value);
261264
}
@@ -427,12 +430,28 @@ void HTMLAnchorElement::handleClick(Event& event)
427430
#endif
428431

429432
ShouldSendReferrer shouldSendReferrer = hasRel(Relation::NoReferrer) ? NeverSendReferrer : MaybeSendReferrer;
430-
auto newFrameOpenerPolicy = hasRel(Relation::NoOpener) ? std::make_optional(NewFrameOpenerPolicy::Suppress) : std::nullopt;
431-
frame->loader().urlSelected(completedURL, target(), &event, LockHistory::No, LockBackForwardList::No, shouldSendReferrer, document().shouldOpenExternalURLsPolicyToPropagate(), newFrameOpenerPolicy, downloadAttribute, systemPreviewInfo);
433+
434+
auto effectiveTarget = this->effectiveTarget();
435+
std::optional<NewFrameOpenerPolicy> newFrameOpenerPolicy;
436+
if (hasRel(Relation::Opener))
437+
newFrameOpenerPolicy = NewFrameOpenerPolicy::Allow;
438+
else if (hasRel(Relation::NoOpener) || (RuntimeEnabledFeatures::sharedFeatures().blankAnchorTargetImpliesNoOpenerEnabled() && equalIgnoringASCIICase(effectiveTarget, "_blank")))
439+
newFrameOpenerPolicy = NewFrameOpenerPolicy::Suppress;
440+
441+
frame->loader().urlSelected(completedURL, effectiveTarget, &event, LockHistory::No, LockBackForwardList::No, shouldSendReferrer, document().shouldOpenExternalURLsPolicyToPropagate(), newFrameOpenerPolicy, downloadAttribute, systemPreviewInfo);
432442

433443
sendPings(completedURL);
434444
}
435445

446+
// Falls back to using <base> element's target if the anchor does not have one.
447+
String HTMLAnchorElement::effectiveTarget() const
448+
{
449+
auto effectiveTarget = target();
450+
if (effectiveTarget.isEmpty())
451+
effectiveTarget = document().baseTarget();
452+
return effectiveTarget;
453+
}
454+
436455
HTMLAnchorElement::EventType HTMLAnchorElement::eventType(Event& event)
437456
{
438457
if (!is<MouseEvent>(event))

‎Source/WebCore/html/HTMLAnchorElement.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ class DOMTokenList;
3737
enum class Relation {
3838
NoReferrer = 1 << 0,
3939
NoOpener = 1 << 1,
40+
Opener = 1 << 2,
4041
};
4142

4243
class HTMLAnchorElement : public HTMLElement, public URLUtils<HTMLAnchorElement> {
@@ -90,6 +91,8 @@ class HTMLAnchorElement : public HTMLElement, public URLUtils<HTMLAnchorElement>
9091
int tabIndex() const final;
9192
bool draggable() const final;
9293

94+
String effectiveTarget() const;
95+
9396
void sendPings(const URL& destinationURL);
9497

9598
void handleClick(Event&);

‎Source/WebCore/page/RuntimeEnabledFeatures.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@ namespace WebCore {
4343
class RuntimeEnabledFeatures {
4444
WTF_MAKE_NONCOPYABLE(RuntimeEnabledFeatures);
4545
public:
46+
void setBlankAnchorTargetImpliesNoOpenerEnabled(bool isEnabled) { m_blankAnchorTargetImpliesNoOpenerEnabled = isEnabled; }
47+
bool blankAnchorTargetImpliesNoOpenerEnabled() const { return m_blankAnchorTargetImpliesNoOpenerEnabled; }
48+
4649
void setDisplayContentsEnabled(bool isEnabled) { m_isDisplayContentsEnabled = isEnabled; }
4750
bool displayContentsEnabled() const { return m_isDisplayContentsEnabled; }
4851

@@ -300,6 +303,7 @@ class RuntimeEnabledFeatures {
300303
// Never instantiate.
301304
RuntimeEnabledFeatures();
302305

306+
bool m_blankAnchorTargetImpliesNoOpenerEnabled { true };
303307
bool m_areModernMediaControlsEnabled { false };
304308
bool m_isLinkPreloadEnabled { true };
305309
bool m_isLinkPrefetchEnabled { false };

‎Source/WebKit/ChangeLog

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,12 @@
1+
2018-10-15 Chris Dumez <[email protected]>
2+
3+
Experiment: target=_blank on anchors should imply rel=noopener
4+
https://bugs.webkit.org/show_bug.cgi?id=190481
5+
6+
Reviewed by Alex Christensen.
7+
8+
* Shared/WebPreferences.yaml:
9+
110
2018-10-15 Alex Christensen <[email protected]>
211

312
Remove unused parameters from FrameLoaderClient::createFrame

‎Source/WebKit/Shared/WebPreferences.yaml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
1+
BlankAnchorTargetImpliesNoOpenerEnabled:
2+
type: bool
3+
defaultValue: true
4+
webcoreBinding: RuntimeEnabledFeatures
5+
humanReadableName: "Blank anchor target implies rel=noopener"
6+
humanReadableDescription: "target=_blank on anchor elements implies rel=noopener"
7+
category: experimental
8+
19
JavaScriptEnabled:
210
type: bool
311
defaultValue: true

‎Tools/ChangeLog

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,15 @@
1+
2018-10-15 Chris Dumez <[email protected]>
2+
3+
Experiment: target=_blank on anchors should imply rel=noopener
4+
https://bugs.webkit.org/show_bug.cgi?id=190481
5+
6+
Reviewed by Alex Christensen.
7+
8+
Add API test coverage to make sure we can now swap process when target=_blank
9+
is specified on an anchor but rel=noopener is not.
10+
11+
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
12+
113
2018-10-15 Wenson Hsieh <[email protected]>
214

315
[iOS] Can't select text after dismissing the keyboard when changing focus

‎Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,16 @@ function log(msg)
280280
</script>
281281
)PSONRESOURCE";
282282

283-
static const char* targetBlankCrossSiteWithOpenerTestBytes = R"PSONRESOURCE(
283+
static const char* targetBlankCrossSiteWithExplicitOpenerTestBytes = R"PSONRESOURCE(
284+
<a id="testLink" target="_blank" href="pson://www.apple.com/main.html" rel="opener">Link</a>
285+
<script>
286+
window.onload = function() {
287+
testLink.click();
288+
}
289+
</script>
290+
)PSONRESOURCE";
291+
292+
static const char* targetBlankCrossSiteWithImplicitNoOpenerTestBytes = R"PSONRESOURCE(
284293
<a id="testLink" target="_blank" href="pson://www.apple.com/main.html">Link</a>
285294
<script>
286295
window.onload = function() {
@@ -693,7 +702,7 @@ function log(msg)
693702
auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
694703
[webViewConfiguration setProcessPool:processPool.get()];
695704
auto handler = adoptNS([[PSONScheme alloc] init]);
696-
[handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:targetBlankCrossSiteWithOpenerTestBytes];
705+
[handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:targetBlankCrossSiteWithExplicitOpenerTestBytes];
697706
[webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
698707

699708
auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
@@ -724,6 +733,46 @@ function log(msg)
724733
EXPECT_EQ(pid1, pid2);
725734
}
726735

736+
TEST(ProcessSwap, CrossSiteBlankTargetImplicitNoOpener)
737+
{
738+
auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
739+
processPoolConfiguration.get().processSwapsOnNavigation = YES;
740+
auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
741+
742+
auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
743+
[webViewConfiguration setProcessPool:processPool.get()];
744+
auto handler = adoptNS([[PSONScheme alloc] init]);
745+
[handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:targetBlankCrossSiteWithImplicitNoOpenerTestBytes];
746+
[webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
747+
748+
auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
749+
auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
750+
[webView setNavigationDelegate:navigationDelegate.get()];
751+
auto uiDelegate = adoptNS([[PSONUIDelegate alloc] initWithNavigationDelegate:navigationDelegate.get()]);
752+
[webView setUIDelegate:uiDelegate.get()];
753+
754+
numberOfDecidePolicyCalls = 0;
755+
NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
756+
[webView loadRequest:request];
757+
758+
TestWebKitAPI::Util::run(&done);
759+
done = false;
760+
761+
TestWebKitAPI::Util::run(&didCreateWebView);
762+
didCreateWebView = false;
763+
764+
TestWebKitAPI::Util::run(&done);
765+
766+
EXPECT_EQ(3, numberOfDecidePolicyCalls);
767+
768+
auto pid1 = [webView _webProcessIdentifier];
769+
EXPECT_TRUE(!!pid1);
770+
auto pid2 = [createdWebView _webProcessIdentifier];
771+
EXPECT_TRUE(!!pid2);
772+
773+
EXPECT_NE(pid1, pid2);
774+
}
775+
727776
TEST(ProcessSwap, CrossSiteBlankTargetNoOpener)
728777
{
729778
auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);

0 commit comments

Comments
 (0)