fix(avatar): cap manual avatar file reads - #11
Open
SebTardif wants to merge 1 commit into
Open
Conversation
ReadPath used unbounded io.ReadAll, so `clawdex person avatar set PERSON ~/huge.bin` could exhaust memory before image decode. Cap user-selected reads at 10 MiB, the same limit as Google import avatars, and reject oversize sources with a clear error. Stored avatar inspect uses the same ceiling. Rooted ReadFile stays uncapped for vcard and other repo files. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 4, 2026, 4:56 AM ET / 08:56 UTC. ClawSweeper reviewWhat this changesThe PR limits manual and stored avatar reads to 10 MiB, adds boundary tests, and documents oversized-source rejection. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 3 items remain Current main still needs the read limit, but the previously reported Doctor upgrade regression remains on the unchanged PR head. Priority: P2 Review scores
Verification
How this fits togetherClawdex stores contact avatars beside Markdown records containing their metadata. Manual avatar commands read image files, while Doctor inspects stored images and repairs the corresponding records. flowchart TD
A[Manual image selection] --> C[Safe file reader]
B[Stored avatar checked by Doctor] --> C
C --> D{Within size limit?}
D -->|Yes| E[Inspect image metadata]
D -->|No| F[File too large error]
E --> G[Contact Markdown record]
F --> H[Doctor repair error handling]
H --> G
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Keep reads bounded while reporting oversized stored avatars without clearing their metadata, supported by upgrade-preservation coverage. Do we have a high-confidence way to reproduce the issue? Yes, from source: manual reads on current main are unbounded, and this head makes a pre-existing avatar above 10 MiB reach Doctor's metadata-clearing branch. No reviewer-side runtime reproduction was executed. Is this the best way to solve the issue? Partly: the bounded reader fits the memory problem, but applying it to stored avatars requires distinguishing size rejection from errors that justify clearing metadata. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against baa945d86da0. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
clawdex person avatar set PERSON FILEreads the source image withsafefile.ReadPath, which used unboundedio.ReadAll. A multi-gigabytefile is copied into memory for SHA256 and MIME sniffing before any image
decode. The process can run out of memory.
--dry-runuses the same read.Google import already stops avatar downloads at 10 MiB
(
maxAvatarBytesininternal/google/gog.go). Manual set did not.Evidence
Built
clawdexfrom this branch to/tmp/clawdex-f004-bin. Initialized afresh contacts repo, added
Proof Person, then pointed avatar set at a10485761-byte file (10 MiB plus 1).
Unpatched
ReadPathloaded the whole file (10485761 bytes, no error).After the patch the public command fails closed:
A 69-byte PNG still sets normally (
avatars/avatar.png,image/png, 1x1).Real behavior proof
Behavior or issue addressed: Manual avatar set and dry-run no longer read an oversized source into memory. They reject files above 10 MiB, the same cap as Google import avatars.
Real environment tested: macOS Darwin arm64, Go 1.27.0, clawdex built from this branch to
/tmp/clawdex-f004-bin, isolated--configat/tmp/clawdex-f004-proof/config.toml.Exact steps or command run after this patch:
Evidence after fix: terminal output from the patched binary:
Observed result after fix: Oversized sources return
file too largeand exit 1. A valid small PNG still writesavatars/avatar.png.What was not tested: Apple Contacts thumbnail import of a source larger than 10 MiB (thumbnails are small). Windows ReadPath.