Stop using Response errors when validating API Keys #1498
Conversation
…e a new "Result" type that has success and failure conditions. Adding in a way to progressively adopt because this touches everything.
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant enhancements to the API authentication logic across multiple files. Key changes include the introduction of new types for authentication results, modifications to existing authentication functions to improve error handling and response clarity, and updates to control flow in various components to ensure robust validation of authentication results. These changes provide structured success and failure responses, enhancing the overall reliability and usability of the authentication processes. Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/webapp/app/services/apiAuth.server.ts(node:38517) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files Oops! Something went wrong! :( ESLint: 9.15.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
apps/webapp/app/v3/handleWebsockets.server.ts (2)
54-57: Consider enhancing error feedbackWhile the current error message is clear, it could be more specific when possible. If
authenticationResultcontains error details, consider including them in the close message to help developers diagnose issues faster.- if (!authenticationResult || !authenticationResult.ok) { - ws.close(1008, "Invalid API key"); + if (!authenticationResult || !authenticationResult.ok) { + const errorMessage = authenticationResult?.error ?? "Invalid API key"; + ws.close(1008, errorMessage); return; }
54-57: Consider adding type safety improvementsThe double null check suggests we're working with potentially undefined values. Consider adding TypeScript type guards or assertions to make the code more type-safe and maintainable.
+ interface AuthResult { + ok: boolean; + error?: string; + environment: Environment; + } + - if (!authenticationResult || !authenticationResult.ok) { + if (!authenticationResult || !isValidAuthResult(authenticationResult)) { ws.close(1008, "Invalid API key"); return; }Add this helper function:
function isValidAuthResult(result: any): result is AuthResult { return result.ok !== undefined && result.environment !== undefined; }apps/webapp/app/services/apiRateLimit.server.ts (2)
32-32: LGTM! Consider adding type annotation for better type safety.The additional
okproperty check aligns well with the PR's objective of using a Result type for authentication outcomes. This provides more structured handling of authentication results.Consider adding a type annotation to make the Result type explicit:
- if (!authenticatedEnv || !authenticatedEnv.ok) { + if (!authenticatedEnv || !(authenticatedEnv as { ok: boolean }).ok) {
Line range hint
6-70: Well-structured rate limiting configuration with proper separation of concerns.The rate limiter implementation demonstrates good architectural practices:
- Clear separation between authentication and rate limiting logic
- Flexible configuration through environment variables
- Different rate limit rules based on authentication type
- Comprehensive path whitelisting for special endpoints
Consider documenting the rate limit rules and whitelist reasoning in a separate documentation file for better maintainability.
apps/webapp/app/services/realtime/jwtAuth.server.ts (1)
41-45: Refactor repetitive error handling to reduce duplicationThe error responses within the
switchstatement share a similar structure. Refactoring this into a helper function can improve readability and maintainability.Example refactor:
function createErrorResponse(message: string): ValidatePublicJwtKeyError { return { ok: false, error: message, }; }Then update the switch cases:
-case "ERR_JWT_EXPIRED": { - return { - ok: false, - error: - "Public Access Token has expired. See https://trigger.dev/docs/frontend/overview#authentication for more information.", - }; +case "ERR_JWT_EXPIRED": { + return createErrorResponse( + "Public Access Token has expired. See https://trigger.dev/docs/frontend/overview#authentication for more information." + ); }Also applies to: 48-51, 54-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/webapp/app/services/apiAuth.server.ts(4 hunks)apps/webapp/app/services/apiRateLimit.server.ts(1 hunks)apps/webapp/app/services/realtime/jwtAuth.server.ts(1 hunks)apps/webapp/app/services/routeBuilders/apiBuilder.server.ts(7 hunks)apps/webapp/app/v3/handleWebsockets.server.ts(1 hunks)
🔇 Additional comments (18)
apps/webapp/app/v3/handleWebsockets.server.ts (1)
54-57: Verify integration with the new authentication result type
The authentication check has been updated to work with the new Result type, but let's verify the contract with authenticateApiKey.
✅ Verification successful
Integration with authentication result type is correct and type-safe
The WebSocket authentication check is properly integrated with the authentication system. The code correctly handles both success and failure cases through the discriminated union type ApiAuthenticationResult, which guarantees:
- Success case (
ApiAuthenticationResultSuccess):{ ok: true, ... } - Failure case (
ApiAuthenticationResultFailure):{ ok: false, error: string }
The null check combined with the ok property check (!authenticationResult || !authenticationResult.ok) is the correct pattern to handle both:
- Potential undefined returns from legacy authentication calls
- Explicit failure results from the new type system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the return type of authenticateApiKey matches our usage
# Expected: Should find a type or interface definition indicating {ok: boolean}
# Search for the authenticateApiKey function definition
ast-grep --pattern 'export function authenticateApiKey($_) {
$$$
}'
# Search for related type definitions
ast-grep --pattern 'type $_ = {
ok: boolean,
$$$
}'
Length of output: 129
Script:
#!/bin/bash
# Let's try a broader search to find authentication-related code and type definitions
# Search for files containing "authenticateApiKey"
rg -l "authenticateApiKey"
# Search for any Result type definitions
rg -l "type.*Result.*=.*{.*ok.*:.*boolean"
# Get content of files containing "authenticateApiKey" with context
rg "authenticateApiKey" -A 5 -B 5
# Search for authentication-related imports in the WebSocket handler
rg "import.*authenticate" "apps/webapp/app/v3/handleWebsockets.server.ts"
Length of output: 8297
Script:
#!/bin/bash
# Let's check the type definitions in apiAuth.server.ts
rg "type.*ApiAuthentication" apps/webapp/app/services/apiAuth.server.ts -A 10
rg "interface.*ApiAuthentication" apps/webapp/app/services/apiAuth.server.ts -A 10
# Also check the return type and implementation
cat apps/webapp/app/services/apiAuth.server.ts | rg -A 5 "export type ApiAuthenticationResult"
Length of output: 1225
apps/webapp/app/services/apiRateLimit.server.ts (1)
Line range hint 2-2: Verify consistent usage of new Result type across authentication flows.
Let's ensure the imported authenticateAuthorizationHeader function is updated to use the new Result type pattern consistently.
✅ Verification successful
Result type is consistently used across authentication flows
The verification shows that:
- The
authenticateAuthorizationHeaderfunction correctly returnsApiAuthenticationResult | undefinedtype - The usage in
apiRateLimit.server.tsproperly handles both the success and failure cases by checking!authenticatedEnv || !authenticatedEnv.ok - The implementation follows the new Result type pattern with proper type discrimination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if authenticateAuthorizationHeader is consistently updated across the codebase
# Find the implementation and usages of authenticateAuthorizationHeader
ast-grep --pattern 'authenticateAuthorizationHeader($$$)'
# Look for any remaining Response error usage in authentication
rg -l "new Response\(" --type ts | xargs rg "authenticate.*Authorization"
Length of output: 486
Script:
#!/bin/bash
# Let's check the implementation and return type of authenticateAuthorizationHeader
ast-grep --pattern 'export function authenticateAuthorizationHeader($$$)'
# Check for Result type usage in authentication related files
rg "Result" -A 5 -B 5 "apps/webapp/app/services/apiAuth.server.ts"
# Look for any other authentication flows that might need to be consistent
rg "authenticate.*" --type ts
Length of output: 53994
Script:
#!/bin/bash
# Let's check the implementation of authenticateAuthorizationHeader and its return type
rg -A 10 "export async function authenticateAuthorizationHeader" "apps/webapp/app/services/apiAuth.server.ts"
# Check the usage in apiRateLimit.server.ts to see how the result is handled
rg -A 10 "authenticateAuthorizationHeader" "apps/webapp/app/services/apiRateLimit.server.ts"
Length of output: 1256
apps/webapp/app/services/realtime/jwtAuth.server.ts (6)
4-4: Importing AuthenticatedEnvironment for consistent typing
The import of AuthenticatedEnvironment from ../apiAuth.server ensures consistent use of environment typing across authentication modules.
6-10: Defining structured success result type
Introducing ValidatePublicJwtKeySuccess enhances clarity by explicitly defining the structure of a successful validation result.
12-15: Defining structured error result type
The addition of ValidatePublicJwtKeyError provides a clear and consistent way to represent validation failures without throwing exceptions.
17-17: Creating a unified result type
Combining success and error types into ValidatePublicJwtKeyResult improves the function's return type, making it explicit and easier to handle downstream.
19-19: Updating function signature to return structured result
Changing the return type of validatePublicJwtKey to Promise<ValidatePublicJwtKeyResult> aligns with the new structured error handling approach.
64-64: Consistent success response structure
Returning a structured success result with ok: true ensures consistent handling of both success and error cases by the caller.
apps/webapp/app/services/apiAuth.server.ts (7)
32-34: Refactored authentication result types enhance clarity
The introduction of ApiAuthenticationResult as a union of ApiAuthenticationResultSuccess and ApiAuthenticationResultFailure improves type safety and makes the authentication outcomes explicit.
36-42: Well-defined success result type
The ApiAuthenticationResultSuccess type clearly specifies the properties returned on successful authentication, which aids in maintaining consistent handling throughout the codebase.
44-47: Consistent failure result type
Defining ApiAuthenticationResultFailure provides a standardized structure for handling authentication failures, facilitating uniform error responses.
Line range hint 155-230: Comprehensive error handling in authenticateApiKeyWithFailure
The authenticateApiKeyWithFailure function effectively handles various failure scenarios by returning structured failure results. This approach enhances error clarity and simplifies downstream error handling.
337-339: Correct handling of authentication failure
Throwing a JSON response with a 401 status when authentication fails ensures that unauthorized access is appropriately signaled to the client.
471-477: 🛠️ Refactor suggestion
Update deprecated function usage in JWT renewal
The use of the deprecated authenticateApiRequest function may lead to inconsistent behavior. To adhere to the updated authentication flow and ensure consistent error handling, consider replacing it with authenticateApiRequestWithFailure.
Apply this diff to update the function call and adjust error handling:
try {
const jwt = request.headers.get("x-trigger-jwt");
// ... existing code ...
} catch (error) {
if (error instanceof errors.JWTExpired) {
- const authenticatedEnv = await authenticateApiRequest(request);
+ const authenticatedEnv = await authenticateApiRequestWithFailure(request);
- if (!authenticatedEnv) {
+ if (!authenticatedEnv || !authenticatedEnv.ok) {
logger.error("Failed to renew JWT token, missing or invalid Authorization header", {
- error: error.message,
+ error: authenticatedEnv ? authenticatedEnv.error : "Missing or invalid Authorization header",
});
return;
}
const payload = payloadSchema.safeParse(error.payload);
// ... existing code ...Likely invalid or redundant comment.
Line range hint 214-228: Verify compatibility of returned failure results
When validationResults.ok is false, the function returns validationResults directly. Ensure that validationResults conforms to the ApiAuthenticationResultFailure type to maintain consistency in the returned result.
Run the following script to verify the structure of validationResults:
✅ Verification successful
Return types are compatible, no changes needed
The verification shows that both types ValidatePublicJwtKeyError and ApiAuthenticationResultFailure have identical structure:
{
ok: false;
error: string;
}All failure returns from validatePublicJwtKey follow this structure, making them fully compatible with the ApiAuthenticationResultFailure type. The direct return of validationResults in the failure case is type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that validatePublicJwtKey returns an object matching ApiAuthenticationResultFailure when validation fails.
# Test: Search for return statements in validatePublicJwtKey where ok is false.
ast-grep --pattern 'return { ok: false, error: $_ }' --lang typescript services/realtime/jwtAuth.server.ts
Length of output: 191
Script:
#!/bin/bash
# First, let's find the correct location of the validatePublicJwtKey function
fd validatePublicJwtKey
# Then, let's examine its implementation and return types
rg -A 5 "validatePublicJwtKey"
Length of output: 1934
Script:
#!/bin/bash
# Let's examine the ValidatePublicJwtKeyResult type and the implementation of validatePublicJwtKey
rg -B 5 -A 20 "type ValidatePublicJwtKeyResult" apps/webapp/app/services/realtime/jwtAuth.server.ts
# Also examine the ApiAuthenticationResultFailure type
rg -B 2 -A 5 "type ApiAuthenticationResultFailure" apps/webapp/app/services/apiAuth.server.ts
# And look at the actual implementation of validatePublicJwtKey function to see its return statements
ast-grep --pattern 'return { ok: false, $$$}' --lang typescript apps/webapp/app/services/realtime/jwtAuth.server.ts
Length of output: 2896
apps/webapp/app/services/routeBuilders/apiBuilder.server.ts (3)
2-5: Confirm correct imports and exported entities
The updated imports align with the new authentication mechanism. Ensure that ApiAuthenticationResultSuccess and authenticateApiRequestWithFailure are correctly exported from ../apiAuth.server.
81-81: Ensure proper error handling for authentication failures
The use of authenticateApiRequestWithFailure should account for both success and failure cases. Confirm that the implementation correctly handles scenarios where authentication may fail.
410-410: Ensure action route handles authentication failures appropriately
The authenticateApiRequestWithFailure function may return failures, so it's important to handle both success and failure cases in the action route. Confirm that the code correctly processes authentication results.
| @@ -48,7 +51,7 @@ type ApiKeyHandlerFunction< | |||
| ? z.infer<TSearchParamsSchema> | |||
| : undefined; | |||
| headers: THeadersSchema extends z.AnyZodObject ? z.infer<THeadersSchema> : undefined; | |||
| authentication: ApiAuthenticationResult; | |||
| authentication: ApiAuthenticationResultSuccess; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update all usages of ApiKeyHandlerFunction due to type change
Changing the authentication parameter type from ApiAuthenticationResult to ApiAuthenticationResultSuccess may impact any existing handlers that implement ApiKeyHandlerFunction. Ensure all implementations are updated to accommodate this type change to prevent type errors.
@trigger.dev/build
@trigger.dev/core
@trigger.dev/rsc
trigger.dev
@trigger.dev/sdk
@trigger.dev/react-hooks
commit: |
Stop using Response errors when validating API Keys, instead introduce a new "Result" type that has success and failure conditions. Adding in a way to progressively adopt because this touches everything.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation