Repository navigation
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 critical |
| CodeStyle | 1 minor |
🟢 Metrics 94 complexity
Metric Results Complexity 94
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
954eb9b to
39569b9
Compare
372bc24 to
7c4ed8a
Compare
7c4ed8a to
ae5d6fc
Compare
ae5d6fc to
b5fcb61
Compare
64b25e2 to
b306a32
Compare
1996387 to
59721bc
Compare
8061984 to
e1da7e8
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.
Tika merged the thumbnail catalog preset and ThumbnailUnpackSelector (TIKA-4856): PUT /unpack/preset/thumbnail returns exactly the one raster image a client would show for the document - a stored thumbnail, the rendering of a metafile thumbnail, or the rendered first page. The client-side selection over /unpack/all (embedded-resource-type marks, metafile renderings, id-path matching) moves to the server; the preset zip carries one image without metadata sidecars, so the entry's extension names the type and the bytes break the tie. A 404 now points at the preset activation the server config needs.
The preset ships inert in tika-pipes-core, so PUT /unpack/preset/thumbnail answers 404 until a config names it, and the thumbnailer then finds no thumbnail for any document. The acceptance stack and the example deployment now pass a config that activates it, and the deployment reads the shared OC_TIKA_URL so the thumbnailer uses the Tika that is already there. The acceptance image goes to 4.1, the release the preset is in.
Thumbnails for everything webdav should not decode in-process: raw camera images, audio cover art, the rendered first page of a pdf, the metafile an office document carries, the icon of a windows binary, and whatever else Tika learns next. One generic step, no format-specific code here.
webdav PUTs the file to Tika's
/unpack/preset/thumbnailand hands the image on undecoded, because the generator reads formats webdav cannot. Needs Tika >= 4.1 with the thumbnail preset active in its config ({"presets": {"thumbnail": true}}, TIKA-4856): the preset ships inert, and until a config names it the route answers 404.The thumbnail is written in the format the image carries, mapped through
OC_THUMBNAIL_FORMATS. Its default writes an icon as a PNG, since no generator writes icons back and the fallback is JPEG, which has no alpha channel and would turn a transparent icon black. A file that carries no thumbnail answers 404 rather than 500, so clients cache it as "no preview" instead of retrying.Config:
OC_TIKA_URL,OC_TIKA_THUMBNAIL_MIME_TYPES,OC_THUMBNAIL_FORMATS.Verified end to end against
apache/tika:4.2.0-SNAPSHOT: a pdf renders its first page, an mp3 yields its cover, a windows.exeand.dllyield their icon with transparency intact, an mp3 without cover answers 404. Sharp icons also want apache/tika#3309, which orders an icon group largest first; without it the entry a decoder picks is whatever the resource compiler happened to write first.What a client is told it can preview still comes from the static mime list, so the formats above are reachable by direct request but not advertised yet. #3210 answers that from the index.
Stacked on #3397.