Merge pull request #83 from nahsra/1.6.3

1.6.3

Author: davewichers

pom.xml

@@ -3,7 +3,7 @@
     <groupId>org.owasp.antisamy</groupId>
     <artifactId>antisamy</artifactId>
     <packaging>jar</packaging>
-    <version>1.6.2</version>
+    <version>1.6.3</version>
     <distributionManagement>
         <snapshotRepository>
             <id>ossrh</id>
@@ -42,10 +42,10 @@
 
     <properties>
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
-        <project.build.outputTimestamp>2021-18-03T18:02:00Z</project.build.outputTimestamp>
+        <project.build.outputTimestamp>2021-08-04T01:37:00Z</project.build.outputTimestamp>
         <gpg.skip>true</gpg.skip><!-- by default skip gpg -->
         <version.slf4j>1.7.30</version.slf4j>
-        <version.spotbugs.maven>4.2.0</version.spotbugs.maven>
+        <version.spotbugs.maven>4.2.2</version.spotbugs.maven>
         <version.spotbugs>4.2.2</version.spotbugs>
     </properties>
 
@@ -239,6 +239,9 @@
                                     <ignoredScopes>test</ignoredScopes>
                                     <message>Dependencies shouldn't require Java 8+.</message>
                                 </enforceBytecodeVersion>
+                                <requireMavenVersion>
+                                    <version>3.2.5</version>
+                                </requireMavenVersion>
                             </rules>
                             <fail>true</fail>
                         </configuration>
@@ -330,7 +333,13 @@
             <plugin>
                 <groupId>org.cyclonedx</groupId>
                 <artifactId>cyclonedx-maven-plugin</artifactId>
-                <version>2.3.0</version>
+                <version>2.4.0</version>
+                <executions>
+                  <execution>
+                    <phase>package</phase>
+                    <goals><goal>makeBom</goal></goals>
+                  </execution>
+                </executions>
             </plugin>
             <plugin>
                 <groupId>com.github.spotbugs</groupId>
@@ -367,6 +376,16 @@
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-javadoc-plugin</artifactId>
            </plugin>
+           <plugin>
+               <groupId>org.apache.maven.plugins</groupId>
+               <artifactId>maven-pmd-plugin</artifactId>
+               <version>3.14.0</version>
+               <configuration>
+                   <targetJdk>1.7</targetJdk>
+                   <sourceEncoding>utf-8</sourceEncoding>
+                   <!-- excludeFromFailureFile>exclude-pmd.properties</excludeFromFailureFile -->
+               </configuration>
+           </plugin>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-project-info-reports-plugin</artifactId>

src/main/java/org/owasp/validator/css/CssHandler.java

@@ -498,6 +498,7 @@ public void property(String name, LexicalUnit value, boolean important)
 				styleSheet.append(validator.lexicalValueToString(value));
 				value = value.getNextLexicalUnit();
 			}
+			if (important) { styleSheet.append(" !important"); }
 			styleSheet.append(';');
 			if (!isInline) { styleSheet.append('\n'); }
 

src/main/java/org/owasp/validator/html/InternalPolicy.java

@@ -2,13 +2,12 @@
 
 import org.owasp.validator.html.model.Tag;
 
-import java.net.URL;
 import java.util.Map;
 
 /**
  * Contains a bunch of optimized lookups over the regular Policy Class. For internal use only.
  *
- * Not part of any public api and may explode or self destruct at any given moment, preferably both.
+ * Not part of any public API and may explode or self-destruct at any given moment, preferably both.
  *
  * @author Kristian Rosenvold
  */
