Repository: shapesecurity/salvation (java)

Issue:
Warn on duplicate keyword-sources
Ex. `script-src 'unsafe-inline' 'unsafe-inline'` 

Pull request:
Warn about duplicate source expressions
fixes #185

Patch:
diff --git a/src/main/java/com/shapesecurity/salvation/Constants.java b/src/main/java/com/shapesecurity/salvation/Constants.java
index 6ece263..f78e8a1 100644
--- a/src/main/java/com/shapesecurity/salvation/Constants.java
+++ b/src/main/java/com/shapesecurity/salvation/Constants.java
@@ -14,10 +14,9 @@ import java.util.regex.Pattern;
             + "|scripts|top-navigation)$");
     public static final Pattern requireSriForEnumeratedTokenPattern = Pattern
             .compile("^(?:script|style)$", Pattern.CASE_INSENSITIVE);
-
     // RFC 2045 appendix A: productions of type and subtype
     public static final Pattern mediaTypePattern = Pattern.compile("^(?<type>[a-zA-Z0-9!#$%^&*\\-_+{}|'.`~]+)/(?<subtype>[a-zA-Z0-9!#$%^&*\\-_+{}|'.`~]+)$");
-    public static final Pattern unquotedKeywordPattern = Pattern.compile("^(?:self|unsafe-inline|unsafe-eval|unsafe-redirect|none|strict-dynamic)$");
+    public static final Pattern unquotedKeywordPattern = Pattern.compile("^(?:self|unsafe-inline|unsafe-eval|unsafe-redirect|none|strict-dynamic|unsafe-hashed-attributes|report-sample)$");
     // port-part constants
     public static final int WILDCARD_PORT = -200;
     public static final int EMPTY_PORT = -1;
diff --git a/src/main/java/com/shapesecurity/salvation/Parser.java b/src/main/java/com/shapesecurity/salvation/Parser.java
index cce27e3..d036143 100644
--- a/src/main/java/com/shapesecurity/salvation/Parser.java
+++ b/src/main/java/com/shapesecurity/salvation/Parser.java
@@ -394,7 +394,9 @@ public class Parser {
                 } else if (se == KeywordSource.ReportSample) {
                     seenStates.add(SeenStates.SEEN_REPORT_SAMPLE);
                 }
-                sourceExpressions.add(se);
+                if (!sourceExpressions.add(se)) {
+                    this.warn(this.tokens[this.index - 1],"Source list contains duplicate source expression \"" + se.show() + "\". All but the first instance will be ignored.");
+                }
             } catch (DirectiveValueParseException e) {
                 parseException = true;
             }


