feat(webdav): extract thumbnails through Tika - #3332
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
| CodeStyle | 1 minor |
🟢 Metrics 96 complexity
Metric Results Complexity 96
🟢 Coverage 78.95% diff coverage
Metric Results Coverage variation Report missing for 39569b91 Diff coverage ✅ 78.95% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (39569b9) Report Missing Report Missing Report Missing Head commit (ae5d6fc) 88026 21693 24.64% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3332) 190 150 78.95% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
4043342 to
47a7136
Compare
There was a problem hiding this comment.
Pull request overview
Adds embedded JPEG preview extraction for TIFF-based camera RAW thumbnails.
Changes:
- Adds RAW MIME routing and documentation.
- Implements bounded TIFF/BigTIFF preview extraction with EXIF orientation.
- Adds extraction, hardening, orientation, and decoder tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
services/thumbnails/README.md |
Documents RAW thumbnail support. |
pkg/thumbnail/mimetypes.go |
Registers RAW MIME types. |
pkg/thumbnail/mimetypes_vips.go |
Registers RAW types for VIPS. |
pkg/service/grpc/v0/service.go |
Handles missing RAW previews quietly. |
pkg/preprocessor/rawtiff.go |
Extracts and validates embedded JPEGs. |
pkg/preprocessor/rawtiff_test.go |
Tests RAW extraction and hardening. |
pkg/preprocessor/preprocessor.go |
Routes RAW types to the decoder. |
pkg/preprocessor/preprocessor_test.go |
Tests RAW MIME routing. |
pkg/errors/error.go |
Adds the missing-preview error. |
Suppressed comments (4)
services/thumbnails/pkg/preprocessor/rawtiff.go:142
- This bounds check can overflow before it checks the untrusted BigTIFF offset. For example, a first-IFD offset near
MaxUint64makesifdOffset + 8wrap belowdlen, after which the slice on line 147 panics. Use subtraction-based bounds checks (or explicit overflow checks) before slicing so a crafted raw upload cannot crash the service.
if ifdOffset == 0 || ifdOffset+uint64(countSize) > dlen {
services/thumbnails/pkg/preprocessor/rawtiff.go:204
arrayOffsetis file-controlled, so both additions here can wrap. With an offset nearMaxUint64,pos + offWbecomes a small number and passes the check, thendata[pos:]panics. Reject offsets using subtraction/overflow-safe bounds checks before reading the SubIFD array.
pos := arrayOffset + j*uint64(offW)
if pos+uint64(offW) > dlen || len(queue) >= maxIFDs {
services/thumbnails/pkg/preprocessor/rawtiff.go:117
- BigTIFF widens the entry value field, but accepted
LONGvalues are still 4 bytes. Reading every BigTIFF field asUint64misreads a big-endian inline LONG asvalue << 32; the 8-byte stride used later also misreads LONG SubIFD lists. Decode values and array strides fromtyp, not only from the container, otherwise valid big-endian BigTIFF/DNG previews are missed.
// readOff reads a 4- or 8-byte offset/value depending on the container.
readOff := func(b []byte) uint64 {
if bigTiff {
return order.Uint64(b[:8])
}
return uint64(order.Uint32(b[:4]))
}
services/thumbnails/pkg/preprocessor/rawtiff.go:261
- Returning true immediately on a DCT SOF marker does not establish that the JPEG is renderable: the SOF length may be invalid, or the stream may be truncated before any scan. Because extraction keeps only the largest candidate, a larger corrupt candidate can then mask a valid smaller preview and make
Convertfail instead of falling back. Validate enough of the stream to reject malformed candidates, or try candidates in size order until one decodes.
case 0xc0, 0xc1, 0xc2: // baseline, extended sequential, progressive
return true
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // isLongType reports whether a tag type carries a 32-bit (or, in BigTIFF, a | ||
| // 64-bit) integer offset we can follow. | ||
| func isLongType(typ uint16, bigTiff bool) bool { | ||
| return typ == tiffTypeLong || (bigTiff && typ == tiffTypeLong8) |
… width Review of #3332 surfaced two BigTIFF-only defects in the embedded-preview walker (attacker-controlled input): - 64-bit IFD and SubIFD-array offsets were bounds-checked with `off + n > len`, which wraps for a crafted offset near 2^64, bypassing the guard and slicing out of range -> panic (recovered by the framework into a 500 + stack log on every crafted request, defeating the no-panic goal). Now overflow-safe. - a tag value was always read as an 8-byte Uint64 in BigTIFF; a LONG (4-byte) offset in a big-endian BigTIFF was thereby shifted. Read it at its declared type width instead. Adds tests for the overflow paths (assert ErrNoImageFromRawFile, no panic), the big-endian LONG offset, and strengthens the truncation test to assert the error. Trims a few over-long comments.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
services/thumbnails/pkg/preprocessor/rawtiff.go:201
- SubIFD pointers may use TIFF type IFD (13) and BigTIFF type IFD8 (18), but
isLongTypeaccepts only LONG/LONG8, so valid preview-bearing SubIFDs using the pointer-specific types are skipped. Extend the type-width/read helpers to accept IFD/IFD8; otherwise such DNG/BigTIFF files incorrectly report no preview.
case tiffTagSubIFDs:
if !isLongType(typ, bigTiff) {
continue
services/thumbnails/pkg/preprocessor/rawtiff.go:221
- In BigTIFF the value-or-offset field is 8 bytes, so a SubIFDs value containing exactly two LONG offsets is stored inline. Treating every
count > 1value as an array pointer interprets those two offsets as one bogus uint64 file offset and misses both preview IFDs.
// count > 1: the value field points at an array of count offsets,
// each of the tag's own width (LONG 4B, LONG8 8B)
arrayOffset := readOff(entry[valAt:])
services/thumbnails/pkg/preprocessor/rawtiff.go:196
- Valid TIFF encodes both
StripOffsetsandStripByteCountsas either SHORT or LONG, but these guards reject SHORT. A raw file whose only embedded JPEG is a single strip using SHORT values will therefore produceErrNoImageFromRawFile. Please read scalar SHORT values as 16-bit values for both tags (and add a regression case).
if isLongType(typ, bigTiff) && count == 1 {
stripOffset = readVal(entry[valAt:], typ)
}
case tiffTagStripByteCounts:
if isLongType(typ, bigTiff) && count == 1 {
rhafer
left a comment
There was a problem hiding this comment.
As much as I'd like to have previews for my raw images (DNG in my case), I'd really prefer us to get not into the business of creating/maintaining a TIFF parser (even with the help of AI). Especially given the experience with the recent security flaws in libvips (which were also TIFF related).
I see your PR to evanoberholster/imagemeta got merged. So using that might help.
And then there is #1128 which suggests to replace the embedded thumbnailer with something like https://github.com/cshum/imagor for better scalability and broader images format support. (Unfortunately AFAICS imagor does not extract the embedded previews though, but tries to render the rawdata)
I am not giving this a 👎 , but I think we should not merge without further consideration.
@butonic any thoughts on this?
|
Thanks for the considerate review, @rhafer :) One more idea - tika has a concept of embedded files. I've already implemented audio artwork support using that mechanism. I mainly wanted both extractions to implement the thumbnails relationship on driveItems, but thinking about it in the context of your concerns and the possible move to imagor it makes me think: We could pipe audio files and raw images from the thumbnails service to tika for preview extraction. Having implemented the original audio artwork thumbnails feature, I can say I'd happily drop the built in support for something more maintained like tika. Moreover, we'd use the same mechanism as in the search service and the supported file formats and features would always align (unlike with a completely different code path like right now). Thoughts? |
|
I liked my idea so much, that I immediately wanted to give it a spin 😁 +395 now, the built in approach had +936 - I'm satisfied :D What do you think? |
|
Tika PR is merged 🙂 |
4ba384f to
0d0a6a6
Compare
0d0a6a6 to
9ac2bc4
Compare
05072f1 to
92a183b
Compare
e375cbe to
372bc24
Compare
026b932 to
07eff1b
Compare
954eb9b to
39569b9
Compare
Every file the built-in converters cannot render goes to /unpack/thumbnail (Tika 4.1) and comes back with the image Tika marks as its thumbnail.
372bc24 to
7c4ed8a
Compare
7c4ed8a to
ae5d6fc
Compare
Thumbnails for everything the in-process decoders cannot read: raw camera images, audio cover art, and whatever else Tika learns to extract. One generic step, no format-specific code here.
webdav asks Tika for
/unpack/alland takes the embedded document Tika marked as the document'sTHUMBNAIL. Needs Tika 4.1: in 4.0 cover art is onlyINLINEand raw files come back whole, an 8 MB TIFF instead of a preview.Rendered previews (a pdf page, the metafile an office document carries) are not part of this. Tika renders only when its parsers are configured to, and that cannot be asked for per request: the old design used a
renderThumbnailsflag on an endpoint of its own, which is not going to exist. Doing it from here would mean either turning on per-request config on the Tika server or a catalog preset, so for now those documents have no thumbnail. A rendering that is there anyway is used, nested under a thumbnail or standing for the document itself, so a preset later needs no change here.Verified against
apache/tika:4.1.0-SNAPSHOT-full: mp3 yields its cover asTHUMBNAIL, a NEF yields a 482 KB JPEG preview and is detected asimage/x-raw-nikon, and a pdf yields nothing even withpdfbox-rendererconfigured. The rendering paths follow TIKA-4855 and are covered by unit tests, not measured against a real document.Stacked on #3397.