#467 (related) Prevent CSS import loop

+ With test. + Cleanup of CSS code. + Removed stylesheet cache.
danfickle · Dec 19, 2020 · 34aaf6a · 34aaf6a
1 parent 2d8edb7
commit 34aaf6a
Show file tree

Hide file tree

Showing 14 changed files with 74 additions and 146 deletions.
diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StyleReference.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StyleReference.java
@@ -36,7 +36,6 @@
 import com.openhtmltopdf.css.extend.lib.DOMTreeResolver;
 import com.openhtmltopdf.css.newmatch.CascadedStyle;
 import com.openhtmltopdf.css.newmatch.PageInfo;
-import com.openhtmltopdf.css.newmatch.Selector;
 import com.openhtmltopdf.css.parser.CSSPrimitiveValue;
 import com.openhtmltopdf.css.sheet.PropertyDeclaration;
 import com.openhtmltopdf.css.sheet.Stylesheet;
@@ -211,29 +210,6 @@ public PageInfo getPageStyle(String pageName, String pseudoPage) {
         return _matcher.getPageCascadedStyle(pageName, pseudoPage);
     }
 
-    /**
-     * Flushes any stylesheet associated with this stylereference (based on the user agent callback) that are in cache.
-     * Deprecated for now, until we fix caching, use a new <code>StylesheetFactory</code> each run.
-     */
-    @Deprecated
-    public void flushStyleSheets() {
-        String uri = _uac.getBaseURL();
-        StylesheetInfo info = new StylesheetInfo();
-        info.setUri(uri);
-        info.setOrigin(StylesheetInfo.AUTHOR);
-        if (_stylesheetFactory.containsStylesheet(uri)) {
-            _stylesheetFactory.removeCachedStylesheet(uri);
-            XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.CSS_PARSE_REMOVING_STYLESHEET_URI_FROM_CACHE_BY_REQUEST, uri);
-        } else {
-            XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.CSS_PARSE_REQUESTED_REMOVING_STYLESHEET_URI_NOT_IN_CACHE, uri);
-        }
-    }
-
-    @Deprecated
-    public void flushAllStyleSheets() {
-        _stylesheetFactory.flushCachedStylesheets();
-    }
-
     /**
      * Gets StylesheetInfos for all stylesheets and inline styles associated
      * with the current document. Default (user agent) stylesheet and the inline

diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StylesheetFactoryImpl.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/context/StylesheetFactoryImpl.java
@@ -21,6 +21,8 @@
 
 import java.io.IOException;
 import java.io.Reader;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.logging.Level;
 
 import com.openhtmltopdf.css.extend.StylesheetFactory;
@@ -46,21 +48,19 @@ public class StylesheetFactoryImpl implements StylesheetFactory {
      */
     private UserAgentCallback _userAgentCallback;
 
-    private final int _cacheCapacity = 16;
+    /**
+     * This may avoid @import loops, ie. one.css includes two.css
+     * which then includes one.css.
+     */
+    private final Map<String, Integer> _seenStylesheetUris = new HashMap<>();
 
     /**
-     * an LRU cache
+     * The maximum number of times a stylesheet uri can be link or
+     * imported before we give up and conclude there is a loop.
      */
