Skip to content

feat(core): signed streaming URLs for attachment and Drive file downloads - #888

Open
123andy wants to merge 11 commits into
taylorwilsdon:mainfrom
scientist-hq:signed-attachments
Open

feat(core): signed streaming URLs for attachment and Drive file downloads#888
123andy wants to merge 11 commits into
taylorwilsdon:mainfrom
scientist-hq:signed-attachments

Conversation

@123andy

@123andy 123andy commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Description

Motivation: to make stateless mode genuinely viable for a large, multi-user, horizontally-scaled deployment. Stateless mode exists so the server can run as disk-less, replicated instances — but attachment handling was the gap that broke that promise: base64-through-the-model doesn't scale (slow and token-heavy, especially for big files), and the disk-backed download route assumes a single instance with local storage and serves files from an unauthenticated path. This PR closes that gap so a fleet of replicas serving many users can deliver file downloads efficiently, safely, and without local state.

What this adds: an opt-in signed-URL streaming path behind WORKSPACE_MCP_SIGNED_ATTACHMENT_URLS=true. The download tools (get_gmail_attachment_content, get_drive_file_download_url) return a short-lived capability URL instead of base64; a download route recovers the owner's Google credentials and streams the bytes from Google on demand — nothing base64-encoded through the model, nothing written to disk. Possessing the URL is the per-user authorization, so the GET needs no bearer token and any HTTP client can fetch it.

The fallback ladder (mint time)

When a download tool runs, it picks the best available option and degrades gracefully — each rung only requires strictly less infrastructure than the one above:

  1. /dl/{handle} — short claim-check URL (preferred, default). ~59 chars total. The claims are stored server-side in the encrypted short-TTL KV store under a random 128-bit handle; the URL carries only the handle. ~10x fewer characters (and LLM tokens) than the JWT form — a Gmail attachmentId alone is ~300 chars, pushing a self-contained URL past 600. Needs a KV store write to mint (in-process memory store covers single-container; the shared Valkey store covers multi-replica).
  2. /attachments/signed/{token} — self-contained signed JWT. Used automatically when the handle store is unusable or the write fails, or always when WORKSPACE_MCP_SHORT_SIGNED_URLS=false. An HS256-signed token carries the resource reference, owner, and expiry — no storage required, works anywhere.
  3. No URL at all — fall through to today's download path. If the owner's credentials can't be recovered at mint time, or the URL TTL clamped to the credential's remaining life is ≤ 0, no signed URL is minted (a URL guaranteed to 401 on arrival helps nobody). The tool behaves exactly as it does today: base64 in stateless mode, or the existing disk-backed /attachments/{file_id} route in non-stateless mode.
  4. Feature disabled (the default): with WORKSPACE_MCP_SIGNED_ATTACHMENT_URLS unset, behavior is byte-for-byte today's. Fully opt-in and non-breaking; the legacy disk route is untouched throughout.

Serving (GET time)

Both URL forms converge on one shared streaming path; only claim recovery differs:

  • /dl/{handle} → validate handle format → load claims from the store (store TTL is the expiry; a fail-closed exp claim check backstops it) → 403 if unknown/expired/malformed.
  • /attachments/signed/{token} → verify the HS256 signature and exp403 if invalid.
  • Then, identically for both: two-tier credential recovery — (1) the in-process OAuth 2.1 session store, then (2) a short-TTL, Fernet-encrypted credential cache keyed by email, stashed at mint time (reusing the OAuth-proxy Valkey config — no new infra). Tier 2 is what makes a URL work when the GET lands on a different replica than the tool call that minted it. No credentials → retryable 401; expired non-refreshable credentials → a precise 401 {"error": "credentials expired - re-authenticate"} rather than a generic failure.
  • Fetch + stream: a per-source fetcher registry (core/signed_download.py) turns claims + credentials into bytes — adding a source is one fetcher, no route changes. Drive streams in bounded chunks (default 16 MiB, WORKSPACE_MCP_DRIVE_STREAM_CHUNK_BYTES) so large files never buffer whole in RAM; Gmail attachments arrive whole in one API response, so they're buffered.

Sources

Gmail attachments, and Google Drive — binary files via get_media, native Google files via export_media (Docs→PDF, Sheets→XLSX, Slides→PPTX).

Known limitation (disclosed): in OAuth 2.1 proxy mode the recovered credential carries no refresh token, so once the underlying Google access token expires the route can't renew it. The URL TTL is clamped to the token's remaining life (and the tool response reports the real clamped lifetime), and an already-expired credential returns the retryable 401 above (re-running the tool mints a fresh URL). Root cause is that _build_credentials_from_provider reads OAuthProxy attributes that no longer exist on current FastMCP (reported separately in #886); a future change to recover the upstream refresh token via the JWT jti mapping would lift this limitation.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this change manually

Local run: 45+ new tests (tests/core/test_attachment_signing.py, test_attachment_cred_cache.py, test_signed_download.py, test_download_handles.py — the last covering the handle store, the mint-time fallback ladder, and the /dl route) pass; ruff check clean on all changed files. Full suite adds no new failures — same 21 pre-existing failures (in gcontacts/gforms, unrelated, pass in isolation) as upstream main (6e1d145), plus the 29 new passing tests. Manually verified end-to-end against a running stack: real Gmail attachment and Drive get_media/export_media downloads (PDF/XLSX/.mov) stream correctly; an expired non-refreshable credential returns 401 (not a generic 502) while a valid credential passes through; and the TTL clamp shortens a near-expiry URL.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have enabled "Allow edits from maintainers" for this pull request

Additional Notes

  • New modules: core/attachment_signing.py (HS256 capability tokens + TTL clamp), core/attachment_cred_cache.py (encrypted short-TTL Valkey cred cache), core/signed_download.py (per-source fetcher registry), core/download_handles.py (short claim-check handles). Modified: core/server.py (the /attachments/signed route + a token_expiry_threshold_seconds=300 on GoogleProvider so the proxy refreshes proactively), gmail/gmail_tools.py and gdrive/drive_tools.py (return a signed URL when enabled). Plus three test modules and a README section.
  • The signing key is WORKSPACE_MCP_ATTACHMENT_SIGNING_KEY, falling back to GOOGLE_OAUTH_CLIENT_SECRET. The feature requires WORKSPACE_MCP_STATELESS_MODE=true + OAuth 2.1.

Summary by CodeRabbit

  • New Features
    • Added stateless-friendly, signed attachment links for Gmail and Drive via GET /attachments/signed/{token}.
    • Enabled optional shorter link mode via claim-check handles at GET /dl/{handle}.
    • Drive downloads now stream in configurable chunks for reduced upfront memory usage.
  • Bug Fixes
    • Improved handling for expired/tampered links with consistent 401/403/502 responses and safe fallback to the prior non-streaming flow.
  • Documentation
    • Documented new environment variables and stateless streaming credential recovery behavior/limitations.
  • Tests
    • Added coverage for token verification, handle-based short links, Drive streaming behavior, and the credential cache.

123andy and others added 6 commits June 26, 2026 08:13
In stateless mode get_gmail_attachment_content either dumped the
attachment as base64 through the model (slow, token-hungry) or, in
non-stateless mode, wrote it to disk and served it from an
unauthenticated route. Neither suits a stateless, multi-replica
deployment.

Behind WORKSPACE_MCP_SIGNED_ATTACHMENT_URLS, the tool now returns a
short-lived signed URL instead. The /attachments/signed/{token} route
verifies the HS256 token, recovers the owner's Google credentials,
fetches the attachment from Gmail on demand, and streams the bytes —
no base64 in context, nothing on disk. The signature is the per-user
authorization, so the GET needs no bearer token.

Credential recovery is two-tier so the URL survives a request landing
on a replica that never ran the originating tool call:
  1. in-process OAuth 2.1 session store (same-process case), then
  2. a short-TTL, Fernet-encrypted Valkey cache keyed by email,
     stashed at mint time. Reuses the existing
     WORKSPACE_MCP_OAUTH_PROXY_VALKEY_* config and the proxy's
     derive_jwt_key derivation (distinct salt); no new env, and the
     cred record expires on the same 15-min horizon as the URL.

Filename/MIME are resolved in the route after streaming, by matching
the message payload on byte size: Gmail attachment ids are ephemeral
and rotate between fetches, so id-matching is unreliable (the download
tool's own fallback already keys on size). The token can additionally
carry signed fn/mt claims for stable-id sources (e.g. Drive file ids).

Signing key: WORKSPACE_MCP_ATTACHMENT_SIGNING_KEY, falling back to
GOOGLE_OAUTH_CLIENT_SECRET. Off by default.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Refactor the signed-URL mechanism so it covers any Google resource
fetched via an authenticated API call, not just Gmail attachments, and
wire up Drive (get_media / export_media).

- attachment_signing: add mint_download_token(source, ref, ...) which
  carries a source tag plus source-specific locator claims;
  mint_attachment_token becomes a thin Gmail wrapper over it.
- core/signed_download: a per-source fetcher registry. Each fetcher
  turns (claims, credentials) into (bytes, filename, mime_type). The
  Gmail fetcher holds the existing attachments().get + size-based
  filename logic; the Drive fetcher streams get_media, or export_media
  for native Google files (Docs/Sheets/Slides). Adding a source = add a
  fetcher; the route is untouched.
- server route: now verifies the token, recovers creds (session →
  Valkey cache), dispatches on claims["src"], and streams the result.
- gdrive get_drive_file_download_url: returns a signed URL (early
  return before download) when signed URLs are enabled. Drive file ids
  are stable, so filename/MIME are resolved here and signed into the
  token; the fetcher uses them directly.

Drive vs Gmail naming: Drive ids are stable so filename is signed in at
mint; Gmail ids are ephemeral so the route resolves by byte size.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A 26MB Drive fetch surfaced that the route buffered the whole file in
memory before responding — fine for small files, but a multi-GB video
would pin that much RAM per concurrent download.

Fetchers now return a DownloadResult that is either buffered (content)
or streamed (stream). Gmail attachments arrive whole in a single API
response and are email-size-bounded, so they stay buffered. Drive
downloads chunk via MediaIoBaseDownload: the fetcher pulls the first
chunk eagerly (so auth/not-found errors still surface as 502 before the
response starts), then yields 8MB chunks, truncating the buffer after
each so only one chunk is ever in RAM. The route returns a
StreamingResponse for streamed results and a plain Response otherwise.

googleapiclient's default chunk size is 100MB, which would defeat the
purpose; pinned to 8MB.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add WORKSPACE_MCP_DRIVE_STREAM_CHUNK_BYTES to tune the per-download
memory/throughput trade-off: bigger chunks mean fewer range requests
(faster) but more RAM per concurrent download. Defaults to 16MB (up
from the hardcoded 8MB), parsed once at import with a warn-and-default
fallback on a missing/invalid value.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In OAuth 2.1 proxy mode the credential recovered by the signed-download
route is a bare Google access token with no refresh_token (FastMCP's
proxy holds upstream refresh, unreachable from this bearer-less route),
so an expired token cannot be renewed at fetch time. Previously this
surfaced as a confusing generic 502.

Two in-fork changes, no FastMCP coupling:

- Floor: serve_signed_attachment now guards on `not credentials.valid`
  (mirroring auth/google_auth.py) and returns a precise, retryable
  401 "credentials expired - re-authenticate" — distinct from the
  existing 401 (no session), the 403s (bad token), and the 502 (genuine
  fetch failure). A refresh branch is kept for forward-compat but is
  inert today (refresh_token is always None). `.valid` (not `.expired`)
  is used so a cache record with expiry=None can't slip a dead token
  through.

- TTL clamp: both mint sites now clamp the URL/cache TTL to the token's
  remaining life via attachment_signing.clamp_ttl_to_expiry(), so a URL
  can't outlive the snapshot it depends on. The snapshot expiry is real
  because the proxy refreshed the token during this request.

- Set token_expiry_threshold_seconds=300 on GoogleProvider so the proxy
  refreshes the upstream token ~5 min early, keeping stashed snapshots
  fuller-lived and clamped URLs off their floor near the hourly boundary.

Tests cover the clamp (none/far/near/expired/floor + the never-outlive
property).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a "Signed Attachment URLs (Streaming, Stateless-Friendly)" section
covering the feature, two-tier credential recovery, bounded Drive
streaming, the configuration env vars, and the OAuth-2.1-proxy refresh
limitation (with the retryable 401). Document the three new env vars in
the configuration table.

Also lengthen the signing-test HMAC keys past the 32-byte recommendation
(silences InsecureKeyLengthWarning) and drop an unused import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds signed attachment URLs for Gmail and Drive tools, an encrypted credential cache, a signed-download route and fetcher layer, and tests/docs for the new streaming path.

Changes

Signed attachment delivery

Layer / File(s) Summary
Capability tokens and credential snapshots
core/storage.py, core/attachment_signing.py, core/attachment_cred_cache.py, core/download_handles.py, tests/core/test_attachment_signing.py, tests/core/test_attachment_cred_cache.py, tests/core/test_download_handles.py
Adds shared key derivation, JWT minting and verification for signed attachment URLs, TTL clamping, encrypted credential snapshot storage, short-lived download-handle storage, and tests for token, TTL, cache, and handle round-trips.
Signed-download route and fetchers
core/signed_download.py, core/server.py, tests/core/test_signed_download.py, tests/core/test_download_handles.py
Adds per-source signed-download fetchers, Drive chunked streaming, token validation, credential recovery, and the /attachments/signed/{token} and /dl/{handle} HTTP handlers.
Tool fast paths and docs
gmail/gmail_tools.py, gdrive/drive_tools.py, README.md
Adds signed-URL early returns in the attachment tools and documents the new environment variables and stateless streaming mode.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested labels: enhancement

Suggested reviewers: taylorwilsdon

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely summarizes the main change: signed streaming URLs for attachment and Drive downloads.
Description check ✅ Passed The description covers all required template sections and provides detailed, relevant implementation, testing, checklist, and notes content.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/core/test_signed_download.py (1)

1-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Run Ruff format before merge.

The Ruff job is currently failing with format --check for this file.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/core/test_signed_download.py` around lines 1 - 92, The test module
formatting is out of sync with Ruff, causing format --check to fail. Run the
Ruff formatter on this test file and keep the existing behavior of TestRegistry,
TestDownloadResult, patched_drive, and TestDriveStreaming unchanged while
applying only formatting updates.

Source: Pipeline failures

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/attachment_cred_cache.py`:
- Around line 1-223: The file is not matching Ruff’s formatter output, so update
the formatting in attachment_cred_cache.py to satisfy format --check before
merge. Re-run Ruff format on the module and keep the existing logic in
_build_store, stash_credentials, and load_credentials unchanged while applying
only formatting-driven changes.
- Around line 82-86: The Valkey-backed credential cache only looks at
WORKSPACE_MCP_OAUTH_PROXY_VALKEY_HOST, so it misses valid configurations that
set WORKSPACE_MCP_OAUTH_PROXY_STORAGE_BACKEND=valkey. Update the cache setup in
attachment_cred_cache to honor the same backend selection logic used by
server.py: treat storage_backend=valkey as Valkey, default the host to localhost
when needed, and keep the existing behavior when a specific host is provided.
Make the change in the credential-cache initialization path that reads
valkey_host so signed-download credentials use Valkey instead of falling back to
per-process memory.

In `@core/attachment_signing.py`:
- Around line 76-80: The TTL calculation in the attachment signing flow is
flooring the result to _TTL_FLOOR_SECONDS even when the credential has less than
that lifetime remaining, so adjust the logic in the TTL helper around the
max/min expression to return a non-positive value whenever seconds_left minus
_TTL_EXPIRY_MARGIN_SECONDS is below the floor. Update the caller that mints URLs
to treat that non-positive TTL as a signal to skip minting or remint instead of
producing a 30-second URL that can outlive the credential.
- Around line 130-135: The payload construction in attachment_signing.py’s token
minting flow merges ref after the reserved JWT claims, so colliding keys can
silently override src, sub, iat, or exp. Update the function that builds this
payload to validate ref before merging and reject any reserved claim names,
ensuring only non-reserved metadata is accepted and the trusted authorization
claims remain authoritative.

In `@core/server.py`:
- Around line 841-850: The download response in the file-serving path is
building Content-Disposition with raw result.filename, which can allow malformed
headers or injection. Update the response header construction in the download
logic around the StreamingResponse/Response return path to encode or safely
quote the filename before inserting it into Content-Disposition, and ensure any
CR/LF or quotes are handled by the filename encoding helper used for downloads.

In `@gdrive/drive_tools.py`:
- Around line 478-498: The signed Drive URL flow in get_drive_file_download_url
can currently proceed even when get_oauth21_session_store().get_credentials
returns None, which leaves the download route without recoverable credentials.
Update the logic around creds, eff_ttl, mint_download_token, and
get_signed_attachment_url so that a signed URL is only returned when credentials
are available and stashed; otherwise fall back to the existing direct download
behavior or raise an immediate error before generating the token.

In `@gmail/gmail_tools.py`:
- Around line 1784-1803: The get_gmail_attachment_content flow still mints a
signed attachment URL even when
get_oauth21_session_store().get_credentials(user_google_email) fails and creds
is None. Update the logic around mint_attachment_token and
get_signed_attachment_url so that this branch does not create a signed link
without credentials; instead, fall back to the existing download path or return
the same explicit tool error used elsewhere when credential recovery fails.

---

Outside diff comments:
In `@tests/core/test_signed_download.py`:
- Around line 1-92: The test module formatting is out of sync with Ruff, causing
format --check to fail. Run the Ruff formatter on this test file and keep the
existing behavior of TestRegistry, TestDownloadResult, patched_drive, and
TestDriveStreaming unchanged while applying only formatting updates.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 48d8fb8d-04b7-48aa-b969-1a0f16409696

📥 Commits

Reviewing files that changed from the base of the PR and between 6e1d145 and 9fb0fc3.

📒 Files selected for processing (10)
  • README.md
  • core/attachment_cred_cache.py
  • core/attachment_signing.py
  • core/server.py
  • core/signed_download.py
  • gdrive/drive_tools.py
  • gmail/gmail_tools.py
  • tests/core/test_attachment_cred_cache.py
  • tests/core/test_attachment_signing.py
  • tests/core/test_signed_download.py

Comment thread core/attachment_cred_cache.py
Comment thread core/attachment_cred_cache.py Outdated
Comment thread core/attachment_signing.py Outdated
Comment thread core/attachment_signing.py
Comment thread core/server.py Outdated
Comment thread gdrive/drive_tools.py Outdated
Comment thread gmail/gmail_tools.py Outdated
123andy and others added 2 commits June 26, 2026 12:53
Fixes the failing `ruff format --check` CI job. Formatting-only — no
behavior change (collapses stray blank lines left by earlier import
removals, normalizes call wrapping).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- attachment_cred_cache: honor WORKSPACE_MCP_OAUTH_PROXY_STORAGE_BACKEND=valkey
  (host defaults to localhost), mirroring server.py, so signed-download creds
  don't silently fall back to per-process memory and break cross-replica recovery.
- attachment_signing.clamp_ttl_to_expiry: no longer floors the TTL above the
  credential's remaining life; returns a non-positive value when the token is
  at/near expiry so a URL can never outlive its (non-refreshable) snapshot.
- attachment_signing.mint_download_token: reject `ref` keys that collide with
  reserved JWT claims (src/sub/iat/exp/fn/mt) instead of letting a locator
  override them.
- server signed-download route: sanitize the Content-Disposition filename
  (strip quotes/CR/LF for the ASCII fallback, add RFC 5987 filename*) to prevent
  header injection from attacker-influenced Gmail/Drive filenames.
- gmail/drive tools: only return a signed URL when credentials are recoverable
  and the clamped TTL is positive; otherwise fall back to the normal download
  path rather than emitting a URL guaranteed to 401.

Tests updated for the no-floor clamp behavior; added ref-collision rejection
test and made the cred-cache fixture hermetic against STORAGE_BACKEND.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@123andy

123andy commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Pushed c6ca6cd addressing the CodeRabbit review:

  • attachment_cred_cache — honor WORKSPACE_MCP_OAUTH_PROXY_STORAGE_BACKEND=valkey (host defaults to localhost), mirroring server.py, so signed-download creds don't silently fall back to per-process memory.
  • clamp_ttl_to_expiry — no longer floors the TTL above the credential's remaining life; returns a non-positive value near expiry so a URL can't outlive its (non-refreshable) snapshot.
  • mint_download_token — rejects ref keys that collide with reserved JWT claims (src/sub/exp/…).
  • signed-download route — sanitizes the Content-Disposition filename (strips quotes/CR/LF, adds RFC 5987 filename*) to prevent header injection from attacker-influenced Gmail/Drive filenames.
  • gmail/drive tools — only return a signed URL when credentials are recoverable and the clamped TTL is positive; otherwise fall back to the normal download path instead of emitting a URL that's guaranteed to 401.

Tests updated for the no-floor clamp behavior, plus a ref-collision rejection test.

Note on the red pytest check

The single failing test, tests/gforms/test_forms_tools.py::test_set_publish_settings_builds_publish_state_body, is pre-existing on main (6e1d145) and unrelated to this PR — it's in gforms, which this PR doesn't touch (the diff is confined to core/attachment_*, core/server.py, gmail/, gdrive/, and tests/core/). The other 1296 tests pass, including the 31 added here. Happy to open a separate issue/PR for the forms test if useful.

@123andy

123andy commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Failing Pytest is outside the scope of this PR and is addressed in #890

@123andy

123andy commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

A few cross-links for reviewer context:

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
gmail/gmail_tools.py (1)

92-97: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

<br> word-break fix looks correct; block-level tags still merge text.

The <br> space insertion correctly prevents adjacent text from concatenating (and also covers self-closing <br/> via handle_startendtag). Block-level boundaries such as </p><p>, <div>, and <li> still merge into a single token (e.g. <p>a</p><p>b</p>ab) because only <br> emits a separator. If cleaner extracted text matters here, consider emitting a space for common block tags too.

♻️ Optional: separate block-level content
-        if tag == "br" and not self._skip:
+        if tag in ("br", "p", "div", "li", "tr") and not self._skip:
             self._text.append(" ")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gmail/gmail_tools.py` around lines 92 - 97, The HTML text extraction in
GmailHtmlParser.handle_starttag only inserts a separator for br, so block-level
elements still collapse adjacent text. Update the parser to emit a space or
equivalent separator for common block tags such as p, div, and li (and any
similar closing/opening boundaries you already handle), while preserving the
existing skip logic for script/style and the br handling in
handle_starttag/handle_startendtag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@gmail/gmail_tools.py`:
- Around line 92-97: The HTML text extraction in GmailHtmlParser.handle_starttag
only inserts a separator for br, so block-level elements still collapse adjacent
text. Update the parser to emit a space or equivalent separator for common block
tags such as p, div, and li (and any similar closing/opening boundaries you
already handle), while preserving the existing skip logic for script/style and
the br handling in handle_starttag/handle_startendtag.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8654b2a8-b901-40bd-a2f6-4a32b1f4b1f8

📥 Commits

Reviewing files that changed from the base of the PR and between c6ca6cd and ff13e9d.

📒 Files selected for processing (1)
  • gmail/gmail_tools.py

@123andy 123andy changed the title Add signed streaming URLs for attachment/file downloads feat(core): signed streaming URLs for attachment and Drive file downloads Jul 8, 2026
A self-contained signed-JWT download URL runs ~700 chars for Gmail (the
attachmentId alone is ~300) — roughly 250 LLM tokens every time the URL
passes through the model, and a long opaque string for a model to corrupt.

This adds the claim-check alternative: the claims that would have been signed
into the JWT are stored server-side under a random 128-bit handle, in the same
encrypted store as the attachment credential cache (own collection), TTL'd
exactly like the JWT exp. The URL becomes ~59 chars: {base}/dl/{22-char
handle}. Security is equivalent (CSPRNG handle ≈ HMAC unguessability; store
TTL ≈ exp claim, kept as a read-time backstop) and handles gain revocability.

- core/download_handles.py: store/load, handle-format validation; reuses the
  credential cache's store so backend selection cannot drift; when no store
  is usable, returns None and the caller falls back.
- attachment_signing.build_download_url(): short form when storable, else the
  JWT form; tools (gmail, drive) now call this. JWT mint/verify unchanged;
  /attachments/signed/{token} still served.
- server: /dl/{handle} route; post-verification streaming logic shared with
  the JWT route via _stream_download_for_claims().
- storage: derive_shared_fernet_key(salt) helper; the credential cache now
  uses it (identical derivation, single source).
- Env: WORKSPACE_MCP_SHORT_SIGNED_URLS (default true; false forces JWT form).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@123andy

123andy commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 0d20400, folding in a token-efficiency improvement to the URLs themselves: short claim-check download URLs.

Problem: the signed JWT URL is self-contained, which makes it huge — a Gmail attachmentId alone is ~300 chars, so a full URL runs ~690 chars. That's ~250 LLM tokens every time the URL passes through the model (once in the tool result, again when the model relays it to the user), and long opaque strings are exactly what models corrupt when reproducing them.

Change: by default the tools now return {base}/dl/{handle} (~59 chars, measured) — a random 128-bit handle referencing the claims stored server-side in the same encrypted short-TTL store as the credential cache, with the TTL the JWT exp would have carried (the exp claim is also kept in the record as a read-time backstop). Security is equivalent — a 128-bit CSPRNG handle is as unguessable as an HMAC-SHA256 signature, and the URL remains a bearer capability either way — plus handles gain revocability, which a self-contained JWT can't have.

Nothing regresses: when no store is usable, minting falls back automatically to the self-contained JWT form; /attachments/signed/{token} is still served; and WORKSPACE_MCP_SHORT_SIGNED_URLS=false pins the previous behavior. 12 new tests cover the handle round-trip, TTL + exp-backstop expiry, malformed-handle rejection (incl. path-traversal shapes), the JWT fallback, and the /dl route streaming path.

Happy to split this back out into a follow-up PR if you'd rather review the original scope — just say the word.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
gmail/gmail_tools.py (1)

1825-1826: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Report the clamped lifetime instead of always saying 15 minutes.

eff_ttl can be much less than the default TTL after credential-expiry clamping, so near-expiry links may expire almost immediately while the response still says “~15 minutes.”

Proposed fix
+            ttl_label = (
+                f"{int(eff_ttl)} seconds"
+                if eff_ttl < 120
+                else f"{int(eff_ttl // 60)} minutes"
+            )
             return "\n".join(
                 [
                     "Attachment ready — streamed on demand (no base64, nothing stored).",
                     f"Message ID: {message_id}",
                     f"\n📎 Download URL: {download_url}",
                     "\nThe server streams the bytes directly from Gmail when this URL is "
-                    "fetched; the link is signed to you and expires in ~15 minutes.",
+                    f"fetched; the link is signed to you and expires in ~{ttl_label}.",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gmail/gmail_tools.py` around lines 1825 - 1826, The download link description
in the Gmail URL generation path always ამბობს “~15 minutes” even when the
effective TTL has been clamped lower. Update the message in the URL-building
code that uses eff_ttl so it reports the clamped lifetime actually being issued,
rather than the default TTL; keep the wording tied to the link-generation logic
in gmail_tools and make the displayed expiration match the computed expiry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@core/download_handles.py`:
- Around line 102-107: The handle-loading expiry check in load handle records
currently only rejects numeric exp values that are already expired, which allows
missing or malformed exp claims to pass through. Update the logic in the record
validation path around exp handling to fail closed: if exp is absent, not an
int/float, or is otherwise invalid, return None instead of accepting the record.
Keep the existing expiration comparison behavior for valid numeric exp values.

---

Outside diff comments:
In `@gmail/gmail_tools.py`:
- Around line 1825-1826: The download link description in the Gmail URL
generation path always ამბობს “~15 minutes” even when the effective TTL has been
clamped lower. Update the message in the URL-building code that uses eff_ttl so
it reports the clamped lifetime actually being issued, rather than the default
TTL; keep the wording tied to the link-generation logic in gmail_tools and make
the displayed expiration match the computed expiry.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f400b316-f98d-4218-8631-b6bc0d73c5b8

📥 Commits

Reviewing files that changed from the base of the PR and between ff13e9d and 0d20400.

📒 Files selected for processing (9)
  • README.md
  • core/attachment_cred_cache.py
  • core/attachment_signing.py
  • core/download_handles.py
  • core/server.py
  • core/storage.py
  • gdrive/drive_tools.py
  • gmail/gmail_tools.py
  • tests/core/test_download_handles.py
✅ Files skipped from review due to trivial changes (1)
  • README.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • core/attachment_cred_cache.py
  • gdrive/drive_tools.py

Comment thread core/download_handles.py
…sponses

- download_handles.load_download_ref: reject records with a missing or
  malformed exp claim, not just numerically-expired ones — the /dl route
  trusts loaded claims, so the backstop must fail closed.
- Tool responses no longer always claim "~15 minutes": eff_ttl is clamped to
  the credential's remaining life, so near-expiry links could die in seconds
  while the text promised minutes. New attachment_signing.format_ttl() reports
  the real lifetime; used by both gmail and drive call sites.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@123andy

123andy commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Pushed d5cb159 addressing both findings:

  • load_download_ref now fails closed on a missing or non-numeric exp claim (not isinstance(exp, (int, float)) or exp < now → reject). Practically unreachable — records are self-written by _build_claims() (which always sets exp) and Fernet-encrypted — but a backstop that trusts the record it's checking isn't a backstop. Two new tests cover the missing-exp and malformed-exp cases.
  • Tool responses now report the real clamped lifetime instead of always "~15 minutes": new attachment_signing.format_ttl() ("45 seconds" / "~4 minutes"), used by both the Gmail and Drive call sites — the Drive message had the same stale wording, so both are fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant