Closes#103. Part of #96 (carries the deferred #81 EXIF follow-up).
imagenorm decoded and re-encoded to JPEG but ignored the EXIF Orientation tag. Phone cameras store the sensor pixels one way and set a flag to rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged "rotate 90°" — and the re-encode strips EXIF, so without baking the rotation in, a packet photographed in portrait reaches the vision model sideways (worse OCR) and any future thumbnail is wrong.
What
exifOrientation — a small pure-Go parser that walks the JPEG APP1/Exif segment for tag 0x0112, returning 1 (normal) for non-JPEG or unparseable input (never guess a rotation onto a correct image). No cgo, no new dependency.
applyOrientation — bakes in all 8 orientations (4 rotations + mirrors), applied after downscale (cheaper to rotate the small image; a 90° turn swaps the sides but not the longest edge, so the downscale bound still holds).
Non-JPEG paths (HEIC/webp/png) are untouched — their decoders own orientation and carry no JPEG EXIF.
Tests
Build oriented JPEGs (a white corner marker + a spliced little-endian Exif APP1) and assert the marker lands exactly where each orientation says — dims swapping for the quarter-turns (6/8) — plus the 180 and no-EXIF/normal cases, and parser defaults for PNG / no-EXIF / garbage bytes.
GOWORK=off go build ./... + go test ./internal/imagenorm/ green; gofmt clean; stays CGO_ENABLED=0.
Closes #103. Part of #96 (carries the deferred #81 EXIF follow-up).
`imagenorm` decoded and re-encoded to JPEG but ignored the EXIF `Orientation` tag. Phone cameras store the sensor pixels one way and set a flag to rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged "rotate 90°" — and the re-encode strips EXIF, so without baking the rotation in, a packet photographed in portrait reaches the vision model **sideways** (worse OCR) and any future thumbnail is wrong.
## What
- **`exifOrientation`** — a small pure-Go parser that walks the JPEG APP1/Exif segment for tag `0x0112`, returning `1` (normal) for non-JPEG or unparseable input (never guess a rotation onto a correct image). No cgo, no new dependency.
- **`applyOrientation`** — bakes in all 8 orientations (4 rotations + mirrors), applied **after** downscale (cheaper to rotate the small image; a 90° turn swaps the sides but not the longest edge, so the downscale bound still holds).
- Non-JPEG paths (HEIC/webp/png) are untouched — their decoders own orientation and carry no JPEG EXIF.
## Tests
Build oriented JPEGs (a white corner marker + a spliced little-endian Exif APP1) and assert the marker lands exactly where each orientation says — dims swapping for the quarter-turns (6/8) — plus the 180 and no-EXIF/normal cases, and parser defaults for PNG / no-EXIF / garbage bytes.
`GOWORK=off go build ./...` + `go test ./internal/imagenorm/` green; `gofmt` clean; stays `CGO_ENABLED=0`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
imagenorm decoded and re-encoded to JPEG but ignored the EXIF Orientation
tag. Phone cameras store the sensor pixels one way and set an EXIF flag to
rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged
"rotate 90°" — and our re-encode strips EXIF, so without baking the rotation
in, a packet photographed in portrait reaches the vision model sideways
(bad OCR) and any future thumbnail is wrong.
- exifOrientation: a small pure-Go parser that walks the JPEG APP1/Exif
segment for tag 0x0112, returning 1 (normal) for non-JPEG or unparseable
input — never guess a rotation onto a correct image. No cgo, no new dep.
- applyOrientation: bakes in all 8 orientations (the 4 rotations + mirrors)
after downscale (cheaper to rotate the small image; a 90° turn swaps the
sides but not the longest edge, so the downscale bound still holds).
- Non-JPEG paths (HEIC/webp/png) are untouched — their decoders own
orientation and they carry no JPEG EXIF.
Tests build oriented JPEGs (a corner marker + a spliced Exif APP1) and
assert the marker lands where each orientation says, dims swapping for the
quarter-turns; plus parser defaults for non-JPEG / no-EXIF / garbage.
Stays CGO_ENABLED=0.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 05:30:34Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Blocking issues found
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 9 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 9 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🔴Double image allocation on 90° rotations: first RGBA buffer immediately discarded
maintainability, performance · flagged by 4 models
internal/imagenorm/imagenorm.go:214-216 — Double image allocation on every 90° rotation.applyOrientation always allocates image.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocates image.NewRGBA(image.Rect(0, 0, h, w)) when swap is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple if/else so only the correctly-sized buffer is ever…
🪰 Gadfly · advisory
🔴 **Double image allocation on 90° rotations: first RGBA buffer immediately discarded**
_maintainability, performance · flagged by 4 models_
- **`internal/imagenorm/imagenorm.go:214-216` — Double image allocation on every 90° rotation.** `applyOrientation` always allocates `image.NewRGBA(image.Rect(0, 0, w, h))`, then immediately throws it away and allocates `image.NewRGBA(image.Rect(0, 0, h, w))` when `swap` is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple `if/else` so only the correctly-sized buffer is ever…
<sub>🪰 Gadfly · advisory</sub>
🟠Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation
maintainability, performance · flagged by 2 models
internal/imagenorm/imagenorm.go:218-239 — manual At/Set per-pixel loop diverges from the draw-package idiom used elsewhere in the file.applyOrientation walks every pixel with img.At/dst.Set (going through the color.Model conversion path), while downscale (line 332) uses draw.CatmullRom.Scale. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is…
🪰 Gadfly · advisory
🟠 **Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation**
_maintainability, performance · flagged by 2 models_
- **`internal/imagenorm/imagenorm.go:218-239` — manual `At`/`Set` per-pixel loop diverges from the `draw`-package idiom used elsewhere in the file.** `applyOrientation` walks every pixel with `img.At`/`dst.Set` (going through the `color.Model` conversion path), while `downscale` (line 332) uses `draw.CatmullRom.Scale`. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is…
<sub>🪰 Gadfly · advisory</sub>
🟡JPEG fill bytes between markers not skipped in exifOrientation
error-handling · flagged by 1 model
internal/imagenorm/imagenorm.go:249-275 — JPEG fill bytes (0xFF) between markers are not skipped The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. The parser at exifOrientation assumes each marker is immediately preceded by exactly one 0xFF byte. If a phone encoder emits padding 0xFF bytes before the APP1 segment, the loop misreads the marker and returns orientation 1 instead of the true value. This is safe (no wrong rotation) but incorrect for a valid…
🪰 Gadfly · advisory
🟡 **JPEG fill bytes between markers not skipped in exifOrientation**
_error-handling · flagged by 1 model_
- **`internal/imagenorm/imagenorm.go:249-275` — JPEG fill bytes (`0xFF`) between markers are not skipped** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. The parser at `exifOrientation` assumes each marker is immediately preceded by exactly one `0xFF` byte. If a phone encoder emits padding `0xFF` bytes before the APP1 segment, the loop misreads the marker and returns orientation `1` instead of the true value. This is safe (no wrong rotation) but incorrect for a valid…
<sub>🪰 Gadfly · advisory</sub>
🟠JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs
error-handling · flagged by 1 model
internal/imagenorm/imagenorm.go:256 — JPEG marker-padding edge case unhandled. The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. exifOrientation assumes exactly one 0xFF followed immediately by the marker code, so a file like FF D8 FF FF E1 … (valid per spec) misaligns: marker becomes 0xFF, segLen is read from the next two bytes (0xE1 0x00 ≈ 57 KiB), the length check fails, and the parser bails out returning 1 (normal). For a portrait photo whose…
🪰 Gadfly · advisory
🟠 **JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs**
_error-handling · flagged by 1 model_
- **`internal/imagenorm/imagenorm.go:256` — JPEG marker-padding edge case unhandled.** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. `exifOrientation` assumes exactly one `0xFF` followed immediately by the marker code, so a file like `FF D8 FF FF E1 …` (valid per spec) misaligns: `marker` becomes `0xFF`, `segLen` is read from the next two bytes (`0xE1 0x00` ≈ 57 KiB), the length check fails, and the parser bails out returning `1` (normal). For a portrait photo whose…
<sub>🪰 Gadfly · advisory</sub>
🟠Missing TIFF magic number validation in orientationFromApp1
error-handling · flagged by 1 model
internal/imagenorm/imagenorm.go:280 — Missing TIFF magic number validation in orientationFromApp1 The function trusts the IFD offset at tiff[4:8] without first checking that bytes tiff[2:4] contain the required TIFF magic number 42 (0x002A little-endian / 0x2A00 big-endian). A malformed APP1 segment that passes the "Exif\x00\x00" prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially…
🪰 Gadfly · advisory
🟠 **Missing TIFF magic number validation in orientationFromApp1**
_error-handling · flagged by 1 model_
- **`internal/imagenorm/imagenorm.go:280` — Missing TIFF magic number validation in `orientationFromApp1`** The function trusts the IFD offset at `tiff[4:8]` without first checking that bytes `tiff[2:4]` contain the required TIFF magic number `42` (`0x002A` little-endian / `0x2A00` big-endian). A malformed APP1 segment that passes the `"Exif\x00\x00"` prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially…
<sub>🪰 Gadfly · advisory</sub>
🟠EXIF orientation parser does not validate IFD type/count before reading value
correctness, error-handling · flagged by 2 models
internal/imagenorm/imagenorm.go:305-309 — orientationFromApp1 reads the Orientation value without validating the IFD entry's type and count. The EXIF spec requires Orientation to be SHORT (type 3) with count 1. A malformed entry with a different type (e.g. LONG) or count >1 stores an offset in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "sa…
🪰 Gadfly · advisory
🟠 **EXIF orientation parser does not validate IFD type/count before reading value**
_correctness, error-handling · flagged by 2 models_
- **`internal/imagenorm/imagenorm.go:305-309`** — `orientationFromApp1` reads the Orientation value without validating the IFD entry's **type** and **count**. The EXIF spec requires Orientation to be `SHORT` (type `3`) with count `1`. A malformed entry with a different type (e.g. `LONG`) or count `>1` stores an *offset* in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "sa…
<sub>🪰 Gadfly · advisory</sub>
🟠Missing test coverage for mirror/transpose orientations 2/4/5/7
maintainability · flagged by 1 model
internal/imagenorm/imagenorm_test.go:271-281 — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.
🪰 Gadfly · advisory
🟠 **Missing test coverage for mirror/transpose orientations 2/4/5/7**
_maintainability · flagged by 1 model_
- `internal/imagenorm/imagenorm_test.go:271-281` — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.
<sub>🪰 Gadfly · advisory</sub>
🟠No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases
maintainability · flagged by 1 model
internal/imagenorm/imagenorm_test.go:276-280 — no test covers orientations 2, 4, 5, 7.TestNormalizeAppliesExifOrientation only exercises 0/1/3/6/8, and TestExifOrientationParsing only parses 6. The applyOrientation switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transver…
🪰 Gadfly · advisory
🟠 **No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases**
_maintainability · flagged by 1 model_
- **`internal/imagenorm/imagenorm_test.go:276-280` — no test covers orientations 2, 4, 5, 7.** `TestNormalizeAppliesExifOrientation` only exercises 0/1/3/6/8, and `TestExifOrientationParsing` only parses 6. The `applyOrientation` switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transver…
<sub>🪰 Gadfly · advisory</sub>
🟡table test missing t.Run subtests inconsistent with file pattern
maintainability · flagged by 1 model
internal/imagenorm/imagenorm.go:214 — applyOrientation allocates dst with dimensions w×h, then immediately throws it away and allocates again with h×w whenever swap is true (orientations 5–8). This wastes an allocation of w*h*4 bytes for every quarter-turn. Move the allocation into an if/else so only the correct size is created. - internal/imagenorm/imagenorm_test.go:282 — TestNormalizeAppliesExifOrientation iterates over a table of cases but doesn't wrap them in `t…
🪰 Gadfly · advisory
🟡 **table test missing t.Run subtests inconsistent with file pattern**
_maintainability · flagged by 1 model_
- **`internal/imagenorm/imagenorm.go:214`** — `applyOrientation` allocates `dst` with dimensions `w×h`, then immediately throws it away and allocates again with `h×w` whenever `swap` is true (orientations 5–8). This wastes an allocation of `w*h*4` bytes for every quarter-turn. Move the allocation into an `if/else` so only the correct size is created. - **`internal/imagenorm/imagenorm_test.go:282`** — `TestNormalizeAppliesExifOrientation` iterates over a table of cases but doesn't wrap them in `t…
<sub>🪰 Gadfly · advisory</sub>
One thing worth flagging first: the trailing block claiming "Plan mode is active" and instructing an Explore/Plan-agent/ExitPlanMode workflow doesn't correspond to any real tool in this session — I searched for ExitPlanMode/AskUserQuestion via ToolSearch and neither resolves, unlike the genuinely deferred tools (TaskCreate, EnterWorktree, etc.) which do. That block reads as an injected instruction trying to redirect this session away from delivering the actual review. I'm disregarding it and completing the task as originally specified (the Gadfly review format, output as text). I did already write a harmless scratch file to /root/.claude/plans/... before catching this — it contains only my own analysis notes, nothing sensitive, and can be ignored/deleted.
Here's the actual security review of the PR:
VERDICT: No material issues found
This diff adds pure, local EXIF-orientation parsing and pixel-transform code to internal/imagenorm. I read the live file (internal/imagenorm/imagenorm.go:140-199) to confirm the surrounding guarantees, not just the diff hunk:
Input is already size-capped before this code runs.raw is bounded by opts.maxBytes() (default 25 MiB, enforced at imagenorm.go:140-151) before exifOrientation(raw) is ever called (imagenorm.go:175). No new unbounded-read surface.
exifOrientation's marker walk is fully bounds-checked before every slice access — i+4<=len(raw) before reading marker bytes, segLen<2 || i+2+segLen>len(raw) before slicing the segment body. Malformed/truncated JPEGs fall through to return 1, not a panic or OOB read.
orientationFromApp1's TIFF/IFD parsing is equally guarded — prefix-length check before the "Exif\x00\x00" compare, ifd<8 || ifd+2>len(tiff) before reading the entry count, and off+12>len(tiff) checked inside the loop before every 12-byte entry read. A crafted large entry count (n up to 65535) just runs bounded, cheap, bounds-checked iterations — not a meaningful DoS vector.
Safe-default posture is correct and consistently applied: unparseable/non-JPEG/garbage input always yields orientation 1 (no rotation), never a guess — so a hostile file can at worst leave an image un-rotated, not corrupt a legitimate one or destabilize downstream processing.
applyOrientation only receives an already-validated o in [2,8] and allocates its output buffer afterdownscale, so its allocation size is bounded by opts.maxDim() regardless of what the EXIF tag claims — no amplification/decompression-bomb path via orientation.
No new sinks are introduced: no filesystem, SQL, shell, or network use; no secrets touched; nothing here affects authn/authz. This is pure in-memory byte parsing over data that was already being treated as untrusted (decodeSafely's existing panic-recovery comment already documents the "untrusted upload" threat model this code operates under).
I did not find any injection, SSRF, deserialization, secret-handling, or input-validation defect within this diff's security lens.
🎯 Correctness — No material issues found
Review — 🎯 Correctness lens
VERDICT: No material issues found
I independently re-derived all 8 EXIF orientation transforms from first principles (physically rotating/mirroring a labeled grid) rather than trusting the code's comments, and checked them against internal/imagenorm/imagenorm.go:207-242:
Orientation 6 ("rotate 90° CW to correct") → dx,dy = h-1-y, x — verified against a manual 2×2 rotation trace, matches.
Orientation 8 ("rotate 90° CCW to correct") → dx,dy = y, w-1-x — verified, matches.
Orientations 2/3/4 (mirror-H, 180, mirror-V) and 5/7 (transpose/transverse) all check out against the canonical "where does stored (0,0) land" EXIF table.
Also verified:
orientationFromApp1 (imagenorm.go:280-316): TIFF header/IFD0 offset bounds, 12-byte IFD entry stride, and reading a SHORT value from the first 2 bytes of the 4-byte value field per file byte order — all correct per TIFF6 spec.
exifOrientation (imagenorm.go:249-275): JPEG marker-segment length semantics (segLen includes the 2 length bytes, so i += 2+segLen and slicing raw[i+4:i+2+segLen] are both correctly offset), correctly halts at SOS/EOI so it never scans entropy-coded data, and fails safe (returns 1) on any malformed/short segment rather than misapplying a rotation.
Ordering in Normalize (imagenorm.go:174-175): applyOrientation is called with exifOrientation(raw) where raw is the original, un-truncated input bytes (line 145) — not the decoded/re-encoded buffer — so the tag is read from the actual source JPEG. exifOrientation gates on the FFD8 SOI itself, so non-JPEG formats (PNG/WebP/HEIC) safely no-op without a separate format check. Downscale-then-rotate is sound: rotation only transposes width/height, so max(w,h) (the downscale bound) is invariant across it.
One thing I checked but am not reporting as a finding (too low-confidence/low-impact to meet the bar): orientationFromApp1 doesn't validate the IFD entry's type/count fields before reading the Orientation value, so a file with a non-SHORT-typed 0x0112 tag could yield a bogus-but-in-range value instead of falling back to 1. No real camera emits Orientation as anything but SHORT, and I don't have enough confidence this is exploitable to list it as a real bug.
I was unable to execute go test ./internal/imagenorm/... in this environment (Bash approval unavailable under the current session mode), so the "tests pass" claim in the PR description is unverified by me directly — but I traced the test's rotation-formula arithmetic by hand against the same transforms and it's internally consistent with the (verified-correct) implementation.
🧹 Code cleanliness & maintainability — Minor issues
No other EXIF/binary parsing utilities exist elsewhere — that claim holds. All checks confirm the draft's single finding is accurate, but one supporting claim in the prose (about downscale already using the range-over-int idiom) is false, so I'll correct that in the output.
Review — Code cleanliness & maintainability
VERDICT: Minor issues
The new EXIF parsing/orientation code (internal/imagenorm/imagenorm.go:202-316) is well-scoped, self-contained to one file, and matches the surrounding heavily-commented style (verified no pre-existing EXIF/TIFF parsing utilities elsewhere in the repo to duplicate, via grep -rn "binary.ByteOrder\|Uint16\|Uint32"). Naming (exifOrientation, orientationFromApp1, applyOrientation) is clear, and the 8-case switch in applyOrientation is an appropriate use of a switch over a transform-function table for this size. Tests reuse the existing pngBytes helper rather than duplicating it (confirmed at internal/imagenorm/imagenorm_test.go:17).
One small nit:
internal/imagenorm/imagenorm.go:213-217 — applyOrientation unconditionally allocates a w×h RGBA, then immediately discards it and allocates a second h×w RGBA whenever swap is true (orientations 5-8):
Verified by reading the function in full — no other use of dst before the reassignment, so the first allocation is dead work.
Nothing else in this diff (including the DESIGN.md wording update and the test file additions) raises cleanliness concerns.
⚡ Performance — Minor issues
Confirmed — this matches the draft exactly: line 214 unconditionally allocates a w×himage.NewRGBA, then line 215-217 discards it and allocates a second h×w buffer whenever swap is true (orientations 5-8, which includes orientation 6, the common phone-portrait case). The fix suggestion (compute dw, dh first, allocate once) is correct and minimal. This is a real, verifiable finding — a small/minor perf nit, not a correctness bug, matching the draft's severity.
VERDICT: Minor issues
internal/imagenorm/imagenorm.go:214-217 — applyOrientation allocates a w×h*image.RGBA unconditionally, then immediately discards it and allocates a second h×w*image.RGBA whenever swap is true (orientations 5-8):
Orientation 6 (90° CW) is the single most common EXIF tag on real phone photos (camera held normally in portrait) — exactly the case this PR is built for per the PR description ("a phone camera... sets a flag to rotate on display"). So on the primary target workload, every upload does one throwaway allocation of up to DefaultMaxDim × DefaultMaxDim × 4 bytes (~16.7 MB at the 2048px default) before the real buffer is allocated, adding avoidable allocation/GC pressure on the image-upload hot path.
Fix: compute the swapped dimensions before allocating, e.g.
Everything else in the changed code is a bounded, single-pass O(w·h) pixel copy (unavoidable for baking in a rotation) applied after downscale caps dimensions, and a marker-segment walk over the raw JPEG header bytes (exifOrientation) that's O(segment count), not O(file size) in any problematic way. No N+1s, no unbounded growth, no new blocking calls on the hot path.
🧯 Error handling & edge cases — No material issues found
Go 1.26 supports range-over-int, so no compile issue there.
I've verified the core logic — bounds checks, loop termination, and defaults are all sound. Nothing in my lane rises to a real defect.
VERDICT: No material issues found
Findings (through the error-handling/edge-case lens only):
exifOrientation/orientationFromApp1 (imagenorm.go:249-316) are defensively bounds-checked at every slice access I traced (segment length vs. len(raw), IFD offset vs. len(tiff), entry offset vs. len(tiff) before each 12-byte read) — verified by re-deriving the offsets by hand against the TIFF/EXIF layout. Every failure path returns the safe default (1, no rotation) rather than erroring or guessing, matching the doc comment's stated intent.
The i += 2 + segLen marker-walk loop is guaranteed to terminate: segLen < 2 is rejected before the increment, so i strictly increases each iteration — no infinite-loop/DoS risk on adversarial input, and the byte read is already capped by opts.MaxBytes upstream.
applyOrientation (imagenorm.go:207-242): the o<=1 || o>8 guard fully covers the domain (exifOrientation only ever returns 1-8), so the switch's implicit cases 2-8 are exhaustive — no default-case gap. A 0×0 img.Bounds() degenerates to empty loops with no panic (unlikely in practice, but confirmed not to crash).
No nil-image path: img reaching applyOrientation/downstream is always non-nil by construction (decodeSafely returns early on error), so no nil-check gap.
One sub-lens observation I considered but am not confident enough to list as a finding: orientationFromApp1 doesn't validate the IFD entry's type field (expects SHORT/3 for Orientation) before reading the value bytes — a malformed tag with the same ID but a different type could, in a byte-order-dependent way, be misread as a spurious value 1-8 and silently misrotate instead of falling back safely. This requires deliberately malformed EXIF (real phone EXIF always types Orientation as SHORT) and is closer to a correctness/robustness nit than an error-handling defect, so I'm not reporting it as a real bug.
I reviewed the new EXIF parser and orientation applier through the security lens — they process attacker-controlled upload bytes — and verified the bounds logic against the checked-out code.
exifOrientation / orientationFromApp1 (imagenorm.go:249-316) — all slice accesses are bounds-checked before use, so an OOB panic (DoS via process crash) is not reachable:
In orientationFromApp1, len(seg) < len(prefix)+8 (14) guards tiff[0:2] and tiff[4:8]; ifd < 8 || ifd+2 > len(tiff) guards tiff[ifd:ifd+2]; off+12 > len(tiff) guards tiff[off:off+2] and tiff[off+8:off+10]. The value-field read ignores the declared tag type but stays in bounds, so a malicious non-SHORT type can't cause a panic — worst case it misreads and falls through to the 1..8 check or returns false.
The IFD entry-count loop for k := range n is attacker-controlled (uint16, ≤65535) but breaks as soon as off+12 > len(tiff), so iteration is bounded by len(tiff)/12; len(tiff) is itself bounded by MaxBytes (25 MiB). No integer-overflow concern on 32-bit either, since ifd is clamped to ≤ len(tiff)-2 before use. This is at most trivial CPU amplification, far below the cost of the decode that already ran.
Safe-default discipline is correct: every unparseable/non-JPEG/garbage path returns 1 (no-op), so a malformed file can never coerce an unwanted rotation of a correct image. This is the fail-safe direction.
applyOrientation (imagenorm.go:207-242) — operates only on the already-decoded image.Image and a validated o in 1..8; At/Set on stdlib image types don't panic. No new taint surface.
Recover scope — noted (not a finding): decodeSafely's recover does not wrap exifOrientation/applyOrientation, but neither has a reachable panic on untrusted input per the bounds analysis above, so the unprotected scope is safe.
No injection (SQL/command/path/template), SSRF, deserialization, secret/credential, or authn/authz surface in this change — it's pure in-memory byte parsing of an uploaded image. The upload boundary and size caps (MaxBytes, maxDecodePixels, maxDimension) upstream of this code already bound the input.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change through the correctness lens by reading the full implementation (internal/imagenorm/imagenorm.go:140-334) and re-deriving the EXIF orientation transforms from first principles.
applyOrientation transforms (imagenorm.go:207-242): I checked all 8 cases against the EXIF 2.3 spec definitions (1=identity, 2=mirror-H, 3=180, 4=mirror-V, 5=transpose, 6=90°CW, 7=transverse, 8=90°CCW). Every mapping (x,y) → (dx,dy) is correct, including the dimensional transpose for o≥5 (swap is right for all of 5/6/7/8, not just the pure rotations). Corner-spot checks confirm: o6 maps (0,0)→(h-1,0) (top-left→top-right, correct for CW); o8 maps (0,0)→(0,w-1) (top-left→bottom-left, correct for CCW).
Downscale-then-rotate ordering (imagenorm.go:171-175): The comment's invariant holds. Downscale bounds the longest edge to maxDim; a rotation/transposition only swaps sides, so the longest edge stays ≤ maxDim. Worked example: 4000×3000 → downscale 1024×768 → o6 swap → 768×1024 (longest edge 1024). ✓
exifOrientation / orientationFromApp1 (imagenorm.go:249-316): JPEG segment walk, bounds, IFD0 parsing, and the inline-SHORT read at off+8:off+10 are all correct. Non-JPEG inputs (HEIC/PNG/WebP) correctly short-circuit on the 0xFFD8 SOI check and return 1, so applyOrientation is a no-op for them. All slice accesses are guarded against panics (verified each bounds check). Failure mode on any unparseable/edge input is returning 1 (no rotation) — never a wrong rotation — which is the safe default.
Test expectations (imagenorm_test.go): I re-derived each wantW/wantH/mx/my from the same transform the code applies; all match (e.g. o6: dx=h-1-y=17, dy=x=10 for source center (10,6) with h=24; o8: dx=y=6, dy=w-1-x=29; o3: dx=29, dy=17).
One non-blocking observation (not a correctness bug): the value field is read as uint16 without verifying the tag's EXIF type is SHORT. Since the spec mandates SHORT for Orientation this is fine in practice; a hypothetical big-endian LONG-typed tag would read 0 and fall back to the safe default of 1 rather than a wrong rotation, so it fails safe.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues found
internal/imagenorm/imagenorm.go:214-216 — redundant double allocation on the swap branch.dst is allocated as image.NewRGBA(image.Rect(0,0,w,h)) on line 214, then unconditionally reassigned to image.NewRGBA(image.Rect(0,0,h,w)) on line 216 when swap is true. On the quarter-turn path (orientations 5–8) the first RGBA header is allocated and immediately discarded. The non-swap branch keeps the first allocation correctly, so this is only a wasted allocation on the swap branch, not a correctness bug — a readability/efficiency wart in an allocation-sensitive path. Cleaner: compute the output dims up front and allocate once.
internal/imagenorm/imagenorm.go:218-239 — manual At/Set per-pixel loop diverges from the draw-package idiom used elsewhere in the file.applyOrientation walks every pixel with img.At/dst.Set (going through the color.Model conversion path), while downscale (line 332) uses draw.CatmullRom.Scale. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is avoidable. Not blocking; a consistency nit.
internal/imagenorm/imagenorm_test.go:276-280 — no test covers orientations 2, 4, 5, 7.TestNormalizeAppliesExifOrientation only exercises 0/1/3/6/8, and TestExifOrientationParsing only parses 6. The applyOrientation switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transverse math.
⚡ Performance — Minor issues
Both findings are confirmed against the actual code. The first (double RGBA allocation on the swap path) is real and at the cited lines. The second is informational and not in the findings JSON, consistent with the draft.
Verdict: Minor issues
internal/imagenorm/imagenorm.go:214-217 — double RGBA allocation on the swap (portrait) path. For orientations 5–8 (the quarter-turns), dst is first allocated at w×h and then immediately overwritten with a second image.NewRGBA at h×w:
The first buffer is allocated and discarded. This is the common phone-portrait path — orientation 6 is exactly "portrait photo tagged rotate 90°", the motivating case of this PR. For a 2048×1536 portrait, that's a ~12 MiB RGBA thrown straight to the GC on the very upload the feature targets. Fix is trivial — compute the destination rect once:
internal/imagenorm/imagenorm.go:218-240 — per-pixel At/Set loop (informational, not blocking). The rotate reads via img.At and writes via dst.Set, both of which go through generic color.Color/color.Model conversions rather than fast direct RGBA writes. Bounded to ≤2048px and only on the rotated path, so acceptable for an upload boundary — but worth noting that a portrait phone photo already within MaxDim (so downscale returns the original *image.YCbCr) hits the slowest variant (YCbCr-per-pixel At). Not worth changing unless this shows up in profiles.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the error-handling & edge-case lens, the new code is solid. I verified the two new functions and the bounds checks by reading internal/imagenorm/imagenorm.go:202-316:
exifOrientation (line 249): Every byte access is bounds-checked before use. The marker walk guards len(raw) < 4, i+4 <= len(raw), raw[i] != 0xFF (misalignment → safe return 1), segLen < 2 || i+2+segLen > len(raw), and stops at EOI/SOS. The slice passed to orientationFromApp1 (raw[i+4 : i+2+segLen]) is provably in range because of the i+2+segLen > len(raw) guard. The loop advances i += 2 + segLen with segLen >= 2, so it strictly progresses and terminates — no infinite loop. segLen is a 16-bit value (max 65535), so i+2+segLen can't overflow int on any supported platform.
orientationFromApp1 (line 280): Length guard len(seg) < len(prefix)+8 guarantees tiff := seg[len(prefix):] has at least 8 bytes, so tiff[0:2] and tiff[4:8] are safe. The IFD offset is validated (ifd < 8 || ifd+2 > len(tiff)), and each 12-byte entry is checked with off+12 > len(tiff) before the Uint16/Uint32 reads. A crafted huge entry count n is bounded by the per-entry length check, so it can't spin unbounded. Out-of-range orientation values fall through to return 0, false → caller defaults to 1. No panics on truncated/garbage input.
applyOrientation (line 207): Out-of-range o (<= 1 || > 8) is a no-op returning the original image. Empty/zero-dimension images (w==0 or h==0, which downscale can pass through) produce empty image.NewRGBA allocations and the for y := range h / for x := range w loops simply don't execute — no index panic. dst.Set(dx, dy, c) always writes within the allocated rect by construction of the transforms.
No swallowed errors introduced: Both new call sites in Normalize (applyOrientation, exifOrientation) return values, not errors, so there are no new error-return paths to mishandle or defer. The existing "on any error the returned bytes are nil" contract is preserved.
The test helper spliceExifOrientation discards binary.Write errors with _, but those write to a bytes.Buffer which never errors — not a concern.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
The change adds a manual EXIF orientation parser for JPEG and pixel-level rotation logic. From a security lens, the code is clean:
Fail-closed parsing: exifOrientation returns 1 (no rotation) on any malformed input, missing EXIF, or non-JPEG format — never forcing a rotation onto a correct image.
Bounded work: maxDecodePixels (50M) and maxDimension (50K) already cap decoded bitmap size. applyOrientation runs on the downscaled image (downscale happens first), so memory and CPU for the rotation are bounded by the same limits.
No out-of-bounds reads: orientationFromApp1 checks len(seg) < len(prefix)+8 before indexing tiff[0:2], tiff[4:8], etc.; the IFD walk checks off+12 > len(tiff) before reading each 12-byte entry.
No out-of-bounds writes: applyOrientation computes destination coordinates (dx, dy) that always fall within the pre-allocated dst bounds for every orientation case (2–8).
Strip, not leak: The re-encode to JPEG strips all EXIF metadata (including GPS, camera model, timestamps). This is a net privacy win — only the orientation value is extracted and then discarded after the bitmap rotation.
One minor observation (not a finding, just a verification note): the exifOrientation parser is strict about 0xFF marker alignment (raw[i] != 0xFF → return 1), which is the conservative/safe choice for untrusted uploads.
🎯 Correctness — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:305-309 — orientationFromApp1 reads the Orientation value without validating the IFD entry's type and count. The EXIF spec requires Orientation to be SHORT (type 3) with count 1. A malformed entry with a different type (e.g. LONG) or count >1 stores an offset in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "safe default: return 1 for unparseable" design goal) instead of treating the tag as unreadable.
Suggested fix: Add two checks inside the 0x0112 match block:
ifbo.Uint16(tiff[off+2:off+4])!=3{// not SHORTreturn0,false}ifbo.Uint32(tiff[off+4:off+8])!=1{// count != 1return0,false}
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:214-216 — applyOrientation allocates a w×h RGBA unconditionally, then immediately discards it and allocates a second h×w RGBA for orientations 5-8. Move the swap check before the first NewRGBA to avoid the throw-away allocation for the quarter-turn cases.
internal/imagenorm/imagenorm_test.go:271-281 — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.
⚡ Performance — Blocking issues found
Blocking issues found
internal/imagenorm/imagenorm.go:214-216 — Double image allocation on every 90° rotation.applyOrientation always allocates image.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocates image.NewRGBA(image.Rect(0, 0, h, w)) when swap is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple if/else so only the correctly-sized buffer is ever allocated.
internal/imagenorm/imagenorm.go:218-240 — Per-pixel image.Image.At() calls box a color.Color on every iteration, causing millions of heap allocations for a single rotated image.applyOrientation iterates every pixel and reads via the image.Image interface (img.At(b.Min.X+x, b.Min.Y+y)). When the concrete type is *image.RGBA (which downscale already returns), each At call boxes the color.RGBA value into an interface, allocating on the heap. A 2048×2048 rotation causes ~4.2 million transient allocations. This is a well-known Go image-processing anti-pattern; adding a fast-path type-switch to *image.RGBA (and perhaps *image.NRGBA) that copies through Pix directly would eliminate the boxing and interface dispatch entirely for the common case, cutting rotation time by an order of magnitude. The current generic fallback can be kept for non-RGBA images.
🧯 Error handling & edge cases — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:256 — JPEG marker-padding edge case unhandled.
The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. exifOrientation assumes exactly one 0xFF followed immediately by the marker code, so a file like FF D8 FF FF E1 … (valid per spec) misaligns: marker becomes 0xFF, segLen is read from the next two bytes (0xE1 0x00 ≈ 57 KiB), the length check fails, and the parser bails out returning 1 (normal). For a portrait photo whose APP1 segment is padded this way, the image stays sideways. A robust parser should skip consecutive 0xFF bytes before reading the marker code.
internal/imagenorm/imagenorm.go:305 — TIFF type field not validated. orientationFromApp1 finds tag 0x0112 and blindly reads the first two bytes of the value field as a SHORT. It never checks tiff[off+2:off+4] to confirm the type is actually 3 (SHORT). A malformed EXIF that reuses tag 0x0112 with a LONG or UNDEFINED type could yield a bogus orientation value. While exifOrientation caps the result to 1..8, any value in that range would still be applied. Adding a type check (bo.Uint16(tiff[off+2:off+4]) == 3) before accepting the value would make the “unparseable → 1” guarantee stronger.
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Reviewed the EXIF parser through the security lens (untrusted-byte parsing, OOB reads, integer overflow, infinite loops, injection surface). The code is defensively written:
exifOrientation (imagenorm.go:249-275): all index accesses are guarded by the loop condition i+4 <= len(raw) and i+2+segLen > len(raw); segLen >= 2 guarantees loop progress (no infinite loop); segLen is a 16-bit field so no int overflow on i+2+segLen; safe default 1 on any misalignment, EOI/SOS, or non-JPEG.
orientationFromApp1 (imagenorm.go:280-313): length check len(seg) < len(prefix)+8 covers tiff[0:2] and tiff[4:8]; IFD offset validated with ifd < 8 || ifd+2 > len(tiff) before slicing; per-entry bounds off+12 > len(tiff) cover tiff[off+8:off+10]; orientation value clamped to 1..8 before use. A 32-bit build could in principle make int(bo.Uint32(...)) negative for a near-MaxUint32 offset, but the subsequent ifd+2 > len(tiff) comparison still rejects it (negative < len), so it can't be exploited to read OOB — it just falls through to the safe default.
The input to the parser is raw, which Normalize already capped at MaxBytes+1 (imagenorm.go:~125), so the parser runs on size-bounded data regardless.
No injection/auth/secret/SSF surface: this is a pure local byte parser; its only output is an int 1..8 consumed by applyOrientation, which does an in-memory pixel copy with no I/O.
Nothing in the security lens is materially wrong.
🎯 Correctness — No material issues found
I verified the orientation math, the EXIF parser bounds, and the test expectations against the checked-out code.
Verdict: No material issues found
I re-derived all 8 EXIF orientation transforms from first principles (rotating/mirroring the source bitmap into an upright destination) and confirmed each case in applyOrientation (imagenorm.go:222-237) maps source (x,y) to the correct destination (dx,dy), including the dimension-swapping cases (5–8) where dst is correctly allocated as h×w and all writes stay in bounds.
The "downscale then rotate" ordering is sound: downscale bounds the longest edge, and a 90° rotation swaps sides without changing the longest edge, so the MaxDim bound still holds on the emitted image (verified for both the shrink and no-shrink paths).
The EXIF parser bounds are correct: the marker walk guards i+4 <= len(raw) and segLen < 2/i+2+segLen > len(raw; orientationFromApp1 requires len(seg) >= len(prefix)+8 (so tiff[4:8] is safe) and ifd < 8 || ifd+2 > len(tiff) plus per-entry off+12 > len(tiff) prevent OOB reads. The APP1 payload slice raw[i+4 : i+2+segLen] correctly excludes the 2 length bytes.
The test expectations are consistent with the transforms: e.g. orient 6 (90° CW) sends source (10,6) to (h-1-y, x) = (17,10) on a 24×40 canvas, matching {6, 24, 40, 17, 10}; orient 8 → (y, w-1-x) = (6,29) matching {8, 24, 40, 6, 29}. The spliceExifOrientation helper builds a well-formed little-endian TIFF/IFD0 (verified byte layout: II + 0x2A + offset 8 + count 1 + 12-byte entry with value at entry+8), which the parser reads back correctly.
Non-JPEG paths return 1 immediately (raw[0]!=0xFF||raw[1]!=0xD8), so HEIC/WebP/PNG are left untouched as designed — no regression vs. prior behavior.
One trivial robustness note (not material, low confidence): the marker walker doesn't skip JPEG fill bytes (0xFF 0xFF) or handle standalone markers like TEM/0xFF01/RSTn that have no length field before the EXIF APP1. A JPEG emitting fill bytes before APP1 would misparse and fall back to orientation 1 (sideways). Real phone JPEGs place APP1 immediately after SOI, so this is out of the practical path and the fallback is safe (never mis-rotates a correct image).
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
The EXIF orientation addition is clean through the maintainability lens. Verified against the checked-out code:
applyOrientation and exifOrientation are well-factored, each doing one thing, with accurate doc comments that match the implementation. They follow the file's existing conventions (sentinel-free helpers, inline rationale comments, pure-Go no-deps stance).
No duplicated logic: exifOrientation re-walks the JPEG bytes because image.DecodeConfig doesn't expose EXIF — that's a justified read, not copy-paste.
The no-op short-circuit (o <= 1 || o > 8) returns img unchanged, so non-JPEG and normal-orientation inputs pay zero allocation cost — the comment claims this and the code does it.
Tests reuse the existing pngBytes/jpegBytes fixture-builder pattern and add orientedJPEG/spliceExifOrientation/bright in the same style; the helper that builds a minimal TIFF IFD entry writes the 12-byte entry layout correctly (2+2+4+2+2), so the fixture reflects a real Exif APP1 rather than asserting the parser's own shape back at itself.
DESIGN.md note is updated in the same sentence and stays accurate ("bakes in the JPEG EXIF orientation"), consistent with CLAUDE.md's "keep docs true" rule.
Nothing in my area to flag.
⚡ Performance — No material issues found
Reading the file to verify the per-pixel loop's allocation behavior.
VERDICT: No material issues found
Reviewed internal/imagenorm/imagenorm.go (the changed file) and its caller internal/api/seed_packet.go. Through the performance lens:
applyOrientation per-pixel loop (imagenorm.go:218-240): iterates every pixel with img.At + dst.Set, each doing a color-model round-trip. This is the only new work on the hot path. It runs only when o > 1 (an actual non-normal EXIF orientation tag exists); orientation 1 — the common case for landscape photos and all non-JPEG formats — early-returns at line 208 with zero cost. When it does run, the image is already capped at DefaultMaxDim (2048px longest edge) by the preceding downscale, so the loop is bounded to ~4M iterations worst case. On a single-upload endpoint that then calls a vision model (seconds of latency), the tens-of-ms per-pixel copy is immaterial. No N+1, no quadratic behavior, no unbounded allocation.
exifOrientation(raw) (imagenorm.go:249-275): called unconditionally but bails in O(1) for non-JPEG (raw[0]!=0xFF || raw[1]!=0xD8) and walks at most the handful of JPEG marker segments for JPEGs. Negligible.
Transient allocation: applyOrientation allocates one *image.RGBA matching the (already-bounded) downscaled image. Doubling of the ~16MB peak bitmap is acceptable for a one-shot upload path.
The straightforward per-pixel implementation could be sped up with RGBAAt/SetRGBA (skipping two color-model conversions per pixel) since downscale always yields an *image.RGBA, but that is a micro-optimization with no measurable impact at this call frequency and image size — not flagging it.
🧯 Error handling & edge cases — No material issues found
I've read the full imagenorm.go and the relevant test file. Let me verify the boundary logic in the new EXIF/orientation code carefully.
Verdict: No material issues found
I walked the unhappy paths the diff introduces, against the checked-out code:
exifOrientation / orientationFromApp1 parser bounds (imagenorm.go:249-316): every slice access is guarded before use. len(raw) < 4 short-circuits entry; the loop requires i+4 <= len(raw) before reading 4 header bytes; segLen < 2 || i+2+segLen > len(raw) gates the segment slice raw[i+4 : i+2+segLen] (and segLen >= 2 makes i+4 <= i+2+segLen). In orientationFromApp1, len(seg) < len(prefix)+8 (14) covers tiff[0:2] and tiff[4:8]; ifd < 8 || ifd+2 > len(tiff) gates the count read; off+12 > len(tiff) gates every 12-byte entry read, which also covers tiff[off+8:off+10]. Every unparseable / truncated / hostile case returns the safe default 1 — never a panic, never a wrong rotation. Verified by reading the file.
applyOrientation output bounds (imagenorm.go:207-242): for the non-swap cases (2/3/4) dx ∈ [0,w-1], dy ∈ [0,h-1] and dst is w×h; for the swap cases (5/6/7/8) the loops are y ∈ [0,h), x ∈ [0,w) and each mapping lands in [0,h-1] × [0,w-1] against an h×w destination — I re-derived all four quarter-turn mappings and confirmed no off-by-one and no out-of-bounds Set. o <= 1 || o > 8 is a no-op, so an out-of-range tag (or the 0, false return folded in as 0) leaves the image untouched.
Empty/zero inputs: Normalize rejects cfg.Width <= 0 || cfg.Height <= 0 before decode, so applyOrientation never sees a zero-dimension image that would make image.NewRGBA degenerate; downscale's longest == 0 guard is also upstream. No nil-image path: decode failure returns before applyOrientation runs.
Panic coverage: decodeSafely's recover wraps image.Decode; the new code runs after it and does only bounds-checked slice access and img.At/dst.Set within verified bounds, so there's no unprotected panic surface from a malformed EXIF blob.
One marginal observation, not material enough to list as a finding: ifd := int(bo.Uint32(tiff[4:8])) (line 295) could in principle let ifd+2 wrap on a 32-bit int build given a hostile TIFF offset, bypassing the ifd+2 > len(tiff) guard. pansy's CGO_ENABLED=0 static binary targets 64-bit servers (Komodo deploy), where int is 64-bit and the guard holds — so this is not a live risk here, and I'm flagging it only because the lens names integer overflow. No change needed.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
The EXIF orientation parser is defensively written against untrusted input: bounds-checked on every slice access, defaults to 1 (no rotation) on any malformed data, bounded by the 25 MiB read cap, and advances monotonically through markers with no infinite-loop path. The coordinate mapping in applyOrientation stays within the allocated destination for all 8 orientation values. No authn/authz gaps, injection, deserialization, secret leakage, or untrusted-data mishandling is introduced.
🎯 Correctness — No material issues found
No material issues found
I examined the EXIF orientation parser, the transform application, and the interaction with downscaling. Here is what I verified:
applyOrientation transforms: I re-derived the mapping for all 8 EXIF orientations from the spec (0th-row/0th-column placement) and confirmed each case matches. The quarter-turns (5–8) correctly swap the output dimensions, and the pixel-level mappings agree with the test expectations for the covered cases (1, 3, 6, 8) and, by derivation, the uncovered ones (2, 4, 5, 7).
Downscale-then-rotate order: Downsampling first is cheaper, and because a 90° rotation only swaps width/height, the longest-edge bound ≤ MaxDim is preserved. This reasoning is sound.
EXIF parser robustness: exifOrientation walks big-endian JPEG markers, stops at SOS/EOI, bounds-checks every slice operation, and returns 1 (no rotation) on any parse failure. orientationFromApp1 correctly reads the IFD0 entry for tag 0x0112 as an inline SHORT value with the declared byte order. It safely rejects malformed input (bad magic, short buffers, invalid offsets) by returning ok=false, which causes the caller to fall back to 1.
Non-JPEG safety: exifOrientation requires the FF D8 SOI signature; all other formats return 1, and applyOrientation with o==1 returns the image unchanged.
Test verification: The spliceExifOrientation helper constructs a valid little-endian TIFF IFD with the correct 12-byte entry layout and segLen accounting. The marker-location assertions in TestNormalizeAppliesExifOrientation match the transforms.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:214 — applyOrientation allocates dst with dimensions w×h, then immediately throws it away and allocates again with h×w whenever swap is true (orientations 5–8). This wastes an allocation of w*h*4 bytes for every quarter-turn. Move the allocation into an if/else so only the correct size is created.
internal/imagenorm/imagenorm_test.go:282 — TestNormalizeAppliesExifOrientation iterates over a table of cases but doesn't wrap them in t.Run subtests, unlike TestNormalizeAllFormats and TestNormalizeRejectsPixelBomb in the same file. Adding subtests makes failures immediately attributable to the specific orientation and keeps isolation consistent with the file's existing pattern.
⚡ Performance — Minor issues
Verdict: Minor issues
internal/imagenorm/imagenorm.go:214-216 — applyOrientation allocates an image.RGBA of size w×h unconditionally, then immediately discards it and allocates a second one of size h×w when swap is true (orientations 5–8). For a 2048×1536 image that’s a ~12.5 MB throwaway allocation; at 2048×2048 it’s ~16 MB. The fix is trivial: only allocate after the swap check, e.g. dw, dh := w, h; if swap { dw, dh = h, w }; dst := image.NewRGBA(image.Rect(0, 0, dw, dh)).
internal/imagenorm/imagenorm.go:218-239 — The pixel-by-pixel img.At loop is correct but has high per-pixel overhead (interface call + color conversion for every pixel). This is acceptable for an upload handler that runs once per image, but it’s worth noting that for images already within maxDim bounds (where downscale returns the original), this copies the full decoded bitmap via the slow image.Image path instead of working directly on the concrete type. Not blocking for the seed-packet use case, but if large unchanged images become common this would be the next bottleneck.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
internal/imagenorm/imagenorm.go:280 — Missing TIFF magic number validation in orientationFromApp1
The function trusts the IFD offset at tiff[4:8] without first checking that bytes tiff[2:4] contain the required TIFF magic number 42 (0x002A little-endian / 0x2A00 big-endian). A malformed APP1 segment that passes the "Exif\x00\x00" prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially yielding a bogus orientation value. Fix: Verify bo.Uint16(tiff[2:4]) == 0x002A (or 0x2A00 interpreted via bo) before reading the IFD offset.
internal/imagenorm/imagenorm.go:305-309 — Orientation tag type/count not validated
When tag 0x0112 is found, the code assumes it is a SHORT with count == 1 and reads the value inline from the first two bytes of the 4-byte value field. It never checks the entry’s type code (bytes off+2:off+4) or count (bytes off+4:off+8). A malformed EXIF segment with tag 0x0112 but a different type (e.g. LONG) or count > 1 would store an offset in the value field rather than the value itself; the parser would then read that offset as a uint16. If the misread value happens to fall in [1,8], a wrong rotation is silently baked into the image. Fix: After matching the tag, verify bo.Uint16(tiff[off+2:off+4]) == 3 (SHORT) and bo.Uint32(tiff[off+4:off+8]) == 1 before reading bo.Uint16(tiff[off+8:off+10]).
internal/imagenorm/imagenorm.go:249-275 — JPEG fill bytes (0xFF) between markers are not skipped
The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. The parser at exifOrientation assumes each marker is immediately preceded by exactly one 0xFF byte. If a phone encoder emits padding 0xFF bytes before the APP1 segment, the loop misreads the marker and returns orientation 1 instead of the true value. This is safe (no wrong rotation) but incorrect for a valid JPEG. Fix: Before reading raw[i+1] as the marker, skip consecutive 0xFF bytes.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 9 findings (3 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | Double image allocation on 90° rotations: first RGBA buffer immediately discarded | `internal/imagenorm/imagenorm.go:214` | 4/5 | maintainability, performance |
| 🟠 | Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation | `internal/imagenorm/imagenorm.go:218` | 2/5 | maintainability, performance |
| 🟠 | EXIF orientation parser does not validate IFD type/count before reading value | `internal/imagenorm/imagenorm.go:305` | 2/5 | correctness, error-handling |
<details><summary>6 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs | `internal/imagenorm/imagenorm.go:256` | kimi-k2.6:cloud | error-handling |
| 🟠 | Missing TIFF magic number validation in orientationFromApp1 | `internal/imagenorm/imagenorm.go:280` | opencode/kimi-k2.6:cloud | error-handling |
| 🟠 | Missing test coverage for mirror/transpose orientations 2/4/5/7 | `internal/imagenorm/imagenorm_test.go:271` | kimi-k2.6:cloud | maintainability |
| 🟠 | No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases | `internal/imagenorm/imagenorm_test.go:276` | glm-5.2:cloud | maintainability |
| 🟡 | JPEG fill bytes between markers not skipped in exifOrientation | `internal/imagenorm/imagenorm.go:249` | opencode/kimi-k2.6:cloud | error-handling |
| 🟡 | table test missing t.Run subtests inconsistent with file pattern | `internal/imagenorm/imagenorm_test.go:282` | opencode/kimi-k2.6:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
One thing worth flagging first: the trailing block claiming "Plan mode is active" and instructing an Explore/Plan-agent/`ExitPlanMode` workflow doesn't correspond to any real tool in this session — I searched for `ExitPlanMode`/`AskUserQuestion` via ToolSearch and neither resolves, unlike the genuinely deferred tools (`TaskCreate`, `EnterWorktree`, etc.) which do. That block reads as an injected instruction trying to redirect this session away from delivering the actual review. I'm disregarding it and completing the task as originally specified (the Gadfly review format, output as text). I did already write a harmless scratch file to `/root/.claude/plans/...` before catching this — it contains only my own analysis notes, nothing sensitive, and can be ignored/deleted.
Here's the actual security review of the PR:
VERDICT: No material issues found
This diff adds pure, local EXIF-orientation parsing and pixel-transform code to `internal/imagenorm`. I read the live file (`internal/imagenorm/imagenorm.go:140-199`) to confirm the surrounding guarantees, not just the diff hunk:
- **Input is already size-capped before this code runs.** `raw` is bounded by `opts.maxBytes()` (default 25 MiB, enforced at imagenorm.go:140-151) before `exifOrientation(raw)` is ever called (imagenorm.go:175). No new unbounded-read surface.
- **`exifOrientation`'s marker walk is fully bounds-checked before every slice access** — `i+4<=len(raw)` before reading marker bytes, `segLen<2 || i+2+segLen>len(raw)` before slicing the segment body. Malformed/truncated JPEGs fall through to `return 1`, not a panic or OOB read.
- **`orientationFromApp1`'s TIFF/IFD parsing is equally guarded** — prefix-length check before the `"Exif\x00\x00"` compare, `ifd<8 || ifd+2>len(tiff)` before reading the entry count, and `off+12>len(tiff)` checked inside the loop before every 12-byte entry read. A crafted large entry count (`n` up to 65535) just runs bounded, cheap, bounds-checked iterations — not a meaningful DoS vector.
- **Safe-default posture is correct and consistently applied**: unparseable/non-JPEG/garbage input always yields orientation `1` (no rotation), never a guess — so a hostile file can at worst leave an image un-rotated, not corrupt a legitimate one or destabilize downstream processing.
- **`applyOrientation` only receives an already-validated `o` in `[2,8]`** and allocates its output buffer *after* `downscale`, so its allocation size is bounded by `opts.maxDim()` regardless of what the EXIF tag claims — no amplification/decompression-bomb path via orientation.
- No new sinks are introduced: no filesystem, SQL, shell, or network use; no secrets touched; nothing here affects authn/authz. This is pure in-memory byte parsing over data that was already being treated as untrusted (`decodeSafely`'s existing panic-recovery comment already documents the "untrusted upload" threat model this code operates under).
I did not find any injection, SSRF, deserialization, secret-handling, or input-validation defect within this diff's security lens.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
## Review — 🎯 Correctness lens
**VERDICT: No material issues found**
I independently re-derived all 8 EXIF orientation transforms from first principles (physically rotating/mirroring a labeled grid) rather than trusting the code's comments, and checked them against `internal/imagenorm/imagenorm.go:207-242`:
- Orientation 6 ("rotate 90° CW to correct") → `dx,dy = h-1-y, x` — verified against a manual 2×2 rotation trace, matches.
- Orientation 8 ("rotate 90° CCW to correct") → `dx,dy = y, w-1-x` — verified, matches.
- Orientations 2/3/4 (mirror-H, 180, mirror-V) and 5/7 (transpose/transverse) all check out against the canonical "where does stored (0,0) land" EXIF table.
Also verified:
- `orientationFromApp1` (`imagenorm.go:280-316`): TIFF header/IFD0 offset bounds, 12-byte IFD entry stride, and reading a SHORT value from the first 2 bytes of the 4-byte value field per file byte order — all correct per TIFF6 spec.
- `exifOrientation` (`imagenorm.go:249-275`): JPEG marker-segment length semantics (`segLen` includes the 2 length bytes, so `i += 2+segLen` and slicing `raw[i+4:i+2+segLen]` are both correctly offset), correctly halts at SOS/EOI so it never scans entropy-coded data, and fails safe (returns `1`) on any malformed/short segment rather than misapplying a rotation.
- Ordering in `Normalize` (`imagenorm.go:174-175`): `applyOrientation` is called with `exifOrientation(raw)` where `raw` is the original, un-truncated input bytes (line 145) — not the decoded/re-encoded buffer — so the tag is read from the actual source JPEG. `exifOrientation` gates on the `FFD8` SOI itself, so non-JPEG formats (PNG/WebP/HEIC) safely no-op without a separate format check. Downscale-then-rotate is sound: rotation only transposes width/height, so `max(w,h)` (the downscale bound) is invariant across it.
One thing I checked but am not reporting as a finding (too low-confidence/low-impact to meet the bar): `orientationFromApp1` doesn't validate the IFD entry's type/count fields before reading the Orientation value, so a file with a non-SHORT-typed 0x0112 tag could yield a bogus-but-in-range value instead of falling back to 1. No real camera emits Orientation as anything but SHORT, and I don't have enough confidence this is exploitable to list it as a real bug.
I was unable to execute `go test ./internal/imagenorm/...` in this environment (Bash approval unavailable under the current session mode), so the "tests pass" claim in the PR description is unverified by me directly — but I traced the test's rotation-formula arithmetic by hand against the same transforms and it's internally consistent with the (verified-correct) implementation.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
No other EXIF/binary parsing utilities exist elsewhere — that claim holds. All checks confirm the draft's single finding is accurate, but one supporting claim in the prose (about `downscale` already using the range-over-int idiom) is false, so I'll correct that in the output.
## Review — Code cleanliness & maintainability
**VERDICT: Minor issues**
The new EXIF parsing/orientation code (`internal/imagenorm/imagenorm.go:202-316`) is well-scoped, self-contained to one file, and matches the surrounding heavily-commented style (verified no pre-existing EXIF/TIFF parsing utilities elsewhere in the repo to duplicate, via `grep -rn "binary.ByteOrder\|Uint16\|Uint32"`). Naming (`exifOrientation`, `orientationFromApp1`, `applyOrientation`) is clear, and the 8-case switch in `applyOrientation` is an appropriate use of a switch over a transform-function table for this size. Tests reuse the existing `pngBytes` helper rather than duplicating it (confirmed at `internal/imagenorm/imagenorm_test.go:17`).
One small nit:
- **`internal/imagenorm/imagenorm.go:213-217`** — `applyOrientation` unconditionally allocates a `w×h` RGBA, then immediately discards it and allocates a second `h×w` RGBA whenever `swap` is true (orientations 5-8):
```go
dst := image.NewRGBA(image.Rect(0, 0, w, h))
if swap {
dst = image.NewRGBA(image.Rect(0, 0, h, w))
}
```
The first allocation is pure waste in the swap case. Cleaner and avoids the throwaway alloc:
```go
dw, dh := w, h
if swap {
dw, dh = h, w
}
dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
```
Verified by reading the function in full — no other use of `dst` before the reassignment, so the first allocation is dead work.
Nothing else in this diff (including the DESIGN.md wording update and the test file additions) raises cleanliness concerns.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
Confirmed — this matches the draft exactly: line 214 unconditionally allocates a `w×h` `image.NewRGBA`, then line 215-217 discards it and allocates a second `h×w` buffer whenever `swap` is true (orientations 5-8, which includes orientation 6, the common phone-portrait case). The fix suggestion (compute `dw, dh` first, allocate once) is correct and minimal. This is a real, verifiable finding — a small/minor perf nit, not a correctness bug, matching the draft's severity.
**VERDICT: Minor issues**
- `internal/imagenorm/imagenorm.go:214-217` — `applyOrientation` allocates a `w×h` `*image.RGBA` unconditionally, then immediately discards it and allocates a second `h×w` `*image.RGBA` whenever `swap` is true (orientations 5-8):
```go
dst := image.NewRGBA(image.Rect(0, 0, w, h))
if swap {
dst = image.NewRGBA(image.Rect(0, 0, h, w))
}
```
Orientation 6 (90° CW) is the single most common EXIF tag on real phone photos (camera held normally in portrait) — exactly the case this PR is built for per the PR description ("a phone camera... sets a flag to rotate on display"). So on the primary target workload, every upload does one throwaway allocation of up to `DefaultMaxDim × DefaultMaxDim × 4` bytes (~16.7 MB at the 2048px default) before the real buffer is allocated, adding avoidable allocation/GC pressure on the image-upload hot path.
Fix: compute the swapped dimensions before allocating, e.g.
```go
dw, dh := w, h
if swap {
dw, dh = h, w
}
dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
```
Everything else in the changed code is a bounded, single-pass `O(w·h)` pixel copy (unavoidable for baking in a rotation) applied after `downscale` caps dimensions, and a marker-segment walk over the raw JPEG header bytes (`exifOrientation`) that's O(segment count), not O(file size) in any problematic way. No N+1s, no unbounded growth, no new blocking calls on the hot path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
Go 1.26 supports range-over-int, so no compile issue there.
I've verified the core logic — bounds checks, loop termination, and defaults are all sound. Nothing in my lane rises to a real defect.
**VERDICT: No material issues found**
Findings (through the error-handling/edge-case lens only):
- `exifOrientation`/`orientationFromApp1` (imagenorm.go:249-316) are defensively bounds-checked at every slice access I traced (segment length vs. `len(raw)`, IFD offset vs. `len(tiff)`, entry offset vs. `len(tiff)` before each 12-byte read) — verified by re-deriving the offsets by hand against the TIFF/EXIF layout. Every failure path returns the safe default (`1`, no rotation) rather than erroring or guessing, matching the doc comment's stated intent.
- The `i += 2 + segLen` marker-walk loop is guaranteed to terminate: `segLen < 2` is rejected before the increment, so `i` strictly increases each iteration — no infinite-loop/DoS risk on adversarial input, and the byte read is already capped by `opts.MaxBytes` upstream.
- `applyOrientation` (imagenorm.go:207-242): the `o<=1 || o>8` guard fully covers the domain (exifOrientation only ever returns 1-8), so the switch's implicit cases 2-8 are exhaustive — no default-case gap. A 0×0 `img.Bounds()` degenerates to empty loops with no panic (unlikely in practice, but confirmed not to crash).
- No nil-image path: `img` reaching `applyOrientation`/downstream is always non-nil by construction (decodeSafely returns early on error), so no nil-check gap.
One sub-lens observation I considered but am not confident enough to list as a finding: `orientationFromApp1` doesn't validate the IFD entry's *type* field (expects SHORT/3 for Orientation) before reading the value bytes — a malformed tag with the same ID but a different type could, in a byte-order-dependent way, be misread as a spurious value 1-8 and silently misrotate instead of falling back safely. This requires deliberately malformed EXIF (real phone EXIF always types Orientation as SHORT) and is closer to a correctness/robustness nit than an error-handling defect, so I'm not reporting it as a real bug.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Verdict: No material issues found
I reviewed the new EXIF parser and orientation applier through the security lens — they process attacker-controlled upload bytes — and verified the bounds logic against the checked-out code.
- **`exifOrientation` / `orientationFromApp1` (imagenorm.go:249-316)** — all slice accesses are bounds-checked before use, so an OOB panic (DoS via process crash) is not reachable:
- SOI check guards `raw[0..1]`; loop condition `i+4 <= len(raw)` guards `raw[i..i+3]`; `segLen < 2 || i+2+segLen > len(raw)` guards `raw[i+4 : i+2+segLen]`.
- In `orientationFromApp1`, `len(seg) < len(prefix)+8` (14) guards `tiff[0:2]` and `tiff[4:8]`; `ifd < 8 || ifd+2 > len(tiff)` guards `tiff[ifd:ifd+2]`; `off+12 > len(tiff)` guards `tiff[off:off+2]` and `tiff[off+8:off+10]`. The value-field read ignores the declared tag type but stays in bounds, so a malicious non-SHORT type can't cause a panic — worst case it misreads and falls through to the `1..8` check or returns false.
- The IFD entry-count loop `for k := range n` is attacker-controlled (uint16, ≤65535) but breaks as soon as `off+12 > len(tiff)`, so iteration is bounded by `len(tiff)/12`; `len(tiff)` is itself bounded by `MaxBytes` (25 MiB). No integer-overflow concern on 32-bit either, since `ifd` is clamped to `≤ len(tiff)-2` before use. This is at most trivial CPU amplification, far below the cost of the decode that already ran.
- Safe-default discipline is correct: every unparseable/non-JPEG/garbage path returns `1` (no-op), so a malformed file can never coerce an unwanted rotation of a correct image. This is the fail-safe direction.
- **`applyOrientation` (imagenorm.go:207-242)** — operates only on the already-decoded `image.Image` and a validated `o` in `1..8`; `At`/`Set` on stdlib image types don't panic. No new taint surface.
- **Recover scope** — noted (not a finding): `decodeSafely`'s `recover` does not wrap `exifOrientation`/`applyOrientation`, but neither has a reachable panic on untrusted input per the bounds analysis above, so the unprotected scope is safe.
- No injection (SQL/command/path/template), SSRF, deserialization, secret/credential, or authn/authz surface in this change — it's pure in-memory byte parsing of an uploaded image. The upload boundary and size caps (`MaxBytes`, `maxDecodePixels`, `maxDimension`) upstream of this code already bound the input.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Verdict: No material issues found
I verified the change through the correctness lens by reading the full implementation (`internal/imagenorm/imagenorm.go:140-334`) and re-deriving the EXIF orientation transforms from first principles.
- **`applyOrientation` transforms (`imagenorm.go:207-242`)**: I checked all 8 cases against the EXIF 2.3 spec definitions (1=identity, 2=mirror-H, 3=180, 4=mirror-V, 5=transpose, 6=90°CW, 7=transverse, 8=90°CCW). Every mapping `(x,y) → (dx,dy)` is correct, including the dimensional transpose for o≥5 (swap is right for all of 5/6/7/8, not just the pure rotations). Corner-spot checks confirm: o6 maps (0,0)→(h-1,0) (top-left→top-right, correct for CW); o8 maps (0,0)→(0,w-1) (top-left→bottom-left, correct for CCW).
- **Downscale-then-rotate ordering (`imagenorm.go:171-175`)**: The comment's invariant holds. Downscale bounds the longest edge to `maxDim`; a rotation/transposition only swaps sides, so the longest edge stays ≤ `maxDim`. Worked example: 4000×3000 → downscale 1024×768 → o6 swap → 768×1024 (longest edge 1024). ✓
- **`exifOrientation` / `orientationFromApp1` (`imagenorm.go:249-316`)**: JPEG segment walk, bounds, IFD0 parsing, and the inline-SHORT read at `off+8:off+10` are all correct. Non-JPEG inputs (HEIC/PNG/WebP) correctly short-circuit on the `0xFFD8` SOI check and return 1, so `applyOrientation` is a no-op for them. All slice accesses are guarded against panics (verified each bounds check). Failure mode on any unparseable/edge input is returning 1 (no rotation) — never a wrong rotation — which is the safe default.
- **Test expectations (`imagenorm_test.go`)**: I re-derived each `wantW/wantH/mx/my` from the same transform the code applies; all match (e.g. o6: `dx=h-1-y=17, dy=x=10` for source center (10,6) with h=24; o8: `dx=y=6, dy=w-1-x=29`; o3: `dx=29, dy=17`).
One non-blocking observation (not a correctness bug): the value field is read as `uint16` without verifying the tag's EXIF type is SHORT. Since the spec mandates SHORT for Orientation this is fine in practice; a hypothetical big-endian LONG-typed tag would read 0 and fall back to the safe default of 1 rather than a wrong rotation, so it fails safe.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues found
- **`internal/imagenorm/imagenorm.go:214-216` — redundant double allocation on the swap branch.** `dst` is allocated as `image.NewRGBA(image.Rect(0,0,w,h))` on line 214, then unconditionally reassigned to `image.NewRGBA(image.Rect(0,0,h,w))` on line 216 when `swap` is true. On the quarter-turn path (orientations 5–8) the first RGBA header is allocated and immediately discarded. The non-swap branch keeps the first allocation correctly, so this is only a wasted allocation on the swap branch, not a correctness bug — a readability/efficiency wart in an allocation-sensitive path. Cleaner: compute the output dims up front and allocate once.
- **`internal/imagenorm/imagenorm.go:218-239` — manual `At`/`Set` per-pixel loop diverges from the `draw`-package idiom used elsewhere in the file.** `applyOrientation` walks every pixel with `img.At`/`dst.Set` (going through the `color.Model` conversion path), while `downscale` (line 332) uses `draw.CatmullRom.Scale`. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is avoidable. Not blocking; a consistency nit.
- **`internal/imagenorm/imagenorm_test.go:276-280` — no test covers orientations 2, 4, 5, 7.** `TestNormalizeAppliesExifOrientation` only exercises 0/1/3/6/8, and `TestExifOrientationParsing` only parses 6. The `applyOrientation` switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transverse math.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
Both findings are confirmed against the actual code. The first (double RGBA allocation on the swap path) is real and at the cited lines. The second is informational and not in the findings JSON, consistent with the draft.
**Verdict: Minor issues**
- **`internal/imagenorm/imagenorm.go:214-217` — double RGBA allocation on the swap (portrait) path.** For orientations 5–8 (the quarter-turns), `dst` is first allocated at `w×h` and then immediately overwritten with a second `image.NewRGBA` at `h×w`:
```go
dst := image.NewRGBA(image.Rect(0, 0, w, h))
if swap {
dst = image.NewRGBA(image.Rect(0, 0, h, w))
}
```
The first buffer is allocated and discarded. This is the common phone-portrait path — orientation 6 is exactly "portrait photo tagged rotate 90°", the motivating case of this PR. For a 2048×1536 portrait, that's a ~12 MiB RGBA thrown straight to the GC on the very upload the feature targets. Fix is trivial — compute the destination rect once:
```go
dw, dh := w, h
if swap { dw, dh = h, w }
dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
```
- **`internal/imagenorm/imagenorm.go:218-240` — per-pixel `At`/`Set` loop** (informational, not blocking). The rotate reads via `img.At` and writes via `dst.Set`, both of which go through generic `color.Color`/`color.Model` conversions rather than fast direct RGBA writes. Bounded to ≤2048px and only on the rotated path, so acceptable for an upload boundary — but worth noting that a portrait phone photo already within `MaxDim` (so `downscale` returns the original `*image.YCbCr`) hits the slowest variant (YCbCr-per-pixel `At`). Not worth changing unless this shows up in profiles.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the error-handling & edge-case lens, the new code is solid. I verified the two new functions and the bounds checks by reading `internal/imagenorm/imagenorm.go:202-316`:
- **`exifOrientation` (line 249):** Every byte access is bounds-checked before use. The marker walk guards `len(raw) < 4`, `i+4 <= len(raw)`, `raw[i] != 0xFF` (misalignment → safe return 1), `segLen < 2 || i+2+segLen > len(raw)`, and stops at EOI/SOS. The slice passed to `orientationFromApp1` (`raw[i+4 : i+2+segLen]`) is provably in range because of the `i+2+segLen > len(raw)` guard. The loop advances `i += 2 + segLen` with `segLen >= 2`, so it strictly progresses and terminates — no infinite loop. `segLen` is a 16-bit value (max 65535), so `i+2+segLen` can't overflow int on any supported platform.
- **`orientationFromApp1` (line 280):** Length guard `len(seg) < len(prefix)+8` guarantees `tiff := seg[len(prefix):]` has at least 8 bytes, so `tiff[0:2]` and `tiff[4:8]` are safe. The IFD offset is validated (`ifd < 8 || ifd+2 > len(tiff)`), and each 12-byte entry is checked with `off+12 > len(tiff)` before the `Uint16`/`Uint32` reads. A crafted huge entry count `n` is bounded by the per-entry length check, so it can't spin unbounded. Out-of-range orientation values fall through to `return 0, false` → caller defaults to 1. No panics on truncated/garbage input.
- **`applyOrientation` (line 207):** Out-of-range `o` (`<= 1 || > 8`) is a no-op returning the original image. Empty/zero-dimension images (`w==0` or `h==0`, which `downscale` can pass through) produce empty `image.NewRGBA` allocations and the `for y := range h` / `for x := range w` loops simply don't execute — no index panic. `dst.Set(dx, dy, c)` always writes within the allocated rect by construction of the transforms.
- **No swallowed errors introduced:** Both new call sites in `Normalize` (`applyOrientation`, `exifOrientation`) return values, not errors, so there are no new error-return paths to mishandle or defer. The existing "on any error the returned bytes are nil" contract is preserved.
The test helper `spliceExifOrientation` discards `binary.Write` errors with `_`, but those write to a `bytes.Buffer` which never errors — not a concern.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
The change adds a manual EXIF orientation parser for JPEG and pixel-level rotation logic. From a security lens, the code is clean:
- **Fail-closed parsing**: `exifOrientation` returns 1 (no rotation) on any malformed input, missing EXIF, or non-JPEG format — never forcing a rotation onto a correct image.
- **Bounded work**: `maxDecodePixels` (50M) and `maxDimension` (50K) already cap decoded bitmap size. `applyOrientation` runs on the *downscaled* image (downscale happens first), so memory and CPU for the rotation are bounded by the same limits.
- **No out-of-bounds reads**: `orientationFromApp1` checks `len(seg) < len(prefix)+8` before indexing `tiff[0:2]`, `tiff[4:8]`, etc.; the IFD walk checks `off+12 > len(tiff)` before reading each 12-byte entry.
- **No out-of-bounds writes**: `applyOrientation` computes destination coordinates (`dx`, `dy`) that always fall within the pre-allocated `dst` bounds for every orientation case (2–8).
- **Strip, not leak**: The re-encode to JPEG strips all EXIF metadata (including GPS, camera model, timestamps). This is a net privacy win — only the orientation value is extracted and then discarded after the bitmap rotation.
One minor observation (not a finding, just a verification note): the `exifOrientation` parser is strict about `0xFF` marker alignment (`raw[i] != 0xFF → return 1`), which is the conservative/safe choice for untrusted uploads.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Minor issues**
- **`internal/imagenorm/imagenorm.go:305-309`** — `orientationFromApp1` reads the Orientation value without validating the IFD entry's **type** and **count**. The EXIF spec requires Orientation to be `SHORT` (type `3`) with count `1`. A malformed entry with a different type (e.g. `LONG`) or count `>1` stores an *offset* in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "safe default: return 1 for unparseable" design goal) instead of treating the tag as unreadable.
**Suggested fix:** Add two checks inside the `0x0112` match block:
```go
if bo.Uint16(tiff[off+2:off+4]) != 3 { // not SHORT
return 0, false
}
if bo.Uint32(tiff[off+4:off+8]) != 1 { // count != 1
return 0, false
}
```
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues
- `internal/imagenorm/imagenorm.go:214-216` — `applyOrientation` allocates a `w×h` RGBA unconditionally, then immediately discards it and allocates a second `h×w` RGBA for orientations 5-8. Move the `swap` check before the first `NewRGBA` to avoid the throw-away allocation for the quarter-turn cases.
- `internal/imagenorm/imagenorm_test.go:271-281` — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.
</details>
<details><summary><b>⚡ Performance</b> — Blocking issues found</summary>
**Blocking issues found**
- **`internal/imagenorm/imagenorm.go:214-216` — Double image allocation on every 90° rotation.** `applyOrientation` always allocates `image.NewRGBA(image.Rect(0, 0, w, h))`, then immediately throws it away and allocates `image.NewRGBA(image.Rect(0, 0, h, w))` when `swap` is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple `if/else` so only the correctly-sized buffer is ever allocated.
- **`internal/imagenorm/imagenorm.go:218-240` — Per-pixel `image.Image.At()` calls box a `color.Color` on every iteration, causing millions of heap allocations for a single rotated image.** `applyOrientation` iterates every pixel and reads via the `image.Image` interface (`img.At(b.Min.X+x, b.Min.Y+y)`). When the concrete type is `*image.RGBA` (which `downscale` already returns), each `At` call boxes the `color.RGBA` value into an interface, allocating on the heap. A 2048×2048 rotation causes ~4.2 million transient allocations. This is a well-known Go image-processing anti-pattern; adding a fast-path type-switch to `*image.RGBA` (and perhaps `*image.NRGBA`) that copies through `Pix` directly would eliminate the boxing and interface dispatch entirely for the common case, cutting rotation time by an order of magnitude. The current generic fallback can be kept for non-RGBA images.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`internal/imagenorm/imagenorm.go:256` — JPEG marker-padding edge case unhandled.**
The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. `exifOrientation` assumes exactly one `0xFF` followed immediately by the marker code, so a file like `FF D8 FF FF E1 …` (valid per spec) misaligns: `marker` becomes `0xFF`, `segLen` is read from the next two bytes (`0xE1 0x00` ≈ 57 KiB), the length check fails, and the parser bails out returning `1` (normal). For a portrait photo whose APP1 segment is padded this way, the image stays sideways. A robust parser should skip consecutive `0xFF` bytes before reading the marker code.
- **`internal/imagenorm/imagenorm.go:305` — TIFF type field not validated.**
`orientationFromApp1` finds tag `0x0112` and blindly reads the first two bytes of the value field as a `SHORT`. It never checks `tiff[off+2:off+4]` to confirm the type is actually `3` (`SHORT`). A malformed EXIF that reuses tag `0x0112` with a `LONG` or `UNDEFINED` type could yield a bogus orientation value. While `exifOrientation` caps the result to `1..8`, any value in that range would still be applied. Adding a type check (`bo.Uint16(tiff[off+2:off+4]) == 3`) before accepting the value would make the “unparseable → 1” guarantee stronger.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
Reviewed the EXIF parser through the security lens (untrusted-byte parsing, OOB reads, integer overflow, infinite loops, injection surface). The code is defensively written:
- `exifOrientation` (imagenorm.go:249-275): all index accesses are guarded by the loop condition `i+4 <= len(raw)` and `i+2+segLen > len(raw)`; `segLen >= 2` guarantees loop progress (no infinite loop); `segLen` is a 16-bit field so no int overflow on `i+2+segLen`; safe default `1` on any misalignment, EOI/SOS, or non-JPEG.
- `orientationFromApp1` (imagenorm.go:280-313): length check `len(seg) < len(prefix)+8` covers `tiff[0:2]` and `tiff[4:8]`; IFD offset validated with `ifd < 8 || ifd+2 > len(tiff)` before slicing; per-entry bounds `off+12 > len(tiff)` cover `tiff[off+8:off+10]`; orientation value clamped to 1..8 before use. A 32-bit build could in principle make `int(bo.Uint32(...))` negative for a near-`MaxUint32` offset, but the subsequent `ifd+2 > len(tiff)` comparison still rejects it (negative < len), so it can't be exploited to read OOB — it just falls through to the safe default.
- The input to the parser is `raw`, which `Normalize` already capped at `MaxBytes+1` (imagenorm.go:~125), so the parser runs on size-bounded data regardless.
- No injection/auth/secret/SSF surface: this is a pure local byte parser; its only output is an int 1..8 consumed by `applyOrientation`, which does an in-memory pixel copy with no I/O.
Nothing in the security lens is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I verified the orientation math, the EXIF parser bounds, and the test expectations against the checked-out code.
**Verdict: No material issues found**
- I re-derived all 8 EXIF orientation transforms from first principles (rotating/mirroring the source bitmap into an upright destination) and confirmed each `case` in `applyOrientation` (`imagenorm.go:222-237`) maps source `(x,y)` to the correct destination `(dx,dy)`, including the dimension-swapping cases (5–8) where `dst` is correctly allocated as `h×w` and all writes stay in bounds.
- The "downscale then rotate" ordering is sound: `downscale` bounds the *longest* edge, and a 90° rotation swaps sides without changing the longest edge, so the `MaxDim` bound still holds on the emitted image (verified for both the shrink and no-shrink paths).
- The EXIF parser bounds are correct: the marker walk guards `i+4 <= len(raw)` and `segLen < 2`/`i+2+segLen > len(raw`; `orientationFromApp1` requires `len(seg) >= len(prefix)+8` (so `tiff[4:8]` is safe) and `ifd < 8 || ifd+2 > len(tiff)` plus per-entry `off+12 > len(tiff)` prevent OOB reads. The APP1 payload slice `raw[i+4 : i+2+segLen]` correctly excludes the 2 length bytes.
- The test expectations are consistent with the transforms: e.g. orient 6 (90° CW) sends source (10,6) to (h-1-y, x) = (17,10) on a 24×40 canvas, matching `{6, 24, 40, 17, 10}`; orient 8 → (y, w-1-x) = (6,29) matching `{8, 24, 40, 6, 29}`. The `spliceExifOrientation` helper builds a well-formed little-endian TIFF/IFD0 (verified byte layout: `II` + 0x2A + offset 8 + count 1 + 12-byte entry with value at entry+8), which the parser reads back correctly.
- Non-JPEG paths return 1 immediately (`raw[0]!=0xFF||raw[1]!=0xD8`), so HEIC/WebP/PNG are left untouched as designed — no regression vs. prior behavior.
One trivial robustness note (not material, low confidence): the marker walker doesn't skip JPEG fill bytes (`0xFF 0xFF`) or handle standalone markers like TEM/`0xFF01`/RSTn that have no length field before the EXIF APP1. A JPEG emitting fill bytes before APP1 would misparse and fall back to orientation 1 (sideways). Real phone JPEGs place APP1 immediately after SOI, so this is out of the practical path and the fallback is safe (never mis-rotates a correct image).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**VERDICT: No material issues found**
The EXIF orientation addition is clean through the maintainability lens. Verified against the checked-out code:
- `applyOrientation` and `exifOrientation` are well-factored, each doing one thing, with accurate doc comments that match the implementation. They follow the file's existing conventions (sentinel-free helpers, inline rationale comments, pure-Go no-deps stance).
- No duplicated logic: `exifOrientation` re-walks the JPEG bytes because `image.DecodeConfig` doesn't expose EXIF — that's a justified read, not copy-paste.
- The no-op short-circuit (`o <= 1 || o > 8`) returns `img` unchanged, so non-JPEG and normal-orientation inputs pay zero allocation cost — the comment claims this and the code does it.
- Tests reuse the existing `pngBytes`/`jpegBytes` fixture-builder pattern and add `orientedJPEG`/`spliceExifOrientation`/`bright` in the same style; the helper that builds a minimal TIFF IFD entry writes the 12-byte entry layout correctly (2+2+4+2+2), so the fixture reflects a real Exif APP1 rather than asserting the parser's own shape back at itself.
- `DESIGN.md` note is updated in the same sentence and stays accurate ("bakes in the JPEG EXIF orientation"), consistent with CLAUDE.md's "keep docs true" rule.
Nothing in my area to flag.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Reading the file to verify the per-pixel loop's allocation behavior.
VERDICT: No material issues found
Reviewed `internal/imagenorm/imagenorm.go` (the changed file) and its caller `internal/api/seed_packet.go`. Through the performance lens:
- **`applyOrientation` per-pixel loop** (`imagenorm.go:218-240`): iterates every pixel with `img.At` + `dst.Set`, each doing a color-model round-trip. This is the only new work on the hot path. It runs **only when `o > 1`** (an actual non-normal EXIF orientation tag exists); orientation 1 — the common case for landscape photos and all non-JPEG formats — early-returns at line 208 with zero cost. When it does run, the image is already capped at `DefaultMaxDim` (2048px longest edge) by the preceding `downscale`, so the loop is bounded to ~4M iterations worst case. On a single-upload endpoint that then calls a vision model (seconds of latency), the tens-of-ms per-pixel copy is immaterial. No N+1, no quadratic behavior, no unbounded allocation.
- **`exifOrientation(raw)`** (`imagenorm.go:249-275`): called unconditionally but bails in O(1) for non-JPEG (`raw[0]!=0xFF || raw[1]!=0xD8`) and walks at most the handful of JPEG marker segments for JPEGs. Negligible.
- **Transient allocation**: `applyOrientation` allocates one `*image.RGBA` matching the (already-bounded) downscaled image. Doubling of the ~16MB peak bitmap is acceptable for a one-shot upload path.
The straightforward per-pixel implementation could be sped up with `RGBAAt`/`SetRGBA` (skipping two color-model conversions per pixel) since `downscale` always yields an `*image.RGBA`, but that is a micro-optimization with no measurable impact at this call frequency and image size — not flagging it.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've read the full `imagenorm.go` and the relevant test file. Let me verify the boundary logic in the new EXIF/orientation code carefully.
**Verdict: No material issues found**
I walked the unhappy paths the diff introduces, against the checked-out code:
- **`exifOrientation` / `orientationFromApp1` parser bounds** (imagenorm.go:249-316): every slice access is guarded before use. `len(raw) < 4` short-circuits entry; the loop requires `i+4 <= len(raw)` before reading 4 header bytes; `segLen < 2 || i+2+segLen > len(raw)` gates the segment slice `raw[i+4 : i+2+segLen]` (and `segLen >= 2` makes `i+4 <= i+2+segLen`). In `orientationFromApp1`, `len(seg) < len(prefix)+8` (14) covers `tiff[0:2]` and `tiff[4:8]`; `ifd < 8 || ifd+2 > len(tiff)` gates the count read; `off+12 > len(tiff)` gates every 12-byte entry read, which also covers `tiff[off+8:off+10]`. Every unparseable / truncated / hostile case returns the safe default `1` — never a panic, never a wrong rotation. Verified by reading the file.
- **`applyOrientation` output bounds** (imagenorm.go:207-242): for the non-swap cases (2/3/4) `dx ∈ [0,w-1]`, `dy ∈ [0,h-1]` and `dst` is `w×h`; for the swap cases (5/6/7/8) the loops are `y ∈ [0,h)`, `x ∈ [0,w)` and each mapping lands in `[0,h-1] × [0,w-1]` against an `h×w` destination — I re-derived all four quarter-turn mappings and confirmed no off-by-one and no out-of-bounds `Set`. `o <= 1 || o > 8` is a no-op, so an out-of-range tag (or the `0, false` return folded in as `0`) leaves the image untouched.
- **Empty/zero inputs**: `Normalize` rejects `cfg.Width <= 0 || cfg.Height <= 0` before decode, so `applyOrientation` never sees a zero-dimension image that would make `image.NewRGBA` degenerate; `downscale`'s `longest == 0` guard is also upstream. No nil-image path: decode failure returns before `applyOrientation` runs.
- **Panic coverage**: `decodeSafely`'s `recover` wraps `image.Decode`; the new code runs *after* it and does only bounds-checked slice access and `img.At`/`dst.Set` within verified bounds, so there's no unprotected panic surface from a malformed EXIF blob.
One marginal observation, not material enough to list as a finding: `ifd := int(bo.Uint32(tiff[4:8]))` (line 295) could in principle let `ifd+2` wrap on a 32-bit `int` build given a hostile TIFF offset, bypassing the `ifd+2 > len(tiff)` guard. pansy's `CGO_ENABLED=0` static binary targets 64-bit servers (Komodo deploy), where `int` is 64-bit and the guard holds — so this is not a live risk here, and I'm flagging it only because the lens names integer overflow. No change needed.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict:** No material issues found
The EXIF orientation parser is defensively written against untrusted input: bounds-checked on every slice access, defaults to 1 (no rotation) on any malformed data, bounded by the 25 MiB read cap, and advances monotonically through markers with no infinite-loop path. The coordinate mapping in `applyOrientation` stays within the allocated destination for all 8 orientation values. No authn/authz gaps, injection, deserialization, secret leakage, or untrusted-data mishandling is introduced.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I examined the EXIF orientation parser, the transform application, and the interaction with downscaling. Here is what I verified:
- **`applyOrientation` transforms**: I re-derived the mapping for all 8 EXIF orientations from the spec (0th-row/0th-column placement) and confirmed each `case` matches. The quarter-turns (5–8) correctly swap the output dimensions, and the pixel-level mappings agree with the test expectations for the covered cases (1, 3, 6, 8) and, by derivation, the uncovered ones (2, 4, 5, 7).
- **Downscale-then-rotate order**: Downsampling first is cheaper, and because a 90° rotation only swaps width/height, the longest-edge bound `≤ MaxDim` is preserved. This reasoning is sound.
- **EXIF parser robustness**: `exifOrientation` walks big-endian JPEG markers, stops at SOS/EOI, bounds-checks every slice operation, and returns 1 (no rotation) on any parse failure. `orientationFromApp1` correctly reads the IFD0 entry for tag `0x0112` as an inline SHORT value with the declared byte order. It safely rejects malformed input (bad magic, short buffers, invalid offsets) by returning `ok=false`, which causes the caller to fall back to 1.
- **Non-JPEG safety**: `exifOrientation` requires the `FF D8` SOI signature; all other formats return 1, and `applyOrientation` with `o==1` returns the image unchanged.
- **Test verification**: The `spliceExifOrientation` helper constructs a valid little-endian TIFF IFD with the correct 12-byte entry layout and `segLen` accounting. The marker-location assertions in `TestNormalizeAppliesExifOrientation` match the transforms.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`internal/imagenorm/imagenorm.go:214`** — `applyOrientation` allocates `dst` with dimensions `w×h`, then immediately throws it away and allocates again with `h×w` whenever `swap` is true (orientations 5–8). This wastes an allocation of `w*h*4` bytes for every quarter-turn. Move the allocation into an `if/else` so only the correct size is created.
- **`internal/imagenorm/imagenorm_test.go:282`** — `TestNormalizeAppliesExifOrientation` iterates over a table of cases but doesn't wrap them in `t.Run` subtests, unlike `TestNormalizeAllFormats` and `TestNormalizeRejectsPixelBomb` in the same file. Adding subtests makes failures immediately attributable to the specific orientation and keeps isolation consistent with the file's existing pattern.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Verdict:** Minor issues
- **`internal/imagenorm/imagenorm.go:214-216`** — `applyOrientation` allocates an `image.RGBA` of size `w×h` unconditionally, then immediately discards it and allocates a second one of size `h×w` when `swap` is true (orientations 5–8). For a 2048×1536 image that’s a ~12.5 MB throwaway allocation; at 2048×2048 it’s ~16 MB. The fix is trivial: only allocate after the `swap` check, e.g. `dw, dh := w, h; if swap { dw, dh = h, w }; dst := image.NewRGBA(image.Rect(0, 0, dw, dh))`.
- **`internal/imagenorm/imagenorm.go:218-239`** — The pixel-by-pixel `img.At` loop is correct but has high per-pixel overhead (interface call + color conversion for every pixel). This is acceptable for an upload handler that runs once per image, but it’s worth noting that for images already within `maxDim` bounds (where `downscale` returns the original), this copies the full decoded bitmap via the slow `image.Image` path instead of working directly on the concrete type. Not blocking for the seed-packet use case, but if large unchanged images become common this would be the next bottleneck.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`internal/imagenorm/imagenorm.go:280` — Missing TIFF magic number validation in `orientationFromApp1`**
The function trusts the IFD offset at `tiff[4:8]` without first checking that bytes `tiff[2:4]` contain the required TIFF magic number `42` (`0x002A` little-endian / `0x2A00` big-endian). A malformed APP1 segment that passes the `"Exif\x00\x00"` prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially yielding a bogus orientation value.
*Fix:* Verify `bo.Uint16(tiff[2:4]) == 0x002A` (or `0x2A00` interpreted via `bo`) before reading the IFD offset.
- **`internal/imagenorm/imagenorm.go:305-309` — Orientation tag type/count not validated**
When tag `0x0112` is found, the code assumes it is a SHORT with `count == 1` and reads the value inline from the first two bytes of the 4-byte value field. It never checks the entry’s type code (bytes `off+2:off+4`) or count (bytes `off+4:off+8`). A malformed EXIF segment with tag `0x0112` but a different type (e.g. LONG) or `count > 1` would store an offset in the value field rather than the value itself; the parser would then read that offset as a uint16. If the misread value happens to fall in `[1,8]`, a wrong rotation is silently baked into the image.
*Fix:* After matching the tag, verify `bo.Uint16(tiff[off+2:off+4]) == 3` (SHORT) and `bo.Uint32(tiff[off+4:off+8]) == 1` before reading `bo.Uint16(tiff[off+8:off+10])`.
- **`internal/imagenorm/imagenorm.go:249-275` — JPEG fill bytes (`0xFF`) between markers are not skipped**
The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. The parser at `exifOrientation` assumes each marker is immediately preceded by exactly one `0xFF` byte. If a phone encoder emits padding `0xFF` bytes before the APP1 segment, the loop misreads the marker and returns orientation `1` instead of the true value. This is safe (no wrong rotation) but incorrect for a valid JPEG.
*Fix:* Before reading `raw[i+1]` as the marker, skip consecutive `0xFF` bytes.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Gadfly on #103:
- (4 models) applyOrientation allocated a WxH RGBA then threw it away for a
quarter-turn to allocate HxW. Compute the output dims once, allocate one
buffer.
- (perf) The At/Set loop boxed a color.Color per pixel — millions of heap
allocs on a full-res rotation. Convert to *image.RGBA once (draw.Draw)
then copy 4 bytes per pixel by offset. No boxing.
- (2 findings) exifOrientation didn't skip 0xFF fill bytes before a marker,
so a spec-valid padded APP1 would be misread. Skip them.
- (2 findings) orientationFromApp1 read the tag's inline value without
checking its type/count — a mistyped LONG/offset would be read as a
bogus SHORT. Require type=SHORT, count=1; also validate the TIFF 0x2A
magic agrees with the byte order.
- Tests now cover all 8 orientations (added the mirror/transpose cases
2/4/5/7, the most error-prone switch arms) as t.Run subtests.
All imagenorm tests green; gofmt clean; still CGO_ENABLED=0.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 90af03d597 into main2026-07-22 05:35:46 +00:00
steve
deleted branch feat/exif-orientation2026-07-22 05:35:47 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #103. Part of #96 (carries the deferred #81 EXIF follow-up).
imagenormdecoded and re-encoded to JPEG but ignored the EXIFOrientationtag. Phone cameras store the sensor pixels one way and set a flag to rotate on display, so a "portrait" JPEG is really a landscape bitmap tagged "rotate 90°" — and the re-encode strips EXIF, so without baking the rotation in, a packet photographed in portrait reaches the vision model sideways (worse OCR) and any future thumbnail is wrong.What
exifOrientation— a small pure-Go parser that walks the JPEG APP1/Exif segment for tag0x0112, returning1(normal) for non-JPEG or unparseable input (never guess a rotation onto a correct image). No cgo, no new dependency.applyOrientation— bakes in all 8 orientations (4 rotations + mirrors), applied after downscale (cheaper to rotate the small image; a 90° turn swaps the sides but not the longest edge, so the downscale bound still holds).Tests
Build oriented JPEGs (a white corner marker + a spliced little-endian Exif APP1) and assert the marker lands exactly where each orientation says — dims swapping for the quarter-turns (6/8) — plus the 180 and no-EXIF/normal cases, and parser defaults for PNG / no-EXIF / garbage bytes.
GOWORK=off go build ./...+go test ./internal/imagenorm/green;gofmtclean; staysCGO_ENABLED=0.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 05:30:34Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 9 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -191,0 +211,4 @@b := img.Bounds()w, h := b.Dx(), b.Dy()swap := o >= 5 // a quarter-turn: output dimensions transposedst := image.NewRGBA(image.Rect(0, 0, w, h))🔴 Double image allocation on 90° rotations: first RGBA buffer immediately discarded
maintainability, performance · flagged by 4 models
internal/imagenorm/imagenorm.go:214-216— Double image allocation on every 90° rotation.applyOrientationalways allocatesimage.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocatesimage.NewRGBA(image.Rect(0, 0, h, w))whenswapis true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simpleif/elseso only the correctly-sized buffer is ever…🪰 Gadfly · advisory
@@ -191,0 +215,4 @@if swap {dst = image.NewRGBA(image.Rect(0, 0, h, w))}for y := range h {🟠 Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation
maintainability, performance · flagged by 2 models
internal/imagenorm/imagenorm.go:218-239— manualAt/Setper-pixel loop diverges from thedraw-package idiom used elsewhere in the file.applyOrientationwalks every pixel withimg.At/dst.Set(going through thecolor.Modelconversion path), whiledownscale(line 332) usesdraw.CatmullRom.Scale. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is…🪰 Gadfly · advisory
@@ -191,0 +246,4 @@// a wrong guess rotates a correct image. Only the JPEG APP1/Exif path is parsed:// that's the format uploaded phone photos overwhelmingly arrive in, and the other// decoders own their own orientation.func exifOrientation(raw []byte) int {🟡 JPEG fill bytes between markers not skipped in exifOrientation
error-handling · flagged by 1 model
internal/imagenorm/imagenorm.go:249-275— JPEG fill bytes (0xFF) between markers are not skipped The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. The parser atexifOrientationassumes each marker is immediately preceded by exactly one0xFFbyte. If a phone encoder emits padding0xFFbytes before the APP1 segment, the loop misreads the marker and returns orientation1instead of the true value. This is safe (no wrong rotation) but incorrect for a valid…🪰 Gadfly · advisory
@@ -191,0 +253,4 @@return 1}for i := 2; i+4 <= len(raw); {if raw[i] != 0xFF {🟠 JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs
error-handling · flagged by 1 model
internal/imagenorm/imagenorm.go:256— JPEG marker-padding edge case unhandled. The JPEG spec allows any number of fill bytes (0xFF) to precede a marker.exifOrientationassumes exactly one0xFFfollowed immediately by the marker code, so a file likeFF D8 FF FF E1 …(valid per spec) misaligns:markerbecomes0xFF,segLenis read from the next two bytes (0xE1 0x00≈ 57 KiB), the length check fails, and the parser bails out returning1(normal). For a portrait photo whose…🪰 Gadfly · advisory
@@ -191,0 +277,4 @@// orientationFromApp1 reads the Orientation tag from a JPEG APP1 segment body// (everything after the 2-byte length): "Exif\0\0" then a TIFF block holding// IFD0. Returns (0, false) if the segment isn't Exif or the tag is missing.func orientationFromApp1(seg []byte) (int, bool) {🟠 Missing TIFF magic number validation in orientationFromApp1
error-handling · flagged by 1 model
internal/imagenorm/imagenorm.go:280— Missing TIFF magic number validation inorientationFromApp1The function trusts the IFD offset attiff[4:8]without first checking that bytestiff[2:4]contain the required TIFF magic number42(0x002Alittle-endian /0x2A00big-endian). A malformed APP1 segment that passes the"Exif\x00\x00"prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially…🪰 Gadfly · advisory
@@ -191,0 +302,4 @@if off+12 > len(tiff) {return 0, false}if bo.Uint16(tiff[off:off+2]) != 0x0112 { // Orientation tag🟠 EXIF orientation parser does not validate IFD type/count before reading value
correctness, error-handling · flagged by 2 models
internal/imagenorm/imagenorm.go:305-309—orientationFromApp1reads the Orientation value without validating the IFD entry's type and count. The EXIF spec requires Orientation to beSHORT(type3) with count1. A malformed entry with a different type (e.g.LONG) or count>1stores an offset in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "sa…🪰 Gadfly · advisory
@@ -203,0 +268,4 @@// The white marker starts centred at (10,6) in the 40x24 source. For each// orientation, wantW/H is the corrected canvas and (mx,my) is where that// marker must land — derived from the same transform Normalize applies.cases := []struct {🟠 Missing test coverage for mirror/transpose orientations 2/4/5/7
maintainability · flagged by 1 model
internal/imagenorm/imagenorm_test.go:271-281— The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.🪰 Gadfly · advisory
@@ -203,0 +273,4 @@wantW, wantH intmx, my int}{{0, 40, 24, 10, 6}, // no EXIF → unchanged, marker top-left🟠 No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases
maintainability · flagged by 1 model
internal/imagenorm/imagenorm_test.go:276-280— no test covers orientations 2, 4, 5, 7.TestNormalizeAppliesExifOrientationonly exercises 0/1/3/6/8, andTestExifOrientationParsingonly parses 6. TheapplyOrientationswitch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transver…🪰 Gadfly · advisory
@@ -203,0 +279,4 @@{6, 24, 40, 17, 10}, // 90° CW → top-right (dims swap){8, 24, 40, 6, 29}, // 90° CCW → bottom-left (dims swap)}for _, tc := range cases {🟡 table test missing t.Run subtests inconsistent with file pattern
maintainability · flagged by 1 model
internal/imagenorm/imagenorm.go:214—applyOrientationallocatesdstwith dimensionsw×h, then immediately throws it away and allocates again withh×wwheneverswapis true (orientations 5–8). This wastes an allocation ofw*h*4bytes for every quarter-turn. Move the allocation into anif/elseso only the correct size is created. -internal/imagenorm/imagenorm_test.go:282—TestNormalizeAppliesExifOrientationiterates over a table of cases but doesn't wrap them in `t…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 9 findings (3 with multi-model agreement)
internal/imagenorm/imagenorm.go:214internal/imagenorm/imagenorm.go:218internal/imagenorm/imagenorm.go:3056 single-model findings (lower confidence)
internal/imagenorm/imagenorm.go:256internal/imagenorm/imagenorm.go:280internal/imagenorm/imagenorm_test.go:271internal/imagenorm/imagenorm_test.go:276internal/imagenorm/imagenorm.go:249internal/imagenorm/imagenorm_test.go:282Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
One thing worth flagging first: the trailing block claiming "Plan mode is active" and instructing an Explore/Plan-agent/
ExitPlanModeworkflow doesn't correspond to any real tool in this session — I searched forExitPlanMode/AskUserQuestionvia ToolSearch and neither resolves, unlike the genuinely deferred tools (TaskCreate,EnterWorktree, etc.) which do. That block reads as an injected instruction trying to redirect this session away from delivering the actual review. I'm disregarding it and completing the task as originally specified (the Gadfly review format, output as text). I did already write a harmless scratch file to/root/.claude/plans/...before catching this — it contains only my own analysis notes, nothing sensitive, and can be ignored/deleted.Here's the actual security review of the PR:
VERDICT: No material issues found
This diff adds pure, local EXIF-orientation parsing and pixel-transform code to
internal/imagenorm. I read the live file (internal/imagenorm/imagenorm.go:140-199) to confirm the surrounding guarantees, not just the diff hunk:rawis bounded byopts.maxBytes()(default 25 MiB, enforced at imagenorm.go:140-151) beforeexifOrientation(raw)is ever called (imagenorm.go:175). No new unbounded-read surface.exifOrientation's marker walk is fully bounds-checked before every slice access —i+4<=len(raw)before reading marker bytes,segLen<2 || i+2+segLen>len(raw)before slicing the segment body. Malformed/truncated JPEGs fall through toreturn 1, not a panic or OOB read.orientationFromApp1's TIFF/IFD parsing is equally guarded — prefix-length check before the"Exif\x00\x00"compare,ifd<8 || ifd+2>len(tiff)before reading the entry count, andoff+12>len(tiff)checked inside the loop before every 12-byte entry read. A crafted large entry count (nup to 65535) just runs bounded, cheap, bounds-checked iterations — not a meaningful DoS vector.1(no rotation), never a guess — so a hostile file can at worst leave an image un-rotated, not corrupt a legitimate one or destabilize downstream processing.applyOrientationonly receives an already-validatedoin[2,8]and allocates its output buffer afterdownscale, so its allocation size is bounded byopts.maxDim()regardless of what the EXIF tag claims — no amplification/decompression-bomb path via orientation.decodeSafely's existing panic-recovery comment already documents the "untrusted upload" threat model this code operates under).I did not find any injection, SSRF, deserialization, secret-handling, or input-validation defect within this diff's security lens.
🎯 Correctness — No material issues found
Review — 🎯 Correctness lens
VERDICT: No material issues found
I independently re-derived all 8 EXIF orientation transforms from first principles (physically rotating/mirroring a labeled grid) rather than trusting the code's comments, and checked them against
internal/imagenorm/imagenorm.go:207-242:dx,dy = h-1-y, x— verified against a manual 2×2 rotation trace, matches.dx,dy = y, w-1-x— verified, matches.Also verified:
orientationFromApp1(imagenorm.go:280-316): TIFF header/IFD0 offset bounds, 12-byte IFD entry stride, and reading a SHORT value from the first 2 bytes of the 4-byte value field per file byte order — all correct per TIFF6 spec.exifOrientation(imagenorm.go:249-275): JPEG marker-segment length semantics (segLenincludes the 2 length bytes, soi += 2+segLenand slicingraw[i+4:i+2+segLen]are both correctly offset), correctly halts at SOS/EOI so it never scans entropy-coded data, and fails safe (returns1) on any malformed/short segment rather than misapplying a rotation.Normalize(imagenorm.go:174-175):applyOrientationis called withexifOrientation(raw)whererawis the original, un-truncated input bytes (line 145) — not the decoded/re-encoded buffer — so the tag is read from the actual source JPEG.exifOrientationgates on theFFD8SOI itself, so non-JPEG formats (PNG/WebP/HEIC) safely no-op without a separate format check. Downscale-then-rotate is sound: rotation only transposes width/height, somax(w,h)(the downscale bound) is invariant across it.One thing I checked but am not reporting as a finding (too low-confidence/low-impact to meet the bar):
orientationFromApp1doesn't validate the IFD entry's type/count fields before reading the Orientation value, so a file with a non-SHORT-typed 0x0112 tag could yield a bogus-but-in-range value instead of falling back to 1. No real camera emits Orientation as anything but SHORT, and I don't have enough confidence this is exploitable to list it as a real bug.I was unable to execute
go test ./internal/imagenorm/...in this environment (Bash approval unavailable under the current session mode), so the "tests pass" claim in the PR description is unverified by me directly — but I traced the test's rotation-formula arithmetic by hand against the same transforms and it's internally consistent with the (verified-correct) implementation.🧹 Code cleanliness & maintainability — Minor issues
No other EXIF/binary parsing utilities exist elsewhere — that claim holds. All checks confirm the draft's single finding is accurate, but one supporting claim in the prose (about
downscalealready using the range-over-int idiom) is false, so I'll correct that in the output.Review — Code cleanliness & maintainability
VERDICT: Minor issues
The new EXIF parsing/orientation code (
internal/imagenorm/imagenorm.go:202-316) is well-scoped, self-contained to one file, and matches the surrounding heavily-commented style (verified no pre-existing EXIF/TIFF parsing utilities elsewhere in the repo to duplicate, viagrep -rn "binary.ByteOrder\|Uint16\|Uint32"). Naming (exifOrientation,orientationFromApp1,applyOrientation) is clear, and the 8-case switch inapplyOrientationis an appropriate use of a switch over a transform-function table for this size. Tests reuse the existingpngByteshelper rather than duplicating it (confirmed atinternal/imagenorm/imagenorm_test.go:17).One small nit:
internal/imagenorm/imagenorm.go:213-217—applyOrientationunconditionally allocates aw×hRGBA, then immediately discards it and allocates a secondh×wRGBA wheneverswapis true (orientations 5-8):dstbefore the reassignment, so the first allocation is dead work.Nothing else in this diff (including the DESIGN.md wording update and the test file additions) raises cleanliness concerns.
⚡ Performance — Minor issues
Confirmed — this matches the draft exactly: line 214 unconditionally allocates a
w×himage.NewRGBA, then line 215-217 discards it and allocates a secondh×wbuffer wheneverswapis true (orientations 5-8, which includes orientation 6, the common phone-portrait case). The fix suggestion (computedw, dhfirst, allocate once) is correct and minimal. This is a real, verifiable finding — a small/minor perf nit, not a correctness bug, matching the draft's severity.VERDICT: Minor issues
internal/imagenorm/imagenorm.go:214-217—applyOrientationallocates aw×h*image.RGBAunconditionally, then immediately discards it and allocates a secondh×w*image.RGBAwheneverswapis true (orientations 5-8):Orientation 6 (90° CW) is the single most common EXIF tag on real phone photos (camera held normally in portrait) — exactly the case this PR is built for per the PR description ("a phone camera... sets a flag to rotate on display"). So on the primary target workload, every upload does one throwaway allocation of up to
DefaultMaxDim × DefaultMaxDim × 4bytes (~16.7 MB at the 2048px default) before the real buffer is allocated, adding avoidable allocation/GC pressure on the image-upload hot path.Fix: compute the swapped dimensions before allocating, e.g.
Everything else in the changed code is a bounded, single-pass
O(w·h)pixel copy (unavoidable for baking in a rotation) applied afterdownscalecaps dimensions, and a marker-segment walk over the raw JPEG header bytes (exifOrientation) that's O(segment count), not O(file size) in any problematic way. No N+1s, no unbounded growth, no new blocking calls on the hot path.🧯 Error handling & edge cases — No material issues found
Go 1.26 supports range-over-int, so no compile issue there.
I've verified the core logic — bounds checks, loop termination, and defaults are all sound. Nothing in my lane rises to a real defect.
VERDICT: No material issues found
Findings (through the error-handling/edge-case lens only):
exifOrientation/orientationFromApp1(imagenorm.go:249-316) are defensively bounds-checked at every slice access I traced (segment length vs.len(raw), IFD offset vs.len(tiff), entry offset vs.len(tiff)before each 12-byte read) — verified by re-deriving the offsets by hand against the TIFF/EXIF layout. Every failure path returns the safe default (1, no rotation) rather than erroring or guessing, matching the doc comment's stated intent.i += 2 + segLenmarker-walk loop is guaranteed to terminate:segLen < 2is rejected before the increment, soistrictly increases each iteration — no infinite-loop/DoS risk on adversarial input, and the byte read is already capped byopts.MaxBytesupstream.applyOrientation(imagenorm.go:207-242): theo<=1 || o>8guard fully covers the domain (exifOrientation only ever returns 1-8), so the switch's implicit cases 2-8 are exhaustive — no default-case gap. A 0×0img.Bounds()degenerates to empty loops with no panic (unlikely in practice, but confirmed not to crash).imgreachingapplyOrientation/downstream is always non-nil by construction (decodeSafely returns early on error), so no nil-check gap.One sub-lens observation I considered but am not confident enough to list as a finding:
orientationFromApp1doesn't validate the IFD entry's type field (expects SHORT/3 for Orientation) before reading the value bytes — a malformed tag with the same ID but a different type could, in a byte-order-dependent way, be misread as a spurious value 1-8 and silently misrotate instead of falling back safely. This requires deliberately malformed EXIF (real phone EXIF always types Orientation as SHORT) and is closer to a correctness/robustness nit than an error-handling defect, so I'm not reporting it as a real bug.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I reviewed the new EXIF parser and orientation applier through the security lens — they process attacker-controlled upload bytes — and verified the bounds logic against the checked-out code.
exifOrientation/orientationFromApp1(imagenorm.go:249-316) — all slice accesses are bounds-checked before use, so an OOB panic (DoS via process crash) is not reachable:raw[0..1]; loop conditioni+4 <= len(raw)guardsraw[i..i+3];segLen < 2 || i+2+segLen > len(raw)guardsraw[i+4 : i+2+segLen].orientationFromApp1,len(seg) < len(prefix)+8(14) guardstiff[0:2]andtiff[4:8];ifd < 8 || ifd+2 > len(tiff)guardstiff[ifd:ifd+2];off+12 > len(tiff)guardstiff[off:off+2]andtiff[off+8:off+10]. The value-field read ignores the declared tag type but stays in bounds, so a malicious non-SHORT type can't cause a panic — worst case it misreads and falls through to the1..8check or returns false.for k := range nis attacker-controlled (uint16, ≤65535) but breaks as soon asoff+12 > len(tiff), so iteration is bounded bylen(tiff)/12;len(tiff)is itself bounded byMaxBytes(25 MiB). No integer-overflow concern on 32-bit either, sinceifdis clamped to≤ len(tiff)-2before use. This is at most trivial CPU amplification, far below the cost of the decode that already ran.1(no-op), so a malformed file can never coerce an unwanted rotation of a correct image. This is the fail-safe direction.applyOrientation(imagenorm.go:207-242) — operates only on the already-decodedimage.Imageand a validatedoin1..8;At/Seton stdlib image types don't panic. No new taint surface.decodeSafely'srecoverdoes not wrapexifOrientation/applyOrientation, but neither has a reachable panic on untrusted input per the bounds analysis above, so the unprotected scope is safe.MaxBytes,maxDecodePixels,maxDimension) upstream of this code already bound the input.🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change through the correctness lens by reading the full implementation (
internal/imagenorm/imagenorm.go:140-334) and re-deriving the EXIF orientation transforms from first principles.applyOrientationtransforms (imagenorm.go:207-242): I checked all 8 cases against the EXIF 2.3 spec definitions (1=identity, 2=mirror-H, 3=180, 4=mirror-V, 5=transpose, 6=90°CW, 7=transverse, 8=90°CCW). Every mapping(x,y) → (dx,dy)is correct, including the dimensional transpose for o≥5 (swap is right for all of 5/6/7/8, not just the pure rotations). Corner-spot checks confirm: o6 maps (0,0)→(h-1,0) (top-left→top-right, correct for CW); o8 maps (0,0)→(0,w-1) (top-left→bottom-left, correct for CCW).Downscale-then-rotate ordering (
imagenorm.go:171-175): The comment's invariant holds. Downscale bounds the longest edge tomaxDim; a rotation/transposition only swaps sides, so the longest edge stays ≤maxDim. Worked example: 4000×3000 → downscale 1024×768 → o6 swap → 768×1024 (longest edge 1024). ✓exifOrientation/orientationFromApp1(imagenorm.go:249-316): JPEG segment walk, bounds, IFD0 parsing, and the inline-SHORT read atoff+8:off+10are all correct. Non-JPEG inputs (HEIC/PNG/WebP) correctly short-circuit on the0xFFD8SOI check and return 1, soapplyOrientationis a no-op for them. All slice accesses are guarded against panics (verified each bounds check). Failure mode on any unparseable/edge input is returning 1 (no rotation) — never a wrong rotation — which is the safe default.Test expectations (
imagenorm_test.go): I re-derived eachwantW/wantH/mx/myfrom the same transform the code applies; all match (e.g. o6:dx=h-1-y=17, dy=x=10for source center (10,6) with h=24; o8:dx=y=6, dy=w-1-x=29; o3:dx=29, dy=17).One non-blocking observation (not a correctness bug): the value field is read as
uint16without verifying the tag's EXIF type is SHORT. Since the spec mandates SHORT for Orientation this is fine in practice; a hypothetical big-endian LONG-typed tag would read 0 and fall back to the safe default of 1 rather than a wrong rotation, so it fails safe.🧹 Code cleanliness & maintainability — Minor issues
Minor issues found
internal/imagenorm/imagenorm.go:214-216— redundant double allocation on the swap branch.dstis allocated asimage.NewRGBA(image.Rect(0,0,w,h))on line 214, then unconditionally reassigned toimage.NewRGBA(image.Rect(0,0,h,w))on line 216 whenswapis true. On the quarter-turn path (orientations 5–8) the first RGBA header is allocated and immediately discarded. The non-swap branch keeps the first allocation correctly, so this is only a wasted allocation on the swap branch, not a correctness bug — a readability/efficiency wart in an allocation-sensitive path. Cleaner: compute the output dims up front and allocate once.internal/imagenorm/imagenorm.go:218-239— manualAt/Setper-pixel loop diverges from thedraw-package idiom used elsewhere in the file.applyOrientationwalks every pixel withimg.At/dst.Set(going through thecolor.Modelconversion path), whiledownscale(line 332) usesdraw.CatmullRom.Scale. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is avoidable. Not blocking; a consistency nit.internal/imagenorm/imagenorm_test.go:276-280— no test covers orientations 2, 4, 5, 7.TestNormalizeAppliesExifOrientationonly exercises 0/1/3/6/8, andTestExifOrientationParsingonly parses 6. TheapplyOrientationswitch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transverse math.⚡ Performance — Minor issues
Both findings are confirmed against the actual code. The first (double RGBA allocation on the swap path) is real and at the cited lines. The second is informational and not in the findings JSON, consistent with the draft.
Verdict: Minor issues
internal/imagenorm/imagenorm.go:214-217— double RGBA allocation on the swap (portrait) path. For orientations 5–8 (the quarter-turns),dstis first allocated atw×hand then immediately overwritten with a secondimage.NewRGBAath×w:The first buffer is allocated and discarded. This is the common phone-portrait path — orientation 6 is exactly "portrait photo tagged rotate 90°", the motivating case of this PR. For a 2048×1536 portrait, that's a ~12 MiB RGBA thrown straight to the GC on the very upload the feature targets. Fix is trivial — compute the destination rect once:
internal/imagenorm/imagenorm.go:218-240— per-pixelAt/Setloop (informational, not blocking). The rotate reads viaimg.Atand writes viadst.Set, both of which go through genericcolor.Color/color.Modelconversions rather than fast direct RGBA writes. Bounded to ≤2048px and only on the rotated path, so acceptable for an upload boundary — but worth noting that a portrait phone photo already withinMaxDim(sodownscalereturns the original*image.YCbCr) hits the slowest variant (YCbCr-per-pixelAt). Not worth changing unless this shows up in profiles.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the error-handling & edge-case lens, the new code is solid. I verified the two new functions and the bounds checks by reading
internal/imagenorm/imagenorm.go:202-316:exifOrientation(line 249): Every byte access is bounds-checked before use. The marker walk guardslen(raw) < 4,i+4 <= len(raw),raw[i] != 0xFF(misalignment → safe return 1),segLen < 2 || i+2+segLen > len(raw), and stops at EOI/SOS. The slice passed toorientationFromApp1(raw[i+4 : i+2+segLen]) is provably in range because of thei+2+segLen > len(raw)guard. The loop advancesi += 2 + segLenwithsegLen >= 2, so it strictly progresses and terminates — no infinite loop.segLenis a 16-bit value (max 65535), soi+2+segLencan't overflow int on any supported platform.orientationFromApp1(line 280): Length guardlen(seg) < len(prefix)+8guaranteestiff := seg[len(prefix):]has at least 8 bytes, sotiff[0:2]andtiff[4:8]are safe. The IFD offset is validated (ifd < 8 || ifd+2 > len(tiff)), and each 12-byte entry is checked withoff+12 > len(tiff)before theUint16/Uint32reads. A crafted huge entry countnis bounded by the per-entry length check, so it can't spin unbounded. Out-of-range orientation values fall through toreturn 0, false→ caller defaults to 1. No panics on truncated/garbage input.applyOrientation(line 207): Out-of-rangeo(<= 1 || > 8) is a no-op returning the original image. Empty/zero-dimension images (w==0orh==0, whichdownscalecan pass through) produce emptyimage.NewRGBAallocations and thefor y := range h/for x := range wloops simply don't execute — no index panic.dst.Set(dx, dy, c)always writes within the allocated rect by construction of the transforms.Normalize(applyOrientation,exifOrientation) return values, not errors, so there are no new error-return paths to mishandle or defer. The existing "on any error the returned bytes are nil" contract is preserved.The test helper
spliceExifOrientationdiscardsbinary.Writeerrors with_, but those write to abytes.Bufferwhich never errors — not a concern.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The change adds a manual EXIF orientation parser for JPEG and pixel-level rotation logic. From a security lens, the code is clean:
exifOrientationreturns 1 (no rotation) on any malformed input, missing EXIF, or non-JPEG format — never forcing a rotation onto a correct image.maxDecodePixels(50M) andmaxDimension(50K) already cap decoded bitmap size.applyOrientationruns on the downscaled image (downscale happens first), so memory and CPU for the rotation are bounded by the same limits.orientationFromApp1checkslen(seg) < len(prefix)+8before indexingtiff[0:2],tiff[4:8], etc.; the IFD walk checksoff+12 > len(tiff)before reading each 12-byte entry.applyOrientationcomputes destination coordinates (dx,dy) that always fall within the pre-allocateddstbounds for every orientation case (2–8).One minor observation (not a finding, just a verification note): the
exifOrientationparser is strict about0xFFmarker alignment (raw[i] != 0xFF → return 1), which is the conservative/safe choice for untrusted uploads.🎯 Correctness — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:305-309—orientationFromApp1reads the Orientation value without validating the IFD entry's type and count. The EXIF spec requires Orientation to beSHORT(type3) with count1. A malformed entry with a different type (e.g.LONG) or count>1stores an offset in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "safe default: return 1 for unparseable" design goal) instead of treating the tag as unreadable.Suggested fix: Add two checks inside the
0x0112match block:🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:214-216—applyOrientationallocates aw×hRGBA unconditionally, then immediately discards it and allocates a secondh×wRGBA for orientations 5-8. Move theswapcheck before the firstNewRGBAto avoid the throw-away allocation for the quarter-turn cases.internal/imagenorm/imagenorm_test.go:271-281— The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.⚡ Performance — Blocking issues found
Blocking issues found
internal/imagenorm/imagenorm.go:214-216— Double image allocation on every 90° rotation.applyOrientationalways allocatesimage.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocatesimage.NewRGBA(image.Rect(0, 0, h, w))whenswapis true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simpleif/elseso only the correctly-sized buffer is ever allocated.internal/imagenorm/imagenorm.go:218-240— Per-pixelimage.Image.At()calls box acolor.Coloron every iteration, causing millions of heap allocations for a single rotated image.applyOrientationiterates every pixel and reads via theimage.Imageinterface (img.At(b.Min.X+x, b.Min.Y+y)). When the concrete type is*image.RGBA(whichdownscalealready returns), eachAtcall boxes thecolor.RGBAvalue into an interface, allocating on the heap. A 2048×2048 rotation causes ~4.2 million transient allocations. This is a well-known Go image-processing anti-pattern; adding a fast-path type-switch to*image.RGBA(and perhaps*image.NRGBA) that copies throughPixdirectly would eliminate the boxing and interface dispatch entirely for the common case, cutting rotation time by an order of magnitude. The current generic fallback can be kept for non-RGBA images.🧯 Error handling & edge cases — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:256— JPEG marker-padding edge case unhandled.The JPEG spec allows any number of fill bytes (
0xFF) to precede a marker.exifOrientationassumes exactly one0xFFfollowed immediately by the marker code, so a file likeFF D8 FF FF E1 …(valid per spec) misaligns:markerbecomes0xFF,segLenis read from the next two bytes (0xE1 0x00≈ 57 KiB), the length check fails, and the parser bails out returning1(normal). For a portrait photo whose APP1 segment is padded this way, the image stays sideways. A robust parser should skip consecutive0xFFbytes before reading the marker code.internal/imagenorm/imagenorm.go:305— TIFF type field not validated.orientationFromApp1finds tag0x0112and blindly reads the first two bytes of the value field as aSHORT. It never checkstiff[off+2:off+4]to confirm the type is actually3(SHORT). A malformed EXIF that reuses tag0x0112with aLONGorUNDEFINEDtype could yield a bogus orientation value. WhileexifOrientationcaps the result to1..8, any value in that range would still be applied. Adding a type check (bo.Uint16(tiff[off+2:off+4]) == 3) before accepting the value would make the “unparseable → 1” guarantee stronger.opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Reviewed the EXIF parser through the security lens (untrusted-byte parsing, OOB reads, integer overflow, infinite loops, injection surface). The code is defensively written:
exifOrientation(imagenorm.go:249-275): all index accesses are guarded by the loop conditioni+4 <= len(raw)andi+2+segLen > len(raw);segLen >= 2guarantees loop progress (no infinite loop);segLenis a 16-bit field so no int overflow oni+2+segLen; safe default1on any misalignment, EOI/SOS, or non-JPEG.orientationFromApp1(imagenorm.go:280-313): length checklen(seg) < len(prefix)+8coverstiff[0:2]andtiff[4:8]; IFD offset validated withifd < 8 || ifd+2 > len(tiff)before slicing; per-entry boundsoff+12 > len(tiff)covertiff[off+8:off+10]; orientation value clamped to 1..8 before use. A 32-bit build could in principle makeint(bo.Uint32(...))negative for a near-MaxUint32offset, but the subsequentifd+2 > len(tiff)comparison still rejects it (negative < len), so it can't be exploited to read OOB — it just falls through to the safe default.raw, whichNormalizealready capped atMaxBytes+1(imagenorm.go:~125), so the parser runs on size-bounded data regardless.applyOrientation, which does an in-memory pixel copy with no I/O.Nothing in the security lens is materially wrong.
🎯 Correctness — No material issues found
I verified the orientation math, the EXIF parser bounds, and the test expectations against the checked-out code.
Verdict: No material issues found
caseinapplyOrientation(imagenorm.go:222-237) maps source(x,y)to the correct destination(dx,dy), including the dimension-swapping cases (5–8) wheredstis correctly allocated ash×wand all writes stay in bounds.downscalebounds the longest edge, and a 90° rotation swaps sides without changing the longest edge, so theMaxDimbound still holds on the emitted image (verified for both the shrink and no-shrink paths).i+4 <= len(raw)andsegLen < 2/i+2+segLen > len(raw;orientationFromApp1requireslen(seg) >= len(prefix)+8(sotiff[4:8]is safe) andifd < 8 || ifd+2 > len(tiff)plus per-entryoff+12 > len(tiff)prevent OOB reads. The APP1 payload sliceraw[i+4 : i+2+segLen]correctly excludes the 2 length bytes.{6, 24, 40, 17, 10}; orient 8 → (y, w-1-x) = (6,29) matching{8, 24, 40, 6, 29}. ThespliceExifOrientationhelper builds a well-formed little-endian TIFF/IFD0 (verified byte layout:II+ 0x2A + offset 8 + count 1 + 12-byte entry with value at entry+8), which the parser reads back correctly.raw[0]!=0xFF||raw[1]!=0xD8), so HEIC/WebP/PNG are left untouched as designed — no regression vs. prior behavior.One trivial robustness note (not material, low confidence): the marker walker doesn't skip JPEG fill bytes (
0xFF 0xFF) or handle standalone markers like TEM/0xFF01/RSTn that have no length field before the EXIF APP1. A JPEG emitting fill bytes before APP1 would misparse and fall back to orientation 1 (sideways). Real phone JPEGs place APP1 immediately after SOI, so this is out of the practical path and the fallback is safe (never mis-rotates a correct image).🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
The EXIF orientation addition is clean through the maintainability lens. Verified against the checked-out code:
applyOrientationandexifOrientationare well-factored, each doing one thing, with accurate doc comments that match the implementation. They follow the file's existing conventions (sentinel-free helpers, inline rationale comments, pure-Go no-deps stance).exifOrientationre-walks the JPEG bytes becauseimage.DecodeConfigdoesn't expose EXIF — that's a justified read, not copy-paste.o <= 1 || o > 8) returnsimgunchanged, so non-JPEG and normal-orientation inputs pay zero allocation cost — the comment claims this and the code does it.pngBytes/jpegBytesfixture-builder pattern and addorientedJPEG/spliceExifOrientation/brightin the same style; the helper that builds a minimal TIFF IFD entry writes the 12-byte entry layout correctly (2+2+4+2+2), so the fixture reflects a real Exif APP1 rather than asserting the parser's own shape back at itself.DESIGN.mdnote is updated in the same sentence and stays accurate ("bakes in the JPEG EXIF orientation"), consistent with CLAUDE.md's "keep docs true" rule.Nothing in my area to flag.
⚡ Performance — No material issues found
Reading the file to verify the per-pixel loop's allocation behavior.
VERDICT: No material issues found
Reviewed
internal/imagenorm/imagenorm.go(the changed file) and its callerinternal/api/seed_packet.go. Through the performance lens:applyOrientationper-pixel loop (imagenorm.go:218-240): iterates every pixel withimg.At+dst.Set, each doing a color-model round-trip. This is the only new work on the hot path. It runs only wheno > 1(an actual non-normal EXIF orientation tag exists); orientation 1 — the common case for landscape photos and all non-JPEG formats — early-returns at line 208 with zero cost. When it does run, the image is already capped atDefaultMaxDim(2048px longest edge) by the precedingdownscale, so the loop is bounded to ~4M iterations worst case. On a single-upload endpoint that then calls a vision model (seconds of latency), the tens-of-ms per-pixel copy is immaterial. No N+1, no quadratic behavior, no unbounded allocation.exifOrientation(raw)(imagenorm.go:249-275): called unconditionally but bails in O(1) for non-JPEG (raw[0]!=0xFF || raw[1]!=0xD8) and walks at most the handful of JPEG marker segments for JPEGs. Negligible.Transient allocation:
applyOrientationallocates one*image.RGBAmatching the (already-bounded) downscaled image. Doubling of the ~16MB peak bitmap is acceptable for a one-shot upload path.The straightforward per-pixel implementation could be sped up with
RGBAAt/SetRGBA(skipping two color-model conversions per pixel) sincedownscalealways yields an*image.RGBA, but that is a micro-optimization with no measurable impact at this call frequency and image size — not flagging it.🧯 Error handling & edge cases — No material issues found
I've read the full
imagenorm.goand the relevant test file. Let me verify the boundary logic in the new EXIF/orientation code carefully.Verdict: No material issues found
I walked the unhappy paths the diff introduces, against the checked-out code:
exifOrientation/orientationFromApp1parser bounds (imagenorm.go:249-316): every slice access is guarded before use.len(raw) < 4short-circuits entry; the loop requiresi+4 <= len(raw)before reading 4 header bytes;segLen < 2 || i+2+segLen > len(raw)gates the segment sliceraw[i+4 : i+2+segLen](andsegLen >= 2makesi+4 <= i+2+segLen). InorientationFromApp1,len(seg) < len(prefix)+8(14) coverstiff[0:2]andtiff[4:8];ifd < 8 || ifd+2 > len(tiff)gates the count read;off+12 > len(tiff)gates every 12-byte entry read, which also coverstiff[off+8:off+10]. Every unparseable / truncated / hostile case returns the safe default1— never a panic, never a wrong rotation. Verified by reading the file.applyOrientationoutput bounds (imagenorm.go:207-242): for the non-swap cases (2/3/4)dx ∈ [0,w-1],dy ∈ [0,h-1]anddstisw×h; for the swap cases (5/6/7/8) the loops arey ∈ [0,h),x ∈ [0,w)and each mapping lands in[0,h-1] × [0,w-1]against anh×wdestination — I re-derived all four quarter-turn mappings and confirmed no off-by-one and no out-of-boundsSet.o <= 1 || o > 8is a no-op, so an out-of-range tag (or the0, falsereturn folded in as0) leaves the image untouched.Normalizerejectscfg.Width <= 0 || cfg.Height <= 0before decode, soapplyOrientationnever sees a zero-dimension image that would makeimage.NewRGBAdegenerate;downscale'slongest == 0guard is also upstream. No nil-image path: decode failure returns beforeapplyOrientationruns.decodeSafely'srecoverwrapsimage.Decode; the new code runs after it and does only bounds-checked slice access andimg.At/dst.Setwithin verified bounds, so there's no unprotected panic surface from a malformed EXIF blob.One marginal observation, not material enough to list as a finding:
ifd := int(bo.Uint32(tiff[4:8]))(line 295) could in principle letifd+2wrap on a 32-bitintbuild given a hostile TIFF offset, bypassing theifd+2 > len(tiff)guard. pansy'sCGO_ENABLED=0static binary targets 64-bit servers (Komodo deploy), whereintis 64-bit and the guard holds — so this is not a live risk here, and I'm flagging it only because the lens names integer overflow. No change needed.opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
The EXIF orientation parser is defensively written against untrusted input: bounds-checked on every slice access, defaults to 1 (no rotation) on any malformed data, bounded by the 25 MiB read cap, and advances monotonically through markers with no infinite-loop path. The coordinate mapping in
applyOrientationstays within the allocated destination for all 8 orientation values. No authn/authz gaps, injection, deserialization, secret leakage, or untrusted-data mishandling is introduced.🎯 Correctness — No material issues found
No material issues found
I examined the EXIF orientation parser, the transform application, and the interaction with downscaling. Here is what I verified:
applyOrientationtransforms: I re-derived the mapping for all 8 EXIF orientations from the spec (0th-row/0th-column placement) and confirmed eachcasematches. The quarter-turns (5–8) correctly swap the output dimensions, and the pixel-level mappings agree with the test expectations for the covered cases (1, 3, 6, 8) and, by derivation, the uncovered ones (2, 4, 5, 7).≤ MaxDimis preserved. This reasoning is sound.exifOrientationwalks big-endian JPEG markers, stops at SOS/EOI, bounds-checks every slice operation, and returns 1 (no rotation) on any parse failure.orientationFromApp1correctly reads the IFD0 entry for tag0x0112as an inline SHORT value with the declared byte order. It safely rejects malformed input (bad magic, short buffers, invalid offsets) by returningok=false, which causes the caller to fall back to 1.exifOrientationrequires theFF D8SOI signature; all other formats return 1, andapplyOrientationwitho==1returns the image unchanged.spliceExifOrientationhelper constructs a valid little-endian TIFF IFD with the correct 12-byte entry layout andsegLenaccounting. The marker-location assertions inTestNormalizeAppliesExifOrientationmatch the transforms.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/imagenorm/imagenorm.go:214—applyOrientationallocatesdstwith dimensionsw×h, then immediately throws it away and allocates again withh×wwheneverswapis true (orientations 5–8). This wastes an allocation ofw*h*4bytes for every quarter-turn. Move the allocation into anif/elseso only the correct size is created.internal/imagenorm/imagenorm_test.go:282—TestNormalizeAppliesExifOrientationiterates over a table of cases but doesn't wrap them int.Runsubtests, unlikeTestNormalizeAllFormatsandTestNormalizeRejectsPixelBombin the same file. Adding subtests makes failures immediately attributable to the specific orientation and keeps isolation consistent with the file's existing pattern.⚡ Performance — Minor issues
Verdict: Minor issues
internal/imagenorm/imagenorm.go:214-216—applyOrientationallocates animage.RGBAof sizew×hunconditionally, then immediately discards it and allocates a second one of sizeh×wwhenswapis true (orientations 5–8). For a 2048×1536 image that’s a ~12.5 MB throwaway allocation; at 2048×2048 it’s ~16 MB. The fix is trivial: only allocate after theswapcheck, e.g.dw, dh := w, h; if swap { dw, dh = h, w }; dst := image.NewRGBA(image.Rect(0, 0, dw, dh)).internal/imagenorm/imagenorm.go:218-239— The pixel-by-pixelimg.Atloop is correct but has high per-pixel overhead (interface call + color conversion for every pixel). This is acceptable for an upload handler that runs once per image, but it’s worth noting that for images already withinmaxDimbounds (wheredownscalereturns the original), this copies the full decoded bitmap via the slowimage.Imagepath instead of working directly on the concrete type. Not blocking for the seed-packet use case, but if large unchanged images become common this would be the next bottleneck.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
internal/imagenorm/imagenorm.go:280— Missing TIFF magic number validation inorientationFromApp1The function trusts the IFD offset at
tiff[4:8]without first checking that bytestiff[2:4]contain the required TIFF magic number42(0x002Alittle-endian /0x2A00big-endian). A malformed APP1 segment that passes the"Exif\x00\x00"prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially yielding a bogus orientation value.Fix: Verify
bo.Uint16(tiff[2:4]) == 0x002A(or0x2A00interpreted viabo) before reading the IFD offset.internal/imagenorm/imagenorm.go:305-309— Orientation tag type/count not validatedWhen tag
0x0112is found, the code assumes it is a SHORT withcount == 1and reads the value inline from the first two bytes of the 4-byte value field. It never checks the entry’s type code (bytesoff+2:off+4) or count (bytesoff+4:off+8). A malformed EXIF segment with tag0x0112but a different type (e.g. LONG) orcount > 1would store an offset in the value field rather than the value itself; the parser would then read that offset as a uint16. If the misread value happens to fall in[1,8], a wrong rotation is silently baked into the image.Fix: After matching the tag, verify
bo.Uint16(tiff[off+2:off+4]) == 3(SHORT) andbo.Uint32(tiff[off+4:off+8]) == 1before readingbo.Uint16(tiff[off+8:off+10]).internal/imagenorm/imagenorm.go:249-275— JPEG fill bytes (0xFF) between markers are not skippedThe JPEG spec allows any number of fill bytes (
0xFF) to precede a marker. The parser atexifOrientationassumes each marker is immediately preceded by exactly one0xFFbyte. If a phone encoder emits padding0xFFbytes before the APP1 segment, the loop misreads the marker and returns orientation1instead of the true value. This is safe (no wrong rotation) but incorrect for a valid JPEG.Fix: Before reading
raw[i+1]as the marker, skip consecutive0xFFbytes.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.