Repository: shapesecurity/salvation (java)

Issue:
“Expecting directive-name but found [newline]” error
See https://github.com/validator/validator/issues/917. The Nu Html Checker uses Salvation to check the CSP policy in the following `meta` element:

```html
<meta http-equiv="Content-Security-Policy" content="
    default-src 'self';
    script-src  'self' 'unsafe-eval';
    style-src   'self' 'unsafe-inline';
    connect-src 'self' data:;
"/>
```
… and Salvation responds with this error:

> Expecting directive-name but found [newline]

Looking at https://w3c.github.io/webappsec-csp/#grammardef-serialized-policy
> **serialized-policy** =
>    serialized-directive *( optional-ascii-whitespace ";" [ optional-ascii-whitespace serialized-directive ] )

…I see that per the spec, newlines are allowed after the `;` separator and before directive names.

So does Salvation not allow newlines there?

Pull request description:
fixes #234: treat newline, carriage return, and form feed as whitespace

Patch:
diff --git a/src/main/java/com/shapesecurity/salvation/Tokeniser.java b/src/main/java/com/shapesecurity/salvation/Tokeniser.java
index 32fc749..a530e52 100644
--- a/src/main/java/com/shapesecurity/salvation/Tokeniser.java
+++ b/src/main/java/com/shapesecurity/salvation/Tokeniser.java
@@ -19,12 +19,12 @@ import com.shapesecurity.salvation.tokens.Token;
 import com.shapesecurity.salvation.tokens.UnknownToken;
 
 public class Tokeniser {
-	private static final Pattern WSP = Pattern.compile("[ \t]+");
-	private static final Pattern NotWSP = Pattern.compile("[^ \t]+");
+	private static final Pattern WSP = Pattern.compile("[\t\n\f\r ]+");
+	private static final Pattern NotWSP = Pattern.compile("[^\t\n\f\r ]+");
 	private static final Pattern directiveSeparator = Pattern.compile(";");
 	private static final Pattern policySeparator = Pattern.compile(",");
 	private static final Pattern directiveNamePattern = Pattern.compile("[a-zA-Z0-9-]+");
-	private static final Pattern directiveValuePattern = Pattern.compile("[ \\t!-+--:<-~]+");
+	private static final Pattern directiveValuePattern = Pattern.compile("[\t\n\f\r !-+--:<-~]+");
 	private static final Pattern notSeparator = Pattern.compile("[^;,]+");
 	@Nonnull
 	protected ArrayList<Token> tokens;
@@ -40,7 +40,7 @@ public class Tokeniser {
 	}
 
 	private static boolean isWhitespace(char ch) {
-		return ch == ' ' || ch == '\t';
+		return ch == '\t' || ch == '\n' || ch == '\f' || ch == '\r' || ch == ' ';
 	}
 
 	protected boolean eat(@Nonnull Function<String, Token> ctor, @Nonnull Pattern pattern) {
@@ -67,7 +67,7 @@ public class Tokeniser {
 	}
 
 	private boolean eatDirectiveValue() {
-		return this.eat(DirectiveValueToken::new, Tokeniser.directiveValuePattern);
+		return this.eat(s -> new DirectiveValueToken(trimRHSWS(s)), Tokeniser.notSeparator);
 	}
 
 	private boolean eatUntilSeparator() {
@@ -82,10 +82,13 @@ public class Tokeniser {
 		return false;
 	}
 
-	private void eatWhitespace() {
+	private boolean eatWhitespace() {
+		boolean found = false;
 		while (this.hasNext() && Tokeniser.isWhitespace(this.sourceText.charAt(this.index))) {
 			++this.index;
+			found = true;
 		}
+		return found;
 	}
 
 	private boolean hasNext() {
@@ -108,7 +111,7 @@ public class Tokeniser {
 				this.eatUntilSeparator();
 				continue;
 			}
-			if (!this.eatSingleWhitespace()) {
+			if (!this.eatWhitespace()) {
 				this.eatUntilSeparator();
 				continue;
 			}


Test patch:
diff --git a/src/test/java/com/shapesecurity/salvation/LocationTest.java b/src/test/java/com/shapesecurity/salvation/LocationTest.java
index 2c12494..d46b76f 100644
--- a/src/test/java/com/shapesecurity/salvation/LocationTest.java
+++ b/src/test/java/com/shapesecurity/salvation/LocationTest.java
@@ -224,7 +224,7 @@ public class LocationTest extends CSPTest {
 
 		notices.clear();
 		ParserWithLocation.parse("referrer абц no-referrer", "https://origin", notices);
-		assertEquals(3, notices.size());
+		assertEquals(2, notices.size());
 		assertNotNull(notices.get(0).startLocation);
 		assertEquals(1, notices.get(0).startLocation.line);
 		assertEquals(1, notices.get(0).startLocation.column);
@@ -236,22 +236,13 @@ public class LocationTest extends CSPTest {
 		assertEquals("Warning: The referrer directive was an experimental directive that was proposed but never added to the CSP specification. Support for this directive will be removed. See Referrer Policy specification.", notices.get(0).toString());
 		assertNotNull(notices.get(1).startLocation);
 		assertEquals(1, notices.get(1).startLocation.line);
-		assertEquals(1, notices.get(1).startLocation.column);
-		assertEquals(0, notices.get(1).startLocation.offset);
+		assertEquals(10, notices.get(1).startLocation.column);
+		assertEquals(9, notices.get(1).startLocation.offset);
 		assertNotNull(notices.get(1).endLocation);
 		assertEquals(1, notices.get(1).endLocation.line);
-		assertEquals(9, notices.get(1).endLocation.column);
-		assertEquals(8, notices.get(1).endLocation.offset);
-		assertEquals("Error: The referrer directive must contain exactly one referrer directive value.", notices.get(1).toString());
-		assertNotNull(notices.get(2).startLocation);
-		assertEquals(1, notices.get(2).startLocation.line);
-		assertEquals(10, notices.get(2).startLocation.column);
-		assertEquals(9, notices.get(2).startLocation.offset);
-		assertNotNull(notices.get(2).endLocation);
-		assertEquals(1, notices.get(2).endLocation.line);
-		assertEquals(25, notices.get(2).endLocation.column);
-		assertEquals(24, notices.get(2).endLocation.offset);
-		assertEquals("Expecting directive-value but found U+0430 (а). Non-ASCII and non-printable characters must be percent-encoded.", notices.get(2).message);
+		assertEquals(25, notices.get(1).endLocation.column);
+		assertEquals(24, notices.get(1).endLocation.offset);
+		assertEquals("Error: Expecting referrer directive value but found \"абц no-referrer\".", notices.get(1).toString());
 
 		notices.clear();
 		ParserWithLocation.parse("referrer no-referrer абц", "https://origin", notices);
@@ -267,13 +258,13 @@ public class LocationTest extends CSPTest {
 		assertEquals("The referrer directive was an experimental directive that was proposed but never added to the CSP specification. Support for this directive will be removed. See Referrer Policy specification.", notices.get(0).message);
 		assertNotNull(notices.get(1).startLocation);
 		assertEquals(1, notices.get(1).startLocation.line);
-		assertEquals(22, notices.get(1).startLocation.column);
-		assertEquals(21, notices.get(1).startLocation.offset);
+		assertEquals(10, notices.get(1).startLocation.column);
+		assertEquals(9, notices.get(1).startLocation.offset);
 		assertNotNull(notices.get(1).endLocation);
 		assertEquals(1, notices.get(1).endLocation.line);
 		assertEquals(25, notices.get(1).endLocation.column);
 		assertEquals(24, notices.get(1).endLocation.offset);
-		assertEquals("Expecting directive-value but found U+0430 (а). Non-ASCII and non-printable characters must be percent-encoded.", notices.get(1).message);
+		assertEquals("Expecting referrer directive value but found \"no-referrer абц\".", notices.get(1).message);
 	}
 
 	@Test
@@ -345,23 +336,34 @@ public class LocationTest extends CSPTest {
 
 		notices.clear();
 		ParserWithLocation.parse("require-sri-for     абц ert", "https://origin", notices);
-		assertEquals(2, notices.size());
+		assertEquals(3, notices.size());
 		assertNotNull(notices.get(0).startLocation);
 		assertEquals(1, notices.get(0).startLocation.line);
-		assertEquals(1, notices.get(0).startLocation.column);
-		assertEquals(0, notices.get(0).startLocation.offset);
+		assertEquals(21, notices.get(0).startLocation.column);
+		assertEquals(20, notices.get(0).startLocation.offset);
 		assertNotNull(notices.get(0).endLocation);
 		assertEquals(1, notices.get(0).endLocation.line);
-		assertEquals(16, notices.get(0).endLocation.column);
-		assertEquals(15, notices.get(0).endLocation.offset);
+		assertEquals(24, notices.get(0).endLocation.column);
+		assertEquals(23, notices.get(0).endLocation.offset);
+		assertEquals("Warning: The require-sri-for directive should contain only \"script\", \"style\" tokens.", notices.get(0).toString());
 		assertNotNull(notices.get(1).startLocation);
 		assertEquals(1, notices.get(1).startLocation.line);
 		assertEquals(21, notices.get(1).startLocation.column);
 		assertEquals(20, notices.get(1).startLocation.offset);
 		assertNotNull(notices.get(1).endLocation);
 		assertEquals(1, notices.get(1).endLocation.line);
-		assertEquals(28, notices.get(1).endLocation.column);
-		assertEquals(27, notices.get(1).endLocation.offset);
+		assertEquals(24, notices.get(1).endLocation.column);
+		assertEquals(23, notices.get(1).endLocation.offset);
+		assertEquals("Error: Expecting RFC 7230 token but found \"абц\".", notices.get(1).toString());
+		assertNotNull(notices.get(2).startLocation);
+		assertEquals(1, notices.get(2).startLocation.line);
+		assertEquals(25, notices.get(2).startLocation.column);
+		assertEquals(24, notices.get(2).startLocation.offset);
+		assertNotNull(notices.get(2).endLocation);
+		assertEquals(1, notices.get(2).endLocation.line);
+		assertEquals(28, notices.get(2).endLocation.column);
+		assertEquals(27, notices.get(2).endLocation.offset);
+		assertEquals("Warning: The require-sri-for directive should contain only \"script\", \"style\" tokens.", notices.get(2).toString());
 	}
 
 
@@ -405,8 +407,8 @@ public class LocationTest extends CSPTest {
 		assertEquals(1, notices.size());
 		assertNotNull(notices.get(0).startLocation);
 		assertEquals(1, notices.get(0).startLocation.line);
-		assertEquals(13, notices.get(0).startLocation.column);
-		assertEquals(12, notices.get(0).startLocation.offset);
+		assertEquals(11, notices.get(0).startLocation.column);
+		assertEquals(10, notices.get(0).startLocation.offset);
 
 	}
 
diff --git a/src/test/java/com/shapesecurity/salvation/ParserTest.java b/src/test/java/com/shapesecurity/salvation/ParserTest.java
index 94b674d..7117e2b 100644
--- a/src/test/java/com/shapesecurity/salvation/ParserTest.java
+++ b/src/test/java/com/shapesecurity/salvation/ParserTest.java
@@ -231,11 +231,11 @@ public class ParserTest extends CSPTest {
 		assertEquals("directive-name, path-part", "base-uri */abc", parseAndShow("base-uri */abc"));
 
 		notices.clear();
-		p = parseWithNotices("base-uri *\n", notices);
+		p = parseWithNotices("base-uri *\0", notices);
 		assertEquals(0, p.getDirectives().size());
 		assertEquals(1, notices.size());
 		assertEquals(
-				"Expecting directive-value but found U+000A (\n). Non-ASCII and non-printable characters must be percent-encoded.",
+				"Expecting source-expression but found \"*\0\".",
 				notices.get(0).message);
 
 		assertEquals("directive-name, full host source", "base-uri https://a.com:888/ert",
@@ -585,9 +585,8 @@ public class ParserTest extends CSPTest {
 
 		notices.clear();
 		parseWithNotices("report-to д", notices);
-		assertEquals(2, notices.size());
-		assertEquals("The report-to directive must contain exactly one RFC 7230 token.", notices.get(0).message);
-		assertEquals("Expecting directive-value but found U+0434 (д). Non-ASCII and non-printable characters must be percent-encoded.", notices.get(1).message);
+		assertEquals(1, notices.size());
+		assertEquals("Expecting RFC 7230 token but found \"д\".", notices.get(0).message);
 
 		notices.clear();
 		parseWithNotices("report-to a b", notices);
@@ -601,7 +600,7 @@ public class ParserTest extends CSPTest {
 
 		p = parse("report-to        a");
 		q = parse("report-to a; ");
-		assertFalse("report-to equals", p.equals(q));
+		assertTrue("report-to equals", p.equals(q));
 	}
 
 	@Test
@@ -646,14 +645,13 @@ public class ParserTest extends CSPTest {
 				notices.get(0).message);
 
 		notices.clear();
-		p = parseWithNotices("sandbox a!*\n", notices);
+		p = parseWithNotices("sandbox a!*\0", notices);
 		assertEquals(0, p.getDirectives().size());
 		assertEquals(2, notices.size());
 		assertEquals(
 				"The sandbox directive should contain only allow-forms, allow-modals, allow-pointer-lock, allow-popups, allow-popups-to-escape-sandbox, allow-same-origin, allow-scripts, or allow-top-navigation.",
 				notices.get(0).message);
-		assertEquals("Expecting directive-value but found U+000A (\n"
-				+ "). Non-ASCII and non-printable characters must be percent-encoded.", notices.get(1).message);
+		assertEquals("Expecting RFC 7230 token but found \"a!*\0\".", notices.get(1).message);
 
 		notices.clear();
 		p = parseWithNotices("sandbox a!*^:", notices);
@@ -806,7 +804,19 @@ public class ParserTest extends CSPTest {
 		assertEquals("some-directive-name", tokens[0].value);
 		assertEquals(DirectiveNameToken.DirectiveNameSubtype.Unrecognised, ((DirectiveNameToken) tokens[0]).subtype);
 		assertTrue(tokens[1] instanceof DirectiveValueToken);
-		assertEquals("  a  ", tokens[1].value);
+		assertEquals("a", tokens[1].value);
+	}
+
+	@Test
+	public void testWhitespace() {
+		Token[] tokens = Tokeniser.tokenise("\t\n\f\r some-directive-name\t\n\f\r a\t\n\f\r ");
+
+		assertEquals(2, tokens.length);
+		assertTrue(tokens[0] instanceof DirectiveNameToken);
+		assertEquals("some-directive-name", tokens[0].value);
+		assertEquals(DirectiveNameToken.DirectiveNameSubtype.Unrecognised, ((DirectiveNameToken) tokens[0]).subtype);
+		assertTrue(tokens[1] instanceof DirectiveValueToken);
+		assertEquals("a", tokens[1].value);
 	}
 
 	@Test
@@ -816,7 +826,7 @@ public class ParserTest extends CSPTest {
 		assertEquals(0, p.getDirectives().size());
 		assertEquals(1, notices.size());
 		assertEquals(
-				"Expecting directive-value but found U+221A (√). Non-ASCII and non-printable characters must be percent-encoded.",
+				"Expecting source-expression but found \"√\".",
 				notices.get(0).message);
 
 		notices.clear();
@@ -824,7 +834,7 @@ public class ParserTest extends CSPTest {
 		assertEquals(1, p.getDirectives().size());
 		assertEquals(1, notices.size());
 		assertEquals(
-				"Expecting directive-value but found U+221A (√). Non-ASCII and non-printable characters must be percent-encoded.",
+				"Expecting source-expression but found \"√\".",
 				notices.get(0).message);
 
 		notices.clear();
@@ -838,18 +848,14 @@ public class ParserTest extends CSPTest {
 		assertEquals(0, p.getDirectives().size());
 		assertEquals(1, notices.size());
 		assertEquals(
-				"Expecting directive-value but found U+1D306 (𝌆). Non-ASCII and non-printable characters must be percent-encoded.",
+				"Expecting source-expression but found \"\uD834\uDF06\".",
 				notices.get(0).message);
 
 		notices.clear();
 		p = parseWithNotices("plugin-types х/п", notices);
 		assertEquals(0, p.getDirectives().size());
-		assertEquals(2, notices.size());
-		assertEquals("The media-type-list must contain at least one media-type.", notices.get(0).message);
-		assertEquals(
-				"Expecting directive-value but found U+0445 (х). Non-ASCII and non-printable characters must be percent-encoded.",
-				notices.get(1).message);
-
+		assertEquals(1, notices.size());
+		assertEquals("Expecting media-type but found \"х/п\".", notices.get(0).message);
 	}
 
 
@@ -923,9 +929,8 @@ public class ParserTest extends CSPTest {
 
 		notices.clear();
 		p = parseWithNotices("referrer   no-referrer  ", notices);
-		assertEquals(0, p.getDirectives().size());
-		assertEquals(2, notices.size());
-		assertEquals("Expecting referrer directive value but found \"  no-referrer  \".", notices.get(1).message);
+		assertEquals(1, p.getDirectives().size());
+		assertEquals(1, notices.size());
 
 		notices.clear();
 		p = parseWithNotices("referrer aaa", notices);