fix: restrict API keys to run domain only#2478
Conversation
API keys were accepted on manage endpoints, granting OWNER-level access to administrative operations. This removes API key validation from manage auth middleware and adds agent-scope mismatch warnings in run auth. - Remove validateAndGetApiKey from manageAuth middleware - Add warning log when x-inkeep-agent-id mismatches key-bound agent - Add test for manage endpoint API key rejection - Add test for agent-scope enforcement in run auth - Update OpenAPI snapshot and .gitignore Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Restricts API key authentication to the run domain only, closing the surface where API keys could previously authenticate against manage endpoints (agent config, projects, tools, credentials).
|
There was a problem hiding this comment.
The core security change is correct — manageAuth.ts no longer calls validateAndGetApiKey, so API keys fall through all auth checks and get a 401. The workAppsAuth.ts middleware delegates to the same manageApiKeyAuth(), so that path is also covered. Two items worth addressing: the warning in buildExecutionContext has a broader blast radius than intended (fires for non-API-key auth methods), and the test for the mismatch warning doesn't actually assert the warning was logged.
| if ( | ||
| !authResult.metadata?.teamDelegation && | ||
| reqData.agentId && | ||
| reqData.agentId !== authResult.agentId | ||
| ) { | ||
| logger.warn( | ||
| { | ||
| requestedAgentId: reqData.agentId, | ||
| apiKeyAgentId: authResult.agentId, | ||
| apiKeyId: authResult.apiKeyId, | ||
| }, | ||
| 'API key agent scope mismatch: ignoring x-inkeep-agent-id header, using key-bound agent' | ||
| ); | ||
| } |
There was a problem hiding this comment.
This warning fires for all non-teamDelegation auth strategies where reqData.agentId differs from authResult.agentId — not just database API keys. JWT temp tokens (apiKeyId='temp-jwt'), bypass auth, and Slack user tokens all set authResult.agentId from their own source. If a Slack user passes a different x-inkeep-agent-id header, this logs API key agent scope mismatch with apiKeyAgentId, which is misleading.
Consider gating on a real DB key ID (e.g. authResult.apiKeyId && !authResult.apiKeyId.startsWith('temp-')) or renaming the log fields to be auth-method-agnostic.
| validateAndGetApiKeyMock.mockResolvedValueOnce({ | ||
| id: 'key_123', | ||
| tenantId: 'tenant_123', | ||
| projectId: 'project_123', | ||
| agentId: 'agent_123', | ||
| publicId: 'pub_123', | ||
| keyHash: 'hash_123', | ||
| keyPrefix: 'sk_test_', | ||
| expiresAt: null, | ||
| lastUsedAt: null, | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| }); |
There was a problem hiding this comment.
This mockResolvedValueOnce setup is dead code — manageAuth.ts no longer imports or calls validateAndGetApiKey, so this mock can never fire. The assertion on line 89 (expect(validateAndGetApiKeyMock).not.toHaveBeenCalled()) already proves the point. Removing this setup would make the test clearer about what it's actually verifying: a complete code path removal, not conditional rejection.
| it('should use key-bound agentId and ignore x-inkeep-agent-id header for API key auth', async () => { | ||
| const mockApiKey: ApiKeySelect = { | ||
| id: 'key_123', | ||
| name: 'test-api-key', | ||
| tenantId: 'tenant_123', | ||
| projectId: 'project_123', | ||
| agentId: 'agent_from_key', | ||
| publicId: 'pub_123', | ||
| keyHash: 'hash_123', | ||
| keyPrefix: 'sk_test_', | ||
| expiresAt: null, | ||
| lastUsedAt: null, | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| }; | ||
|
|
||
| validateAndGetApiKeyMock.mockResolvedValueOnce(mockApiKey); | ||
|
|
||
| app.use('*', apiKeyAuth()); | ||
| app.get('/', (c) => { | ||
| const executionContext = (c as any).get('executionContext'); | ||
| return c.json(executionContext); | ||
| }); | ||
|
|
||
| const res = await app.request('/', { | ||
| headers: { | ||
| Authorization: 'Bearer sk_test_1234567890abcdef.verylongsecretkey', | ||
| 'x-inkeep-agent-id': 'different_agent', | ||
| }, | ||
| }); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| const body = await res.json(); | ||
| expect(body.agentId).toBe('agent_from_key'); | ||
| }); |
There was a problem hiding this comment.
This test verifies the correct agentId is returned but doesn't assert that logger.warn() was called with the mismatch details. Since the warning is the main deliverable of the new code in buildExecutionContext (lines 104-117 of runAuth.ts), assert on the logger mock to prevent silent regression.
Also missing negative cases: (1) no x-inkeep-agent-id header (should not warn), (2) header matches key-bound agentId (should not warn). Without these, the warning could fire on every request without a test catching it.
| * NOTE: Database API keys are intentionally NOT accepted on manage endpoints. | ||
| * API keys are restricted to the run domain only (chat, agent execution). | ||
| */ | ||
| export const manageApiKeyAuth = () => |
There was a problem hiding this comment.
Nit (non-blocking): manageApiKeyAuth no longer accepts API keys, making the name misleading. Callers like workAppsAuth.ts and manageApiKeyOrSessionAuth reference this export. Consider renaming to manageBearerAuth or manageTokenAuth.
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
manageAuth.ts:168-171— Stale JSDoc comment contradicts new implementation
💭 Consider (2) 💭
💭 1) workAppsAuth.ts:3-13 Stale comments reference API key auth
Issue: The workAppsAuth.ts doc comment (lines 3-13) references 'session/API key auth' and 'Bearer token → manageApiKeyAuth', but manageApiKeyAuth no longer validates database API keys. The comments are now misleading.
Why: These comments describe middleware behavior to developers. After this change, they are inaccurate and could cause confusion when debugging auth issues.
Fix: Update the doc comment at line 4 from 'session/API key auth' and line 11 from 'Bearer token → manageApiKeyAuth' to reflect that only bypass secrets, session tokens, Slack user JWTs, and internal service tokens are accepted.
Refs:
Inline Comments:
- 💭 Consider:
manage-auth-api-key-rejection.test.ts:88— Verify rejection message for regression safety
🧹 While You're Here (1) 🧹
🧹 1) projectAccess.ts:57 Dead code path for apikey: userId
Issue: The projectAccess.ts middleware has a bypass for userId.startsWith('apikey:') at line 57, but with API keys removed from manage auth, no requests will ever have a userId matching this pattern on manage endpoints. This code path is now unreachable.
Why: The removed code in manageAuth.ts previously set c.set('userId', 'apikey:${validatedKey.id}'). With this removal, no code path on manage endpoints will produce an 'apikey:*' userId.
Fix: Consider removing the apikey: check or leaving a comment explaining it's preserved for backward compatibility.
Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-executed security improvement that correctly restricts API keys to the run domain only. The implementation is complete — validateAndGetApiKey is properly removed from manageAuth.ts, the auth chain fallback works correctly, and tests verify both the rejection and bypass secret behaviors. The IAM security review found no issues. The minor suggestions above are documentation cleanup and test hardening opportunities that don't block merge. Good work on this security fix! 🔐
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
manage-auth-api-key-rejection.test.ts |
Test file naming differs from peer tests | Developer preference — naming is reasonable |
manage-auth-api-key-rejection.test.ts |
Missing test for session auth fallback | Low confidence — session auth is tested elsewhere |
api-key-auth.test.ts |
No test for matching header case (no warning) | Very minor — implicit coverage via existing tests |
api-key-auth.test.ts |
Test doesn't verify warning log is emitted | Lower priority — warning is observability feature, not correctness |
manageAuth.ts |
Non-constant-time bypass secret comparison | Pre-existing pattern, not introduced by this PR |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
5 | 0 | 1 | 1 | 0 | 0 | 2 |
pr-review-tests |
4 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 10 | 0 | 1 | 1 | 2 | 0 | 5 |
Note: Security and error handling reviewers found no issues — the auth removal is complete and error handling is appropriate.
| }, | ||
| }); | ||
|
|
||
| expect(res.status).toBe(401); |
There was a problem hiding this comment.
💭 Consider: Verify rejection message for regression safety
Issue: The test asserts validateAndGetApiKeyMock was not called, but doesn't verify the 401 response body. If API keys were accidentally re-enabled via a different code path, this test might miss the regression.
Why: Asserting the response body contains 'Invalid Token' would catch cases where API keys are accepted through an unintended path.
Fix: Consider adding after line 88:
const body = await res.text();
expect(body).toContain('Invalid Token');- Narrow agent-scope warning to real DB API keys only (skip temp-jwt, bypass, slack-user-token, team-agent-token, test-key) - Rename manageApiKeyAuth → manageBearerAuth and manageApiKeyOrSessionAuth → manageBearerOrSessionAuth - Remove dead mock setup and runDbClient mock from manage auth test - Add response body assertion for rejection message - Update stale JSDoc comments in manageAuth.ts and workAppsAuth.ts - Update OpenAPI snapshot for renames Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta re-review scoped to the 1 commit since the last automated review. The delta addresses prior feedback correctly:
✅ Prior Feedback Addressed
-
Warning blast radius narrowed — The warning condition in
buildExecutionContextnow correctly excludes non-database API key auth methods (temp-*,bypass,slack-user-token,team-agent-token,test-key). The security reviewer confirmed the exclusion list is complete. -
Naming consistency —
manageApiKeyAuth→manageBearerAuthrename applied consistently across all 8 affected files. No stale references remain. -
Doc comments updated — The header doc blocks in
workAppsAuth.ts,inherited-auth.ts, andmanageAuth.tsnow accurately describe the auth flow. -
Test improvements — Removed dead mock code, added response body assertion per prior suggestion.
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
workAppsAuth.ts:63Stale inline comment (header was updated but inline comment still says "API key auth")
🕐 Pending Recommendations (2)
- 🧹
projectAccess.ts:57Dead code path forapikey:userId prefix (pre-existing, raised in prior review) - 🟡
api-key-auth.test.ts:245Test doesn't assert warning log was called (raised by pullfrog in prior review)
✅ APPROVE
Summary: The delta changes correctly address all prior feedback. The security boundary is properly enforced — API keys are restricted to the run domain only, and the warning logic now correctly fires only for actual database API keys. The one inline comment nitpick is optional cleanup. Ship it! 🚀
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-comments |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 1 | 0 | 0 |
Note: Security and consistency reviewers confirmed all changes are correct — no issues found in the delta.
| @@ -63,7 +63,7 @@ export const workAppsAuth = async (c: Context, next: Next) => { | |||
| // Bearer token → API key auth, otherwise → session auth (dashboard cookies) | |||
There was a problem hiding this comment.
💭 Consider: Stale inline comment
Issue: The inline comment still says "API key auth" but manageBearerAuth() no longer validates database API keys. The header doc block (lines 1-13) was correctly updated, but this inline comment was missed.
Why: Developers debugging auth issues who read this comment would expect API key validation here, but it doesn't happen.
Fix:
| // Bearer token → API key auth, otherwise → session auth (dashboard cookies) | |
| // Bearer token → manageBearerAuth (bypass secret, session, Slack JWT, internal service), otherwise → session auth (dashboard cookies) |
Refs:
|
Thanks for this security improvement! 🔒 I reviewed the documentation impact and found that the Authentication & API Keys section in The current documentation doesn't contain incorrect information, but adding this clarification would help users understand the security model better. A follow-up docs PR may be warranted to add a note like:
|
Summary
manageApiKeyAuth, which granted OWNER-level access to administrative operations (project config, agent CRUD, credential management). This was a security concern since API keys are designed for embedding in client-side widgets and should only grant agent execution access.x-inkeep-agent-idheader that doesn't match the key's bound agent, a warning is now logged. The key-bound agent always wins (existing behavior), but the warning improves observability.What changed
agents-api/src/middleware/manageAuth.tsvalidateAndGetApiKeycall andrunDbClientimport. API keys are no longer accepted.agents-api/src/middleware/runAuth.tsx-inkeep-agent-idheader mismatches key-bound agent.agents-api/src/__tests__/manage/middleware/manage-auth-api-key-rejection.test.tsagents-api/src/__tests__/run/middleware/api-key-auth.test.tsx-inkeep-agent-idheader.agents-api/__snapshots__/openapi.json.gitignorereportsand.claude/worktrees/entries.What is NOT affected
Test plan
pnpm checkpasses (lint + typecheck + test + format)🤖 Generated with Claude Code