From 23885557bb385af0aabb12ea34bd6f20e72c12ea Mon Sep 17 00:00:00 2001 From: Hunter Stern Date: Tue, 19 Jan 2016 18:07:05 -0800 Subject: [PATCH 1/5] Allow spaces in JavaScript urls, but only if they have a known good file extension --- .../main/java/org/archive/util/UriUtils.java | 31 ++++++++++++++----- .../modules/extractor/ExtractorJS.java | 2 +- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/commons/src/main/java/org/archive/util/UriUtils.java b/commons/src/main/java/org/archive/util/UriUtils.java index 5b78f4fb..f89fb9f9 100644 --- a/commons/src/main/java/org/archive/util/UriUtils.java +++ b/commons/src/main/java/org/archive/util/UriUtils.java @@ -381,6 +381,7 @@ public class UriUtils { } protected static final Set KNOWN_GOOD_FILE_EXTENSIONS = new HashSet(); + static { /* * Real known use cases for this are .min.js, .min.css, and we've seen @@ -389,19 +390,20 @@ public class UriUtils { */ KNOWN_GOOD_FILE_EXTENSIONS.addAll(Arrays.asList(".jpg", ".js", ".css", ".png", ".gif", ".swf", ".flv", ".mp4", ".mp3", ".jpeg", - ".html")); + ".html", ".pdf")); } protected static final String QNV = "[a-zA-Z_]+=(?:[\\w-/.]|%[0-9a-fA-F]{2})*"; // name=value for query strings // group(1) filename // group(2) filename extension with leading '.' protected static final String LIKELY_RELATIVE_URI_PATTERN = - "(?:\\.?/)?" // may start with "/" or "./" - + "(?:(?:[\\w-]+|\\.\\.)/)*" // may have path/segments/ - + "([\\w-]+(?:\\.[\\w-]+)?(\\.[a-zA-Z0-9]{2,5})?)?" // may have a filename with or without an extension - + "(?:\\?(?:"+ QNV + ")(?:&(?:" + QNV + "))*)?" // may have a ?query=string - + "(?:#[\\w-]+)?"; // may have a #fragment - + "(?:\\.?/|\\\\u002f)?" // may start with "/" or "./" or utf16 / which is (\u002f) + + "(?:(?:[\\s\\w-]+|\\.\\.)(?:/|\\\\u002f))*" // may have path/segments/segment2\u002fa\u002f + + "([\\s\\w-]+(?:\\.[\\w-]+)?(\\.[a-zA-Z0-9]{2,5})?)?" // may have a filename with or without an extension + + "(?:\\?(?:"+ QNV + ")(?:&(?:" + QNV + "))*)?" // may have a ?query=string + + "(?:#[\\w-]+)?"; // may have a #fragment + + public static boolean isVeryLikelyUri(CharSequence candidate) { // must have a . or / if (!TextUtils.matches(NAIVE_LIKELY_URI_PATTERN, candidate)) { @@ -423,6 +425,21 @@ public class UriUtils { if (!matcher.matches()) { return false; } + + // if spaces in url, only allow file extensions that match known good extensions + if (TextUtils.matches(".*[\\s)]+.*", candidate)) { + String filename = matcher.group(1); + String extension = matcher.group(2); + if (filename != null && extension != null + && KNOWN_GOOD_FILE_EXTENSIONS.contains(extension)) { + return true; + } + } + + // if spaces in url but doesn't match a known good file extension, discard + if (TextUtils.matches(".*[\\s)]+.*", candidate)) { + return false; + } /* * Remaining tests discard stuff that the diff --git a/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java b/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java index 10de06cf..72358815 100644 --- a/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java +++ b/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java @@ -67,7 +67,7 @@ public class ExtractorJS extends ContentExtractor { // (areas between paired ' or " characters, possibly backslash-quoted // on the ends, but not in the middle) protected static final String JAVASCRIPT_STRING_EXTRACTOR = - "(\\\\{0,8}+(?:['\"]|u002[27]))([^\\s'\"]{1,"+UURI.MAX_URL_LENGTH+"})(?:\\1)"; + "(\\\\{0,8}+(?:['\"]|u002[27]))([^'\"]{0,"+UURI.MAX_URL_LENGTH+"})(?:\\1)"; // GROUPS: // (G1) ' or " with optional leading backslashes From 12a88d6d2387d7239972c150042a03e4d0365d20 Mon Sep 17 00:00:00 2001 From: Hunter Stern Date: Wed, 20 Jan 2016 10:44:20 -0800 Subject: [PATCH 2/5] Allow spaces in urls extracted from JS. --- .../main/java/org/archive/util/UriUtils.java | 22 +++++++++---------- .../modules/extractor/ExtractorJS.java | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/commons/src/main/java/org/archive/util/UriUtils.java b/commons/src/main/java/org/archive/util/UriUtils.java index f89fb9f9..0bace0ef 100644 --- a/commons/src/main/java/org/archive/util/UriUtils.java +++ b/commons/src/main/java/org/archive/util/UriUtils.java @@ -89,9 +89,9 @@ public class UriUtils { private static final Logger LOGGER = Logger.getLogger(UriUtils.class.getName()); // naive likely-uri test: - // no whitespace or '<' or '>' + // no '<' or '>' // at least one '.' or '/'; - protected static final String NAIVE_LIKELY_URI_PATTERN = "[^<>\\s]*[\\./][^<>\\s]*"; + protected static final String NAIVE_LIKELY_URI_PATTERN = "[^<>]*[\\./][^<>]*"; public static boolean isPossibleUri(CharSequence candidate) { return TextUtils.matches(NAIVE_LIKELY_URI_PATTERN, candidate); @@ -429,18 +429,18 @@ public class UriUtils { // if spaces in url, only allow file extensions that match known good extensions if (TextUtils.matches(".*[\\s)]+.*", candidate)) { String filename = matcher.group(1); - String extension = matcher.group(2); - if (filename != null && extension != null - && KNOWN_GOOD_FILE_EXTENSIONS.contains(extension)) { - return true; + if (filename != null) { + int lastIndexOfDot = filename.lastIndexOf("."); + if (lastIndexOfDot != -1) { + String extension = filename.substring(lastIndexOfDot); + if (KNOWN_GOOD_FILE_EXTENSIONS.contains(extension)) { + return true; + } + } + return false; } } - // if spaces in url but doesn't match a known good file extension, discard - if (TextUtils.matches(".*[\\s)]+.*", candidate)) { - return false; - } - /* * Remaining tests discard stuff that the * LIKELY_RELATIVE_URI_PATTERN can't catch diff --git a/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java b/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java index 72358815..8c89c40f 100644 --- a/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java +++ b/modules/src/main/java/org/archive/modules/extractor/ExtractorJS.java @@ -63,7 +63,7 @@ public class ExtractorJS extends ContentExtractor { private static Logger LOGGER = Logger.getLogger(ExtractorJS.class.getName()); - // finds whitespace- and quote-free strings in Javascript + // finds strings in Javascript // (areas between paired ' or " characters, possibly backslash-quoted // on the ends, but not in the middle) protected static final String JAVASCRIPT_STRING_EXTRACTOR = From 4518bc2da82c35ea90a1c0c20a684a98f2f6e1fc Mon Sep 17 00:00:00 2001 From: Hunter Stern Date: Wed, 20 Jan 2016 14:29:42 -0800 Subject: [PATCH 3/5] Make sure no urls with whitespace and not having good extension slip through --- commons/src/main/java/org/archive/util/UriUtils.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/commons/src/main/java/org/archive/util/UriUtils.java b/commons/src/main/java/org/archive/util/UriUtils.java index 0bace0ef..cddd11b3 100644 --- a/commons/src/main/java/org/archive/util/UriUtils.java +++ b/commons/src/main/java/org/archive/util/UriUtils.java @@ -437,8 +437,9 @@ public class UriUtils { return true; } } - return false; } + + return false; } /* From 088189ee8b0d9a250186f7cf7ff9e7f768cd174a Mon Sep 17 00:00:00 2001 From: Hunter Stern Date: Thu, 28 Jan 2016 15:11:49 -0800 Subject: [PATCH 4/5] Fix up code based on pull requests comments. Add test for urls with spaces in them. --- commons/src/main/java/org/archive/util/UriUtils.java | 6 +++--- .../java/org/archive/modules/extractor/ExtractorJSTest.java | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/commons/src/main/java/org/archive/util/UriUtils.java b/commons/src/main/java/org/archive/util/UriUtils.java index cddd11b3..0fe494a9 100644 --- a/commons/src/main/java/org/archive/util/UriUtils.java +++ b/commons/src/main/java/org/archive/util/UriUtils.java @@ -397,8 +397,8 @@ public class UriUtils { // group(1) filename // group(2) filename extension with leading '.' protected static final String LIKELY_RELATIVE_URI_PATTERN = - "(?:\\.?/|\\\\u002f)?" // may start with "/" or "./" or utf16 / which is (\u002f) - + "(?:(?:[\\s\\w-]+|\\.\\.)(?:/|\\\\u002f))*" // may have path/segments/segment2\u002fa\u002f + "(?:\\.?/)?" // may start with "/" or "./" + + "(?:(?:[\\s\\w-]+|\\.\\.)(?:/))*" // may have path/segments/segment2 + "([\\s\\w-]+(?:\\.[\\w-]+)?(\\.[a-zA-Z0-9]{2,5})?)?" // may have a filename with or without an extension + "(?:\\?(?:"+ QNV + ")(?:&(?:" + QNV + "))*)?" // may have a ?query=string + "(?:#[\\w-]+)?"; // may have a #fragment @@ -427,7 +427,7 @@ public class UriUtils { } // if spaces in url, only allow file extensions that match known good extensions - if (TextUtils.matches(".*[\\s)]+.*", candidate)) { + if (TextUtils.matches(".*\\s+.*", candidate)) { String filename = matcher.group(1); if (filename != null) { int lastIndexOfDot = filename.lastIndexOf("."); diff --git a/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java b/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java index 917f0efc..9c351430 100644 --- a/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java +++ b/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java @@ -124,7 +124,10 @@ public class ExtractorJSTest extends StringExtractorTestBase { "http://www.archive.org/static/0000/2683/good_filename_with.two_dots.jpg", "{nonUrl: 'non-filename.with_two.dots',etc:'foo foo' }", - null + null, + + "\"FileRef\": \"\\u002fsites\\u002fprb\\u002fPublic Comment Emails PDF\\u002fOpen Records Law_1r71vq4i.pdf\",\"", + "http://www.archive.org/sites/prb/Public Comment Emails PDF/Open Records Law_1r71vq4i.pdf" }; @Override From cbb5e3ab996c6ce369461ea58ab5c0a3ab05acc9 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Tue, 9 Feb 2016 15:02:11 -0800 Subject: [PATCH 5/5] Simplify logic for urls with spaces, and make it better follow the pattern of UriUtils.isVeryLikelyUri(). Involves a subtle adjustment to the regex LIKELY_RELATIVE_URI_PATTERN to ensure the 2nd capturing group always gets the file extension. Also add a couple of tests of strings with spaces and file extensions that are not known good extensions. --- .../main/java/org/archive/util/UriUtils.java | 24 +++++----------- .../modules/extractor/ExtractorJSTest.java | 28 ++++++++++++++++--- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/commons/src/main/java/org/archive/util/UriUtils.java b/commons/src/main/java/org/archive/util/UriUtils.java index 0fe494a9..e9a7bba5 100644 --- a/commons/src/main/java/org/archive/util/UriUtils.java +++ b/commons/src/main/java/org/archive/util/UriUtils.java @@ -399,7 +399,7 @@ public class UriUtils { protected static final String LIKELY_RELATIVE_URI_PATTERN = "(?:\\.?/)?" // may start with "/" or "./" + "(?:(?:[\\s\\w-]+|\\.\\.)(?:/))*" // may have path/segments/segment2 - + "([\\s\\w-]+(?:\\.[\\w-]+)?(\\.[a-zA-Z0-9]{2,5})?)?" // may have a filename with or without an extension + + "([\\s\\w-]+(?:\\.[\\w-]+)??(\\.[a-zA-Z0-9]{2,5})?)?" // may have a filename with or without an extension + "(?:\\?(?:"+ QNV + ")(?:&(?:" + QNV + "))*)?" // may have a ?query=string + "(?:#[\\w-]+)?"; // may have a #fragment @@ -426,22 +426,6 @@ public class UriUtils { return false; } - // if spaces in url, only allow file extensions that match known good extensions - if (TextUtils.matches(".*\\s+.*", candidate)) { - String filename = matcher.group(1); - if (filename != null) { - int lastIndexOfDot = filename.lastIndexOf("."); - if (lastIndexOfDot != -1) { - String extension = filename.substring(lastIndexOfDot); - if (KNOWN_GOOD_FILE_EXTENSIONS.contains(extension)) { - return true; - } - } - } - - return false; - } - /* * Remaining tests discard stuff that the * LIKELY_RELATIVE_URI_PATTERN can't catch @@ -456,6 +440,12 @@ public class UriUtils { return false; } + if (TextUtils.matches(".*\\s+.*", candidate) + && (extension == null + || !KNOWN_GOOD_FILE_EXTENSIONS.contains(extension))) { + return false; + } + // text or application mimetype if (TextUtils.matches("(?:text|application)/[^/]+", candidate)) { return false; diff --git a/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java b/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java index 9c351430..c684c231 100644 --- a/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java +++ b/modules/src/test/java/org/archive/modules/extractor/ExtractorJSTest.java @@ -125,11 +125,32 @@ public class ExtractorJSTest extends StringExtractorTestBase { "{nonUrl: 'non-filename.with_two.dots',etc:'foo foo' }", null, - + "\"FileRef\": \"\\u002fsites\\u002fprb\\u002fPublic Comment Emails PDF\\u002fOpen Records Law_1r71vq4i.pdf\",\"", - "http://www.archive.org/sites/prb/Public Comment Emails PDF/Open Records Law_1r71vq4i.pdf" + "http://www.archive.org/sites/prb/Public Comment Emails PDF/Open Records Law_1r71vq4i.pdf", + + "\"FileRef\": \"\\u002fsites\\u002fprb\\u002fPublic Comment Emails PDF\\u002fOpen Records Law_1r71vq4i.bad\",\"", + null, + + "\"FileRef\": \"\\u002fsites\\u002fprb\\u002fPublic Comment Emails PDF\\u002fOpenRecordsLaw_1r71vq4i.bad\",\"", + null, + + "\"FileRef\": \"\\u002fsites\\u002fprb\\u002fPublic Comment Emails PDF\\u002fOpen Records Law_1r71vq4i\",\"", + null, + + "\"FileRef\": \"\\u002fsites\\u002fprb\\u002fPublic Comment Emails PDF\\u002fOpenRecordsLaw_1r71vq4i\",\"", + null, + + /* + * XXX this one fails currently because the string has no slashes or + * dots, until it is javascript-unescaped, which happens too late. + * Unescaping earlier involves converting many more CharSubSequence to + * String. Is it worth the performance hit? + */ + // "\"FileRef\": \"\\u002fsites\\u002fprb\\u002fPublicCommentEmailsPDF\\u002fOpenRecordsLaw_1r71vq4i\",\"", + // "http://www.archive.org/sites/prb/PublicCommentEmailsPDF/OpenRecordsLaw_1r71vq4i", }; - + @Override protected String[] getValidTestData() { return VALID_TEST_DATA; @@ -137,7 +158,6 @@ public class ExtractorJSTest extends StringExtractorTestBase { @Override protected Extractor makeExtractor() { - ExtractorJS result = new ExtractorJS(); UriErrorLoggerModule ulm = new UnitTestUriLoggerModule(); result.setLoggerModule(ulm);