Conversation
- isolate dynamic client redirect URIs per registration - bound token/pending/client in-memory stores - redact sensitive fields in HTTP logs - enforce upload path allowlist and size limits - update OAuth and upload security tests Story: https://app.shortcut.com/internal/story/308997
📝 WalkthroughWalkthroughThis PR introduces OAuth security enhancements including a proxy state mechanism to prevent cross-client collisions in authorization flows, in-memory caching with TTL for tokens and pending authorizations, and file upload validation safeguards. Additionally, it adds HTTP logging sanitization for sensitive data and refactors OAuth integration tests with conditional execution based on local port binding capability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP Server
participant AuthSrv as Authorization Server
participant TokenSrv as Token Server
Client->>MCP: Initiate OAuth authorization
MCP->>MCP: Generate cryptographic proxyState
MCP->>MCP: Create PendingAuthorization entry (proxyState → auth details)
MCP->>AuthSrv: Redirect to auth endpoint with proxyState as state parameter
AuthSrv->>AuthSrv: User grants permission
AuthSrv->>MCP: Redirect callback with proxyState + auth code
MCP->>MCP: Lookup PendingAuthorization via proxyState
MCP->>MCP: Validate redirectUri against registered URIs
MCP->>TokenSrv: Exchange code + static client credentials for token
TokenSrv->>MCP: Return access/refresh tokens
MCP->>MCP: Cache tokens with TTL/eviction
MCP->>Client: Token exchange complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/auth/oauth.test.ts (1)
220-252: Consider extracting shared OAuth HTTP test helpers.Lines 220-252 duplicate helper logic that also exists in
src/auth/oauth-integration.test.ts(canBindLocalPort, registration flow). A shared test helper would reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/oauth.test.ts` around lines 220 - 252, The canBindLocalPort function, the CAN_BIND_LOCAL_PORT constant and registerClient helper are duplicated in src/auth/oauth.test.ts and src/auth/oauth-integration.test.ts; extract them into a shared test helper module (e.g., src/auth/test-helpers.ts) that exports async function canBindLocalPort(), exported const CAN_BIND_LOCAL_PORT (computed once) and registerClient(redirectUris?: string[]) returning ClientInfoResponse, then import those symbols into both oauth.test.ts and oauth-integration.test.ts and remove the local duplicates; ensure you preserve behavior of createServer/probe listen/close and the fetch/expect flow in registerClient and update imports/exports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth/provider.ts`:
- Around line 290-297: issuedTokenOrder is never pruned when issuedTokens
entries are removed, allowing the array to grow unbounded; implement a helper
(e.g., deleteIssuedToken(tokenId: string)) that deletes from the issuedTokens
Map and also removes the tokenId from issuedTokenOrder, update any direct
issuedTokens.delete(...) calls to use deleteIssuedToken, and ensure when
enforcing MAX_ISSUED_TOKENS you pop/shift oldest IDs from issuedTokenOrder and
call deleteIssuedToken on them so both structures remain in sync.
- Around line 551-573: The current code stores the raw client `state`
(params.state) into pendingAuthorizations and embeds it in the proxy state,
which can cause unbounded memory/URL pressure; modify the logic around
makeProxyState/normalizeClientState and pendingAuthorizations.set to enforce a
maximum client-state size: add a MAX_CLIENT_STATE_LENGTH constant,
validate/truncate (or replace with a short cryptographic hash) of
normalizeClientState(params.state) before calling makeProxyState and before
persisting clientState in the pendingAuthorizations entry so only a bounded-size
value (or its hash) is stored and relayed.
In `@src/client/shortcut.test.ts`:
- Around line 479-491: The test restores environment variables by assigning
previous values back to process.env which will set them to the string
"undefined" if they were originally undefined; update the teardown code in
shortcut.test.ts (the blocks around client.uploadFile / mockClient.uploadFiles
and the rmSync cleanup) to check the saved previous values for
SHORTCUT_UPLOAD_ALLOWED_DIRS and SHORTCUT_UPLOAD_MAX_FILE_BYTES and if a saved
value is undefined use delete process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS (and
delete process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES) otherwise reassign the saved
value, ensuring you update all identical restore sites (the blocks that
currently set process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS = previousAllowedDirs and
process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES = previousMaxBytes).
In `@src/client/shortcut.ts`:
- Around line 2-3: The path containment check uses the value `rel` returned by
`relative()` but doesn't reject absolute paths (which on Windows can occur
across drives), so update the code to import `isAbsolute` from "node:path"
alongside `relative`/`resolve`/`sep` and change the containment condition to
also reject when `isAbsolute(rel)` is true (i.e., treat absolute `rel` as
outside the allowed directory) so that the check requires !isAbsolute(rel) &&
rel !== ".." && !rel.startsWith(`..${sep}`).
In `@src/server-http.ts`:
- Around line 79-80: The SENSITIVE_FIELD_PATTERN currently redacts code_verifier
but not OAuth authorization codes; update the regex constant
SENSITIVE_FIELD_PATTERN to include "code" (e.g., add |code with appropriate
boundaries or case-insensitivity) so that query/body fields named "code" are
also redacted, and ensure the same updated pattern is used wherever verbose HTTP
request/response logging/redaction is performed (the places referencing
SENSITIVE_FIELD_PATTERN for logging of query/body params).
- Around line 821-825: The callback currently only checks presence of
oauthProvider.pendingAuthorizations.get(state) but not whether that
pendingAuthorization is expired, so stale states may be accepted; update the
handler that reads pendingAuthorization (the code around
oauthProvider.pendingAuthorizations.get(state) / pendingAuthorization) to verify
its expiry (e.g., check pendingAuthorization.expiresAt or compute from
pendingAuthorization.createdAt + TTL), and if expired remove it from
oauthProvider.pendingAuthorizations and respond with 400/“Unknown or expired
authorization state”; otherwise continue with normal processing.
---
Nitpick comments:
In `@src/auth/oauth.test.ts`:
- Around line 220-252: The canBindLocalPort function, the CAN_BIND_LOCAL_PORT
constant and registerClient helper are duplicated in src/auth/oauth.test.ts and
src/auth/oauth-integration.test.ts; extract them into a shared test helper
module (e.g., src/auth/test-helpers.ts) that exports async function
canBindLocalPort(), exported const CAN_BIND_LOCAL_PORT (computed once) and
registerClient(redirectUris?: string[]) returning ClientInfoResponse, then
import those symbols into both oauth.test.ts and oauth-integration.test.ts and
remove the local duplicates; ensure you preserve behavior of createServer/probe
listen/close and the fetch/expect flow in registerClient and update
imports/exports accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/auth/oauth-integration.test.tssrc/auth/oauth.test.tssrc/auth/provider.tssrc/client/shortcut.test.tssrc/client/shortcut.tssrc/server-http.ts
| const issuedTokenOrder: string[] = []; | ||
|
|
||
| // Keep in-memory stores bounded to reduce memory DoS risk. | ||
| const MAX_ISSUED_TOKENS = 10_000; | ||
| const MAX_PENDING_AUTHORIZATIONS = 2_000; | ||
| const PENDING_AUTH_TTL_MS = 10 * 60 * 1000; | ||
| const MAX_REGISTERED_CLIENTS = 10_000; | ||
| const REGISTERED_CLIENT_TTL_MS = 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
issuedTokenOrder can grow unbounded despite token-map caps.
Line 376 appends new tokens, but deletions from issuedTokens don’t remove entries from issuedTokenOrder. Over time, the array can grow without bound and weaken the memory-hardening objective.
🔧 Proposed fix
const issuedTokens = new Map<string, TokenCacheEntry>();
const issuedTokenOrder: string[] = [];
+
+function deleteIssuedToken(token: string): void {
+ if (issuedTokens.delete(token)) {
+ const idx = issuedTokenOrder.indexOf(token);
+ if (idx >= 0) issuedTokenOrder.splice(idx, 1);
+ }
+}// Then replace direct `issuedTokens.delete(...)` calls with `deleteIssuedToken(...)`.Also applies to: 374-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auth/provider.ts` around lines 290 - 297, issuedTokenOrder is never
pruned when issuedTokens entries are removed, allowing the array to grow
unbounded; implement a helper (e.g., deleteIssuedToken(tokenId: string)) that
deletes from the issuedTokens Map and also removes the tokenId from
issuedTokenOrder, update any direct issuedTokens.delete(...) calls to use
deleteIssuedToken, and ensure when enforcing MAX_ISSUED_TOKENS you pop/shift
oldest IDs from issuedTokenOrder and call deleteIssuedToken on them so both
structures remain in sync.
| if (!params.state) { | ||
| throw new InvalidRequestError("Missing state"); | ||
| } | ||
|
|
||
| const registered = await getClient(client.client_id); | ||
| if (!registered) { | ||
| throw new InvalidClientError("Unknown client_id"); | ||
| } | ||
| if (!registered.redirect_uris?.includes(params.redirectUri)) { | ||
| throw new InvalidRequestError("Unregistered redirect_uri"); | ||
| } | ||
|
|
||
| // Save callback relay info under an internal proxy-state to avoid cross-client collisions. | ||
| const proxyState = makeProxyState(normalizeClientState(params.state)); | ||
| while (pendingAuthorizations.size >= MAX_PENDING_AUTHORIZATIONS) { | ||
| evictOldestFromMap(pendingAuthorizations); | ||
| } | ||
| pendingAuthorizations.set(proxyState, { | ||
| clientId: client.client_id, | ||
| clientState: params.state, | ||
| redirectUri: params.redirectUri, | ||
| createdAtMs: nowMs, | ||
| }); |
There was a problem hiding this comment.
Bound state size before persisting proxy authorization state.
Lines 564-573 store raw client state in memory and embed it in the upstream state value. Without a size bound, very large state values can create avoidable memory/URL pressure.
🔧 Proposed fix
const MAX_REDIRECT_URIS_PER_CLIENT = 20;
+const MAX_STATE_BYTES = 1024;
...
if (!params.state) {
throw new InvalidRequestError("Missing state");
}
+ if (Buffer.byteLength(params.state, "utf8") > MAX_STATE_BYTES) {
+ throw new InvalidRequestError(
+ `state is too large; max allowed is ${MAX_STATE_BYTES} bytes`,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!params.state) { | |
| throw new InvalidRequestError("Missing state"); | |
| } | |
| const registered = await getClient(client.client_id); | |
| if (!registered) { | |
| throw new InvalidClientError("Unknown client_id"); | |
| } | |
| if (!registered.redirect_uris?.includes(params.redirectUri)) { | |
| throw new InvalidRequestError("Unregistered redirect_uri"); | |
| } | |
| // Save callback relay info under an internal proxy-state to avoid cross-client collisions. | |
| const proxyState = makeProxyState(normalizeClientState(params.state)); | |
| while (pendingAuthorizations.size >= MAX_PENDING_AUTHORIZATIONS) { | |
| evictOldestFromMap(pendingAuthorizations); | |
| } | |
| pendingAuthorizations.set(proxyState, { | |
| clientId: client.client_id, | |
| clientState: params.state, | |
| redirectUri: params.redirectUri, | |
| createdAtMs: nowMs, | |
| }); | |
| if (!params.state) { | |
| throw new InvalidRequestError("Missing state"); | |
| } | |
| if (Buffer.byteLength(params.state, "utf8") > MAX_STATE_BYTES) { | |
| throw new InvalidRequestError( | |
| `state is too large; max allowed is ${MAX_STATE_BYTES} bytes`, | |
| ); | |
| } | |
| const registered = await getClient(client.client_id); | |
| if (!registered) { | |
| throw new InvalidClientError("Unknown client_id"); | |
| } | |
| if (!registered.redirect_uris?.includes(params.redirectUri)) { | |
| throw new InvalidRequestError("Unregistered redirect_uri"); | |
| } | |
| // Save callback relay info under an internal proxy-state to avoid cross-client collisions. | |
| const proxyState = makeProxyState(normalizeClientState(params.state)); | |
| while (pendingAuthorizations.size >= MAX_PENDING_AUTHORIZATIONS) { | |
| evictOldestFromMap(pendingAuthorizations); | |
| } | |
| pendingAuthorizations.set(proxyState, { | |
| clientId: client.client_id, | |
| clientState: params.state, | |
| redirectUri: params.redirectUri, | |
| createdAtMs: nowMs, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auth/provider.ts` around lines 551 - 573, The current code stores the raw
client `state` (params.state) into pendingAuthorizations and embeds it in the
proxy state, which can cause unbounded memory/URL pressure; modify the logic
around makeProxyState/normalizeClientState and pendingAuthorizations.set to
enforce a maximum client-state size: add a MAX_CLIENT_STATE_LENGTH constant,
validate/truncate (or replace with a short cryptographic hash) of
normalizeClientState(params.state) before calling makeProxyState and before
persisting clientState in the pendingAuthorizations entry so only a bounded-size
value (or its hash) is stored and relayed.
| const previousAllowedDirs = process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS; | ||
| const previousMaxBytes = process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES; | ||
| process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS = allowedDir; | ||
| process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES = "1048576"; | ||
|
|
||
| try { | ||
| const uploadedFile = await client.uploadFile(123, filePath); | ||
| expect(uploadedFile.id).toBe(1); | ||
| expect(mockClient.uploadFiles).toHaveBeenCalled(); | ||
| } finally { | ||
| process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS = previousAllowedDirs; | ||
| process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES = previousMaxBytes; | ||
| rmSync(allowedDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js process.env assign undefined behavior
💡 Result:
In Node.js, process.env is an object view over environment variables, and its property values are treated as strings.
What happens if you do process.env.FOO = undefined?
Node will coerce the assigned value to a string, so undefined becomes the literal string "undefined" (same for null → "null"). This implicit string conversion is deprecated (since Node v10.0.0), and future Node versions may throw if you assign a non-string/number/boolean. [1]
process.env.FOO = undefined;
console.log(process.env.FOO); // "undefined"
console.log(typeof process.env.FOO); // "string"How to actually unset an env var in the current process
Use delete:
delete process.env.FOO;
console.log(process.env.FOO); // undefinedNode’s docs explicitly recommend delete for removing a property from process.env. [1] (This is also the commonly accepted answer in practice. [2])
Related gotcha (Windows)
On Windows, environment variable names are case-insensitive (e.g., TEST and test refer to the same variable in the main thread). [1]
Sources: Node.js process.env documentation [1], Stack Overflow discussion/example [2].
🏁 Script executed:
cat -n src/client/shortcut.test.ts | sed -n '479,491p'Repository: useshortcut/mcp-server-shortcut
Length of output: 814
🏁 Script executed:
cat -n src/client/shortcut.test.ts | sed -n '506,517p'Repository: useshortcut/mcp-server-shortcut
Length of output: 689
🏁 Script executed:
cat -n src/client/shortcut.test.ts | sed -n '531,542p'Repository: useshortcut/mcp-server-shortcut
Length of output: 789
Use delete to unset environment variables when prior value was undefined.
Node.js coerces undefined to the string "undefined" when assigned to process.env, causing the variable to remain set with value "undefined" instead of being unset. This leaks state between tests.
Lines 489–490, 515, and 540–541 restore environment variables without checking whether the prior value was undefined. Use delete process.env[key] when the prior value is undefined to properly unset the variable.
🔧 Proposed fix
+function restoreEnv(key: "SHORTCUT_UPLOAD_ALLOWED_DIRS" | "SHORTCUT_UPLOAD_MAX_FILE_BYTES", prev: string | undefined) {
+ if (prev === undefined) {
+ delete process.env[key];
+ return;
+ }
+ process.env[key] = prev;
+}
...
- process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS = previousAllowedDirs;
- process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES = previousMaxBytes;
+ restoreEnv("SHORTCUT_UPLOAD_ALLOWED_DIRS", previousAllowedDirs);
+ restoreEnv("SHORTCUT_UPLOAD_MAX_FILE_BYTES", previousMaxBytes);Also applies to: 506–517, 531–542
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/shortcut.test.ts` around lines 479 - 491, The test restores
environment variables by assigning previous values back to process.env which
will set them to the string "undefined" if they were originally undefined;
update the teardown code in shortcut.test.ts (the blocks around
client.uploadFile / mockClient.uploadFiles and the rmSync cleanup) to check the
saved previous values for SHORTCUT_UPLOAD_ALLOWED_DIRS and
SHORTCUT_UPLOAD_MAX_FILE_BYTES and if a saved value is undefined use delete
process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS (and delete
process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES) otherwise reassign the saved value,
ensuring you update all identical restore sites (the blocks that currently set
process.env.SHORTCUT_UPLOAD_ALLOWED_DIRS = previousAllowedDirs and
process.env.SHORTCUT_UPLOAD_MAX_FILE_BYTES = previousMaxBytes).
| import { readFileSync, realpathSync, statSync } from "node:fs"; | ||
| import { basename, relative, resolve, sep } from "node:path"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js path.relative() Windows different drive letters behavior
💡 Result:
On Windows, path.relative(from, to) can only produce a “real” relative path when both paths are on the same drive / same root. If the drive letters differ (e.g., C:\... → D:\...), there is no valid relative path, so Node returns the fully-qualified to path instead. This is why you’ll see behavior like:
path.win32.relative('Z:\\tmp\\', 'C:\\Users\\Jane\\')→'C:\\Users\\Jane'(absolute, because drives differ) [2]
More generally, path.relative() works by resolving both inputs first (via path.resolve() semantics) and then computing a relative traversal; if it can’t compute one (different roots), it falls back to returning to as-is (fully qualified on Windows in this case) [1][2].
Implication: if you need a stable “relative-like” reference across drives, you must use another scheme (e.g., keep both paths on the same drive, use UNC paths consistently, or store absolute paths / file URLs). [1]
Sources: Node.js path.relative() docs [1]; Windows cross-drive example/behavior [2].
🏁 Script executed:
# Verify the file exists and check the code around line 62
fd -t f shortcut.ts | head -5Repository: useshortcut/mcp-server-shortcut
Length of output: 97
🏁 Script executed:
# Read the file around lines 2-3 and 60-65 to verify the code
cat -n src/client/shortcut.ts | sed -n '1,10p'Repository: useshortcut/mcp-server-shortcut
Length of output: 435
🏁 Script executed:
# Check the isPathInsideDirectory function around line 62
cat -n src/client/shortcut.ts | sed -n '55,70p'Repository: useshortcut/mcp-server-shortcut
Length of output: 778
🏁 Script executed:
# Search for other uses of isPathInsideDirectory and related path validation
rg -n "isPathInsideDirectory|uploadFile|getAllowedUploadDirectories" src/client/shortcut.ts | head -20Repository: useshortcut/mcp-server-shortcut
Length of output: 523
🏁 Script executed:
# Check if there are any other path-related security checks in the file
rg -n "relative|resolve|realpath" src/client/shortcut.tsRepository: useshortcut/mcp-server-shortcut
Length of output: 718
Fix path containment check to prevent Windows cross-drive bypass.
On Windows, path.relative() returns the fully-qualified absolute path (e.g., D:\malicious) when the source and target are on different drive letters. The current check at line 62 only rejects .. traversal, so an absolute path like D:\malicious passes both conditions (rel !== ".." and !rel.startsWith(\..${sep}`)`) and is incorrectly treated as inside the allowed directory. This bypasses the upload directory allowlist on multi-drive Windows systems.
Add an isAbsolute() check to reject any absolute paths returned by relative():
Fix
-import { basename, relative, resolve, sep } from "node:path";
+import { basename, isAbsolute, relative, resolve, sep } from "node:path";
...
function isPathInsideDirectory(path: string, directory: string): boolean {
const rel = relative(directory, path);
- return rel === "" || (rel !== ".." && !rel.startsWith(`..${sep}`));
+ return rel === "" || (!isAbsolute(rel) && rel !== ".." && !rel.startsWith(`..${sep}`));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/shortcut.ts` around lines 2 - 3, The path containment check uses
the value `rel` returned by `relative()` but doesn't reject absolute paths
(which on Windows can occur across drives), so update the code to import
`isAbsolute` from "node:path" alongside `relative`/`resolve`/`sep` and change
the containment condition to also reject when `isAbsolute(rel)` is true (i.e.,
treat absolute `rel` as outside the allowed directory) so that the check
requires !isAbsolute(rel) && rel !== ".." && !rel.startsWith(`..${sep}`).
| const SENSITIVE_FIELD_PATTERN = | ||
| /(token|secret|password|authorization|cookie|access_token|refresh_token|id_token|code_verifier)/i; |
There was a problem hiding this comment.
Redact OAuth authorization codes in verbose HTTP logs.
Line 80 redacts code_verifier but not code. With DEBUG_LEVEL=2, callback/token requests can still log the authorization code in query/body.
🔧 Proposed fix
const SENSITIVE_FIELD_PATTERN =
- /(token|secret|password|authorization|cookie|access_token|refresh_token|id_token|code_verifier)/i;
+ /(token|secret|password|authorization|cookie|access_token|refresh_token|id_token|code_verifier|authorization_code|(^|_)code$)/i;Also applies to: 124-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server-http.ts` around lines 79 - 80, The SENSITIVE_FIELD_PATTERN
currently redacts code_verifier but not OAuth authorization codes; update the
regex constant SENSITIVE_FIELD_PATTERN to include "code" (e.g., add |code with
appropriate boundaries or case-insensitivity) so that query/body fields named
"code" are also redacted, and ensure the same updated pattern is used wherever
verbose HTTP request/response logging/redaction is performed (the places
referencing SENSITIVE_FIELD_PATTERN for logging of query/body params).
| const pendingAuthorization = oauthProvider.pendingAuthorizations.get(state); | ||
| if (!pendingAuthorization) { | ||
| res.status(400).send("Unknown or expired authorization state"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reject stale pending authorizations at callback time.
Lines 821-825 only check existence. Expiration is cleaned lazily in other paths, so an old state can still be accepted here until cleanup runs elsewhere.
🔧 Proposed fix
const pendingAuthorization = oauthProvider.pendingAuthorizations.get(state);
if (!pendingAuthorization) {
res.status(400).send("Unknown or expired authorization state");
return;
}
+ const PENDING_AUTH_TTL_MS = 10 * 60 * 1000; // keep in sync with provider
+ if (Date.now() - pendingAuthorization.createdAtMs > PENDING_AUTH_TTL_MS) {
+ oauthProvider.pendingAuthorizations.delete(state);
+ res.status(400).send("Unknown or expired authorization state");
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pendingAuthorization = oauthProvider.pendingAuthorizations.get(state); | |
| if (!pendingAuthorization) { | |
| res.status(400).send("Unknown or expired authorization state"); | |
| return; | |
| } | |
| const pendingAuthorization = oauthProvider.pendingAuthorizations.get(state); | |
| if (!pendingAuthorization) { | |
| res.status(400).send("Unknown or expired authorization state"); | |
| return; | |
| } | |
| const PENDING_AUTH_TTL_MS = 10 * 60 * 1000; // keep in sync with provider | |
| if (Date.now() - pendingAuthorization.createdAtMs > PENDING_AUTH_TTL_MS) { | |
| oauthProvider.pendingAuthorizations.delete(state); | |
| res.status(400).send("Unknown or expired authorization state"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server-http.ts` around lines 821 - 825, The callback currently only
checks presence of oauthProvider.pendingAuthorizations.get(state) but not
whether that pendingAuthorization is expired, so stale states may be accepted;
update the handler that reads pendingAuthorization (the code around
oauthProvider.pendingAuthorizations.get(state) / pendingAuthorization) to verify
its expiry (e.g., check pendingAuthorization.expiresAt or compute from
pendingAuthorization.createdAt + TTL), and if expired remove it from
oauthProvider.pendingAuthorizations and respond with 400/“Unknown or expired
authorization state”; otherwise continue with normal processing.
|
Archiving repository, closing out PRs. |
2 participants
Summary
Validation
Shortcut
Summary by CodeRabbit
Bug Fixes