@@ -31,7 +30,7 @@ public class InternalPolicy extends Policy {
     private final boolean allowDynamicAttributes;
 
 
-    protected InternalPolicy(URL baseUrl, ParseContext parseContext) throws PolicyException {
+    protected InternalPolicy(ParseContext parseContext) throws PolicyException {
         super(parseContext);
         this.maxInputSize = determineMaxInputSize();
         this.isNofollowAnchors = isTrue(Policy.ANCHORS_NOFOLLOW);
@@ -41,8 +40,8 @@ protected InternalPolicy(URL baseUrl, ParseContext parseContext) throws PolicyEx
         this.omitXmlDeclaration = isTrue(Policy.OMIT_XML_DECLARATION);
         this.omitDoctypeDeclaration = isTrue(Policy.OMIT_DOCTYPE_DECLARATION);
         this.entityEncodeIntlCharacters = isTrue(Policy.ENTITY_ENCODE_INTL_CHARS);
-        useXhtml = isTrue(Policy.USE_XHTML);
-        embedTag = getTagByLowercaseName("embed");
+        this.useXhtml = isTrue(Policy.USE_XHTML);
+        this.embedTag = getTagByLowercaseName("embed");
         this.onUnknownTag = getDirective("onUnknownTag");
         this.isEncodeUnknownTag = "encode".equals(onUnknownTag);
         this.preserveComments = isTrue(Policy.PRESERVE_COMMENTS);
@@ -61,8 +60,8 @@ protected InternalPolicy(Policy old, Map<String, String> directives, Map<String,
         this.omitXmlDeclaration = isTrue(Policy.OMIT_XML_DECLARATION);
         this.omitDoctypeDeclaration = isTrue(Policy.OMIT_DOCTYPE_DECLARATION);
         this.entityEncodeIntlCharacters = isTrue(Policy.ENTITY_ENCODE_INTL_CHARS);
-        useXhtml = isTrue(Policy.USE_XHTML);
-        embedTag = getTagByLowercaseName("embed");
+        this.useXhtml = isTrue(Policy.USE_XHTML);
+        this.embedTag = getTagByLowercaseName("embed");
         this.onUnknownTag = getDirective("onUnknownTag");
         this.isEncodeUnknownTag = "encode".equals(onUnknownTag);
         this.preserveComments = isTrue(Policy.PRESERVE_COMMENTS);
@@ -127,7 +126,6 @@ private boolean isTrue(String anchorsNofollow) {
         return "true".equals(getDirective(anchorsNofollow));
     }
 
-
     public String getOnUnknownTag() {
         return onUnknownTag;
     }
@@ -144,7 +142,7 @@ public boolean isAllowDynamicAttributes() {
      * Returns the maximum input size. If this value is not specified by
      * the policy, the <code>DEFAULT_MAX_INPUT_SIZE</code> is used.
      *
-     * @return the maximium input size.
+     * @return the maximum input size.
      */
     public int determineMaxInputSize() {
         int maxInputSize = Policy.DEFAULT_MAX_INPUT_SIZE;

src/main/java/org/owasp/validator/html/Policy.java

@@ -254,7 +254,7 @@ public static void setSchemaValidation(boolean enable) {
     }
 
     /**
-     * This retrieves a Policy based on a default location ("resources/antisamy.xml")
+     * Construct a Policy using the default policy file location ("resources/antisamy.xml").
      *
      * @return A populated Policy object based on the XML policy file located in the default location.
      * @throws PolicyException If the file is not found or there is a problem parsing the file.
@@ -264,7 +264,7 @@ public static Policy getInstance() throws PolicyException {
     }
 
     /**
-     * This retrieves a Policy based on the file name passed in
+     * Construct a Policy based on the file whose name is passed in.
      *
      * @param filename The path to the XML policy file.
      * @return A populated Policy object based on the XML policy file located in the location passed in.
@@ -276,9 +276,9 @@ public static Policy getInstance(String filename) throws PolicyException {
     }
 
     /**
-     * This retrieves a Policy based on the InputStream object passed in
+     * Construct a Policy from the InputStream object passed in.
      *
-     * @param inputStream An InputStream which contains thhe XML policy information.
+     * @param inputStream An InputStream which contains the XML policy information.
      * @return A populated Policy object based on the XML policy file pointed to by the inputStream parameter.
      * @throws PolicyException If there is a problem parsing the input stream.
      */
@@ -287,11 +287,11 @@ public static Policy getInstance(InputStream inputStream) throws PolicyException
         // If schema validation is disabled, we elevate this msg to the warn level to match the
         // level of the mandatory warning that will follow. We do the same below.
         if (validateSchema) logger.info(logMsg); else logger.warn(logMsg);
-        return new InternalPolicy(null, getSimpleParseContext(getTopLevelElement(inputStream)));
+        return new InternalPolicy(getSimpleParseContext(getTopLevelElement(inputStream)));
     }
 
     /**
-     * This retrieves a Policy based on the File object passed in
+     * Construct a Policy from the File object passed in.
      *
      * @param file A File object which contains the XML policy information.
      * @return A populated Policy object based on the XML policy file pointed to by the File parameter.
@@ -307,10 +307,17 @@ public static Policy getInstance(File file) throws PolicyException {
     }
 
     /**
-     * This retrieves a Policy based on the URL object passed in.
+     * Construct a Policy from the target of the URL passed in.
      * <br><br>
      * NOTE: This is the only factory method that will work with &lt;include&gt; tags
      * in AntiSamy policy files.
+     * <br><br>
+     * For security reasons, the provided URL must point to a local file. Currently only 'file:' and 'jar:'
+     * URL prefixes are allowed. If you want to use a different URL format, and are confident that the URL
+     * points to a safe source, you can open the target of the URL with URL.openStream(), and use the
+     * getInstance(InputStream) constructor instead. For example, Spring has classpath: and Wildfly/Jboss
+     * supports vfs: for accessing local files. Just be aware that this alternate constructor doesn't support
+     * the use of &lt;include&gt; tags, per the NOTE above.
      *
      * @param url A URL object which contains the XML policy information.
      * @return A populated Policy object based on the XML policy file pointed to by the File parameter.
@@ -319,7 +326,7 @@ public static Policy getInstance(File file) throws PolicyException {
     public static Policy getInstance(URL url) throws PolicyException {
         String logMsg = "Attempting to load AntiSamy policy from URL: " + url.toString();
         if (validateSchema) logger.info(logMsg); else logger.warn(logMsg);
-        return new InternalPolicy(url, getParseContext(getTopLevelElement(url), url));
+        return new InternalPolicy(getParseContext(getTopLevelElement(url), url));
     }
 
     protected Policy(ParseContext parseContext) throws PolicyException {
@@ -438,7 +445,7 @@ protected static Element getTopLevelElement(InputSource source, Callable<InputSo
             thrownException = e;
             throw new PolicyException(e);
         } finally {
-            if (!validateSchema && (thrownException == null)) {
+            if (!validateSchema && thrownException == null) {
                 // We warn when the policy has a valid schema, but schema validation is disabled.
                 logger.warn("XML schema validation is disabled for a valid AntiSamy policy. Please reenable policy validation.");
             }
@@ -542,7 +549,7 @@ private static Element getPolicy(String href, URL baseUrl) throws PolicyExceptio
             thrownException = e;
             throw new PolicyException(e);
         } finally {
-            if (!validateSchema && (thrownException == null)) {
+            if (!validateSchema && thrownException == null) {
                 // We warn when the policy has a valid schema, but schema validation is disabled.
                 logger.warn("XML schema validation is disabled for a valid AntiSamy policy. Please reenable policy validation.");
             }
@@ -561,10 +568,7 @@ private static Element getDocumentElementByUrl(String href, URL baseUrl, boolean
         // system id, since we have a base URI.
         if (href != null && baseUrl != null) {
 
-            if (!"file".equals(baseUrl.getProtocol())) {
-                throw new MalformedURLException(
-                    "Only local files can be accessed with the baseURL. Illegal value supplied was: " + baseUrl);
-            }
+            verifyLocalUrl(baseUrl);
 
             URL url;
 
@@ -1007,10 +1011,7 @@ public static InputSource resolveEntity(final String systemId, URL baseUrl) thro
         // system id, since we have a base URI.
         if (systemId != null && baseUrl != null) {
 
-            if (!"file".equals(baseUrl.getProtocol())) {
-                throw new MalformedURLException(
-                    "Only local files can be accessed with the baseURL. Illegal value supplied was: " + baseUrl);
-            }
+            verifyLocalUrl(baseUrl);
 
             URL url;
 
@@ -1019,7 +1020,7 @@ public static InputSource resolveEntity(final String systemId, URL baseUrl) thro
                 source = new InputSource(url.openStream());
                 source.setSystemId(systemId);
                 return source;
-            } catch (MalformedURLException | java.io.FileNotFoundException e) {
+            } catch (MalformedURLException | FileNotFoundException e) {
                 try {
                     String absURL = URIUtils.resolveAsString(systemId, baseUrl.toString());
                     url = new URL(absURL);
@@ -1037,6 +1038,24 @@ public static InputSource resolveEntity(final String systemId, URL baseUrl) thro
         return null;
     }
 
+    /**
+     * Verify that the target of the URL is a local file only. Currently, we allow file: and jar: URLs.
+     * The target of the URL is typically an AntiSamy policy file.
+     * @param url The URL to verify.
+     * @throws MalformedURLException If the supplied URL does not reference a local file directly, or one inside
+     * a local JAR file.
+     */
+    private static void verifyLocalUrl(URL url) throws MalformedURLException {
+
+        switch (url.getProtocol()) {
+            case "file":
+            case "jar" : break; // These are OK.
+
+            default: throw new MalformedURLException(
+                "Only local files can be accessed with a policy URL. Illegal value supplied was: " + url);
+        }
+    }
+
     private static Element getFirstChild(Element element, String tagName) {
         if (element == null) return null;
         NodeList elementsByTagName = element.getElementsByTagName(tagName);

src/main/java/org/owasp/validator/html/scan/MagicSAXFilter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li
+ * Copyright (c) 2007-2021, Arshan Dabirsiaghi, Jason Li
  * 
  * All rights reserved.
  * 
@@ -58,7 +58,7 @@
 @SuppressFBWarnings(value = "REDOS", justification="Tested the Regex against saferegex and safe-regex and not vulnerable")
 public class MagicSAXFilter extends DefaultFilter implements XMLDocumentFilter {
 
-    private static enum Ops {
+    private enum Ops {
         CSS, FILTER, REMOVE, TRUNCATE, KEEP, ENCODE
     }
 	private final Stack<Ops> operations = new Stack<Ops>();
@@ -97,7 +97,7 @@ public void reset(InternalPolicy instance){
     }
 
 	public void characters(XMLString text, Augmentations augs) throws XNIException {
-        //noinspection StatementWithEmptyBody
+
         Ops topOp = peekTop();
         //noinspection StatementWithEmptyBody
         if (topOp ==  Ops.REMOVE) {
@@ -319,15 +319,13 @@ public void startElement(QName element, XMLAttributes attributes, Augmentations
                             isValid = true;
                         }
 
-
                         if (!isValid) {
                             isValid = attribute.matchesAllowedExpression(value);
                             if (isValid) {
                                 validattributes.addAttribute(makeSimpleQname(name), "CDATA", value);
                             }
                         }
 
-
                         // if value or regexp matched, attribute is already
 						// copied, but what happens if not
 						if (!isValid && "removeTag".equals(attribute.getOnInvalid())) {

src/test/java/org/owasp/validator/html/test/AntiSamyTest.java

@@ -1479,5 +1479,16 @@ public void testGithubIssue62() {
             fail(exc.getMessage());
         }
     }
+
+    @Test
+    public void testGithubIssue81() throws ScanException, PolicyException {
+        // Concern is that "!important" is missing after processing CSS
+        assertThat(as.scan("<p style=\"color: red !important\">Some Text</p>", policy, AntiSamy.DOM).getCleanHTML(), containsString("!important"));
+        assertThat(as.scan("<p style=\"color: red !important\">Some Text</p>", policy, AntiSamy.SAX).getCleanHTML(), containsString("!important"));
+
+        // Just to check scan keeps working accordingly without "!important"
+        assertThat(as.scan("<p style=\"color: red\">Some Text</p>", policy, AntiSamy.DOM).getCleanHTML(), not(containsString("!important")));
+        assertThat(as.scan("<p style=\"color: red\">Some Text</p>", policy, AntiSamy.SAX).getCleanHTML(), not(containsString("!important")));
+    }
 }