fix(server-v3): enforce API key authentication#2393
Open
GautamSharma99 wants to merge 1 commit into
Open
Conversation
🦋 Changeset detectedLatest commit: 89b81eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
This PR is from an external contributor and must be approved by a stagehand team member with write access before CI can run. |
github-actions
Bot
added
external-contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 12 files
Confidence score: 2/5
- In
packages/server-v3/src/lib/auth.ts, the ownership-only branch can let revoked Browserbase credentials keep controlling an existing session, creating a concrete auth bypass risk with ongoing unauthorized access—validate the matching key against the Browserbase cache/verification path before allowing session reuse. - In
packages/server-v3/tests/unit/auth.test.ts, the current cache test inauth.test.tscan pass even if plaintext API keys are retained, so the intended security guarantee is unproven and regressions could slip in—add an assertion on fingerprinted/hashed cache contents (or an inspectable cache view) to verify key material is not stored in plaintext. - In
packages/server-v3/scripts/test-server.ts, an emptySTAGEHAND_SERVER_API_KEYis treated as set and then rejected bygetHeaders(), causing local/SEA integration runs to fail before requests—treat empty values as unset so defaults apply consistently.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server-v3/tests/unit/auth.test.ts">
<violation number="1" location="packages/server-v3/tests/unit/auth.test.ts:124">
P2: The cache security guarantee is not actually covered: this test validates reuse but would still pass if the cache retained plaintext API keys. An inspectable cache representation or a focused assertion around fingerprinted cache keys would make the no-plaintext requirement executable.</violation>
</file>
<file name="packages/server-v3/scripts/test-server.ts">
<violation number="1" location="packages/server-v3/scripts/test-server.ts:195">
P2: Local/SEA integration runs fail before requests when callers export `STAGEHAND_SERVER_API_KEY=`: the empty value bypasses this default, then `getHeaders()` rejects it as missing. Treat an empty value as unset so this runner always provisions its isolated test credential.</violation>
</file>
<file name="packages/server-v3/src/lib/auth.ts">
<violation number="1" location="packages/server-v3/src/lib/auth.ts:117">
P1: Revoked Browserbase credentials can continue controlling an existing session indefinitely. This branch checks only stored-key ownership and bypasses cached Browserbase validation, so validate the matching key as well and fail closed on verification failure.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant MW as authMiddleware
participant Cache as Auth Cache
participant VKey as verifyBrowserbaseApiKey()
participant BB as Browserbase API
participant SessionStore as Session Store
participant Route as Protected Route Handler
Note over Client,Route: Authentication Flow (NEW: enforced API key auth)
Client->>MW: Request with x-bb-api-key or x-stagehand-api-key
MW->>MW: Check x-stagehand-api-key header
alt Valid self-hosted key (timingSafeEqual match)
MW-->>Client: true → proceed to route
else Missing or invalid self-hosted key
MW->>MW: Check x-bb-api-key header
alt No or empty x-bb-api-key
MW-->>Client: false → 401 Unauthorized
else x-bb-api-key present
MW->>MW: Check session ID in route params
alt Session ID present
MW->>SessionStore: getSessionConfig(sessionId)
SessionStore-->>MW: browserbaseApiKey or undefined
alt Matching key
MW-->>Client: true → proceed to route
else Key mismatch or missing session
MW-->>Client: false → 401 Unauthorized
end
else No session ID
MW->>MW: fingerprintApiKey(x-bb-api-key)
MW->>Cache: Lookup fingerprint
alt Cached result (valid TTL)
Cache-->>MW: { authenticated }
MW->>Cache: Refresh LRU order
alt authenticated
MW-->>Client: true
else not authenticated
MW-->>Client: false
end
else Cache miss or expired
alt Concurrent request in-flight for same key?
Cache-->>MW: Found pending verification
MW->>MW: await existing promise
MW-->>Client: result
else First request for this key
MW->>VKey: verifyApiKey(apiKey)
VKey->>BB: projects.list({ maxRetries: 0, timeout: 5000 })
alt BB responds successfully
BB-->>VKey: projects list
VKey-->>MW: true
else BB rejects or unavailable
BB-->>VKey: Error
VKey-->>MW: false
end
MW->>Cache: cacheResult(fingerprint, authenticated)
MW-->>Client: authenticated
end
end
end
end
end
alt Authenticated
Client->>Route: Proceed with original request
Note over Route: Route handler may call SessionStore to store<br/>browserbaseApiKey for session ownership (NEW)
Route-->>Client: Response
else Not authenticated
MW-->>Client: 401 Unauthorized
end
Note over Cache: NEW: In-memory LRU cache (max 1000 entries, 5-min TTL)<br/>SHA-256 fingerprints, no plaintext keys stored
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const sessionId = getSessionId(request); | ||
| if (sessionId) { | ||
| const sessionApiKey = await resolveApiKeyForSession(sessionId); | ||
| return sessionApiKey ? apiKeysMatch(headerValue, sessionApiKey) : false; |
Contributor
There was a problem hiding this comment.
P1: Revoked Browserbase credentials can continue controlling an existing session indefinitely. This branch checks only stored-key ownership and bypasses cached Browserbase validation, so validate the matching key as well and fail closed on verification failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-v3/src/lib/auth.ts, line 117:
<comment>Revoked Browserbase credentials can continue controlling an existing session indefinitely. This branch checks only stored-key ownership and bypasses cached Browserbase validation, so validate the matching key as well and fail closed on verification failure.</comment>
<file context>
@@ -1,14 +1,152 @@
+ const sessionId = getSessionId(request);
+ if (sessionId) {
+ const sessionApiKey = await resolveApiKeyForSession(sessionId);
+ return sessionApiKey ? apiKeysMatch(headerValue, sessionApiKey) : false;
+ }
+
</file context>
| assert.equal(browserbaseVerificationCalls, 0); | ||
| }); | ||
|
|
||
| it("caches verification results without retaining plaintext keys", async () => { |
Contributor
There was a problem hiding this comment.
P2: The cache security guarantee is not actually covered: this test validates reuse but would still pass if the cache retained plaintext API keys. An inspectable cache representation or a focused assertion around fingerprinted cache keys would make the no-plaintext requirement executable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-v3/tests/unit/auth.test.ts, line 124:
<comment>The cache security guarantee is not actually covered: this test validates reuse but would still pass if the cache retained plaintext API keys. An inspectable cache representation or a focused assertion around fingerprinted cache keys would make the no-plaintext requirement executable.</comment>
<file context>
@@ -0,0 +1,176 @@
+ assert.equal(browserbaseVerificationCalls, 0);
+ });
+
+ it("caches verification results without retaining plaintext keys", async () => {
+ const verifiedKeys: string[] = [];
+ const authenticate = createAuthMiddleware({
</file context>
| process.env.STAGEHAND_SERVER_TARGET ?? "sea" | ||
| ).toLowerCase(); | ||
| if (serverTarget !== "remote") { | ||
| process.env.STAGEHAND_SERVER_API_KEY ??= "stagehand-integration-test-key"; |
Contributor
There was a problem hiding this comment.
P2: Local/SEA integration runs fail before requests when callers export STAGEHAND_SERVER_API_KEY=: the empty value bypasses this default, then getHeaders() rejects it as missing. Treat an empty value as unset so this runner always provisions its isolated test credential.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-v3/scripts/test-server.ts, line 195:
<comment>Local/SEA integration runs fail before requests when callers export `STAGEHAND_SERVER_API_KEY=`: the empty value bypasses this default, then `getHeaders()` rejects it as missing. Treat an empty value as unset so this runner always provisions its isolated test credential.</comment>
<file context>
@@ -191,6 +191,9 @@ if (!fs.existsSync(allTestsDir)) {
process.env.STAGEHAND_SERVER_TARGET ?? "sea"
).toLowerCase();
+if (serverTarget !== "remote") {
+ process.env.STAGEHAND_SERVER_API_KEY ??= "stagehand-integration-test-key";
+}
const explicitBaseUrl =
</file context>
Suggested change
| process.env.STAGEHAND_SERVER_API_KEY ??= "stagehand-integration-test-key"; | |
| process.env.STAGEHAND_SERVER_API_KEY ||= "stagehand-integration-test-key"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR replaces server-v3’s authentication stub—which previously authorized every request—with enforceable API-key authentication.
Requests can now authenticate using either:
x-bb-api-keyx-stagehand-api-key, configured throughSTAGEHAND_SERVER_API_KEYThe change also associates Browserbase-authenticated sessions with the credential that created them, preventing another valid Browserbase
credential from controlling an existing session.
Problem
The previous
authMiddlewareignored the incoming request and delegated to anisAuthenticated()function that always returnedtrue: