Repository navigation
fix(mcp-server): honour the API key's nonce mode in REST and SDK requests (PREDICT-11349) - #76
SohumDesai27 wants to merge 3 commits into
Conversation
…ests (PREDICT-11349) Keys created with "Uses a time based nonce" (including Agentic onboarding keys) reject millisecond nonces, while the QA100 keys from PR #75 need them. The legacy REST signer always sent milliseconds, and the SDK hardcoded seconds on standard hosts and milliseconds on custom hosts, so a single key could not work on every path. - Add GEMINI_API_NONCE_MODE = incremental | time-based. Unset defaults to incremental, preserving PR #75 behaviour. Any other value fails startup. - Legacy REST signs with the configured unit. Incremental keeps the shared millisecond sequence; time-based uses epoch seconds with no state. - SDK auth follows the same mode on every base URL. Incremental routes through the shared sequence so SDK and legacy requests never interleave out of order. Time-based uses the SDK's native time-based mode. - WebSocket auth is unchanged and always uses epoch seconds. - Nonce rejections get an operator hint naming the mode to set. - Add the variable to the alerts daemon's env whitelist and the README.
|
@svc-grace review |
Change Control Evidence CheckAuthorization — ✅ PassEvidence:
Testing —
|
Nostradamus Risk Rating — HighThe PR modifies HMAC-SHA384 nonce generation in High/Critical — a full threat model follows. |
svc-grace
left a comment
There was a problem hiding this comment.
Agentic Review
Nice, this covers the main nonce-mode paths, but SDK-authenticated nonce errors still miss the mode hint, so users can hit the mismatch without the guidance they get on the REST path. There are also a few test and setup-example gaps around the time-based mode; those are worth a follow-up but aren’t what’s driving the requested changes.
Review process
Grace version: v0.0.254
Files reviewed (13): README.md, config.test.ts, config.ts, http.request.test.ts, http.ts, index.test.ts, index.ts, sdk.test.ts, sdk.ts, signer.test.ts, signer.ts, types.ts
Guidelines: none discovered
Verification: 4 of 6 findings verified
Findings: 0 critical, 1 important, 3 suggestions
Linked tickets: PREDICT-11349
LLM usage: 100 calls — gpt-6-luna: 86 calls, 2434176 tokens, us.anthropic.claude-opus-5-5: 14 calls, 363225 tokens
|
Nostradamus Threat Model - Passed - APPSEC-6108 |
…ts (PREDICT-11349) - SDK nonce rejections now get the mode hint. The SDK keeps only the rejection code (reason=InvalidNonce) and a generic "HTTP <status>" message, so the hint now matches the full error text and falls back to a generic hint covering both mismatch directions when the server sentence is absent. - An out-of-window rejection under time-based mode now reports clock skew. - Cover the time-based "has not increased" branch through wrapHandler, and an SDK-shaped InvalidNonce error end to end. - Load config in a child process with GEMINI_API_NONCE_MODE set, and with an invalid value, to cover the env-to-config startup path. - Add GEMINI_API_NONCE_MODE to the Claude Desktop and ChatGPT README examples.
|
@svc-grace review |
svc-grace
left a comment
There was a problem hiding this comment.
Agentic Review
All four carried-over concerns are resolved — nice progress. I also missed the shared nonce counter’s SDK/REST dispatch issue on an earlier pass; that code hasn’t changed, so it’s a follow-up worth a look, not a blocker. This pass also flags a Windows-specific config test import issue; the inline note has the details.
Review process
Grace version: v0.0.254
Files reviewed (7): README.md, config.test.ts, index.test.ts, index.ts, sdk.test.ts, signer.test.ts, signer.ts
Files skipped: 12
Guidelines: none discovered
Verification: 2 of 5 findings verified
Findings: 0 critical, 0 important, 2 suggestions
Linked tickets: PREDICT-11349, APPSEC-6108
LLM usage: 78 calls — gpt-6-luna: 65 calls, 1685668 tokens, us.anthropic.claude-opus-5-5: 13 calls, 420875 tokens
… file: URL (PREDICT-11349) import() reads a native Windows path such as C:\repo\config.ts as a URL scheme, so the valid-mode config test failed on Windows. Pass a file: URL derived from import.meta.url instead.
|
@svc-grace review |
svc-grace
left a comment
There was a problem hiding this comment.
Agentic Review
The earlier setup-example, environment-config, Windows import, wrapper-coverage, and SDK-hint issues are all resolved. The shared nonce counter still doesn’t serialize SDK and REST dispatch; that’s a non-blocking follow-up if concurrent calls can interleave. This change carries the API key’s configured nonce mode through REST and SDK requests, and the key is keeping both paths aligned on the same mode and counter where incremental signing is used.
Persisted from prior passes (1 open)
Review process
Grace version: v0.0.254
Files reviewed (1): config.test.ts
Files skipped: 23
Guidelines: none discovered
Findings: 0 critical, 0 important, 0 suggestions
Linked tickets: PREDICT-11349, APPSEC-6108
LLM usage: 16 calls — gpt-6-luna: 16 calls, 201994 tokens
Linear Board: https://linear.app/gemini/issue/PREDICT-11349/support-per-key-gemini-api-nonce-modes-in-mcp
What's Included
Per-key nonce mode setting:
GEMINI_API_NONCE_MODE.incremental(default, unchanged PR #75 behaviour) ortime-based. Any other value fails startup. The mode is never inferred from the base URL.Legacy REST signs with the configured unit. Incremental keeps the shared millisecond sequence. Time-based sends epoch seconds with no state, so concurrent requests may share a second, which the server accepts.
SDK auth follows the same mode on every base URL. Previously standard hosts were hardcoded to seconds and custom hosts to milliseconds. Incremental routes through the shared sequence, so SDK and legacy requests never interleave out of order. Time-based uses the SDK's native time-based mode.
WebSocket auth is unchanged. It always uses epoch seconds, as before.
Operator hint on nonce rejection. The tool layer appends the mode to set when the server message identifies a mismatch. A shared incremental key also gets a warning that its sequence is per process.
Alerts daemon and docs.
GEMINI_API_NONCE_MODEis added to the daemon's env whitelist so an installed daemon keeps the mode. The README documents the variable and adds a time-basedclaude mcp addexample.Not in this PR: the onboarding-generated MCP setup (
web/in the gemini monorepo) does not yet emitGEMINI_API_NONCE_MODE=time-basedfor keys created withtimeBasedNonce: true. That change is tracked in a separate web ticket. Until it ships, Agentic onboarding keys need the variable set by hand.Testing
npm test: 368/368 passing (348 existing + 20 new)npm run typecheckcleannpm run buildcleanhttp.tsandsdk.tsfails 5 of the new tests, and the files were restoredGEMINI_API_NONCE_MODE=time-based:gemini_get_balancessucceeds, and so does one SDK-authenticated call (positions)InvalidNonceInvalidNonceand shows the hintManual, against real sandbox and production (to run before merge):
GEMINI_API_NONCE_MODE=time-based: balances should succeed. Before this change they failed withNonce '1794333647192' is not within 30 seconds of server time.What Success Looks Like
GEMINI_API_NONCE_MODE=time-basedon a production onboarding key and get balances, and the same key with the variable unset getsInvalidNonceplus a hint that names the variable.GEMINI_API_NONCE_MODE=bogusand see the server refuse to start with a clear message.npm testpasses and the public-only mode still fails closed for authenticated calls.🤖 Generated with Claude Code