Repository: shapesecurity/salvation (java)

Issue:
Handle `source-expression`s that contain both 'unsafe-inline' and hash/nonce
Remove "'usanfe-inline'" if `source-list` contains both "'usanfe-inline'" and hash-source/nonce-source

Pull request description:
fixes #130
remove 'unsafe-inline' if any of hash-source, nonce-source are present in the source-list

Patch:
diff --git a/src/main/java/com/shapesecurity/salvation/Parser.java b/src/main/java/com/shapesecurity/salvation/Parser.java
index 53ed5b6..fecfede 100644
--- a/src/main/java/com/shapesecurity/salvation/Parser.java
+++ b/src/main/java/com/shapesecurity/salvation/Parser.java
@@ -351,12 +351,19 @@ public class Parser {
     @Nonnull private Set<SourceExpression> parseSourceList() throws DirectiveParseException {
         Set<SourceExpression> sourceExpressions = new LinkedHashSet<>();
         boolean parseException = false;
-        boolean seenNone = false;
+        Map<String, Boolean> seenStates = new HashMap<>();
+        seenStates.put("seenNone", false);
+        seenStates.put("seenUnsafeInline", false);
+        seenStates.put("seenHashOrNonce", false);
         while (this.hasNext(SubDirectiveValueToken.class)) {
             try {
-                SourceExpression se = this.parseSourceExpression(seenNone, !sourceExpressions.isEmpty());
+                SourceExpression se = this.parseSourceExpression(seenStates, !sourceExpressions.isEmpty());
                 if (se == None.INSTANCE) {
-                    seenNone = true;
+                    seenStates.put("seenNone", true);
+                } else if (se == KeywordSource.UnsafeInline) {
+                    seenStates.put("seenUnsafeInline", true);
+                } else if (se instanceof HashSource || se instanceof NonceSource) {
+                    seenStates.put("seenHashOrNonce", true);
                 }
                 sourceExpressions.add(se);
             } catch (DirectiveValueParseException e) {
@@ -369,10 +376,10 @@ public class Parser {
         return sourceExpressions;
     }
 
-    @Nonnull private SourceExpression parseSourceExpression(boolean seenNone, boolean seenSome)
+    @Nonnull private SourceExpression parseSourceExpression(Map<String, Boolean> seenStates, boolean seenSome)
         throws DirectiveValueParseException {
         Token token = this.advance();
-        if (seenNone || seenSome && token.value.equalsIgnoreCase("'none'")) {
+        if (seenStates.get("seenNone") || seenSome && token.value.equalsIgnoreCase("'none'")) {
             this.error(token, "'none' must not be combined with any other source-expression.");
             throw INVALID_SOURCE_EXPR;
         }
@@ -382,6 +389,9 @@ public class Parser {
             case "'self'":
                 return KeywordSource.Self;
             case "'unsafe-inline'":
+                if (seenStates.get("seenHashOrNonce")) {
+                    this.warn(token, "The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source");
+                }
                 return KeywordSource.UnsafeInline;
             case "'unsafe-eval'":
                 return KeywordSource.UnsafeEval;
@@ -394,6 +404,9 @@ public class Parser {
                     String nonce = token.value.substring(7, token.value.length() - 1);
                     NonceSource nonceSource = new NonceSource(nonce);
                     nonceSource.validationErrors().forEach(str -> this.warn(token, str));
+                    if (seenStates.get("seenUnsafeInline")) {
+                        this.warn(token, "The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source");
+                    }
                     return nonceSource;
                 } else if (token.value.toLowerCase().startsWith("'sha")) {
                     HashSource.HashAlgorithm algorithm;
@@ -433,6 +446,9 @@ public class Parser {
                         this.error(token, e.getMessage());
                         throw INVALID_SOURCE_EXPR;
                     }
+                    if (seenStates.get("seenUnsafeInline")) {
+                        this.warn(token, "The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source");
+                    }
                     return hashSource;
                 } else if (token.value.matches("^" + Constants.schemePart + ":$")) {
                     return new SchemeSource(token.value.substring(0, token.value.length() - 1));
diff --git a/src/main/java/com/shapesecurity/salvation/data/Policy.java b/src/main/java/com/shapesecurity/salvation/data/Policy.java
index 9dea583..0f33f26 100644
--- a/src/main/java/com/shapesecurity/salvation/data/Policy.java
+++ b/src/main/java/com/shapesecurity/salvation/data/Policy.java
@@ -144,6 +144,8 @@ public class Policy implements Show {
                         .filter(x -> !(x instanceof HostSource))
                         // * remove schemes sources other than about:, data:, blob:, and filesystem: in source list that contains *
                         .filter(x -> !(x instanceof SchemeSource) || ((SchemeSource) x).matchesProtectedScheme())
+                        // remove 'unsafe-inline' if needed
+                        .filter(x -> !((x == KeywordSource.UnsafeInline) && sourceListDirective.containsUnsafeInlineDisabler()))
                         .collect(Collectors.toCollection(LinkedHashSet::new));
                     newSources.add(star.get());
                     this.directives.put(entry.getKey(), sourceListDirective.construct(newSources));
diff --git a/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java b/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java
index b297d91..acad099 100644
--- a/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java
+++ b/src/main/java/com/shapesecurity/salvation/directives/SourceListDirective.java
@@ -1,10 +1,7 @@
 package com.shapesecurity.salvation.directives;
 
 import com.shapesecurity.salvation.data.*;
-import com.shapesecurity.salvation.directiveValues.HashSource;
-import com.shapesecurity.salvation.directiveValues.HostSource;
-import com.shapesecurity.salvation.directiveValues.KeywordSource;
-import com.shapesecurity.salvation.directiveValues.SourceExpression;
+import com.shapesecurity.salvation.directiveValues.*;
 import com.shapesecurity.salvation.interfaces.MatchesHash;
 import com.shapesecurity.salvation.interfaces.MatchesNonce;
 import com.shapesecurity.salvation.interfaces.MatchesSource;
@@ -39,6 +36,10 @@ public abstract class SourceListDirective extends Directive<SourceExpression>
             .anyMatch(x -> ((MatchesNonce) x).matchesNonce(nonce));
     }
 
+    public boolean containsUnsafeInlineDisabler() {
+        return this.values().anyMatch(x -> (x instanceof HashSource || x instanceof NonceSource));
+    }
+
     @Nonnull public Directive<SourceExpression> resolveSelf(@Nonnull Origin origin) {
         return this.bind(dv -> {
             if (dv == KeywordSource.Self) {


Test patch:
diff --git a/src/test/java/com/shapesecurity/salvation/LocationTest.java b/src/test/java/com/shapesecurity/salvation/LocationTest.java
index 95772bf..1a07082 100644
--- a/src/test/java/com/shapesecurity/salvation/LocationTest.java
+++ b/src/test/java/com/shapesecurity/salvation/LocationTest.java
@@ -411,6 +411,27 @@ public class LocationTest extends CSPTest {
             notices.get(1).toString());
     }
 
+    @Test public void testUnsafeInlineWarnings() {
+        ArrayList<Notice> notices = new ArrayList<>();
+        ParserWithLocation
+            .parse("script-src 'unsafe-inline' 'nonce-123'", URI.parse("https://origin"), notices);
+        assertEquals(2, notices.size());
+        Notice notice = notices.get(0);
+        assertEquals(
+            "1:28: Invalid base64-value (should be multiple of 4 bytes: 3). Consider using RFC4648 compliant base64 encoding implementation.",
+            notice.show());
+        assertEquals(
+            "Warning: Invalid base64-value (should be multiple of 4 bytes: 3). Consider using RFC4648 compliant base64 encoding implementation.",
+            notice.toString());
+        notice = notices.get(1);
+        assertEquals(
+            "1:28: The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source",
+            notice.show());
+        assertEquals(
+            "Warning: The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source",
+            notice.toString());
+    }
+
     @Test public void testNoticeHelpers() {
         ArrayList<Notice> notices = new ArrayList<>();
         ParserWithLocation.parse(
diff --git a/src/test/java/com/shapesecurity/salvation/ParserTest.java b/src/test/java/com/shapesecurity/salvation/ParserTest.java
index b0f9c09..4535455 100644
--- a/src/test/java/com/shapesecurity/salvation/ParserTest.java
+++ b/src/test/java/com/shapesecurity/salvation/ParserTest.java
@@ -192,6 +192,10 @@ public class ParserTest extends CSPTest {
         assertEquals("optimisation", "", parseAndShow("script-src 'self' *"));
         assertEquals("optimisation", "script-src 'unsafe-inline'; style-src 'unsafe-inline'",
             parseAndShow("script-src 'unsafe-inline'; style-src 'unsafe-inline';"));
+        assertEquals("optimisation", "script-src 'unsafe-inline' *", parseAndShow("script-src example.com * 'unsafe-inline'"));
+        assertEquals("optimisation", "script-src 'nonce-123' *", parseAndShow("script-src example.com * 'unsafe-inline' 'nonce-123'"));
+        assertEquals("optimisation", "script-src 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols=' *", parseAndShow("script-src example.com * 'unsafe-inline' 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols='"));
+        assertEquals("optimisation", "script-src 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols=' 'nonce-123' *", parseAndShow("script-src example.com * 'sha256-K7gNU3sdo+OL0wNhqoVWhr3g6s1xYv72ol/pe/Unols=' 'nonce-123' 'unsafe-inline'"));
 
 
     }
@@ -815,7 +819,37 @@ public class ParserTest extends CSPTest {
         assertEquals(0, p.getDirectives().size());
         assertEquals(1, notices.size());
         assertEquals("Expecting end of policy but found \",\".", notices.get(0).message);
+    }
+
+    @Test public void testUnsafeInlineWithHashNonce() {
+        Policy p;
+        ArrayList<Notice> notices = new ArrayList<>();
+        p = parseWithNotices("default-src 'unsafe-inline' 'nonce-abcdef'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(2, notices.size());
+        assertEquals("The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source", notices.get(1).message);
+
+        notices.clear();
+        p = parseWithNotices("default-src 'Unsafe-INLINE' 'nonce-abcd'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(2, notices.size());
+        assertEquals("The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source", notices.get(1).message);
 
+        notices.clear();
+        p = parseWithNotices("default-src 'unsafe-inline' 'sha512-vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg=='", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(1, notices.size());
+        assertEquals("The 'unsafe-inline' keyword-source has no effect in source lists that contain hash-source or nonce-source", notices.get(0).message);
+
+        notices.clear();
+        p = parseWithNotices("default-src 'unsafe-inline' 'unsafe-redirect'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(1, notices.size());
+        assertEquals("'unsafe-redirect' has been removed from CSP as of version 2.0.", notices.get(0).message);
 
+        notices.clear();
+        p = parseWithNotices("default-src 'unsafe-inline'", notices);
+        assertEquals(1, p.getDirectives().size());
+        assertEquals(0, notices.size());
     }
 }
diff --git a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
index a55b75a..fb23358 100644
--- a/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
+++ b/src/test/java/com/shapesecurity/salvation/PolicyQueryingTest.java
@@ -136,6 +136,8 @@ public class PolicyQueryingTest extends CSPTest {
         assertTrue("style hash is allowed",
             p.allowsStyleWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value("cGl6ZGE=")));
 
+        p = Parser.parse("default-src * 'unsafe-inline' 'nonce-123'", "https://abc.com");
+        assertFalse("inline script is not allowed", p.allowsUnsafeInlineScript());
     }
 
     @Test public void testAllowsPlugin() {
@@ -172,6 +174,14 @@ public class PolicyQueryingTest extends CSPTest {
                 "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
         assertFalse("style hash is not allowed", p.allowsStyleWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value(
             "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
+
+        p = Parser.parse("default-src * 'unsafe-inline' 'sha512-vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg=='", "https://abc.com");
+        assertTrue("script hash is allowed", p.allowsScriptWithHash(HashSource.HashAlgorithm.SHA512, new Base64Value(
+            "vSsar3708Jvp9Szi2NWZZ02Bqp1qRCFpbcTZPdBhnWgs5WtNZKnvCXdhztmeD2cmW192CF5bDufKRpayrW/isg==")));
+        assertFalse("unknown script is not allowed", p.allowsScriptWithHash(HashSource.HashAlgorithm.SHA256, new Base64Value(
+            "7HY1KLziIDGNSsu67SifYO1B69r1EFEfvPg3McqyIcM=")));
+        assertFalse("unknown script is not allowed", p.allowsUnsafeInlineScript());
+        assertFalse("unknown style is not allowed", p.allowsUnsafeInlineStyle());
     }
 
     @Test public void testAllowsNonce() {
@@ -188,6 +198,12 @@ public class PolicyQueryingTest extends CSPTest {
         p = Parser.parse("default-src 'none'", "https://abc.com");
         assertFalse("script nonce is not allowed", p.allowsScriptWithNonce(new Base64Value("0gQAAA==")));
         assertFalse("style nonce is not allowed", p.allowsStyleWithNonce(new Base64Value("0gQAAA==")));
+
+        p = Parser.parse("default-src * 'unsafe-inline' 'nonce-0gQAAA=='", "https://abc.com");
+        assertTrue("script nonce is allowed", p.allowsScriptWithNonce(new Base64Value("0gQAAA==")));
+        assertFalse("script wrong nonce is not allowed", p.allowsScriptWithNonce(new Base64Value("1234")));
+        assertFalse("unsafe inline script is not allowed", p.allowsUnsafeInlineScript());
+        assertFalse("unsafe inline style is not allowed", p.allowsUnsafeInlineStyle());
     }
 
     @Test public void testAllowsConnect() {