Harden MCP request limits, key lifecycle, and connection guides - #7
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c5a1106df
ℹ️ 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".
| var count = keyCounts.get(keyId); | ||
| if (count != null) { | ||
| count.decrementAndGet(); | ||
| } |
There was a problem hiding this comment.
After 10,000 distinct key IDs have completed requests, their zero-valued counters remain in keyCounts, so the MAX_TRACKED_KEYS check permanently rejects every new key on this process even when no requests are in flight. AttachmentUploadLimiter.keyBytes has the same retention behavior, eventually rejecting uploads from new keys as well; remove entries when their final permit or reservation is released, with the removal performed atomically against concurrent acquisition.
Useful? React with 👍 / 👎.
| private static long estimatedDecodedBytes(String encoded) { | ||
| return encoded.length() / 4L * 3L + 3L; |
There was a problem hiding this comment.
For a valid padded Base64 value representing an 8 MiB attachment, this formula reserves 8 MiB plus four bytes. Consequently, four simultaneous maximum-size uploads from one key contain exactly the documented 32 MiB budget but the fourth is incorrectly rejected as RATE_LIMITED; account for the final quantum and padding so the reservation matches the decoded length.
Useful? React with 👍 / 👎.
resolveNumericAddress fed unresolved host strings into InetAddress.getByName, which falls back to a blocking DNS lookup on the event loop for values that slip past Spring's numeric heuristic (e.g. hex-digit + colon prefixes from a client-controlled Forwarded header). Parse strict numeric literals with Guava's InetAddresses.forString instead so non-literals resolve to empty without ever touching DNS.
The length/4*3+3 estimate overstates every upload by up to 3 bytes, so four concurrent 8 MiB uploads summed past the 32 MiB per-key budget and the fourth was rejected even though the exact content fits. Derive the exact decoded length from the Base64 padding instead, clamped at zero for malformed input, and cover the boundary with a test admitting four exact 8 MiB uploads while rejecting a fifth.
Both limiters kept their per-key map entries forever, so after 10,000 distinct keys every new key was permanently rejected until restart. Remove the entry when a release brings a key's count back to zero so long-lived processes recover from key churn.
mcpHandler.handle was invoked synchronously while assembling the inner chain, so a synchronous throw would escape before doFinally attached and leak the in-flight permit until the key's budget was exhausted. Defer the invocation so assembly failures flow through the error path. Also document that access-key revalidation relies on the extension client returning freshly deserialized instances.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 330952a6b8
ℹ️ 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".
The zero-count eviction in 1d9030b raced with acquisition: computeIfAbsent handed out the counter before incrementing it, so a concurrent release could evict it in between, leaving new permits on an orphaned counter that bypasses the per-key limit and miscounts the next generation on release. Run create-or-increment and the limit check inside the key's compute bin, and have release decrement its own captured counter, evicting only when it is still the mapped, zero-valued one. Adds concurrency churn tests that assert the per-key ceiling and full budget drain under parallel load.
The 30s deadline only wrapped the MCP handler, so a stalled extension store lookup or password verification could park a request indefinitely before a permit was ever acquired. Move the timeout and its 503 mapping to the outer chain so the deadline covers authentication and handling alike; permits are still released through the inner doFinally when the deadline cancels in-flight handler work.
Updated the access key secret modal warning to explicitly remind users not to disclose or share the key anywhere, reducing accidental leakage risk. The change keeps the existing guidance about single-time visibility and rotation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dfd3e6a2a
ℹ️ 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".
1 participant
What
Applies six findings from a security review of the MCP endpoint, key lifecycle, and
Console connection guides:
validation were constructed before the key's tool allowlist was enforced. They now run
inside
Mono.deferafterrequire(name)succeeds.addresses all shared one
unknownbucket, letting any unauthenticated caller exhaust itand deny the endpoint for every proxied client. Buckets are now keyed by the canonical
numeric client address, resolved through the same abstraction as the IP allowlist.
or deletion committing during asynchronous password verification could still produce a
token from the old snapshot. Authentication now re-fetches the key after verification
and grants only if the spec is unchanged and still active.
not apply the
requestTimeoutbuilder option, and the transport had no timeout orin-flight bound. Requests now run under an enforced 30-second deadline and concurrency
caps (100 global, 16 per key) with excess requests rejected; the dead option is removed.
close to 1 GiB of decoded arrays per minute per key. Uploads now reserve decoded bytes
against in-flight budgets (64 MiB global, 32 MiB per key) before decoding, released on
success, error, timeout, or cancellation.
into CLI arguments, static config headers, and custom-protocol install URLs. Guides no
longer accept the token: Claude Code and Cursor reference the
HALO_MCP_TOKENenvironment variable at runtime, Codex uses
bearer_token_env_var, and VS Code promptsthrough its secure input storage, including in one-click install URLs.
Two reviewed findings were intentionally left out: the mutable reusable-workflow tag in
cd.yamland the concurrent category reparenting race.Verification
./gradlew test— backend suite passes, including new regression tests per finding(allowlist ordering, forwarded-address buckets, revocation races, timeout and permit
release, budget exhaustion and release).
pnpm test:unit,vue-tsctype-check, lint — pass; guide tests assert no generatedcontent or install URL contains bearer material.