Repository: shapesecurity/salvation
Pull request #255 (2022-03-16 00:22:14)

preserve malformed directive names containing '
https://github.com/shapesecurity/salvation/pull/251 added an exception for `default-src'self'`, but to be consistent we should still be able to represent that and just report an error to the error consumer. Thanks to @kingthorin for pointing this out in https://github.com/shapesecurity/salvation/pull/251#issuecomment-1068568034.


Patch:
diff --git a/src/main/java/com/shapesecurity/salvation2/Directive.java b/src/main/java/com/shapesecurity/salvation2/Directive.java
index d6dc438..9c33df6 100644
--- a/src/main/java/com/shapesecurity/salvation2/Directive.java
+++ b/src/main/java/com/shapesecurity/salvation2/Directive.java
@@ -9,6 +9,7 @@ import java.util.regex.Pattern;
 
 
 public class Directive {
+	public static Predicate<String> IS_DIRECTIVE_NAME = Pattern.compile("^[A-Za-z0-9\\-]+$").asPredicate();
 	public static Predicate<String> containsNonDirectiveCharacter = Pattern.compile("[" + Constants.WHITESPACE_CHARS + ",;]").asPredicate();
 	protected List<String> values;
 
diff --git a/src/main/java/com/shapesecurity/salvation2/Policy.java b/src/main/java/com/shapesecurity/salvation2/Policy.java
index b6cf720..3087e2e 100644
--- a/src/main/java/com/shapesecurity/salvation2/Policy.java
+++ b/src/main/java/com/shapesecurity/salvation2/Policy.java
@@ -123,16 +123,12 @@ public class Policy {
 				++index[0];
 				continue;
 			}
-			String directiveName = collect(strippedLeadingAndTrailingWhitespace, "[^'" + Constants.WHITESPACE_CHARS + "]+");
+			String directiveName = collect(strippedLeadingAndTrailingWhitespace, "[^" + Constants.WHITESPACE_CHARS + "]+");
 
 			// Note: we do not lowercase directive names or skip duplicates during parsing, to allow round-tripping even invalid policies
 
 			String remainingToken = strippedLeadingAndTrailingWhitespace.substring(directiveName.length());
 
-			if (remainingToken.length() > 0 && !containsLeadingWhitespace(remainingToken)) {
-				throw new IllegalArgumentException("directive value requires leading ascii whitespace");
-			}
-
 			List<String> directiveValues = Utils.splitOnAsciiWhitespace(remainingToken);
 
 			policy.add(directiveName, directiveValues, directiveErrorConsumer);
@@ -150,6 +146,8 @@ public class Policy {
 	// We do not provide a generic method for updating an existing directive in-place. Just remove the existing one and add it back.
 	public Directive add(String name, List<String> values, Directive.DirectiveErrorConsumer directiveErrorConsumer) {
 		enforceAscii(name);
+
+		// the parser will never hit these errors by construction, but use of the manipulation APIs can
 		if (Directive.containsNonDirectiveCharacter.test(name)) {
 			throw new IllegalArgumentException("directive names must not contain whitespace, ',', or ';'");
 		}
@@ -292,6 +290,11 @@ public class Policy {
 				break;
 			}
 			default: {
+				if (!Directive.IS_DIRECTIVE_NAME.test(name)) {
+					directiveErrorConsumer.add(Severity.Error, "Directive name " + name + " contains characters outside the range ALPHA / DIGIT / \"-\"", -1);
+					newDirective = new Directive(values);
+					break;
+				}
 				FetchDirectiveKind fetchDirectiveKind = FetchDirectiveKind.fromString(lowcaseDirectiveName);
 				if (fetchDirectiveKind != null) {
 					SourceExpressionDirective thisDirective = new SourceExpressionDirective(values, directiveErrorConsumer);


Test patch:
diff --git a/src/test/java/com/shapesecurity/salvation2/ParserTest.java b/src/test/java/com/shapesecurity/salvation2/ParserTest.java
index 6fcd5db..676943a 100644
--- a/src/test/java/com/shapesecurity/salvation2/ParserTest.java
+++ b/src/test/java/com/shapesecurity/salvation2/ParserTest.java
@@ -135,13 +135,13 @@ public class ParserTest extends TestBase {
 		);
 
 		roundTrips(
-				"&",
-				e(Policy.Severity.Warning, "Unrecognized directive &", 0, -1)
+				"default-src 'not-keyword'",
+				e(Policy.Severity.Error, "Unrecognized source-expression 'not-keyword'", 0, 0)
 		);
 
 		roundTrips(
-				"default-src 'not-keyword'",
-				e(Policy.Severity.Error, "Unrecognized source-expression 'not-keyword'", 0, 0)
+				"default-src 'self'a",
+				e(Policy.Severity.Error, "Unrecognized source-expression 'self'a", 0, 0)
 		);
 
 		roundTrips(
@@ -224,6 +224,26 @@ public class ParserTest extends TestBase {
 				"upgrade-insecure-requests a",
 				e(Policy.Severity.Error, "The upgrade-insecure-requests directive does not support values", 0, 0)
 		);
+
+		roundTrips(
+				"&",
+				e(Policy.Severity.Error, "Directive name & contains characters outside the range ALPHA / DIGIT / \"-\"", 0, -1)
+		);
+
+		roundTrips(
+				"default-src'self'",
+				e(Policy.Severity.Error, "Directive name default-src'self' contains characters outside the range ALPHA / DIGIT / \"-\"", 0, -1)
+		);
+
+		roundTrips(
+				"default-src'self' a",
+				e(Policy.Severity.Error, "Directive name default-src'self' contains characters outside the range ALPHA / DIGIT / \"-\"", 0, -1)
+		);
+
+		roundTrips(
+				"default-src'self'a",
+				e(Policy.Severity.Error, "Directive name default-src'self'a contains characters outside the range ALPHA / DIGIT / \"-\"", 0, -1)
+		);
 	}
 
 	@Test
@@ -407,13 +427,6 @@ public class ParserTest extends TestBase {
 		);
 	}
 
-	@Test(expected = IllegalArgumentException.class)
-	public void testIllegalLackOfWhitespace() {
-		roundTrips(
-				"default-src'self'"
-		);
-	}
-
 	@Test
 	public void testNone() {
 		// This asserts that it serializes to the same, uppercased, value