Provide API Tokens for AppBuilders #1343
Conversation
WalkthroughAdds an auth code exchange flow with a dedicated lazy auth Redis connection, new unauthenticated GET/POST endpoints to issue short-lived codes and exchange them for a cookie, refactors the project token route to use authenticated locals.security and standardized errors, and applies a minor formatting tweak in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Native App (SAB)
participant Browser
participant S2 as S2 API
participant Redis as Auth Store (Redis)
App->>Browser: Open /api/auth/token?challenge=<hash>&redirect-url=app://callback
Browser->>S2: GET /api/auth/token
Note over S2: locals.security.requireAuthenticated()
S2->>Redis: SET auth:code:<code> = challenge (TTL 300s)
S2->>Redis: SET auth:cookie:<code> = serialized session cookie (TTL 301s)
S2-->>Browser: 302 redirect to app://callback?code=<code>
Browser-->>App: app://callback?code=<code>
App->>S2: POST /api/auth/exchange { code, verify }
S2->>Redis: GET auth:code:<code>
S2->>Redis: GET auth:cookie:<code>
S2->>Redis: DEL auth:code:<code>
S2->>Redis: DEL auth:cookie:<code>
alt sha256(verify) == challenge
S2-->>App: 200 + Set-Cookie (session)
else missing/invalid
S2-->>App: 400 error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/auth.ts(1 hunks)src/lib/prisma/migrations/13_publication_date_resolved/migration.sql(1 hunks)src/lib/prisma/migrations/14_api_tokens/migration.sql(1 hunks)src/lib/prisma/schema.prisma(3 hunks)src/lib/server/bullmq/queues.ts(1 hunks)src/routes/(unauthenticated)/api/auth/exchange/+server.ts(1 hunks)src/routes/(unauthenticated)/api/auth/token/+server.ts(1 hunks)src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/auth.ts (2)
src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (2)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(89-92)src/lib/server/database/index.ts (1)
DatabaseWrites(68-71)
src/routes/(unauthenticated)/api/auth/token/+server.ts (1)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(89-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (2)
11-19: Rename to avoid confusion with existing ErrorResponse type.The function name
ErrorResponseconflicts with theErrorResponsetype defined insrc/lib/server/build-engine-api/types.ts. This could cause confusion when reading the code.Consider renaming the function to make its purpose clearer:
-function ErrorResponse(status: number, title: string) { +function createAppBuilderErrorResponse(status: number, title: string) { return new Response( JSON.stringify({ errors: [{ title }] }), { status } ); }Then update all usages accordingly (lines 54, 57, 88, 94, 109, 111).
108-111: Consider using 500 for external API failures.When the BuildEngine API fails to return a token, a 500 status code might be more appropriate than 400, as this represents a server-side failure rather than a client error. However, 400 is acceptable if the intent is to indicate that the token request cannot be fulfilled.
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (4)
6-7: Add TypeScript type annotations for improved type safety.The function parameters lack explicit type annotations. Consider adding the standard SvelteKit types:
+import type { RequestEvent } from '@sveltejs/kit'; + -export async function POST({ locals, request }) { +export async function POST({ locals, request }: RequestEvent) {
22-30: Consider the trade-off: invalidation before verification prevents retry.The code deletes both Redis entries (lines 23-24) before verifying the hash (line 30). This means:
- Good: Prevents replay attacks (code is single-use).
- Consequence: A user who provides an incorrect
verifyvalue cannot retry with the same code, even if the issue was a typo or timing problem.This one-time-use behavior is likely intentional for security, but ensure this aligns with the desired user experience.
Additionally, consider using constant-time comparison for the digest to mitigate timing attacks:
+import { timingSafeEqual } from 'node:crypto'; + const hash = createHash('sha256'); hash.update(body.output.verify); const digest = hash.digest('hex'); -if (digest !== challenge) error(400, 'Failed Verification'); +const digestBuffer = Buffer.from(digest, 'hex'); +const challengeBuffer = Buffer.from(challenge, 'hex'); +if (!timingSafeEqual(digestBuffer, challengeBuffer)) { + error(400, 'Failed Verification'); +}
20-20: Address TODO comments about error codes.The TODO comments question whether HTTP 400 is the appropriate error code. Consider:
- 400 Bad Request: Appropriate for invalid client input (malformed code, failed verification).
- 401 Unauthorized: Could be used for authentication failures, but 400 is reasonable here since the issue is with the provided credentials.
- 404 Not Found: Could be used when the code doesn't exist, but 400 is acceptable to avoid leaking information about code existence.
HTTP 400 is reasonable for both cases. Remove the TODO comments if this is the intended behavior, or update the codes if a different status is preferred.
Also applies to: 30-30
6-36: Add error handling and observability for production readiness.Consider the following improvements:
- Redis error handling: Wrap Redis operations in try-catch to handle connection failures gracefully.
- Security event logging: Log failed verification attempts, missing challenges, and other security-relevant events for monitoring and alerting.
- Rate limiting: Consider adding rate limiting to prevent brute-force attacks on the verification endpoint.
Example of adding error handling:
try { const challenge = await getAuthConnection().get(`auth0:code:${body.output.code}`); const cookie = await getAuthConnection().get(`auth0:cookie:${body.output.code}`); // ... rest of logic } catch (err) { console.error('Redis operation failed:', err); error(503, 'Service temporarily unavailable'); }src/routes/(unauthenticated)/api/auth/token/+server.ts (1)
17-19: Consider renaming the key prefix to avoid confusion.The
auth0:prefix in the Redis keys is misleading since this implementation doesn't use Auth0 directly (per the PR description, the Auth0 signIn function couldn't be made to work). Consider using a more generic prefix likeauth:orappbuilder:.Apply this diff to use a more accurate prefix:
- await getAuthConnection().set(`auth0:code:${code}`, challenge, 'EX', 300); // 5 minute (300 s) TTL + await getAuthConnection().set(`auth:code:${code}`, challenge, 'EX', 300); // 5 minute (300 s) TTL await getAuthConnection().set( - `auth0:cookie:${code}`, + `auth:cookie:${code}`, cookies.get('authjs.session-token') ?? '', 'EX', 301 ); // 5 minute (300 s) TTL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/auth.ts(1 hunks)src/lib/prisma/schema.prisma(1 hunks)src/routes/(unauthenticated)/api/auth/exchange/+server.ts(1 hunks)src/routes/(unauthenticated)/api/auth/token/+server.ts(1 hunks)src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/auth.ts
- src/lib/prisma/schema.prisma
🧰 Additional context used
🧬 Code graph analysis (3)
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (4)
src/lib/server/build-engine-api/types.ts (2)
ErrorResponse(29-36)Response(21-27)src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (1)
POST(7-50)src/routes/(authenticated)/tasks/sse/+server.ts (1)
POST(6-32)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
src/routes/(unauthenticated)/api/auth/token/+server.ts (1)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(89-92)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (1)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(89-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (10)
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (5)
21-32: LGTM: Authentication flow is clearer.The refactored authentication flow using
locals.security.requireAuthenticated()andlocals.security.userIdis cleaner and more maintainable than the previous JWT-based approach. ThefindUniqueOrThrowcall is safe here because authentication ensures the userId exists.
54-57: LGTM: Clear and descriptive error messages.The error responses provide helpful context (project ID and specific issue) for debugging.
87-97: LGTM: Clear permission error messages.The permission denied errors provide helpful context (project ID, user name, email) for troubleshooting access issues.
140-144: LGTM: Clean usage of user properties.Using the fetched user object properties directly is cleaner and more maintainable than the previous approach.
67-84: Avoid catching all errors in authorization
The blanketcatcharoundrequireAdminOfOrg/requireHasRolewill swallow non-authorization errors. I didn’t find any non-throwing checks (isAdminOfOrg/hasRole) in the codebase—please verify their existence or implement them. Otherwise, catch only the specific authorization error type.src/routes/(unauthenticated)/api/auth/exchange/+server.ts (2)
1-4: LGTM!The imports are appropriate for implementing the auth code exchange endpoint.
9-15: LGTM!The input validation using valibot is well-structured. The schema correctly enforces a UUID for the code and validates the verify string.
src/routes/(unauthenticated)/api/auth/token/+server.ts (3)
1-3: LGTM!The imports are appropriate for the endpoint's functionality.
14-14: LGTM!Using
randomUUID()for code generation is cryptographically secure and appropriate for this use case.
25-25: Redirect implementation is correct.The redirect with the appended code parameter is implemented correctly. However, ensure the redirect URL validation concern raised earlier (Line 7) is addressed to prevent open redirect vulnerabilities.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/routes/(unauthenticated)/api/auth/token/+server.ts (1)
6-11: Critical: Open redirect vulnerability still unresolved.The
redirectUrlquery parameter is not validated before use in the redirect on line 27. A previous review flagged this as a critical issue, but it remains unaddressed. An attacker can craft a malicious URL that redirects authenticated users to a phishing site.Apply this diff to validate the redirect URL:
const challenge = url.searchParams.get('challenge'); const redirectUrl = url.searchParams.get('redirect-url'); if (!challenge || !redirectUrl) { locals.security.requireNothing(); error(400, 'Missing URL Search Params'); } + + // Validate redirect URL to prevent open redirects + if (!redirectUrl.startsWith('/') && !redirectUrl.startsWith('scriptureappbuilder://')) { + error(400, 'Invalid redirect URL'); + } + locals.security.requireAuthenticated();Note: Adjust the allowed schemes based on your specific requirements. The example allows relative paths and the
scriptureappbuilder://scheme mentioned in the PR objectives.
🧹 Nitpick comments (4)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (1)
36-36: Consider returning structured JSON response.The cookie is returned as plain text in the response body without a Content-Type header. For better client integration and clarity, consider returning a JSON response with an explicit structure.
Apply this diff to return structured JSON:
- return new Response(cookie); + return new Response(JSON.stringify({ token: cookie }), { + headers: { 'Content-Type': 'application/json' } + });src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (3)
108-111: Consider differentiating error status codes.All token generation errors return status 400. Consider using 502 (Bad Gateway) or 503 (Service Unavailable) when the BuildEngine service fails, to distinguish between client errors and upstream service issues.
Example:
-if (!tokenResult || tokenResult.responseType === 'error') - return createAppBuildersError(400, `Project id=${projectId}: GetProjectToken returned null`); +if (!tokenResult || tokenResult.responseType === 'error') + return createAppBuildersError(502, `Project id=${projectId}: GetProjectToken failed`);
67-85: Consider adding audit logging for authorization checks.For security and debugging purposes, consider logging failed authorization attempts, especially when multiple checks are performed. This helps with security auditing and troubleshooting access issues.
Example:
if (readOnly === null) { try { locals.security.requireAdminOfOrg(project.OrganizationId); readOnly = false; } catch (e) { console.info('Admin check failed for user', user.Id, 'on project', projectId); // Continue to next check } }
24-32: Handle nullable WorkflowUserId
WorkflowUserId is defined as a nullable String in the Prisma schema; passing it directly to productTransitions.createMany may insert nulls—ensure you handle or validate null WorkflowUserId before calling createMany.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts(1 hunks)src/routes/(unauthenticated)/api/auth/token/+server.ts(1 hunks)src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/routes/(unauthenticated)/api/auth/token/+server.ts (1)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(89-92)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (1)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(89-92)
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (4)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (1)
POST(6-40)src/routes/(authenticated)/projects/[id=idNumber]/sse/+server.ts (1)
POST(7-50)src/routes/(authenticated)/tasks/sse/+server.ts (1)
POST(6-32)src/lib/server/database/prisma.ts (1)
DatabaseReads(22-22)
🔇 Additional comments (6)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (2)
20-20: 400 is appropriate for invalid or expired codes.The TODO questions the error code choice. 400 Bad Request is correct here because an invalid or expired code represents a client error—the client provided a code that cannot be exchanged. This aligns with standard practices for token exchange flows.
30-34: Hash verification logic is correct.The SHA-256 challenge-response verification is implemented correctly. The 400 error code for failed verification is appropriate, as it indicates a client error (incorrect verify value).
src/routes/(unauthenticated)/api/auth/token/+server.ts (1)
16-17: Session cookie validation added correctly.The explicit check for the session cookie and 401 error response properly addresses the previous review concern about handling missing cookies.
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts (3)
11-19: LGTM! Standardized error helper.The
createAppBuildersErrorhelper provides a consistent error response format for AppBuilders, improving maintainability.
38-57: LGTM! Project validation is clear.The project lookup and validation logic properly handles missing projects and null
WorkflowProjectUrl, returning appropriate 404 errors with descriptive messages.
87-97: LGTM! Permission validation is thorough.The final permission checks properly handle the case where no authorization succeeds, and correctly prevent upload operations for read-only users. Error messages include helpful context.
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts
Show resolved
Hide resolved
src/routes/(unauthenticated)/api/projects/[id=idNumber]/token/+server.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (1)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(89-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
FyreByrd
force-pushed
the
feature/auth-for-sab
branch
from
66d6a15 to
fbc7e52
Compare
Allow localhost, 127.0.0.1, or org.sil.{sab,rab,dab,kab} as hostname
1 participant
Fixes #1341
Auth flow:
GET /api/auth/token?challenge=<sha256 hash of verification string>&redirect-url=<redirect url>.redirects to
<redirect url>?code=<temporary code>POST /api/auth/exchangewith body{ "code": "<temporary code>", "verify": "<verification string>" }returns a response with
Set-Cookieheaders.TODO: validate redirect-url
Summary by CodeRabbit
New Features
Refactor