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.

Sign up for GitHub

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

Jump to bottom

SONARHTML-220 Fix FP S5148: Only raise an issue if rel="opener" is explicitly set #337

Merged
merged 1 commit into from
Nov 22, 2024
Merged

SONARHTML-220 Fix FP S5148: Only raise an issue if rel="opener" is explicitly set #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions its/ruling/src/test/resources/expected/Web-HeaderCheck.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1562,6 +1562,9 @@
"project:Silverpeas-Core-master/war-core/src/test/webapp/util/javaScript/silverpeas-datechecker.test.html": [
0
],
"project:custom/S5148.html": [
0
],
"project:custom/S6842.html": [
0
],
Expand Down
3 changes: 3 additions & 0 deletions its/ruling/src/test/resources/expected/Web-S1829.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1372,6 +1372,9 @@
81,
81
],
"project:custom/S5148.html": [
1
],
"project:external_webkit-jb-mr1/LayoutTests/dom/html/level1/core/resources/COPYRIGHT.html": [
18,
19,
Expand Down
64 changes: 2 additions & 62 deletions its/ruling/src/test/resources/expected/Web-S5148.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,65 +1,5 @@
{
"project:Silverpeas-Core-master/war-core/src/main/webapp/admin/jsp/TopBarSilverpeasV5.jsp": [
181
],
"project:Silverpeas-Core-master/war-core/src/main/webapp/attachment/jsp/displayAttachedFiles.jsp": [
245
],
"project:Silverpeas-Core-master/war-core/src/main/webapp/attachment/jsp/editAttachedFiles.jsp": [
465,
473
],
"project:Silverpeas-Core-master/war-core/src/main/webapp/jobManagerPeas/jsp/topBarManager.jsp": [
77
],
"project:external_webkit-jb-mr1/LayoutTests/fast/encoding/hanarei-blog32-fc2-com.html": [
20
],
"project:sonar-master/sonar-server/src/main/webapp/WEB-INF/app/views/drilldown/issues.html.erb": [
143
],
"project:sonar-master/sonar-server/src/main/webapp/WEB-INF/app/views/drilldown/measures.html.erb": [
65
],
"project:voten/resources/assets/js/components/GuestSidebar.vue": [
109,
116,
132
],
"project:voten/resources/assets/js/components/MarkdownGuide.vue": [
49
],
"project:voten/resources/assets/js/components/SearchModal.vue": [
37
],
"project:voten/resources/assets/js/components/auth/LeftSidebar.vue": [
134,
177
],
"project:voten/resources/assets/js/components/auth/RightSidebar.vue": [
55,
79,
88
],
"project:voten/resources/assets/js/components/pages/About.vue": [
44
],
"project:voten/resources/views/backend/spams/comments.blade.php": [
46
],
"project:voten/resources/views/backend/spams/submissions.blade.php": [
46
],
"project:voten/resources/views/vendor/mail/html/button.blade.php": [
10
],
"project:voten/resources/views/vendor/mail/html/promotion/button.blade.php": [
7
],
"project:voten/resources/views/vendor/notifications/email.blade.php": [
73,
123,
157,
179
"project:custom/S5148.html": [
1
]
}
2 changes: 1 addition & 1 deletion its/sources
Open in desktop
Submodule sources updated from 49de9f to ef2791
26 changes: 14 additions & 12 deletions ...ml-plugin/src/main/java/org/sonar/plugins/html/checks/sonar/LinkWithTargetBlankCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
package org.sonar.plugins.html.checks.sonar;

import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.regex.Pattern;
import org.sonar.check.Rule;
Expand All @@ -30,31 +28,35 @@
public class LinkWithTargetBlankCheck extends AbstractPageCheck {

private static final Pattern DYNAMIC_URL = Pattern.compile("[{}$()\\[\\]]");
private static final String NOOPENER = "NOOPENER";

@Override
public void startElement(TagNode node) {
if (isAnchorWithTargetBlank(node) && isInsecureUrl(node) && missingRelAttribute(node)) {
createViolation(node, "Make sure not using rel=\"noopener\" is safe here.");
if (isAnchor(node) && isInsecureUrl(node) && isVulnerable(node)) {
createViolation(node, "Make sure using target=\"_blank\" and rel=\"opener\" is safe here.");
}
}

private static boolean missingRelAttribute(TagNode node) {
return !relAttributeValues(node).contains(NOOPENER);
private static boolean isVulnerable(TagNode node) {
return hasBlankTarget(node) && hasRelOpener(node);
}

private static List<String> relAttributeValues(TagNode node) {
private static boolean hasBlankTarget(TagNode node) {
String target = node.getPropertyValue("TARGET");
return "_blank".equalsIgnoreCase(target);
}

private static boolean hasRelOpener(TagNode node) {
String rel = node.getPropertyValue("REL");
if (rel == null) {
return Collections.emptyList();
return false;
}
return Arrays.stream(rel.split(" "))
.map(s -> s.trim().toUpperCase(Locale.ROOT))
.toList();
.anyMatch("opener"::equalsIgnoreCase);
}

private static boolean isAnchorWithTargetBlank(TagNode node) {
return node.equalsElementName("A") && "_BLANK".equalsIgnoreCase(node.getPropertyValue("TARGET"));
private static boolean isAnchor(TagNode node) {
return node.equalsElementName("A");
}

private static boolean isInsecureUrl(TagNode node) {
Expand Down
16 changes: 6 additions & 10 deletions sonar-html-plugin/src/main/resources/org/sonar/l10n/web/rules/Web/S5148.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,24 @@ <h2>Ask Yourself Whether</h2>
<li> The application opens untrusted external URL. </li>
</ul>
<p>There is a risk if you answered yes to this question.</p>
<h2>Recommended Secure Coding Practices</h2>
<p>Use <code>noopener</code> to prevent untrusted pages from abusing <code>window.opener</code>.</p>
<p>Note: In Chrome 88+, Firefox 79+ or Safari 12.1+ <code>target=_blank</code> on anchors implies <code>rel=noopener</code> which make the protection
enabled by default.</p>
<h2>Sensitive Code Example</h2>
<pre>
&lt;a href="http://example.com/dangerous" target="_blank"&gt; &lt;!-- Sensitive --&gt;
&lt;a href="http://example.com/" rel="opener" target="_blank"&gt; &lt;!-- Sensitive --&gt;

&lt;a href="{{variable}}" target="_blank"&gt; &lt;!-- Sensitive --&gt;
&lt;a href="{{variable}}" rel="opener" target="_blank"&gt; &lt;!-- Sensitive --&gt;
</pre>
<h2>Compliant Solution</h2>
<p>To prevent pages from abusing <code>window.opener</code>, use <code>rel=noopener</code> on <code>&lt;a href=&gt;</code> to force its value to be
<code>null</code> on the opened pages.</p>
<p>In Chrome 88+, Firefox 79+ or Safari 12.1+ <code>target=_blank</code> on anchors implies <code>rel=noopener</code> which makes the protection
enabled by default.</p>
<pre>
&lt;a href="http://petssocialnetwork.io" target="_blank" rel="noopener"&gt;
&lt;a href="https://example.com/" target="_blank" &gt;
</pre>
<h2>Exceptions</h2>
<p>No Issue will be raised when <code>href</code> contains a hardcoded relative url as there it has less chances of being vulnerable. An url is
considered hardcoded and relative if it doesn’t start with <code>http://</code> or <code>https://</code>, and if it does not contain any of the
characters {}$()[]</p>
<pre>
&lt;a href="internal.html" target="_blank" &gt;
&lt;a href="internal.html" rel="opener" target="_blank" &gt;
</pre>
<h2>See</h2>
<ul>
Expand Down
6 changes: 3 additions & 3 deletions ...lugin/src/test/java/org/sonar/plugins/html/checks/sonar/LinkWithTargetBlankCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ public class LinkWithTargetBlankCheckTest {
public void detected() throws Exception {
HtmlSourceCode sourceCode = TestHelper.scan(new File("src/test/resources/checks/LinkWithTargetBlank.html"), new LinkWithTargetBlankCheck());
checkMessagesVerifier.verify(sourceCode.getIssues())
.next().atLocation(1, 0, 1, 54).withMessage("Make sure not using rel=\"noopener\" is safe here.")
.next().atLocation(1, 0, 1, 67).withMessage("Make sure using target=\"_blank\" and rel=\"opener\" is safe here.")
.next().atLine(2)
.next().atLine(7)
.next().atLine(9);
.next().atLine(3)
.next().atLine(8);
}

}
10 changes: 6 additions & 4 deletions sonar-html-plugin/src/test/resources/checks/LinkWithTargetBlank.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
<a href="http://dangerouswebsite.com" target="_blank"> <!-- Noncompliant; Make sure not using rel="noopener" is safe here. -->
<a href="https://dangerouswebsite.com" target="_blank"> <!-- Noncompliant; Make sure not using rel="noopener" is safe here. -->
<a href="http://dangerouswebsite.com" target="_blank" rel="opener"> <!-- Noncompliant; Make sure using target="_blank" and rel="opener" is safe here. -->
<a href="https://dangerouswebsite.com" target="_blank" rel="opener"> <!-- Noncompliant; Make sure using target="_blank" and rel="opener" is safe here. -->
<a href="https://dangerouswebsite.com" target="_blank" rel="whatever opener"> <!-- Noncompliant; Make sure using target="_blank" and rel="opener" is safe here. -->

<a href="http://dangerouswebsite.com" target="_blank" rel="noopener noreferrer"> <!-- Compliant -->
<a href="http://dangerouswebsite.com" target="_blank" rel="noopener"> <!-- Compliant -->

<a href="{{variable}}" target="_blank" rel="noreferrer"> <!-- Noncompliant; Make sure not using rel="noopener" is safe here. -->
<a href="{{variable}}" target="_blank" rel="opener"> <!-- Noncompliant; Make sure using target="_blank" and rel="opener" is safe here. -->
<a href="{{variable}}" target="_blank" rel="noreferrer"> <!-- Compliant -->
<a href="{{variable}}" target="_blank" rel="noopener"> <!-- Compliant -->
<a href="{{variable}}" target="_blank"> <!-- Noncompliant -->
<a href="{{variable}}" target="_blank"> <!-- Compliant -->
<a href="/{{variable}}" target="_blank"> <!-- Compliant -->
<a href="./{{variable}}" target="_blank"> <!-- Compliant -->
<a href="../{{variable}}" target="_blank"> <!-- Compliant -->
Expand Down
Loading