feat(gmail): reference sanitized body links as [link:N] markers, resolved on demand with gmail link - #1008
feat(gmail): reference sanitized body links as [link:N] markers, resolved on demand with gmail link#1008ronny-rentner wants to merge 8 commits into
Conversation
…lve them with gmail link
…dupword-safe test fixtures, command docs, workflows note
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b5fbc1fdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body, isHTML := gmailcontent.BestBodyForDisplay(msg.Payload) | ||
| _, links := sanitizeGmailBodyLinks(body, isHTML) |
There was a problem hiding this comment.
When a normal multipart/alternative message contains both plain text and HTML, BestBodyForDisplay always selects the plain part (internal/gmailcontent/content.go:70-74). If that part says “Pay now” while the HTML part carries <a href="…">Pay now</a>, sanitized output emits no marker and this command reports zero links, so the feature misses the common case it is intended to support. Marker generation and resolution should consistently inspect a link-bearing HTML alternative when one exists.
Useful? React with 👍 / 👎.
| // NUL delimits the anchor placeholders below and cannot occur in tokenizer output | ||
| // (HTML parsing replaces it); strip it from the input so a crafted body cannot | ||
| // fabricate a placeholder. | ||
| text := strings.ReplaceAll(body, "\x00", "") |
There was a problem hiding this comment.
An untrusted body can already contain literal text such as [link:0], and this preprocessing leaves it indistinguishable from a generated marker. For example, Open invoice [link:0]. Footer https://evil.example produces two [link:0] references even though only the footer URL is registered, so resolving the apparent invoice marker returns the unrelated URL. Neutralize or escape pre-existing marker-shaped text before inserting generated markers.
Useful? React with 👍 / 👎.
| u.Out().Linef("url\t%s", link.URL) | ||
| if link.Text != "" { | ||
| u.Out().Linef("text\t%s", link.Text) |
There was a problem hiding this comment.
For default or --plain output, an email sender can place tabs, newlines, or terminal escape characters in an href, and the resolver writes that attacker-controlled value directly through Linef; JSON escapes these characters, but this path can forge TSV records or emit terminal control sequences. Strip control characters from both URL and text before emitting the non-JSON form so stdout remains parseable.
AGENTS.md reference: AGENTS.md:L18-L22
Useful? React with 👍 / 👎.
| // A NUL-delimited anchor placeholder from extractSanitizedHTMLText, or a bare URL. | ||
| sanitizeLinkPattern = regexp.MustCompile("\x00[^\x00]*\x00|" + `https?://[^\s<>"'` + "`" + `\]\)]+`) |
There was a problem hiding this comment.
For bare URLs in prose, this pattern includes ordinary trailing punctuation but categorically excludes closing parentheses. Thus See https://example.com/reset. resolves to a URL ending in ., while https://example.com/wiki/Foo_(bar) resolves only through ...(bar, leaving ) behind. Since these captured strings are now returned as actionable destinations rather than merely removed, the scanner needs punctuation and balanced-delimiter handling to avoid returning altered URLs.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: found issues before merge. Reviewed August 20, 2026, 3:54 PM ET / 19:54 UTC. ClawSweeper reviewWhat this changesAdds Merge readinessKeep open: the feature has strong real-mailbox proof, but it still misses HTML-only links in ordinary multipart mail and lets sender text forge marker-shaped references. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherGmail read commands select a message body, sanitize it for agent-oriented output, and now emit compact link references. The new resolver replays that selection to return a referenced URL, so both paths must share a safe, link-aware representation. flowchart LR
A[Gmail message] --> B[Body selection]
B --> C[Sanitized body conversion]
C --> D[Visible link markers]
A --> E[Link lookup command]
E --> B
E --> F[Resolved URL]
Decision needed
Why: Current behavior deliberately prefers text/plain, while meeting the feature’s goal for common HTML-button mail requires changing what existing sanitized reads display. Before merge
Findings
Agent review detailsSecurityNeeds attention: Quoted line output fixes record injection, but sender-controlled marker-shaped text still breaks reference integrity in an agent-safe read path. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Define an explicit link-aware multipart contract, escape sender-shaped markers before adding generated ones, and cover both rendering and resolution with multipart regression fixtures. Do we have a high-confidence way to reproduce the issue? Yes. A multipart/alternative message with a plain body and an HTML-only Is this the best way to solve the issue? No. The current approach is deterministic, but it cannot resolve the common HTML-only link case and does not reserve the generated marker syntax from untrusted content. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against eb85a99366eb. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
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 (2 earlier review cycles)
|
…ssible trailing prose punctuation on resolution
|
Thanks for the review. Here is the status on each finding: [P1] Read links from the HTML alternative — real, but pre-existing; proposed as [P1] Reserve generated marker syntax — disputed. A sender who can write a literal [P1] Sanitize untrusted values before line output — fixed in 2066386. The non-JSON [P2] Preserve bare URL boundaries — fixed in 4a0d97c. Resolve security concern: untrusted link values can forge CLI records — fixed by the Resolve merge risk: compatibility — the shape change is disclosed in the PR body and |
…ed guess beside the exact capture
…ever strips past the scheme
…lag so the caller chooses and no URL is emitted twice
|
On the punctuation finding: trimming before registering the marker would alter the |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
1 participant
Problem
--sanitize-contentloses every link destination: visible URLs are replaced with[url removed], and anchorhrefs are dropped with the markup. Sanitized mail thereforecannot be acted on — a task like "open the invoice" or "unsubscribe me from this" needs the
destination. Keeping URLs inline is no answer either: they can be really long (400–1000
chars each) and would dominate the sanitized body.
Change
Attachments already solve this shape of problem: the output carries a compact reference,
and the content is fetched on demand. This PR gives URLs the same treatment.
[url removed]in the sanitized body is replaced with[link:N]instead, so you canreference the link by its index
Nwhen you actually want to fetch it.Indexes are assigned in document order; the same href at several sites shares one index.
An anchor without visible text (an image-wrapper link) is named by its image's
alt.Script/style content is dropped as before and never numbered.
New command, mirroring
gmail attachment:It re-runs the same deterministic body conversion, so its indexes are the ones in the
sanitized body. JSON output is
{"url": ..., "text": ...}withtextbeing theanchor text, omitted for a bare URL; a bad index is a usage error.
The
readonlyandagent-safeprofiles allowgmail link; command docs regenerated;docs/gmail-workflows.mdupdated.Proof (real mailbox, redacted)
A payment notice whose actions are HTML buttons. Sanitized body on v0.37.0 — the button
texts survive, their destinations are silently gone:
Same message with this branch:
The resolved URL is byte-identical to the anchor's
hrefin the message's HTML. No URLappears anywhere in the sanitized output; the original example message carries six long
tracked URLs which would otherwise add ~5 kB to every read.
Before this change, needing to open even a single one of these links meant giving up on
sanitizing entirely and reading the message unsanitized, with all URLs and the raw
payload, just to get at one
href.Testing
mailto, duplicatehref sharing one index, image-anchor
altnaming, script/style URLs never numbered).gmail getemits markers and no URLs;gmail linkresolvesindex → exact URL and anchor text; non-numeric / out-of-range indexes are usage errors;
index parity between
getandlinkon one fixture; thread output numbers per message.make fmt-check,make lint,make deadcode,make docs-check,make testall pass.User-facing changes: the new
gmail linkcommand; sanitized bodies now mark links as[link:N]instead of[url removed]/dropping them.