feat(mcp-server): let clients shorten the OAuth token lifetimes - #1788
Conversation
Spendesk asked for short-lived OAuth tokens on the MCP server to force periodic
re-authentication and limit the blast radius of a leaked token. Nothing was
configurable: the access token mirrored Forest Admin's 1h and the refresh token
its 8 days.
agent.mountAiMcpServer({
tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 },
});
Both values are upper bounds, applied as a min() against what Forest Admin
granted, so a client can only ever shorten a lifetime. The cap is applied where
the TTL is computed rather than in the sign() options, which keeps the advertised
expires_in in step with the lifetime signed into the JWT.
The refresh cap is anchored on the interactive login through a sessionStartedAt
claim that is carried across refreshes and never re-stamped. Forest Admin mints a
brand-new full-lifetime refresh token on every refresh grant, so a plain
per-refresh cap would slide forever: an active client would never re-authenticate
and the option would only be an idle timeout. Refresh tokens predating the claim
fall back to their issue date. Once the window elapses the refresh is rejected
before any call to Forest Admin.
Values below 60s are raised to it: the MCP SDK rate-limits /oauth/token to 50
requests per 15 minutes, so a shorter lifetime makes a client exhaust its own
quota and take 429s. Invalid values (zero, negative, fractional) fail at startup
instead of silently leaving the tokens uncapped.
Available embedded and standalone (FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS,
FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS), since standalone is the only mode
available to non-Node back-ends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (4)
🛟 Help
|
Addresses both review findings, which share one cause: the session deadline was enforced at a single point in time and against a single token. An access token could outlive the session. generateAccessToken bounded it only by accessTokenSeconds and the Forest lifetime, so refreshing near the deadline handed out a token valid well past sessionStartedAt + refreshTokenSeconds — with a 24h session and a 1h access token, up to an hour of access without an interactive login. The deadline could also lapse mid-flight. exchangeRefreshToken checked it before the Forest round trips, then the TTL was recomputed after them, so a refresh started just before expiry signed a refresh token with a non-positive lifetime: an already-dead token returned as a success instead of a clean InvalidGrantError. The remaining session lifetime is now measured once, after those round trips, and bounds both tokens; a non-positive value rejects the grant. capRefreshTokenTtl collapses into sessionRemainingSeconds so the anchor arithmetic lives in one place, and capAccessTokenTtl becomes the generic capTtl. Also warns once per process when a configured value exceeds the lifetime Forest Admin granted. The min() silently neutralizes it, which reads as "my setting is ignored" — the same confusion raised while the option was being designed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up, three defects. An expired session raised the wrong OAuth error code depending on timing. The in-flight check throws InvalidGrantError from inside generateAccessToken, but exchangeRefreshToken's catch rewrapped everything as InvalidRequestError, so the same decision surfaced as invalid_grant when caught pre-flight and invalid_request when the Forest round trips outlasted the session. Only invalid_grant makes the client drop its refresh token and re-run the interactive login, so in the window this check exists to close the user got a dead end instead of the re-login the option promises. The passthrough is narrowed to InvalidGrantError rather than OAuthError, leaving Forest's own error surface untouched. The test asserted the message by substring and passed on the wrong class; it now asserts the class. `refreshTokenSeconds` was reported as having no effect when it exceeded one Forest refresh lifetime. It has no inert case: it bounds the whole session, which Forest re-extends on every refresh, so any value binds however large. Warning about it told operators to delete a working control. Only the access cap can be inert. The clamp log said "ignoring" while applying the 60s minimum — the opposite reading, since the result is a 60s lifetime rather than an uncapped one. Also pins the anchor's precedence: flipping `sessionStartedAt ?? iat` passed the whole suite because each test supplied only one claim, while every real refresh token carries both. That mutation is the silent no-op the anchor exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed in depth. The design is right and I want to say so before the criticism: I verified your central argument against the SaaS source, and it holds — Two blockers, both narrow to fix. Everything below was run against 🔴 1. The
|
| Path | Error that reaches the wire | Client outcome |
|---|---|---|
Pre-flight :287-296 (window already elapsed) |
invalid_grant |
✅ re-login — works as designed |
sessionRemaining <= 0 :424-425 (session expires during the Forest round trips) |
invalid_request |
❌ hard fail, no re-login |
Why the code and not the message decides this, in the SDK you're on:
server/auth/handlers/token.js:124-127serialiseserror.toResponseObject()verbatim, so the provider's error class is the client-visibleerrorfield.client/auth.js:177-187—auth()invalidates credentials and restarts the interactive flow only forInvalidGrantError/InvalidClientError/UnauthorizedClientError. Any otherOAuthErrorhits the barethrow error.invalid_requestis precisely the code the client treats as unrecoverable.
The catch has no instanceof filter, so this is wider than the new guard — and the widest case is routine, not a race:
| Inner error | Native code → client | After the catch |
|---|---|---|
InvalidGrantError :425 (new) |
invalid_grant → re-login |
invalid_request → hard fail |
CustomOAuthError('invalid_grant') :356 — user removed from the project, session revoked |
invalid_grant → re-login |
invalid_request → hard fail |
CustomOAuthError('server_error') :356 — Forest 5xx |
server_error → re-login |
invalid_request → hard fail |
plain Error from :380 / :395 / getUserInfo :399 |
server_error → re-login |
invalid_request → hard fail |
For the "user removed from the project" row our JWT is still valid, so the pre-flight and jwt.verify both pass, Forest replies invalid_grant, and the assistant hard-fails forever without ever offering a login. That one is pre-existing — but this PR stakes a re-authentication feature on the same chassis, so the one-line fix now buys a lot more than the race path:
} catch (error) {
// Preserve the OAuth code: `invalid_grant` is what tells the client to re-authenticate,
// and `invalid_request` is the one code it treats as unrecoverable.
if (error instanceof OAuthError) throw error;
...
throw new ServerError(`Failed to refresh token: ${message}`);
}ServerError rather than InvalidRequestError for the genuinely unexpected residue — correct per RFC 6749, and it's also the only code the SDK client still recovers from. Both OAuthError and ServerError are exported from @modelcontextprotocol/sdk/server/auth/errors.js. The same change applies at :229-231 on the authorization-code path.
🔴 2. warnIfCapIsInert tells the operator the refresh cap does nothing, in exactly its flagship configuration
:410-414 compares refreshTokenSeconds against the lifetime of one Forest refresh token. That denominator is right for the access token and wrong for the refresh one: with the session anchor, the uncapped baseline isn't 8 days, it's unbounded — which is your own argument at token-ttl.ts:58-59. So any finite cap has an effect, and the warning is unconditionally false for that field.
Measured with refreshTokenSeconds: 2592000 (30 days) and Forest granting 7 days:
- at login:
tokenTtl.refreshTokenSeconds=2592000 exceeds the 604800s lifetime granted by Forest Admin and has no effect - at day 29: refresh token signed with
expiresIn: 86400— the day left in the window, not Forest's 604800 - at day 30:
InvalidGrantError→ browser re-login, i.e. exactly what the ticket asked for
"Force re-auth monthly" with 8-day upstream tokens is the flagship use case, and we log that it does nothing. The rational operator response is to delete the setting, removing the only absolute session bound that exists.
Same claim in three more places, and contradicted in place:
server.ts:145-147— "so a value above Forest Admin's own TTL has no effect", then two lines later "refreshTokenSecondsbounds the time between two interactive logins". Both can't be true.README.md:116vsREADME.md:135— identical contradiction.- the
warns for each cap that exceeds the lifetime Forest Admin grantedtest (test/forest-oauth-provider.test.ts:1065) regression-protects it.
Suggest scoping the helper to accessTokenSeconds (where it's correct), dropping the :410-414 call, and fixing the two doc surfaces. packages/mcp-server/CLAUDE.md:16 already describes the design correctly.
🟠 Important
3. The tests can't see blocker 1. Both rejection tests assert .rejects.toThrow('Refresh token has expired') — a substring match that passes for InvalidGrantError('Refresh token has expired') and for InvalidRequestError('Failed to refresh token: Refresh token has expired'). forest-oauth-provider.test.ts:1185 and :1203 read identically yet emit different codes, and nothing in the suite can tell them apart. .rejects.toMatchObject({ errorCode: 'invalid_grant' }) on both. Beyond that, refreshTokenSeconds has zero coverage above the mocked provider: the new server.test.ts block only exercises accessTokenSeconds on the authorization-code grant, and the existing refresh e2e tests stop at expect(response.body.error).toBeDefined() (:678, :711). One e2e POST /oauth/token with an elapsed session asserting body.error === 'invalid_grant' would pin the actual contract — and a mid-flight variant would have failed today. The harness already exists at server.test.ts:607-666.
4. ignoring vs clamping. token-ttl.ts:25-29 logs ignoring tokenTtl.${field}: minimum value is 60 seconds and then return MIN_TOKEN_TTL_SECONDS. It clamps and enforces; nothing is ignored. An operator who sets 30 reads "ignoring" and concludes their access tokens keep Forest's 3600s, when they actually live 60s — and if they then debug unexplained 429s on /oauth/token, this log line is what makes them rule out the TTL setting. README.md:137 states the real behaviour, and the test name says clamps, while the test asserts the misleading string verbatim (test/utils/token-ttl.test.ts:52). Suggest tokenTtl.${field}=${value} is below the ${MIN}s minimum and was raised to ${MIN}s.
5. An empty env var silently disables the control. parse-token-ttl.ts:8 — if (!accessTokenSeconds && !refreshTokenSeconds) return undefined. FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS="" is indistinguishable from unset: no cap, no log. The partial case is the dangerous one — access set, refresh empty: the operator watches short access tokens work correctly and reasonably concludes both settings took effect, while the only bound on time-between-logins is off. That's the most common deployment misconfiguration there is (=$UNSET_VAR, an unrendered Helm value, a valueFrom on a missing ConfigMap key), and README.md:137 explicitly promises the opposite: "fails at startup rather than silently leaving the tokens uncapped". Suggest distinguishing undefined from '' and throwing on ''. (Minor, same file: " " currently throws Invalid tokenTtl.accessTokenSeconds "0", reporting a value nobody typed — .trim() fixes the diagnostic.)
6. The refresh-token claim schema can drift silently, and it fails open. The sign() payload (:448-459) and the decoded type (:241-249) describe the same wire format but are fully decoupled by the as typeof decoded cast at :252. Rename sessionStartedAt on the producer side only and ?? decoded.iat takes over — and iat is re-stamped on every rotation, so the window slides forever and the cap never forces a re-login again. That's the exact failure mode token-ttl.ts:58 exists to prevent, with no error and no log. Both sign and verify are mocked in the suite, so nothing catches it. A named RefreshTokenClaims used at both ends turns it into a compile error. Related: the ?? nowInSeconds tail resets the anchor to now — unreachable today since jsonwebtoken always stamps iat, but a fail-open default on a security bound is worth at least a comment.
7. Two docs promise more than the code delivers.
README.md:135— "It is measured from the login itself" is unconditional, but at rollout?? decoded.iatanchors on the last rotation, not the login. An assistant refreshing hourly always presents aniatminutes old, so enablingrefreshTokenSeconds: 86400gives a three-week-old session another 24h. Your PR body is honest about this ("one last window, then bounded"); the README isn't. A caveat is the minimum; rejecting tokens withoutsessionStartedAt(one re-login at deploy) is the strict option.README.md:68— the default forFOREST_MCP_REFRESH_TOKEN_TTL_SECONDSis listed as "Forest Admin's own lifetime". Per the SaaS trace at the top of this comment there is no ceiling at all when it's unset: refresh at least once per 8 days and you never sign in again. Combined with blocker 2 telling operators that values above 8 days do nothing, the docs steer them away from the entire useful range. Suggest "unbounded — Forest Admin re-grants its refresh lifetime on every refresh".
8. Log levels are inverted on the new paths. :291-296 logs at Error what is the feature's success path — a session reaching its configured limit is the control working. With the documented 86400 that's one ERROR per user per day forever, which trains operators and alerting to ignore Error from [ForestOAuthProvider] — exactly where blocker 1 needs to be visible. Meanwhile :424-425, the only genuinely anomalous branch in the new code, logs nothing at all. Suggest Info at :291 reworded as an outcome, and a Warn at :424 including roundTripSeconds — that's the field that distinguishes a real expiry from Forest latency, and it's unobtainable today. (Separately: exchangeAuthorizationCode's catch at :229-231 has no this.logger call at all, so a failed interactive login leaves zero server-side trace. Pre-existing, but this PR touches that function at :215.)
Smaller things
cli.ts:15-18—parseTokenTtl(access, refresh)takes two adjacent positionalstring | undefined. Swapping them compiles, type-checks, passes every test, and turnsaccess=900, refresh=86400intoaccess=86400: 24× longer, in the exact direction the feature exists to prevent. An object parameter makes it impossible.- The 60s floor protects less than its comment claims. The rationale at
token-ttl.ts:3-4is accurate and well sourced — I confirmedwindowMs: 15*60*1000, max: 50inhandlers/token.js:62-63and thatserver.ts:521doesn't override it. ButtokenHandlerpasses nokeyGenerator, soexpress-rate-limitbuckets per IP, andserver.ts:427setstrust proxy 1. "a client exhausts its own quota" understates it: ataccessTokenSeconds: 60, four clients behind one egress IP blow the window, and429→TooManyRequestsErroris neitherServerErrornorInvalidGrantError, so it hard-fails through the same mechanism as blocker 1. Also worth noting the comment derives 18s but the constant guards 60s — the clock-drift reasoning that justifies the gap is in your PR description but not in the code. :428"Capped here, not in the sign() options" —expiresInis passed as asign()option at:444. The intent (compute once, before signing, so the response and the JWT can't disagree) is right and worth keeping; the wording contradicts itself.- No cross-field sanity check:
{ accessTokenSeconds: 3600, refreshTokenSeconds: 60 }is accepted and forces a browser login every minute. Almost certainly swapped values — a warn would catch the same mistake as the positional-args issue above. field: keyof TokenTtlOptionsinnormalizeSeconds,warnIfCapIsInertandfieldsWarnedAsInert: those strings surface in user-facing errors and in the README table, so a rename desynchronises them silently today. Free, and uses a type that already exists.
What I actually verified
- Both blockers reproduced with throwaway tests executed against
5d85f21, not inferred. Blocker 2's numbers (86400 signed at day 29, rejection at day 30) are assertions that passed. - Your
expires_in: expiresIn > 0 ? expiresIn : 3600analysis checks out: the line is pre-existing (origin/main, same file), and the cap genuinely cannot newly reach it, sincesessionRemaining <= 0throws at:424and every normalised cap is>= 60. Leaving it out was the right call. One caveat —CLAUDE.md:16now assertsexpires_in"stays in step with the signed JWT", which is true everywhere except that branch, so it's worth scoping the sentence. - The direction of every rounding is safe:
sessionStartedAtcaptured before the round trip andnowInSecondsre-measured after, so slow upstreams shorten sessions. Pre-check and post-check reduce to the same condition, so no off-by-one atelapsed == cap. Tightening the cap applies immediately to live sessions; loosening it doesn't extend already-issued tokens. 549 tests passedand lint clean on the touched files locally. Caveat: 3 suites (server.test.ts,index.test.ts,tools/get-action-form.test.ts) failed to compile in my sandbox on a stale@forestadmin/datasource-toolkit(ActionField.getPlainFieldmissing) — unrelated to this PR, and I re-ran your newtokenTtl optionblock separately, 2/2 green. So I did not reproduce the 681 figure.
test/utils/token-ttl.test.ts is the strongest file here, by the way — property-based rather than implementation-based ("never extends", "shrinks instead of sliding"), the invalid-input matrix covers both fields, and the 59/60 boundary is distinguished. And bounding the access token by sessionRemaining closes the hole that verifyAccessToken never checking the anchor would otherwise leave wide open; the comment at :416-417 should definitely stay.
Without the numbers a reader cannot tell whether their value will bind. Added to the JSDoc on both the option and the type — that is the copy people read in their IDE — plus the env table and the README section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to @matthv's review. Widened the error passthrough from InvalidGrantError to OAuthError. The catch was flattening every Forest error into invalid_request, the one code the SDK client treats as unrecoverable (client/auth.js:150-162 retries only on invalid_grant, invalid_client and unauthorized_client, and maps the wire code back to a class). So a session Forest itself revoked — user removed from the project, token revoked — hard-failed forever without ever offering a login. Six tests asserted that flattening; they now assert the propagated code. Kept InvalidRequestError for the non-OAuth residue: ServerError reads better per RFC but the client recovers from neither, so it buys nothing here. An env var set to an empty string was indistinguishable from unset, silently leaving that token uncapped — worst when only one of the two is empty, since the operator sees short access tokens working and assumes both took effect. It now coerces to NaN and is rejected, matching what the README already promised. parseTokenTtl takes an object: swapping two adjacent positional strings compiled, type-checked and applied the refresh cap to access tokens. The refresh claims are now a named type used by both the sign() payload and the verify() cast, so renaming one side is a compile error instead of a silent fallback to iat — which is re-stamped on every rotation and would slide the window forever. Log levels were inverted: a session reaching its limit is this option working, not an error, and with the documented 86400 it fired once per user per day. It drops to Info, while the genuinely anomalous branch (round trips outlasting the session) now warns with the elapsed time, the only way to tell it from a plain expiry. Docs: the refresh default is unbounded, not 8 days — Forest re-grants its lifetime on every refresh, so unset means never signing in again. Also documents that tokens predating the option anchor on their last refresh for one longer session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The company renamed. Scoped to the lines this branch adds; the rest of the repo still carries the old name and needs its own sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — genuinely useful review. All of it is addressed. Two of your findings were already fixed in 🔴 1 — fixed, and you pushed me further than I had goneI had already narrowed this to The tests were pinning the bug. One correction: 🔴 2 — was already fixed
🟠 3 — both assertions now check the wire codeSwitched to 🟠 4 — was already fixed
🟠 5 — fixed
🟠 6 — fixedNamed 🟠 7 — fixed, and one of these was my own regressionThe default was wrong in a way I had made worse: I changed it to 🟠 8 — fixed
SmallerDone: object parameter for Deliberately not done: the cross-field warn for Still openThe e2e refresh-grant coverage from your point 3. Everything about On your sandbox caveat: |
The JWT the server signs is not encrypted and carries the Forest token under a serverToken claim, so a leaked MCP access token yields a Forest credential usable against the agent for its own remaining lifetime — bypassing the scopes and the audit log the MCP path enforces. Claiming the cap shortens how long a leaked token stays usable promised more than the code delivers. Also drops the JSDoc over-generalisation: only accessTokenSeconds is bounded by a Forest lifetime, refreshTokenSeconds bounds an otherwise unbounded session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Re-reviewed at
The Also worth calling out Three things still open, one of which I'd fix before merging. 1. `sessionStartedAt=${sessionStartedAt}, roundTripSeconds=${nowInSeconds - sessionStartedAt}`
The comment two lines above says "the elapsed time is the only way to tell that from a plain expiry", and that's the right instinct — but as computed, the value is ~constant at the cap and can't make that distinction. Either diff against the 2.
3. A stale note now contradicts the tests below it — That was true before this PR; the six tests directly underneath it now assert the real codes. Just delete it. Smaller, take or leave:
Locally: 554 tests pass (549 before the fixes), From my side this is good to merge once #1 is sorted — the design was right from the start, and the fixes landed where they needed to. |
Second review round from @matthv. roundTripSeconds was `nowInSeconds - sessionStartedAt`, i.e. the session age. Since that branch is only reached when the session has lapsed, the field always printed at least the configured cap — 86404 on a request whose round trips took 5s — so it could never do the one job the comment claimed. It now diffs against a timestamp captured at the top of generateAccessToken, before the Forest calls. RefreshTokenClaims caught a rename but not an omission: `sessionStartedAt?` was optional on the shared interface, so deleting it from the signed payload still typechecked and `?? iat` took over silently. It is now required on the write side, with a separate read type accepting it missing for tokens minted before the claim. Verified: omitting it is TS2741. Adds the cross-field warn two reviewers asked for — a session shorter than one access token is almost always the two values swapped. Drops a stale note in server.test.ts that contradicted the six assertions under it, restores the comment explaining the 3600 fallback (accurate, and the line had no explanation left), scopes the CLAUDE.md expires_in claim to exclude the already-expired branch, and drops the [ForestOAuthProvider] prefix from the clamp warning — that one is emitted from the ForestMCPServer constructor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three fixed. Thanks for re-running the probes rather than reading the diff — and for catching that 1 —
|
zod is already a runtime dependency of this package (^4.3.5, used by every tool
schema), so this adds no dependency — and it follows the boundary-validation pattern
workflow-executor documents for anything crossing a trust boundary.
Three casts were unchecked. The two Forest tokens are only `decode`d, never verified
— we hold no key — so their shape was entirely unvalidated: a missing or non-numeric
`exp` put NaN into the TTL arithmetic and then into sign(). Our own refresh token is
signature-verified but its payload shape was not, which was the one hole the required
`sessionStartedAt` claim could not close: the compiler now rejects a rename or an
omission, but not a verify() returning a different shape at runtime.
The refresh schema makes `type: z.literal('refresh')` part of the shape, so the parse
subsumes the old type check and a signature-valid token we cannot read is refused
before any call to Forest.
Drops the hand-written DecodedRefreshToken, now redundant with the schema.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither earned its keep.
parse-token-ttl existed to coerce env strings, but Number() already trims (' 900 '
→ 900) and turns '' and ' ' into 0, which the positive-integer check already
rejects. So the trim was dead and the empty-string-to-NaN trick only changed the
rejected value printed in the message from "0" to "NaN" — both values nobody typed.
Its "both undefined → undefined" guard also duplicated normalizeTokenTtl's own.
Replaced by one expression at the only call site.
capTtl was a one-line Math.min wrapper with a single production call site, and it
introduced a second convention for "no bound" (undefined) alongside the Infinity
that sessionRemainingSeconds already returns. Inlining it unifies the sentinel and
keeps the property that matters — expiresIn computed once, before signing.
Its unit tests go with it; the same property is asserted with exact values at the
provider level ("shortens both token lifetimes", "never extends beyond the
lifetimes Forest granted").
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The zod schemas were stricter than the tokens they parse, which on the login path
means nobody signs in.
private-api's auth-token.ts types the payload as `meta?: { renderingId?: number }` —
optional both ways — and the cast this replaced tolerated a stringified id, since it
only ever fed String(renderingId) and getUserInfo. A strict z.number() would have
rejected a token the previous code accepted.
Coerced with int().positive() rather than plain coercion: Number(null) is 0, and an
exp of 0 means expired in 1970, so bare coercion would have let the NaN class of bug
back in through a different door.
Pins that a valid token still passes, including one carrying renderingId as a string.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I justified parsing the SaaS tokens with "a NaN reaches sign()". That is true and harmless: jsonwebtoken.sign rejects a NaN expiresIn, a missing meta already throws on destructuring, and a stringified exp already worked through JS coercion. Every malformed shape failed loudly, so a schema there bought a nicer message — against the risk of rejecting a token the SaaS legitimately sends, which breaks login. That risk materialised twice while writing it. Reverted to the cast. Our own refresh token is the opposite case and keeps its schema: if verify() returns a shape without `sessionStartedAt`, the `?? iat` fallback takes over with a timestamp re-stamped on every rotation, so the session window slides forever with no error and no log. Silent, and a security bound. That is what the parse is for. The Forest-token rejection tests go too: jsonwebtoken is mocked in that suite, so the loud failure they claimed to pin is invisible there. A test that cannot observe its property is worse than none. The tolerance test stays — it guards against someone re-adding a strict schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removed the three that repeated a fact stated elsewhere in the same file (the
sessionStartedAt rollout note, duplicated on the type; the anchor note, same) or that
paraphrased visible code (the parse subsuming the type check, when z.literal('refresh')
sits two lines above). Shortened five others. Deleted all seven in the test file — the
test names already carry the intent.
What survives is the set where the reason is not deducible from the code and where a
future edit would silently break something: the measured rate limit behind the 60s
floor, the SaaS re-granting its refresh lifetime (the whole reason for the anchor), why
the SaaS tokens are cast rather than parsed, why only the access cap can be inert, why
the OAuth code must pass through, why the TTL is computed before signing, and why the
3600 branch is unreachable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The schema and the interface declared the same five claims independently, so renaming `sessionStartedAt` in the schema alone compiled fine while the parse silently stopped finding the claim and `?? iat` slid the window — only a runtime test stood in the way. The write type is now derived from the schema (Required<Pick<...>>), so both a schema rename and a payload omission are compile errors; verified as TS errors by mutation. Also collapses warnIfCapIsInert's per-field Set to a boolean: since the refresh warning was removed there is one call site and one field, and the keyof parameter was generality for a case that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Heads-up before anyone approves: six commits landed after your re-review of The revertI had added zod schemas to all three token payloads. Asked what the parse actually bought on the SaaS tokens, I measured it instead of assuming — and my own justification did not hold:
So nothing failed silently; the schema bought a nicer message. Against that, a schema stricter than the real token breaks login for everyone — and it did, twice, while I wrote it: first Reverted to the cast on both SaaS tokens ( I also dropped the tests that claimed to pin the loud failure — Your points from the second round
Also worth your eyes
Net since your review: -70 lines, |
qlty flagged the diff at 96.7% against a 98% threshold, on lines 400-414 — the two 'not a decodable JWT' guards. They predate this branch but count as modified because the rename touched their messages, and they were never covered. The zod rejection tests I removed had been covering them incidentally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# @forestadmin/mcp-server [1.20.0](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/mcp-server@1.19.1...@forestadmin/mcp-server@1.20.0) (2026-07-29) ### Features * **mcp-server:** let clients shorten the OAuth token lifetimes ([#1788](#1788)) ([6174cc2](6174cc2))
# @forestadmin/agent [1.91.0](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/agent@1.90.4...@forestadmin/agent@1.91.0) (2026-07-29) ### Features * **mcp-server:** let clients shorten the OAuth token lifetimes ([#1788](#1788)) ([6174cc2](6174cc2)) ### Dependencies * **@forestadmin/mcp-server:** upgraded to 1.20.0
2 participants
Spendesk asked to configure short-lived OAuth2 tokens on the Forest MCP server, to force periodic re-authentication and limit the blast radius of a leaked token. Nothing was configurable today: the access token mirrors Forest Admin's 1h and the refresh token its 8 days.
API as agreed in the thread (validated by @gautierd, @pierre.merlet and Guilhem):
Standalone equivalent:
Shorten only, never extend
Both values are applied as
min(forestLifetime, configured), so a client can only shorten. The cap is applied where the TTL is computed, not in thesign()options — that is what keeps the advertisedexpires_inin step with the lifetime actually signed into the JWT.Why the refresh token needed more than a min()
min()would have shipped a feature that does not do what the ticket asks — exactly what @julien.jolles flagged in the thread.refreshAccessTokenon the SaaS side (forestadmin-server,oauth-authorization-service.ts:212) deletes the presented refresh token and then callsgenerateTokens, which mints a brand-new 8-day refresh token (:342/:349). No session anchor is carried anywhere. The refresh chain is an unbounded sliding window: a client that refreshes at least once every 8 days is never asked to re-authenticate.So a per-refresh cap would only be an idle timeout ("if the client goes quiet for N seconds, log in again"), not periodic re-auth.
Fix: anchor the chain on the interactive login with a
sessionStartedAtclaim that is carried across refreshes and never re-stamped. Effective refresh TTL becomesmin(forestRemaining, sessionStartedAt + cap - now), which is still<= forestRemaining, so the shorten-only rule holds. Refresh tokens minted before the claim existed fall back to theiriat(one last window, then bounded). Once the window elapses the refresh is rejected before any round trip to Forest Admin.Precedent for this shape already in the repo:
packages/agent-bff/src/oauth/session-store.ts:148,167-173keeps an absoluteexpiresAtwhile swapping the SaaS tokens underneath.The 60s floor is measured, not guessed
The MCP SDK rate-limits
/oauth/tokentowindowMs: 15min, max: 50(handlers/token.js:31-39) andserver.ts:510does not override it → one token request every 18s. Below that a client exhausts its own quota and takes429s. 60s leaves ~30% of the budget and stays clear of inter-instance clock drift, since bothverifyAccessTokenand the SDK's bearer middleware compare timestamps with a zero clock tolerance. Values under 60s are raised to it with aWarn.Invalid values (zero, negative, fractional, NaN, non-number) throw at startup: a security control that silently degrades to a no-op is worse than one that refuses to boot, since the customer would believe their tokens are short-lived when they are not.
Deliberately left alone
expires_in: expiresIn > 0 ? expiresIn : 3600— that branch is only reachable when the Forest token is already expired (needs >1h of clock skew), and a cap can shorten a positive lifetime but never drive it to zero, so this change cannot widen it. A test pins it so the carry-over is visible rather than accidental. Fixing it properly means throwing instead of forging a dead token, which is a new failure mode on the login path and deserves its own PR.Tests
test/utils/token-ttl.test.ts— validation table per field, the clamp and its boundary, and the security rule (capAccessTokenTtl(900, 3600) === 900)test/forest-oauth-provider.test.ts— frozen clock so lifetimes are asserted as exact values; the crux test asserts the refresh window shrinks as the session ages (6400) even though Forest returns a fresh 8-day tokentest/server.test.ts— one end-to-endPOST /oauth/tokencovering options → constructor → provider → JWT:expires_in === 120andexp - iat === 120on the issued tokentest/cli.test.ts,packages/agent/test/agent.test.ts— env parsing and pass-through681 passing in mcp-server, 1006 in agent, lint clean on both. Each new test verified red before / green after.
Both packages are touched in one PR so
multi-semantic-releasebumps the pin in the same run.Docs for the public site follow in
ForestAdmin/docsanddeveloper-guide(separate repos).🤖 Generated with Claude Code
Note
Allow clients to cap OAuth token lifetimes in the MCP server
tokenTtloption (accessTokenSeconds,refreshTokenSeconds) toForestMCPServerandAgent.mountAiMcpServer, capping issued OAuth token lifetimes below Forest's upstream expiry.sessionStartedAtin refresh token claims so the refresh cap applies to the total session window, not per-rotation; once the session window expires, refresh attempts fail withinvalid_grant.FOREST_MCP_ACCESS_TOKEN_TTL_SECONDSandFOREST_MCP_REFRESH_TOKEN_TTL_SECONDSenvironment variables via a newparseTokenTtlhelper in parse-token-ttl.ts.expires_inin token responses now reflects the signed JWT TTL rather than the upstream Forest value; OAuth error codes from Forest (e.g.invalid_grant) are now preserved instead of being re-wrapped asinvalid_request.Changes since #1788 opened
ForestOAuthProvidermodule and updatedexchangeRefreshTokenandexchangeAuthorizationCodemethods to validate decoded tokens [9bb8e63]exchangeAuthorizationCodeandexchangeRefreshTokenmethods [9bb8e63]parseTokenTtlfunction fromparse-token-ttlutility module andcapTtlfunction fromtoken-ttlutility module [5f21e99]parseTokenTtlutility usage in CLI module with localtoSecondshelper function for converting environment variables to token TTL configuration [5f21e99]capTtlfunction call with inlineMath.minlogic inForestOAuthProviderclass token expiration computation [5f21e99]parseTokenTtlandcapTtlfunctions [5f21e99]ForestAccessTokenSchemaandForestRefreshTokenSchemavalidation to coerce string values to numbers and enforce positive integer requirements forexpfields, and additionally formeta.renderingIdin access tokens [371315a]ForestOAuthProvider.exchangeAuthorizationCodeto verify handling of string-typedmeta.renderingIdvalues in access tokens [371315a]ForestOAuthProvider.exchangeAuthorizationCode[8326551]mcp-serverpackage [06b5a40]ForestOAuthProviderclass [53a442d]RefreshTokenClaimsfrom interface to type alias derived from Zod schema [53a442d]ForestOAuthProvider.exchangeAuthorizationCodethrows errors when tokens are not decodable JWTs [db9e8f0]Macroscope summarized f685a7c.