fix(mail): restore CID validation and stale PartID lookup lost in revert#230
fix(mail): restore CID validation and stale PartID lookup lost in revert#230
Conversation
…ert (#199) The revert of PR #81 (eda2b9c) also removed two independent bugfixes: 1. CID character validation in newInlinePart — reject spaces, tabs, angle brackets, and parentheses to prevent malformed MIME output. 2. Stale PartID lookup in validateInlineCIDAfterApply and validateOrphanedInlineCIDAfterApply — use findPrimaryBodyPart by media type instead of findPart by PrimaryHTMLPartID, which can go stale when ops restructure the MIME tree.
📝 WalkthroughWalkthroughStricter inline CID validation was added and post-apply HTML-part lookup was changed: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
domain/mail
Greptile SummaryThis PR restores two specific bugfixes that were lost in the revert of the auto-resolve feature (PR #199): CID character validation to reject spaces, tabs, angle brackets, and parentheses in Confidence Score: 5/5Safe to merge — changes are tightly scoped restorations of previously shipped bugfixes with full test coverage. Both changes (CID character validation and media-type-based HTML part lookup) are minimal, targeted, and well-tested. No new logic is introduced beyond what existed before the revert, and all edge cases are exercised by the three new test functions. No P1 or P0 issues found. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Apply Patch Op] --> B{Op type?}
B -->|add_inline| C[newInlinePart]
B -->|replace_inline| D[replaceInline]
C --> E[RejectCRLF check]
D --> F[RejectCRLF check]
E --> G["ContainsAny check\n(spaces, tabs, <>, ())"]
F --> H["ContainsAny check\n(spaces, tabs, <>, ())"]
G -->|invalid chars| I[Return error]
H -->|invalid chars| I
G -->|valid| J[Add part to MIME tree]
H -->|valid| K[Update part in MIME tree]
J --> L[validateInlineCIDAfterApply]
K --> L
L --> M["findPrimaryBodyPart(root, 'text/html')\n(by media type — not stale PartID)"]
M --> N[Check all CID refs resolve]
N --> O[validateOrphanedInlineCIDAfterApply]
O --> P["findPrimaryBodyPart(root, 'text/html')"]
P --> Q[Check no inline MIME part is orphaned]
Q -->|orphan found| R[Return orphaned cids error]
Q -->|ok| S[Success]
Reviews (3): Last reviewed commit: "fix(mail): add CID character validation ..." | Re-trigger Greptile |
…ookup - TestAddInlineRejectsInvalidCharactersInCID: verify spaces, tabs, embedded angle brackets, and parentheses in CID are rejected. - TestValidateInlineCIDAfterSetBody: verify inline CID validation works correctly after set_body restructures the MIME tree (covers the findPrimaryBodyPart fix for stale PartID).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/mail/draft/patch.go (1)
608-611:⚠️ Potential issue | 🟡 MinorMissing CID character validation in
replaceInline.
newInlinePartnow validates CID characters (lines 765-767), butreplaceInlineonly checks for CRLF. When a new CID is provided toreplace_inline, it bypasses the stricter validation thatadd_inlineenforces. Both operations receive CID from the same untrusted JSON input.Proposed fix to add consistent validation
finalCID := strings.Trim(strings.TrimSpace(cid), "<>") if err := validate.RejectCRLF(finalCID, "inline cid"); err != nil { return err } + if strings.ContainsAny(finalCID, " \t<>()") { + return fmt.Errorf("inline cid %q contains invalid characters (spaces, tabs, angle brackets, or parentheses are not allowed)", finalCID) + } if err := validate.RejectCRLF(fileName, "inline filename"); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/draft/patch.go` around lines 608 - 611, The replaceInline function currently only trims and checks CRLF on finalCID but skips the CID-character validation performed in newInlinePart; update replaceInline to call the same CID character validation used in newInlinePart (after computing finalCID and before using it) so new CID values go through identical checks—i.e., add the same validate.Reject.../CID-character validation call (the one used in newInlinePart) right after finalCID is computed (replaceInline) and return the error if validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/mail/draft/patch.go`:
- Around line 608-611: The replaceInline function currently only trims and
checks CRLF on finalCID but skips the CID-character validation performed in
newInlinePart; update replaceInline to call the same CID character validation
used in newInlinePart (after computing finalCID and before using it) so new CID
values go through identical checks—i.e., add the same
validate.Reject.../CID-character validation call (the one used in newInlinePart)
right after finalCID is computed (replaceInline) and return the error if
validation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bba901a-2b9e-401d-9041-d6ab425a574d
📒 Files selected for processing (1)
shortcuts/mail/draft/patch.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@bcd7805b7435647e36e8ccf1287298bfa5dea471🧩 Skill updatenpx skills add larksuite/cli#fix/restore-cid-validation-and-stale-partid -y -g |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/mail/draft/patch_test.go (1)
648-650: Strengthen error message assertion for consistency with similar tests.The test only verifies that an error occurred, but doesn't confirm it's the expected "orphaned cids" error. This is weaker than the analogous assertion at line 487 in
TestApplySetBodyOrphanedInlineCIDIsRejected. If a different validation fails unexpectedly, this test would still pass.💡 Suggested improvement
- if err == nil { - t.Fatal("expected orphaned inline error after set_body dropped CID reference, got nil") + if err == nil || !strings.Contains(err.Error(), "orphaned cids") { + t.Fatalf("expected orphaned cid error, got: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/draft/patch_test.go` around lines 648 - 650, The test currently only checks that err is non-nil; update it to assert the error message is the expected orphaned-CID validation error (as done in TestApplySetBodyOrphanedInlineCIDIsRejected). Locate the failing assertion around err in TestApplySetBodySetBodyDroppedCID (or the current test function) and replace the simple nil check with an assertion that err.Error() (or use require/assert helpers) contains or equals the "orphaned cids" message used in the other test so the test fails if a different validation error occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/mail/draft/patch_test.go`:
- Around line 648-650: The test currently only checks that err is non-nil;
update it to assert the error message is the expected orphaned-CID validation
error (as done in TestApplySetBodyOrphanedInlineCIDIsRejected). Locate the
failing assertion around err in TestApplySetBodySetBodyDroppedCID (or the
current test function) and replace the simple nil check with an assertion that
err.Error() (or use require/assert helpers) contains or equals the "orphaned
cids" message used in the other test so the test fails if a different validation
error occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f9d47e2-82ec-4c91-9c0a-d96045218d4d
📒 Files selected for processing (1)
shortcuts/mail/draft/patch_test.go
haidaodashushu
left a comment
haidaodashushu
left a comment
There was a problem hiding this comment.
One blocking issue:
replace_inline still accepts CID values that this PR now rejects in add_inline, so the fix is only partial and behavior becomes inconsistent.
In shortcuts/mail/draft/patch.go, replaceInline still validates only CR/LF before writing part.ContentID, while the new invalid-character guard was added only in newInlinePart. That means a caller can still set CID: "my logo" or CID: "img(1)" through replace_inline and bypass the new restriction entirely.
The new tests also only cover add_inline; the existing replace_inline tests still check only CR/LF, so this gap would not be caught.
Suggested fix: extract CID validation into a shared helper and use it from both add_inline and replace_inline, then add a replace_inline test for the same invalid-character cases.
…en test assertions Address CR feedback: 1. Add the same CID character validation (spaces, tabs, angle brackets, parentheses) to replaceInline, matching the check in newInlinePart. Previously replace_inline could bypass the restriction. 2. Strengthen orphaned CID test assertion to check for specific "orphaned cids" error message, not just non-nil error. 3. Add TestReplaceInlineRejectsInvalidCharactersInCID to cover the new validation in replace_inline.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/mail/draft/patch.go (2)
909-914:⚠️ Potential issue | 🟠 MajorOrphan check should only evaluate inline MIME parts, not every
Content-IDpart.Line 909-Line 914 currently marks any
ContentIDpart as orphan-candidate, which can falsely flag primary HTML/body parts that carryContent-IDbut are not inline attachments.Suggested fix
for _, part := range flattenParts(snapshot.Body) { - if part == nil || part.ContentID == "" { + if part == nil || !strings.EqualFold(part.ContentDisposition, "inline") || strings.TrimSpace(part.ContentID) == "" { continue } - if !refSet[strings.ToLower(part.ContentID)] { - orphaned = append(orphaned, part.ContentID) + cid := strings.Trim(part.ContentID, "<>") + if !refSet[strings.ToLower(cid)] { + orphaned = append(orphaned, cid) } }Based on learnings: In
shortcuts/mail/draft/patch.go, orphan-removal candidates must be constrained toContent-Disposition: inlineplus non-emptyContent-ID; treating anyContentIDpart as inline is a regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/draft/patch.go` around lines 909 - 914, The orphan detection currently treats any MIME part with a non-empty ContentID as an orphan candidate; change the check in the loop over flattenParts(snapshot.Body) so it only considers parts that have a non-empty ContentID AND have Content-Disposition indicating inline (e.g., ContentDisposition == "inline", case-insensitive or via provided enum/helper) before comparing against refSet and appending to orphaned; update the condition around part.ContentID to also verify part.ContentDisposition is inline to avoid flagging primary HTML/body parts.
880-888:⚠️ Potential issue | 🟠 MajorRestrict CID resolution to inline parts only in post-apply validation.
validateInlineCIDAfterApplysays references must resolve to actual inline MIME parts, but Line 880-Line 885 accepts any part withContentID. That can incorrectly pass when a non-inline part shares a CID.Suggested fix
- for _, part := range flattenParts(snapshot.Body) { - if part == nil || part.ContentID == "" { + for _, part := range flattenParts(snapshot.Body) { + if part == nil || !strings.EqualFold(part.ContentDisposition, "inline") || strings.TrimSpace(part.ContentID) == "" { continue } - cids[strings.ToLower(part.ContentID)] = true + cids[strings.ToLower(strings.Trim(part.ContentID, "<>"))] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/draft/patch.go` around lines 880 - 888, The current CID collection loop accepts any part with a ContentID, which can include non-inline attachments; update the loop in validateInlineCIDAfterApply to only register CIDs from inline MIME parts by checking the part's inline/disposition flag (e.g., only add when part.Disposition == "inline" or part.IsInline() / part.Inline == true, using whatever inline indicator exists on the Part type) before setting cids[strings.ToLower(part.ContentID)] = true so non-inline attachments do not satisfy HTML cid references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/mail/draft/patch.go`:
- Around line 909-914: The orphan detection currently treats any MIME part with
a non-empty ContentID as an orphan candidate; change the check in the loop over
flattenParts(snapshot.Body) so it only considers parts that have a non-empty
ContentID AND have Content-Disposition indicating inline (e.g.,
ContentDisposition == "inline", case-insensitive or via provided enum/helper)
before comparing against refSet and appending to orphaned; update the condition
around part.ContentID to also verify part.ContentDisposition is inline to avoid
flagging primary HTML/body parts.
- Around line 880-888: The current CID collection loop accepts any part with a
ContentID, which can include non-inline attachments; update the loop in
validateInlineCIDAfterApply to only register CIDs from inline MIME parts by
checking the part's inline/disposition flag (e.g., only add when
part.Disposition == "inline" or part.IsInline() / part.Inline == true, using
whatever inline indicator exists on the Part type) before setting
cids[strings.ToLower(part.ContentID)] = true so non-inline attachments do not
satisfy HTML cid references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a49b9db8-51af-412b-8112-a03aefb629bb
📒 Files selected for processing (2)
shortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/draft/patch_test.go
2 participants
Summary
The revert of PR #81 (
eda2b9c, #199) removed the entire auto-resolve feature but also rolled back two independent bugfixes. This PR restores only those bugfixes:strings.ContainsAny(cid, " \t<>()")check innewInlinePartto reject invalid CID characters that produce malformed MIME outputfindPart(snapshot.Body, snapshot.PrimaryHTMLPartID)withfindPrimaryBodyPart(snapshot.Body, "text/html")invalidateInlineCIDAfterApplyandvalidateOrphanedInlineCIDAfterApplyto avoid stale PartID after ops restructure the MIME treeTest plan
go build ./shortcuts/mail/draft/...passesgo test ./shortcuts/mail/draft/...passesadd_inlinewith CID containing spaces/angle brackets is correctly rejectedSummary by CodeRabbit
Bug Fixes
Tests