tballison commented on PR #3115:
URL: https://github.com/apache/tika/pull/3115#issuecomment-5508249776
This is really nice.
My bot has maybe one or two things worth changing. The hygiene thing with
MediaType.parse should be fixed now in main(?).
Let me know what you think, and thank you!
```
1. edge-case — the offset gate accepts any recognized type, not a video;
that makes a JPEG recursive
MotionPhoto.java:222 — if (type == null ||
MediaType.OCTET_STREAM.equals(type)) { return; }
Two measured consequences:
- Replace the trailing 1583 bytes of testJPEG_MotionPhoto.jpg with ASCII
text → a second document is emitted, text/plain, named motion-photo.mp4.
CHANGES.txt and the PR description both say nothing is emitted "when the bytes
there are not a video". That is
not what the code does.
- outer || inner, where outer is the fixture with Item:Length rewritten to
|inner| and inner is a motion photo → 3 documents: depth 1 image/jpeg named
motion-photo.mp4, depth 2 its own video/mp4. Chained 1500 deep (~1.4 KB per
link, 2.19 MB input), a plain
AutoDetectParser.parse → StackOverflowError.
The depth cap that saves the /rmeta path is
AbstractRecursiveParserWrapperHandler.MAX_DEPTH = 100, which only exists on the
RecursiveParserWrapper path. ParseRecord.maxEmbeddedDepth defaults to -1, so
/tika, tika-app text output and the Tika facade have no
guard. Even at the capped 100 levels each level re-spools the whole
remaining suffix via tis.getPath(), so a 50 MB input writes gigabytes of temp
files.
Fix is one line and is what the PR already claims: require
"video".equals(type.getType()). It matches both specs (the item's semantic is
the video), and it closes the recursion for good, since a video parser never
re-enters this code. Add the missing test —
a trailer that is recognized but not a video; testDeclaredVideoIsNotOne
only covers unrecognized zeros.
2. edge-case — extension from the declaration, content type from detection
MotionPhoto.java:221-233. The two can disagree (motion-photo.mp4 on
text/plain, above). The PR's rationale — "the parse that follows knows the
format for certain, but by then the name is fixed" — is about the parse; the
detected type is already in hand at
line 221, before the name is set at 229. Also
RESOURCE_NAME_EXTENSION_INFERRED=true is documented as "inferred by Tika (e.g.
from content type detection) rather than provided by the original document",
and here it is the document's declaration. If the
declaration is deliberately preferred, worth saying why detection-at-221
was rejected.
3. edge-case (low) — IOException on the video path fails the whole image
file(tis) maps getPath()'s IOException to null, but Files.size (:217),
detect (:221) and region (:239) propagate out of the image parser. That
contradicts the stated contract ("a video out of reach is no reason to fail an
image that parsed"). Cheap fix:
extend the guard over the rest of the body.
Hygiene
- MediaType.parse(metadata.get(... "Item:Mime")) (:289) interns an
attacker-controlled string into the static, never-evicted
MediaType.SIMPLE_TYPES — the same class of thing TIKA-4826/4862 are removing.
The value is only used for an extension, and
EmbeddedDocumentUtil.getExtensionForMediaType(String) already takes a
String and normalizes itself; dropping the parse removes the interning and a
field.
- CHANGES.txt "the bytes there are not a video" — fixed by finding 1.
- parseEmbedded(..., outputHtml=false) with no <div class="embedded"> of
its own; EMFParser, the closest precedent, passes true.
- 1583 is hardcoded in four places across two tests; derive it from the
fixture.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