Test patch:
diff --git a/src/test/java/com/shapesecurity/salvation/ParserTest.java b/src/test/java/com/shapesecurity/salvation/ParserTest.java
index be2736d..e2ab807 100644
--- a/src/test/java/com/shapesecurity/salvation/ParserTest.java
+++ b/src/test/java/com/shapesecurity/salvation/ParserTest.java
@@ -1024,20 +1024,26 @@ public class ParserTest extends CSPTest {
         notices.clear();
         p = parseWithNotices("default-src 'unsafe-hashed-attributes' 'unsafe-hashed-attributes' 'unsafe-hashed-attributes'", notices);
         assertEquals(1, p.getDirectives().size());
-        assertEquals(1, notices.size());
-        assertEquals("The \"'unsafe-hashed-attributes'\" keyword-source has no effect in source lists that do not contain hash-source in CSP3 and later.", notices.get(0).message);
+        assertEquals(3, notices.size());
+        assertEquals("Source list contains duplicate source expression \"'unsafe-hashed-attributes'\". All but the first instance will be ignored.", notices.get(0).message);
+        assertEquals("Source list contains duplicate source expression \"'unsafe-hashed-attributes'\". All but the first instance will be ignored.", notices.get(1).message);
+        assertEquals("The \"'unsafe-hashed-attributes'\" keyword-source has no effect in source lists that do not contain hash-source in CSP3 and later.", notices.get(2).message);
 
         notices.clear();
         p = parseWithNotices("default-src 'unsafe-hashed-attributes' 'unsafe-hashed-attributes' 'unsafe-hashed-attributes' 'nonce-123'", notices);
         assertEquals(1, p.getDirectives().size());
-        assertEquals(2, notices.size());
-        assertEquals("Invalid base64-value (should be multiple of 4 bytes: 3).", notices.get(0).message);
-        assertEquals("The \"'unsafe-hashed-attributes'\" keyword-source has no effect in source lists that do not contain hash-source in CSP3 and later.", notices.get(1).message);
+        assertEquals(4, notices.size());
+        assertEquals("Source list contains duplicate source expression \"'unsafe-hashed-attributes'\". All but the first instance will be ignored.", notices.get(0).message);
+        assertEquals("Source list contains duplicate source expression \"'unsafe-hashed-attributes'\". All but the first instance will be ignored.", notices.get(1).message);
+        assertEquals("Invalid base64-value (should be multiple of 4 bytes: 3).", notices.get(2).message);
+        assertEquals("The \"'unsafe-hashed-attributes'\" keyword-source has no effect in source lists that do not contain hash-source in CSP3 and later.", notices.get(3).message);
 
         notices.clear();
         p = parseWithNotices("default-src 'unsafe-hashed-attributes' 'unsafe-hashed-attributes' 'unsafe-hashed-attributes' 'sha512-vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg=='", notices);
         assertEquals(1, p.getDirectives().size());
-        assertEquals(0, notices.size());
+        assertEquals(2, notices.size());
+        assertEquals("Source list contains duplicate source expression \"'unsafe-hashed-attributes'\". All but the first instance will be ignored.", notices.get(0).message);
+        assertEquals("Source list contains duplicate source expression \"'unsafe-hashed-attributes'\". All but the first instance will be ignored.", notices.get(1).message);
 
         notices.clear();
         p = parseWithNotices("default-src 'sha512-vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg=='", notices);
@@ -1062,7 +1068,8 @@ public class ParserTest extends CSPTest {
         p = parseWithNotices("script-src 'report-sample' 'report-sample'", notices);
         assertEquals(1, p.getDirectives().size());
         assertEquals("script-src 'report-sample'", p.getDirectiveByType(ScriptSrcDirective.class).show());
-        assertEquals(0, notices.size());
+        assertEquals(1, notices.size());
+        assertEquals("Source list contains duplicate source expression \"'report-sample'\". All but the first instance will be ignored.", notices.get(0).show());
 
         notices.clear();
         p = parseWithNotices("default-src 'strict-dynamic' 'report-sample'", notices);
@@ -1074,4 +1081,52 @@ public class ParserTest extends CSPTest {
         assertEquals(1, p.getDirectives().size());
         assertEquals(0, notices.size());
     }
+
+    @Test public void testDuplicateSourceExpressions() {
+        Policy p;
+        ArrayList<Notice> notices = new ArrayList<>();
+        p = ParserWithLocation.parse("script-src 'unsafe-inline' 'unsafe-inline'", "https://origin.com", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(1, notices.size());
+        assertEquals("1:28: Source list contains duplicate source expression \"'unsafe-inline'\". All but the first instance will be ignored.", notices.get(0).show());
+
+        notices.clear();
+        p = ParserWithLocation.parse("script-src 'unsafe-inline' 'unsafe-inline' 'unsafe-inline'", "https://origin.com", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(2, notices.size());
+        assertEquals("1:28: Source list contains duplicate source expression \"'unsafe-inline'\". All but the first instance will be ignored.", notices.get(0).show());
+        assertEquals("1:44: Source list contains duplicate source expression \"'unsafe-inline'\". All but the first instance will be ignored.", notices.get(1).show());
+
+        notices.clear();
+        p = ParserWithLocation.parse("script-src 'unsafe-inline' a b 'unsafe-INLINE' 'unsafe-inline';", "https://origin.com", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(2, notices.size());
+        assertEquals("1:32: Source list contains duplicate source expression \"'unsafe-inline'\". All but the first instance will be ignored.", notices.get(0).show());
+        assertEquals("1:48: Source list contains duplicate source expression \"'unsafe-inline'\". All but the first instance will be ignored.", notices.get(1).show());
+
+        notices.clear();
+        p = ParserWithLocation.parse("script-src 'unsafe-inline' a 'unsafe-INLINE'; style-src 'unsafe-inline' 'unsafe-INLINE' b;", "https://origin.com", notices);
+        assertEquals(2, p.getDirectives().size());
+        assertEquals(2, notices.size());
+        assertEquals("1:30: Source list contains duplicate source expression \"'unsafe-inline'\". All but the first instance will be ignored.", notices.get(0).show());
+        assertEquals("1:73: Source list contains duplicate source expression \"'unsafe-inline'\". All but the first instance will be ignored.", notices.get(1).show());
+
+
+        notices.clear();
+        p = ParserWithLocation.parse("script-src a A ;", "https://origin.com", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(1, notices.size());
+        assertEquals("1:14: Source list contains duplicate source expression \"A\". All but the first instance will be ignored.", notices.get(0).show());
+
+        notices.clear();
+        p = ParserWithLocation.parse("script-src http: HTTP: ;", "https://origin.com", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(0, notices.size());
+
+        notices.clear();
+        p = ParserWithLocation.parse("script-src http://example.com/a http://example.com/A ;", "https://origin.com", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(0, notices.size());
+
+    }
 }