-    private final java.util.LinkedHashMap<String, Stylesheet> _cache =
-            new java.util.LinkedHashMap<String, Stylesheet>(_cacheCapacity, 0.75f, true) {
-                private static final long serialVersionUID = 1L;
+    private static final int MAX_STYLESHEET_INCLUDES = 10;
 
-                protected boolean removeEldestEntry(java.util.Map.Entry<String, Stylesheet> eldest) {
-                    return size() > _cacheCapacity;
-                }
-            };
-
-    private CSSParser _cssParser;
+    private final CSSParser _cssParser;
 
     public StylesheetFactoryImpl(UserAgentCallback userAgentCallback) {
         _userAgentCallback = userAgentCallback;
@@ -108,74 +108,27 @@ public Ruleset parseStyleDeclaration(int origin, String styleDeclaration) {
     }
 
     /**
-     * Adds a stylesheet to the factory cache. Will overwrite older entry for
-     * same key.
-     *
-     * @param key   Key to use to reference sheet later; must be unique in
-     *              factory.
-     * @param sheet The sheet to cache.
-     */
-    @Deprecated
-    public void putStylesheet(String key, Stylesheet sheet) {
-        _cache.put(key, sheet);
-    }
-
-    /**
-     * @param key
-     * @return true if a Stylesheet with this key has been put in the cache.
-     *         Note that the Stylesheet may be null.
-     */
-    //TODO: work out how to handle caching properly, with cache invalidation
-    @Deprecated
-    public boolean containsStylesheet(String key) {
-        return _cache.containsKey(key);
-    }
-
-    /**
-     * Returns a cached sheet by its key; null if no entry for that key.
-     *
-     * @param key The key for this sheet; same as key passed to
-     *            putStylesheet();
-     * @return The stylesheet
-     */
-    @Deprecated
-    public Stylesheet getCachedStylesheet(String key) {
-        return _cache.get(key);
-    }
-
-    /**
-     * Removes a cached sheet by its key.
-     *
-     * @param key The key for this sheet; same as key passed to
-     *            putStylesheet();
-     */
-    @Deprecated
-    public Object removeCachedStylesheet(String key) {
-        return _cache.remove(key);
-    }
-
-    @Deprecated
-    public void flushCachedStylesheets() {
-        _cache.clear();
-    }
-
-    /**
-     * Returns a cached sheet by its key; loads and caches it if not in cache;
+     * Returns a sheet by its key
      * null if not able to load
      *
+     *
      * @param info The StylesheetInfo for this sheet
      * @return The stylesheet
      */
-    //TODO: this looks a bit odd
     public Stylesheet getStylesheet(StylesheetInfo info) {
         XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.LOAD_REQUESTING_STYLESHEET_AT_URI, info.getUri());
 
-        Stylesheet s = getCachedStylesheet(info.getUri());
-        if (s == null && !containsStylesheet(info.getUri())) {
-            s = parse(info);
-            putStylesheet(info.getUri(), s);
+        Integer includeCount = _seenStylesheetUris.get(info.getUri());
+
+        if (includeCount != null && includeCount >= MAX_STYLESHEET_INCLUDES) {
+            // Probably an import loop.
+            XRLog.log(Level.SEVERE, LogMessageId.LogMessageId2Param.CSS_PARSE_TOO_MANY_STYLESHEET_IMPORTS, includeCount, info.getUri());
+            return null;
         }
-        return s;
+
+        _seenStylesheetUris.merge(info.getUri(), 1, (oldV, newV) -> oldV + 1);
+
+        return parse(info);
     }
 
     public void setUserAgentCallback(UserAgentCallback userAgent) {

diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/CSSParser.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/parser/CSSParser.java
@@ -300,7 +300,7 @@ private void import_rule(Stylesheet stylesheet) throws IOException {
                             t, new Token[] { Token.TK_STRING, Token.TK_URI }, getCurrentLine());
                 }
 
-                if (info.getMedia().size() == 0) {
+                if (info.getMedia().isEmpty()) {
                     info.addMedium("all");
                 }
                 stylesheet.addImportRule(info);

diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/sheet/StylesheetInfo.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/css/sheet/StylesheetInfo.java
@@ -20,6 +20,7 @@
 package com.openhtmltopdf.css.sheet;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Locale;
 
@@ -38,12 +39,12 @@
  */
 public class StylesheetInfo {
 
-    private Stylesheet stylesheet = null;//just to be able to attach "dummy" stylesheets. Also might save a lookup if it's already looked up
+    private Stylesheet stylesheet = null;
     private String title;
     private String uri;
     private int origin = USER_AGENT;
     private String type;
-    private List<String> mediaTypes = new ArrayList<>();
+    private final List<String> mediaTypes = new ArrayList<>();
     private String content;
 
     /** Origin of stylesheet - user agent  */
@@ -54,15 +55,20 @@ public class StylesheetInfo {
 
     /** Origin of stylesheet - author  */
     public final static int AUTHOR = 2;
-
 
     /**
      * @param m  a single media identifier
      * @return   true if the stylesheet referenced applies to the medium
      */
     public boolean appliesToMedia(String m) {
-        return m.toLowerCase(Locale.US).equals("all") || 
-            mediaTypes.contains("all") || mediaTypes.contains(m.toLowerCase(Locale.US));
+        if (mediaTypes.contains("all")) {
+            return true;
+        }
+
+        String media = m.toLowerCase(Locale.US);
+
+        return media.equals("all") ||
+               mediaTypes.contains(media);
     }
 
     /**
@@ -80,18 +86,18 @@ public void setUri(String uri) {
      * @param media  The new media value
      */
     public void setMedia(String media) {
-        String[] mediaTypes = media.split(",");
         this.mediaTypes.clear();
-
-        for (int i = 0; i < mediaTypes.length; i++) {
-            this.mediaTypes.add(mediaTypes[i].trim().toLowerCase(Locale.US));
+
+        if (media == null || media.isEmpty()) {
+            // Common case, no media attribute, applies to all.
+            this.addMedium("all");
+        } else {
+            Arrays.stream(media.split(","))
+                  .map(mediaType -> mediaType.trim().toLowerCase(Locale.US))
+                  .forEach(this::addMedium);
         }
     }
-
-    public void setMedia(List<String> mediaTypes) {
-        this.mediaTypes = mediaTypes;
-    }
-
+
     public void addMedium(String medium) {
         mediaTypes.add(medium);
     }
@@ -193,7 +199,7 @@ public String getContent() {
     public void setContent(String content) {
         this.content = content;
     }
-    
+
     public boolean isInline() {
         return this.content != null;
     }

diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/layout/SharedContext.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/layout/SharedContext.java
@@ -129,24 +129,6 @@ public class SharedContext {
     public SharedContext() {
     }
 
-
-    /**
-     * Constructor for the Context object
-     * @deprecated This stuff should go in the renderers of a specific device.
-     */
-    @Deprecated
-    public SharedContext(UserAgentCallback uac, FontResolver fr, ReplacedElementFactory ref, TextRenderer tr, float dpi) {
-        fontResolver = fr;
-        replacedElementFactory = ref;
-        setMedia("screen");
-        this.uac = uac;
-        setCss(new StyleReference(uac));
-        XRLog.log(Level.INFO, LogMessageId.LogMessageId1Param.RENDER_USING_CSS_IMPLEMENTATION_FROM, getCss().getClass().getName());
-        setTextRenderer(tr);
-        setDPI(dpi);
-    }
-
-
     public LayoutContext newLayoutContextInstance() {
         LayoutContext c = new LayoutContext(this);
         return c;

diff --git a/...opdf-core/src/main/java/com/openhtmltopdf/simple/extend/XhtmlCssOnlyNamespaceHandler.java b/...opdf-core/src/main/java/com/openhtmltopdf/simple/extend/XhtmlCssOnlyNamespaceHandler.java
@@ -255,11 +255,9 @@ private Element findFirstChild(Element parent, String targetName) {
     }
 
     protected StylesheetInfo readStyleElement(Element style) {
-        String media = style.getAttribute("media");
-        if ("".equals(media)) {
-            media = "all";
-        }//default for HTML is "screen", but that is silly and firefox seems to assume "all"
         StylesheetInfo info = new StylesheetInfo();
+        String media = style.getAttribute("media");
+
         info.setMedia(media);
         info.setType(style.getAttribute("type"));
         info.setTitle(style.getAttribute("title"));
@@ -306,12 +304,9 @@ protected StylesheetInfo readLinkElement(Element link) {
         info.setType(type);
 
         info.setOrigin(StylesheetInfo.AUTHOR);
-
         info.setUri(link.getAttribute("href"));
+
         String media = link.getAttribute("media");
-        if ("".equals(media)) {
-            media = "all";
-        }
         info.setMedia(media);
 
         String title = link.getAttribute("title");

diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/LogMessageId.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/util/LogMessageId.java
@@ -195,6 +195,7 @@ public String formatMessage(Object[] args) {
     }
 
     enum LogMessageId2Param implements LogMessageId {
+        CSS_PARSE_TOO_MANY_STYLESHEET_IMPORTS(XRLog.CSS_PARSE, "Gave up after {} attempts to load stlyesheet at {} to avoid possible loop"),
         CSS_PARSE_COULDNT_PARSE_STYLESHEET_AT_URI(XRLog.CSS_PARSE, "Couldn't parse stylesheet at URI {}: {}"),
         CSS_PARSE_GENERIC_MESSAGE(XRLog.CSS_PARSE, "({}) {}"),
 

diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/expected/css-import-loop.pdf b/openhtmltopdf-examples/src/main/resources/visualtest/expected/css-import-loop.pdf
diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/html/css-import-loop.html b/openhtmltopdf-examples/src/main/resources/visualtest/html/css-import-loop.html
@@ -0,0 +1,14 @@
+<html>
+<head>
+<style>
+@page {
+  size: 200px 200px;
+}
+</style>
+<link rel="stylesheet" href="stylesheets/loop-one.css" />
+</head>
+<body>
+<div id="one">RED</div>
+<div id="two">BLUE</div>
+</body>
+</html>
diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-one.css b/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-one.css
@@ -0,0 +1,3 @@
+@import "loop-two.css";
+
+div#one { color: red; }
diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-two.css b/openhtmltopdf-examples/src/main/resources/visualtest/html/stylesheets/loop-two.css
@@ -0,0 +1,3 @@
+@import "loop-one.css";
+
+div#two { color: blue; }
diff --git a/...-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java b/...-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java
@@ -1343,6 +1343,11 @@ public void testMoreRobustDataUriBase64() throws IOException {
         assertTrue(vt.runTest("more-robust-data-uri-base64"));
     }
 
+    @Test
+    public void testCSSImportLoop() throws IOException {
+        assertTrue(vt.runTest("css-import-loop"));
+    }
+
     // TODO:
     // + Elements that appear just on generated overflow pages.
     // + content property (page counters, etc)

diff --git a/openhtmltopdf-java2d/src/main/java/com/openhtmltopdf/java2d/Java2DRenderer.java b/openhtmltopdf-java2d/src/main/java/com/openhtmltopdf/java2d/Java2DRenderer.java
@@ -233,11 +233,6 @@ private void setDocument(Document doc, String url, NamespaceHandler nsh) {
 
         //TODOgetFontResolver().flushFontFaceFonts();
 
-        if (Configuration.isTrue("xr.cache.stylesheets", true)) {
-            _sharedContext.getCss().flushStyleSheets();
-        } else {
-            _sharedContext.getCss().flushAllStyleSheets();
-        }
         _sharedContext.setBaseURL(url);
         _sharedContext.setNamespaceHandler(nsh);
         _sharedContext.getCss().setDocumentContext(_sharedContext, _sharedContext.getNamespaceHandler(), doc, new NullUserInterface());

diff --git a/openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java b/openhtmltopdf-pdfbox/src/main/java/com/openhtmltopdf/pdfboxout/PdfBoxRenderer.java
@@ -314,11 +314,6 @@ private void setDocumentP(Document doc, String url, NamespaceHandler nsh) {
 
         getFontResolver().flushFontFaceFonts();
 
-        if (Configuration.isTrue("xr.cache.stylesheets", true)) {
-            _sharedContext.getCss().flushStyleSheets();
-        } else {
-            _sharedContext.getCss().flushAllStyleSheets();
-        }
         _sharedContext.setBaseURL(url);
         _sharedContext.setNamespaceHandler(nsh);
         _sharedContext.getCss().setDocumentContext(_sharedContext, _sharedContext.getNamespaceHandler(), doc, new NullUserInterface());