feat(auth): improve error messaging with combined strategy failure summary#2698
feat(auth): improve error messaging with combined strategy failure summary#2698amikofalvy wants to merge 2 commits intomainfrom
Conversation
All auth strategies now return AuthAttempt instead of bare AuthResult|null, providing failure reasons. authenticateRequest() collects failures from all strategies and returns a combined summary message when all fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update existing tests to assert the new combined error message format. Add test verifying Slack definitive failure short-circuits without trying further strategies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Functionally correct — the Slack failure path logic, type consistency across all five strategies, and test coverage all look good. One security concern worth addressing before merge.
| const summary = | ||
| failures.length > 0 | ||
| ? `Authentication failed. Tried: ${failures.map((f) => `${f.strategy} (${f.reason})`).join(', ')}` | ||
| : 'Authentication failed: no valid credentials found'; |
There was a problem hiding this comment.
Information leakage in 401 response: The combined error message exposes internal infrastructure details to external API consumers — e.g., no bypass secret configured, no public key configured, strategy names like bypass secret. An attacker can fingerprint deployment configuration and identify which auth mechanisms are enabled/disabled.
Consider using generic failure reasons in the user-facing message (e.g., not applicable) while keeping the detailed reasons only in the logger.debug call on line 672, where they're already logged.
| const summary = | ||
| failures.length > 0 | ||
| ? `Authentication failed. Tried: ${failures.map((f) => `${f.strategy} (${f.reason})`).join(', ')}` | ||
| : 'Authentication failed: no valid credentials found'; |
There was a problem hiding this comment.
Nit: the failures.length > 0 else branch ('Authentication failed: no valid credentials found') is dead code. At this point apiKey is guaranteed truthy (line 632), so tryTempJwtAuth always pushes at least one failure. Low priority but worth removing for clarity.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Low
🟡 Minor (3) 🟡
🟡 1) runAuth.ts:454 & runAuth.ts:171 Info leakage in configuration-dependent failure messages
files: runAuth.ts:454, runAuth.ts:171
Issue: Failure messages like 'no bypass secret configured' and 'no public key configured' reveal whether specific environment variables are set, informing attackers about your auth configuration.
Why: While low-severity (doesn't enable direct exploitation), revealing configuration state helps attackers narrow down which authentication vectors are available and which are not configured. This follows an anti-pattern in authentication error handling. Compare with 'no match' at line 458 which correctly indicates the credential was wrong without revealing config state.
Fix: Use uniform failure messages that don't distinguish between "not configured" and "no match" states:
'no bypass secret configured'→'not applicable''no public key configured'→'not applicable'
The DEBUG-level log at line 672 already captures detailed failure reasons for operators who need them.
Refs:
🟡 2) api-key-auth.test.ts:1075 Slack test assertion doesn't verify full error format
Issue: The new test "should short-circuit with Slack-specific error when token is identified as Slack but fails verification" only checks that the body contains 'Invalid Slack user token' but doesn't verify the specific error reason ('signature verification failed') is included.
Why: If the error message format changes to omit the specific error reason, users would lose diagnostic information about why their Slack token failed, making debugging harder.
Fix: Strengthen the assertion to verify the complete error message:
expect(body).toContain('Invalid Slack user token: signature verification failed');Refs:
- runAuth.ts:285 — error message construction
Inline Comments:
- 🟡 Minor:
runAuth.ts:454Info leakage - bypass secret config state - 🟡 Minor:
runAuth.ts:171Info leakage - public key config state - 🟡 Minor:
api-key-auth.test.ts:1073-1076Test doesn't verify full error format
💭 Consider (1) 💭
💭 1) runAuth.ts:650-653 Asymmetric Slack auth handling breaks normalization pattern
Issue: trySlackUserJwtAuth returns { authResult: null } without a failureMessage when the token isn't a Slack token (line 276), unlike all other strategies that were normalized to return failure messages. This requires special handling with a hardcoded reason at line 653.
Why: The PR's stated goal was to normalize auth strategy return types to AuthAttempt with failure messages. This creates a coupling where the caller knows implementation details of the callee.
Fix: Consider modifying trySlackUserJwtAuth line 276 to return { authResult: null, failureMessage: 'not a Slack token' }, then handle it uniformly like other strategies.
Refs:
Inline Comments:
- 💭 Consider:
runAuth.ts:650-653Asymmetric Slack handling
💡 APPROVE WITH SUGGESTIONS
Summary: This PR successfully improves auth error messaging by normalizing return types and combining strategy failures into a helpful summary. The implementation is clean and well-tested. The minor findings above are suggestions for hardening (info leakage) and consistency (Slack handling symmetry) — none are blocking. The Slack short-circuit behavior is correctly preserved for definitive failures. Good work! 🎉
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
runAuth.ts:667-674 |
Combined error reveals full auth strategy enumeration | Accepted product decision for debugging; INFO level, not actionable |
runAuth.ts:191 |
Removed clarifying comments reduce maintainability | AGENTS.md states "No Comments" guideline; cleanup is acceptable |
api-key-auth.test.ts:149 |
Missing JWT verification failure test path | Low confidence; existing tests provide adequate coverage |
api-key-auth.test.ts:967 |
Missing short-circuit assertion for missing headers | The test validates correct behavior; additional assertion is optional |
| Strategy capitalization | Inconsistent casing ("bypass secret" vs "API key") | Cosmetic nitpick; doesn't affect functionality |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
2 | 1 | 0 | 0 | 2 | 0 | 1 |
pr-review-tests |
5 | 1 | 0 | 0 | 1 | 0 | 3 |
pr-review-consistency |
4 | 0 | 1 | 0 | 1 | 0 | 2 |
pr-review-precision |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-errors |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 14 | 2 | 1 | 0 | 4 | 0 | 9 |
| function tryBypassAuth(apiKey: string, reqData: RequestData): AuthAttempt { | ||
| if (!env.INKEEP_AGENTS_RUN_API_BYPASS_SECRET) { | ||
| return null; | ||
| return { authResult: null, failureMessage: 'no bypass secret configured' }; |
There was a problem hiding this comment.
🟡 Minor: Info leakage - failure message reveals server configuration state
Issue: The failure message 'no bypass secret configured' reveals whether the INKEEP_AGENTS_RUN_API_BYPASS_SECRET environment variable is set, informing attackers that this auth path is unavailable.
Why: While low severity (doesn't enable direct exploitation), revealing configuration state helps attackers narrow down which authentication vectors are available. Compare with line 458 which correctly uses 'no match' without revealing config state.
Fix: Use a uniform failure message that doesn't distinguish between "not configured" and "no match":
| return { authResult: null, failureMessage: 'no bypass secret configured' }; | |
| return { authResult: null, failureMessage: 'not applicable' }; |
Refs:
- OWASP Authentication Errors
- tryTempJwtAuth:171 — same pattern with
'no public key configured'
| } | ||
|
|
||
| if (!env.INKEEP_AGENTS_TEMP_JWT_PUBLIC_KEY) { | ||
| return { authResult: null, failureMessage: 'no public key configured' }; |
There was a problem hiding this comment.
🟡 Minor: Info leakage - failure message reveals server configuration state
Issue: The failure message 'no public key configured' reveals whether INKEEP_AGENTS_TEMP_JWT_PUBLIC_KEY is set.
Why: Same concern as the bypass secret — revealing configuration state helps attackers understand your auth setup.
Fix: Use a generic message:
| return { authResult: null, failureMessage: 'no public key configured' }; | |
| return { authResult: null, failureMessage: 'not applicable' }; |
Refs:
| const slackAttempt = await trySlackUserJwtAuth(apiKey, reqData); | ||
| if (slackAttempt.authResult) return { authResult: slackAttempt.authResult }; | ||
| if (slackAttempt.authResult) return slackAttempt; | ||
| if (slackAttempt.failureMessage) return slackAttempt; | ||
| failures.push({ strategy: 'Slack user JWT', reason: 'not a Slack token' }); |
There was a problem hiding this comment.
💭 Consider: Asymmetric Slack auth handling breaks normalization pattern
Issue: trySlackUserJwtAuth returns { authResult: null } without a failureMessage when the token isn't a Slack token (line 276), unlike all other strategies that were normalized to return failure messages. This requires special handling here with a hardcoded reason.
Why: This creates a coupling where the caller knows implementation details of the callee, and breaks the symmetry of the PR's normalization goal.
Fix: Consider modifying trySlackUserJwtAuth line 276 to return:
return { authResult: null, failureMessage: 'not a Slack token' };Then this block can follow the same pattern as other strategies:
const slackAttempt = await trySlackUserJwtAuth(apiKey, reqData);
if (slackAttempt.authResult) return slackAttempt;
if (slackAttempt.failureMessage) {
// Definitive Slack failures (token IS Slack but invalid) still short-circuit
if (isSlackUserToken(apiKey)) return slackAttempt;
failures.push({ strategy: 'Slack user JWT', reason: slackAttempt.failureMessage });
}Refs:
- trySlackUserJwtAuth:276 — returns without failureMessage
| expect(res.status).toBe(401); | ||
| const body = await res.text(); | ||
| expect(body).toContain('Invalid Slack user token'); | ||
| expect(body).not.toContain('Authentication failed. Tried:'); |
There was a problem hiding this comment.
🟡 Minor: Test doesn't verify complete error message format
Issue: The assertion only checks for 'Invalid Slack user token' but doesn't verify the specific error reason is included (e.g., 'signature verification failed').
Why: If the error message format changes to omit the specific error reason, users would lose diagnostic information. The implementation at line 285 of runAuth.ts constructs: `Invalid Slack user token: ${result.error || 'Invalid token'}`
Fix:
| expect(res.status).toBe(401); | |
| const body = await res.text(); | |
| expect(body).toContain('Invalid Slack user token'); | |
| expect(body).not.toContain('Authentication failed. Tried:'); | |
| expect(res.status).toBe(401); | |
| const body = await res.text(); | |
| expect(body).toContain('Invalid Slack user token: signature verification failed'); | |
| expect(body).not.toContain('Authentication failed. Tried:'); |
Refs:
- runAuth.ts:285 — error message construction
1 participant
Summary
tryTempJwtAuth,tryBypassAuth, andtryApiKeyAuthreturn types fromAuthResult | nulltoAuthAttemptwith descriptive failure messagesauthenticateRequest()to collect failure reasons from all attempted strategies and return a combined summary messageBefore:
Invalid team agent token: signature verification failed(only the last strategy's error)After:
Authentication failed. Tried: JWT temp token (not a JWT), bypass secret (no match), Slack user JWT (not a Slack token), API key (not found), team agent token (signature verification failed)Test plan
Closes ENG-6268
🤖 Generated with Claude Code