Fix six pre-tag review findings, five verified and one disproven - #140
Conversation
An adversarial review of main before the v3.0.0 tag raised three "release blockers" and four lesser items. Every claim was reproduced against the code before acting on it. One blocker does not hold; the rest do. DISPROVEN. publish.yml was said to need `actions: read` for its environments API call, and to be destined for a 403 even after #50 is configured. The repo is public and that endpoint reads with no credentials at all: GH_TOKEN="" GITHUB_TOKEN="" gh api \ repos/karthikcsq/google-tools-mcp/environments/npm-publish --jq .name npm-publish A GITHUB_TOKEN scoped `contents: read` is strictly more privileged than none, so the check works as written. Left alone rather than cargo-culting a permission it does not need. FIXED, in dist/: - auth.js awaited server.listen() on a resolve-only Promise. listen reports failure by emitting 'error', so a busy GOOGLE_MCP_OAUTH_PORT never settled the flow: the EADDRINUSE reached the process handler in index.js, which logs and returns, and the caller waited forever. clients.js has always carried a port_in_use remedy for exactly this, unreachable until now. - auth.js sent no `state` and accepted any callback carrying a code. During the five-minute window a page the user visits could hit localhost:<port>/?code=<attacker's code> and this server would persist the ATTACKER's refresh token. Now issues a random state, compares it in constant time, and IGNORES a mismatch rather than rejecting, so nobody who can reach the port can cancel a sign-in in progress. PKCE S256 added on the same flow. - clients.js ensureAuth() checked authClient then awaited with nothing holding the gap. Six concurrent cold requests produced six authorizations, which cold means six browser windows racing one loopback port. reauthorize() had the same hole and nulled shared clients out from under concurrent rebuilds. Both latch the in-flight Promise and release it on settle. - mcpServer.js answered /healthz above the `closed` check, so a drained handler reported 200 {"status":"ok"} while /mcp already 503'd. The one probe meant to notice a dead runtime was the only route that never did. FIXED, in the live harness (not shipped in the tarball): - The cleanup registry could leave real files in a real Drive and still print "cleanup N/N". live-mission and live-call each kept their own copy of the creating-tools map under a "kept in sync" comment, and both read JSON.parse(result).id only. createDocumentFromTemplate answers in prose, so JSON.parse threw and the id was dropped; createPresentation answers with presentationId. One shared extractor now handles every shape, and an unextractable id fails the run as UNTRACKED instead of passing quietly. Running it live also showed the guard denies Slides creation outright, so only the createDocumentFromTemplate half was ever reachable -- both are pinned by live/missions/verify-created-resource-tracking.mjs. - live-coverage counted tools that can never reach Google. checklist-5 calls forwardMessage once solely to assert the runner blocks it, and that inflated the covered count. Blocked and guard-denied tools are now a separate bucket, the matched `expectError` alternative is gone (no such helper exists), and the output states what the number does and does not prove. DOCS, three code/doc contradictions: - CHANGELOG and README both promised stdio users nothing changes, while http-mode.md said a Codex stdio registration requires CODEX_MCP_PROTOCOL_VERSION. A Codex user trusting the release notes upgrades into a broken server. Both now carve out Codex. - Three places promised /healthz returns exactly {"status":"ok"} with no environment identity. It returns a pid, and that pid is load-bearing: httpLifecycle.js compares it against the state file to prove the process on the port is the one it started. The docs were wrong, not the code. - The #1 round-trip loss is now stated as a known limitation. Google drops all-zero RGB, so #14 forces some non-zero stand-in, and any value chosen is one a person could pick. 94 suites / 1350 passed / 2 skipped. npm audit --omit=dev clean. Tarball unchanged at 184 files. Both live missions pass with the sandbox folder verified empty afterwards. Each fix was confirmed to fail without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Suite count moves 92 -> 94 (1350 passed), the checklist gains the new verify-created-resource-tracking mission and the sandbox-empty check, and the expected live-coverage figures are recorded so a drift is visible. Also records the one review claim that was disproven, so nobody re-adds `actions: read` to publish.yml on the strength of it. The repo is public and the environments endpoint reads unauthenticated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ElliotDrel
left a comment
There was a problem hiding this comment.
Adversarial Review — issue
The two auth latches do not coordinate with each other, so a normal request can still start a second authorization while an invalid_grant re-authorization is already in flight.
Concrete failure sequence:
- An existing API call gets
invalid_grantand entersreauthorize(). performReauthorize()immediately setsauthClientand every cached API client tonull, then awaitsauthorize().- Before that authorization finishes, another HTTP request calls any
initialize*Client(). - That request reaches
ensureAuth().authClientis null andauthInFlightis also null, because the in-progress flow is tracked only inreauthInFlight. ensureAuth()therefore starts a secondperformAuth()/authorize()concurrently. On a cold browser flow, the two authorizations can open two consent windows and race the same configured loopback port, recreating the concurrency failure this PR is intended to eliminate.
The new tests cover many callers sharing authInFlight and many invalid-grant callers sharing reauthInFlight, but not one caller entering through each path at the same time.
Acceptance criteria: make initial auth and re-auth mutually exclusive through one shared in-flight authorization primitive (or make ensureAuth() await reauthInFlight while it exists), and add a regression test where reauthorize() is paused inside authorize() and a concurrent initialize*Client() arrives. The authorize mock should still be called only once for that re-auth episode.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 622df42976
ℹ️ 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".
| if (!authInFlight) { | ||
| authInFlight = performAuth().finally(() => { authInFlight = null; }); | ||
| } | ||
| return authInFlight; |
There was a problem hiding this comment.
Await reauthorization from the normal auth path
In stateless HTTP mode, when one request hits invalid_grant, performReauthorize() clears authClient and sets only reauthInFlight; any unrelated request that initializes a client while that browser flow is pending reaches this block, sees authInFlight unset, and starts a second authorize() call. The flows can open multiple consent windows, fail with EADDRINUSE on a configured callback port, or race to persist credentials. ensureAuth() should await the active reauthorization (or both paths should use one shared latch).
Useful? React with 👍 / 👎.
Pre-tag review of
mainraised three "release blockers" and four lesser items. Every claim was reproduced against the code before acting on it. One blocker does not hold. The rest do, and are fixed here.The one that does not hold
publish.ymlwas said to needactions: readfor its environments API call, and to be headed for a 403 even after #50 is configured. This repo is public, and that endpoint reads with no credentials at all:A
GITHUB_TOKENscopedcontents: readis strictly more privileged than none, so the check works as written. The docs page cited never actually states anActions: readrequirement. Left alone rather than adding a permission it does not need. Worth knowing the caveat: this holds because the repo is public.Shipped code
auth.jsawaitedserver.listen()on a resolve-only Promise.listenreports failure by emittingerror, so a takenGOOGLE_MCP_OAUTH_PORTnever settled the flow — theEADDRINUSEwent to the process handler inindex.js, which logs and returns, and the caller waited forever.clients.js:76has always carried aport_in_useremedy for exactly this; it was unreachable dead code.mcpServer.jsuses the correct pattern 100 lines away.stateon the OAuth flowcode. During the five-minute window, a page the user visits could hitlocalhost:<port>/?code=<attacker's code>and this server would persist the attacker's refresh token — every later tool call running against someone else's Google account, looking entirely normal. Now issues a randomstate, compares it in constant time, and ignores a mismatch rather than rejecting, so nobody who can reach the port can cancel a sign-in in progress. PKCES256added on the same flow.ensureAuth()checkedauthClientthen awaited with nothing holding the gap. Six concurrent cold requests produced six authorizations — cold, that is six browser windows racing one loopback port.reauthorize()had the same hole and nulled the shared clients out from under concurrent rebuilds. Both now latch the in-flight Promise and release it on settle, so a declined consent screen is never replayed./healthzreported a closed runtime as healthyclosedcheck, so a drained handler answered200 {"status":"ok"}while/mcpalready answered503. The one probe meant to notice a dead runtime was the only route that never did.Live harness (not in the tarball)
Both unresolved review threads from #139, now answered and resolved.
Cleanup could leave real files in a real Drive and still print
cleanup N/N.live-missionandlive-calleach kept their own copy of the creating-tools map under a "kept in sync" comment, and both readJSON.parse(result).idonly.createDocumentFromTemplateanswers in prose, soJSON.parsethrew and the id was discarded.createPresentationanswers withpresentationId. The registry is what cleanup iterates, so unregistered means never trashed, andNonly counted what the runner noticed.One shared
createdResource.mjsnow handles every shape, and an unextractable id fails the run asUNTRACKEDinstead of passing quietly.Running it live turned up something neither the review nor I had:
guard.mjs:266deniespresentations.createoutright, because the Slides API creates in Drive root regardless ofparentFolderId. So only thecreateDocumentFromTemplatehalf was ever reachable — the guard was the containment for the other.live/missions/verify-created-resource-tracking.mjspins both.live-coveragecounted tools that can never reach Google.checklist-5callsforwardMessageonce solely to assert the runner blocks it. Blocked and guard-denied tools are now their own bucket, theexpectErroralternative is gone (no such helper exists), and the output states plainly that the number is an upper bound, since a static scan cannot see a schema rejection.Docs: three code/doc contradictions
http-mode.mdsaid a Codex stdio registration requiresCODEX_MCP_PROTOCOL_VERSION. A Codex user trusting the release notes upgrades into a broken server./healthzreturns exactly{"status":"ok"}with no environment identity. It returns apid, and that pid is load-bearing —httpLifecycle.js:119compares it against the state file to prove the process on the port is the one it started. The docs were wrong, not the code.#000001round-trip loss is now stated as a known limitation. Google drops all-zero RGB, so Text inserted by editing tools has no explicit font color — Google Docs shows no color selected #14 forces some non-zero stand-in, and any value chosen is one a person could pick.Verification
npm audit --omit=dev— 0 vulnerabilitiesnpm pack --dry-run— unchanged at 184 files; noscripts/,live/, ortests/live-mission agent-loop-2-fixes— PASS,cleanup 5/5live-mission verify-created-resource-tracking— PASS,cleanup 2/2{"folders":[],"files":[]}mcpServer.jsgivesExpected: 503, Received: 200; revertingclients.jsgivesExpected: 1, Received: 6Does not touch #50, which remains the only thing blocking the tag.
🤖 Generated with Claude Code