Re: [PR] TIKA-4855: Render EMF/WMF through POI and emit the OLE2 SummaryInformation thumbnail [tika]

["tballison (via GitHub)" <gi...@apache.org>]

tballison commented on PR #3095:
URL: https://github.com/apache/tika/pull/3095#issuecomment-5493234683

   From my :robot: ... I think most are useful. One or two are puntable. Let me know what you think. Thank you for iterating.
   
   ```
     ⎿  tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/renderer/microsoft/POIMetafileRenderer.java
          ● 187 [security]          Rendered height is unbounded (only width is capped at 10000): a hostile metafile's aspect ratio drives an arbitrarily large BufferedImage allocation, and the resulting OutOfMemoryError is an Error that escapes every
                                    catch(Exception) in the render path — in both canvas() (line 187) and the scale() bitmap fallback (line 207).
          ● 152 [correctness]       draw(HwmfPicture) catches bare RuntimeException around picture.getSize() when the intended case is exactly POI's IllegalStateException("invalid wmf file - window records are incomplete.") — unrelated runtime failures
                                    (e.g. an NPE from a POI bug) get silently rerouted to the first-bitmap fallback.
        tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/MetafileRendering.java
          ●  61 [correctness]       renderWidth (and imageFormatName) is silently dead in every default configuration: @TikaComponent on POIMetafileRenderer defaults to spi=true, so DefaultParser's always-injected SPI CompositeRenderer claims image/emf|wmf
                                    and takes precedence over defaultRenderer(config) — the PR's own tests request widths 200/300/400, render at 800, and pass because no test asserts output width.
          ●  67 [correctness]       Injected renderers are handed TikaInputStream.get(new byte[0]) with the parsed picture only as an open container — a POIMetafileRenderer-private convention — so any third-party Renderer that follows the Renderer contract
                                    and reads the stream renders 0 bytes on every file.
          ●  90 [correctness]       Rewriting the renderer-assigned RENDERING type to THUMBNAIL hides thumbnail renderings from type-based filters and yields two THUMBNAIL-typed embedded docs for one file.
          ● 145 [simplification]    renderingName() re-implements basename extraction that tika-core's FilenameUtils.getName() already provides, missing its ':' handling and '.'/'..' sanitization.
          ●  68 [efficiency]        The full rasterization (vector draw + PNG encode + temp-file write) runs before extractor.shouldParseEmbedded is consulted, so an extractor that filters out RENDERING docs still pays the entire render cost per image.
        tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/OfficeParser.java
          ● 310 [compatibility]     OLE2 SummaryInformation thumbnail emission is default-on with no opt-out: handleThumbnail runs unconditionally (MetafileParserConfig.renderImage gates only rendering; OfficeParserConfig has no thumbnail switch), adding an
                                    extra thumbnail.wmf embedded document to every OLE2 file with a stored thumbnail.
        tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/MetafileParserConfig.java
          ●  86 [config-validation] renderOnlyEmbeddedResourceTypes is an unvalidated case-sensitive Set<String> matched against EmbeddedResourceType.name(): a config typo silently disables rendering with no error.
        tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/WMFParser.java
          ●  71 [simplification]    EMFParser and WMFParser duplicate ~35 lines of identical wiring (fields, three constructors, getConfig differing only in the "emf-parser"/"wmf-parser" key, setRenderer plus a caller-less getRenderer), and
                                    POIMetafileRenderer's two draw() overloads and canvas()/scale() duplicate the canvas/height-computation logic.
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tika.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org