enhanced gmail gatekeeper support - #367
Conversation
![github-advanced-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Preview:
|
65118bd to
a93ccd2
Compare
a93ccd2 to
f39293d
Compare
f39293d to
d16d020
Compare
![devin-ai-integration[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Findings
|
![ask-bonk[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 8 actionable inline findings. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 8 actionable inline findings. |
ndisidore
left a comment
ndisidore
left a comment
There was a problem hiding this comment.
First pass focusing mostly on the api. But looking really good here! There's a lot to these google things
| store.clearApplying(actionId); | ||
| break; | ||
| } | ||
| if (action.expectedProviderMessageId && |
There was a problem hiding this comment.
[P1] Clear uncertainty once the draft is proven to still exist
After an ambiguous DELETE, this retry GET returning the same draft ID proves that the previous delete did not complete. If the user edited that draft meanwhile, either of these conflict checks throws while gmail:applying remains set; rejectAction() then refuses the action forever and the pending-delete overlay keeps the real draft hidden. Clear the uncertain-write marker before reporting the external-change conflict, or otherwise make the action rejectable.
|
Submitted 5 actionable inline findings. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Findings
|
- split out GmailScopedSession interface - add more metadata to thread interface - add method for reading mailbox address
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 7 actionable inline findings. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Findings
|
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| const canonical = await resolveMutableLabel(this.#ctx, label); | ||
| if (canonical.type !== "custom") throw new Error("System Gmail labels cannot be renamed."); | ||
| const resource = ensureLabelResource(this.#ctx, canonical); | ||
| const dependencies = dependenciesFor(this.#ctx.store, resource.logicalId, "label"); | ||
| await submitAction(this.#ctx, { | ||
| type: "labelRename", labelId: resource.logicalId, name, | ||
| expectedName: canonical.name, dependsOn: dependencies, |
There was a problem hiding this comment.
🟡 Concurrent label renames fail
Concurrent renameLabel calls can capture the same old name before either queues. The second approval fails after the first rename changes that name.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (isLegacyOutboundGmailAction(action)) { | ||
| action = await upgradeLegacyOutboundAction(api, store, actionId, action); | ||
| } |
There was a problem hiding this comment.
🔴 Legacy sends can duplicate email
After an interrupted legacy send, upgradeLegacyOutboundAction assigns a new identity and retries it. Gmail can deliver the same email twice.
Prompt for agents
Legacy outbound actions persisted by the previous implementation have no stable Message-ID or write receipt. A provider write may have succeeded before the worker persisted action removal. The migration in packages/gatekeeper-google/src/gmail.ts upgrades such an action with a fresh Message-ID and sends it, which cannot reconcile the earlier delivery and can duplicate it. Preserve fail-closed handling for legacy sends with uncertain history, or introduce a reconciliation strategy based on data that existed before this deployment. Do not retry them under a newly generated identity.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const recipientPatch = { | ||
| ...(patch.to !== undefined ? {to: normalizeEmailRecipients(patch.to)} : {}), | ||
| ...(patch.cc !== undefined ? {cc: normalizeEmailRecipients(patch.cc)} : {}), | ||
| ...(patch.bcc !== undefined ? {bcc: normalizeEmailRecipients(patch.bcc)} : {}), | ||
| }; | ||
| const patched = applyGmailDraftPatch(state, {...patch, ...recipientPatch}); |
There was a problem hiding this comment.
🟡 Draft updates duplicate recipients
When update changes one recipient field, it skips cross-field normalization. The draft can list one address in multiple recipient fields.
Prompt for agents
GmailDraftStub.update in packages/gatekeeper-google/src/gmail.ts normalizes patch.to, patch.cc, and patch.bcc separately, unlike create/send/reply paths that use normalizeAggregateRecipients. Merge each supplied field with the current draft recipients, then enforce one aggregate recipient set before staging the patch. Define how earlier-field precedence interacts with GmailDraftPatch's promise that omitted fields remain unchanged; rejecting a cross-field duplicate may preserve that contract better than silently editing an omitted field. Add tests where a patched To/CC/BCC address already exists in another field.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function formatApprovalField(label: string, value: string): string { | ||
| // Reject an individually oversized field before newline expansion can allocate millions of | ||
| // intermediate strings. The complete rendered description is checked again at submission. | ||
| validateApprovalDescription(value); | ||
| const block = value.split(/\r\n|\r|\n/).map(line => ` ${line}`).join("\n"); | ||
| return `**${label}:**\n\n${block}`; |
There was a problem hiding this comment.
🟨 Email content can spoof approvals
formatApprovalField uses indentation that blank lines can terminate. Crafted email content can render forged approval details and mislead the approver.
Was this helpful? React with 👍 or 👎 to provide feedback.
| throw new Error( | ||
| "The Gmail draft changed identity while it was being read. Retry it.", {cause: error}); | ||
| } | ||
| current.status = "deleted"; |
There was a problem hiding this comment.
[P1] Do not tombstone a draft whose send is uncertain
If drafts.send delivered the message but its response was lost, the action remains gmail:applying and Gmail legitimately returns 404 for the consumed draft. A subsequent read reaches this branch and changes the resource to deleted. On retry, the delivered message can be found and verified, but completeSentAction() requires the draft resource to still be active, while rejectAction() refuses an applying action. The accepted send is then permanently unreconcilable. Preserve the resource state while a draft-send outcome is uncertain, or allow the verified send completion to consume this tombstone.
| } | ||
| throw error; | ||
| } | ||
| if (current.id !== receipt.draftId || current.message.id !== receipt.messageId) { |
There was a problem hiding this comment.
[P2] Compare the reconciled draft before rejecting its new message ID
A successful draft update stores this receipt before the baseline GET. If that GET fails and another Gmail client subsequently saves the exact same approved MIME, Gmail assigns a new message ID. This check rejects the retry before comparing fingerprints, even though the provider state is exactly the approved output; the receipt and applying marker also make the action unrejectable. Parse and compare the fingerprint first so an identical provider-normalized draft can complete reconciliation despite a later revision ID.
| filename, | ||
| contentType, | ||
| data: foldBase64(bytesToBase64(bytes)), | ||
| ...(attachment.disposition === "inline" ? {disposition: "inline" as const} : {}), |
There was a problem hiding this comment.
[P2] Preserve the PostalMime related attachment flag
A normal multipart/related CID image can have no Content-Disposition; PostalMime represents it as related: true, disposition: null. This conversion (and the analogous one in parseGmailDraft) ignores related, so rebuilding an inline forward or imported draft emits the image as an ordinary attachment rather than a related inline part. Strict clients stop rendering the CID image, and an ambiguous imported-draft update can no longer reconcile because the approved null disposition becomes attachment. Treat attachment.related as inline/related when reconstructing the MIME.
| } else { | ||
| const full = await this.#ctx.api.getMessageFull(this.#messageId); | ||
| if (full.threadId !== this.#threadId) throw new Error("Gmail message identity changed."); | ||
| headers = (full.payload?.headers ?? []).map(header => ({ |
There was a problem hiding this comment.
[P2] Bound the message headers returned over RPC
Unlike body and attachment reads, this maps every provider header without a count or aggregate byte limit. Header content is sender-controlled and a valid Gmail message can devote megabytes to many headers, so getHeaders() can create an oversized RPC result or exhaust the Worker while materializing it. Enforce bounded header count and aggregate UTF-8 bytes before authorization/return.
|
Submitted 4 actionable inline findings. |
4 participants
This PR adds a bunch of new features to the Gmail portion of the Google gatekeeper. The existing gatekeeper connection scopes (whole account, search query, specific label) are unchanged.
For human reviewers, I would focus on the gatekeeper interface changes in
packages/gatekeeper-google/src/types.d.tsand let your agents handle the rest.Features added by this PR
searchThreads(),listMessages(),searchMessages(),getMessage(), andgetThread()star(),unstar(),applyLabel(), andremoveLabel().eml