Skip to content

feat(drive): add secure changes push receiver#753

Closed
steipete wants to merge 2 commits into
feat/poll-state-690from
feat/drive-changes-serve-689
Closed

feat(drive): add secure changes push receiver#753
steipete wants to merge 2 commits into
feat/poll-state-690from
feat/drive-changes-serve-689

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

Closes #689.

Stacked on #751 (feat/poll-state-690) to reuse its reviewed atomic state, Drive pagination, and JSON hook primitives. Retarget to main after #751 lands.

Summary

  • add gog drive changes serve with loopback-default HTTP or direct TLS
  • validate path, method, channel token, notification headers, message ordering, and tracked channel/resource IDs
  • serialize Drive change-log reads and persist page-token/message progress only after hook success
  • support optional file filtering plus JSON-on-stdin shell hooks
  • create and renew Drive notification channels with overlap-safe replacement and old-channel cleanup
  • document reverse-proxy/TLS, manual watch setup, retry semantics, state safety, and channel limits

Security and reliability

  • channel token is checked before header parsing, Drive API access, or hooks
  • state stores only the token SHA-256 digest
  • auto-renew binds notifications to current, previous, or in-flight channel identity
  • hook/API/state failures return 500 without advancing the page token so Google can retry
  • listener binds before channel registration; renewal persists replacement state before stopping the old channel

Proof

  • autoreview: clean, no actionable findings
  • go test -race ./internal/cmd -run 'TestDriveChangesServe' -count=1
  • make lint
  • make ci
  • make build
  • local HTTP notification smoke through httptest.Server
  • CLI dry-run smoke for auto-renew configuration; channel token absent from output

A real Drive watch callback was not created because the clawdbot@gmail.com file-keyring password is unavailable noninteractively. The smoke used --no-input, so no desktop keyring prompt was triggered.

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 7:11 AM ET / 11:11 UTC.

Summary
The branch adds gog drive changes serve, state/channel tracking, token-gated HTTP/TLS notification handling, renewal logic, tests, and generated/docs updates.

Reproducibility: not applicable. this is a feature PR rather than a reported current-main bug. Source inspection confirms main has Drive changes watch/stop support but no Drive serve receiver.

Review metrics: 3 noteworthy metrics.

  • Feature surface: 12 files changed, +1560/-6. The PR adds a network receiver, renewal state, tests, and docs rather than a narrow command alias.
  • Stack dependency: 1 open base PR. The branch is based on feat(poll): add persisted Drive and Docs polling #751, so maintainers need the post-base merge result before landing to main.
  • Receiver tests: 574 added test lines. Focused HTTP/state/renewal coverage exists, but it does not replace live Drive callback proof for the new webhook path.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Risk before merge

  • [P1] The branch targets feat(poll): add persisted Drive and Docs polling #751, so merging to main should wait for that base or retarget and review the combined merge result.
  • [P1] The PR body reports local httptest/dry-run proof but no real Google Drive watch callback, so header, TLS, and retry behavior remain unproven against the live service.
  • [P1] The new receiver accepts public webhook traffic and can run trusted local shell hooks; maintainers should explicitly accept the channel-token and state-binding model before merge.

Maintainer options:

  1. Require live webhook/security signoff (recommended)
    Before merge, have maintainers review the token/channel binding and either capture a live Drive callback proof or explicitly accept the local-only proof gap.
  2. Accept local proof as enough
    Maintainers may intentionally accept the httptest and dry-run coverage without a live Google callback if they are comfortable owning the webhook boundary risk.
  3. Pause until the stack lands
    Keep this PR parked until feat(poll): add persisted Drive and Docs polling #751 is merged or abandoned, then retarget or close based on the surviving base.

Next step before merge

  • [P2] Human review is required because this collaborator PR is stacked on another open PR and adds a security-sensitive webhook receiver; no narrow automation repair is identified.

Security
Cleared: No concrete supply-chain or credential leak was found in the diff; the new exposed webhook and shell-hook surface still warrants maintainer security review.

Review details

Best possible solution:

Land or reject the stacked polling base first, retarget this branch to main, then merge only after maintainer security review accepts live webhook proof or an explicit no-live-proof exception.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this is a feature PR rather than a reported current-main bug. Source inspection confirms main has Drive changes watch/stop support but no Drive serve receiver.

Is this the best way to solve the issue?

Mostly yes: a first-party serve command fits the linked feature request and reuses the #751 primitives, but the stack and security-sensitive live callback path need maintainer signoff before this is the final solution.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 385631d2f594.

Label changes

Label changes:

  • add P2: This is a normal-priority new Drive automation feature with security-sensitive scope but no active production regression.
  • add merge-risk: 🚨 security-boundary: Merging adds a public Drive webhook receiver that authorizes by shared channel token and can trigger local shell hooks.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author association is COLLABORATOR, so the external-contributor proof gate does not apply; the body lists local tests and explicitly notes that no real Drive watch callback was created.

Label justifications:

  • P2: This is a normal-priority new Drive automation feature with security-sensitive scope but no active production regression.
  • merge-risk: 🚨 security-boundary: Merging adds a public Drive webhook receiver that authorizes by shared channel token and can trigger local shell hooks.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author association is COLLABORATOR, so the external-contributor proof gate does not apply; the body lists local tests and explicitly notes that no real Drive watch callback was created.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its PR-review guidance requires reviewing via PR diff/context without switching branches and keeping stdout parseable. (AGENTS.md:1, 385631d2f594)
  • Current main does not already implement the receiver: A current-main search found no DriveChangesServe, changes serve, DriveChangesPoll, or pollState implementation under internal, docs, or README, so the PR is not obsolete on main. (385631d2f594)
  • New command surface: The PR adds Serve DriveChangesServeCmd under DriveChangesCmd, alongside start-token/list/poll/watch/stop. (internal/cmd/drive_changes.go:25, 066bb67e86ae)
  • Receiver security and retry path: The new handler checks path, method, channel token, notification headers, tracked channels, duplicate ordering, hook/API failures, and success acknowledgement statuses. (internal/cmd/drive_changes_server.go:46, 066bb67e86ae)
  • Auto-renew state path: The PR creates replacement Drive channels, stores current/previous channel metadata, hashes the token in state, and stops the previous channel after persistence. (internal/cmd/drive_changes_serve.go:395, 066bb67e86ae)
  • Focused test coverage exists: The added tests cover wrong-token rejection, untracked channel rejection, HTTP notification processing, hook retry state retention, concurrency serialization, renewal, validation, and shutdown. (internal/cmd/drive_changes_serve_test.go:26, 066bb67e86ae)

Likely related people:

  • Peter Steinberger: Introduced the current Drive changes/watch API in fc7c536..., introduced the adjacent Gmail watch receiver in b59ea1e..., and authored the stacked Drive receiver branch. (role: feature owner and recent area contributor; confidence: high; commits: fc7c536a48f3, b59ea1e8cb02, 066bb67e86ae; files: internal/cmd/drive_changes.go, internal/cmd/gmail_watch_server.go, internal/cmd/drive_changes_serve.go)
  • salmonumbrella: Made prior merged fixes and feature work in the Gmail watch serve path that this Drive receiver mirrors for webhook handling and reliability patterns. (role: adjacent receiver contributor; confidence: medium; commits: caf38a3d33c8, e593322d79b7; files: internal/cmd/gmail_watch_server.go, internal/cmd/gmail_watch_serve_test.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 066bb67e86

ℹ️ 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".

Comment on lines +150 to +154
case "sync":
if err := s.acknowledgeNotificationLocked(notification); err != nil {
return err
}
return errDriveChangesIgnoredNotification

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Process sync notifications to drain missed changes

When --auto-renew creates a new state file, the page token is captured before the watch channel is established, so any Drive edit that lands in that gap will not have a later change notification on the new channel; the initial sync request is the only request this receiver is guaranteed to see. Returning here without calling loadDriveChanges leaves the page token stale and the hook silent until some future unrelated edit happens to trigger another notification.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 11, 2026
@sebsnyk

sebsnyk commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

hi @steipete bot, please ensure this functionality (looking very much forward to it) is documented in detail

@steipete steipete force-pushed the feat/poll-state-690 branch from bebd261 to b1e5d49 Compare June 12, 2026 02:22
@steipete steipete deleted the branch feat/poll-state-690 June 12, 2026 02:42
@steipete steipete closed this Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants