Update authentication endpoints for BuildEngine usage #1421
Conversation
📝 WalkthroughWalkthroughAdds optional Redis Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Auth Server
participant Redis
participant DB as SuperAdmin Auth/DB
Client->>Server: POST /api/auth/token (code, redirect_uri, scope)
Server->>Redis: MGET (code:<code>, token:<code>) on auth connection (keyPrefix=auth)
Redis-->>Server: return code/token data or nil
alt scope=admin
Server->>DB: verify SuperAdmin credentials/session
DB-->>Server: SuperAdmin OK / denied
end
Server->>Server: validate redirect_uri and scope rules
Server->>Server: generate JWT (includes scope)
Server->>Redis: SET token:<code> with TTL on auth connection
Server-->>Client: 200 JWT / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/server/bullmq/queues.ts(2 hunks)src/routes/(unauthenticated)/api/auth/exchange/+server.ts(1 hunks)src/routes/(unauthenticated)/api/auth/token/+server.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-13T17:26:55.905Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-portal PR: 1343
File: src/routes/(unauthenticated)/api/auth/token/+server.ts:15-19
Timestamp: 2025-10-13T17:26:55.905Z
Learning: In src/routes/(unauthenticated)/api/auth/token/+server.ts, the GET handler correctly uses requireNothing() at the beginning because this endpoint initiates the authentication flow for AppBuilders. Auth establishment endpoints should allow unauthenticated access initially, as users may need to be redirected to login.
Applied to files:
src/routes/(unauthenticated)/api/auth/exchange/+server.tssrc/routes/(unauthenticated)/api/auth/token/+server.ts
📚 Learning: 2025-10-13T19:53:32.811Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1343
File: src/routes/(unauthenticated)/api/auth/token/+server.ts:17-19
Timestamp: 2025-10-13T19:53:32.811Z
Learning: In src/routes/(unauthenticated)/api/auth/token/+server.ts, the challenge parameter is a SHA-256 hash encoded in Base64URL format (43 characters), not hex format. Validation should use the regex /^[A-Za-z0-9_-]{43}$/.
Applied to files:
src/routes/(unauthenticated)/api/auth/exchange/+server.tssrc/routes/(unauthenticated)/api/auth/token/+server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function properly handles null/undefined invite tokens by accepting inviteToken?: string | null and having an explicit guard clause if (!inviteToken) return { error: 'not found' }, making the non-null assertion on searchParams.get('t')! safe since error handling is centralized.
Applied to files:
src/routes/(unauthenticated)/api/auth/token/+server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function handles validation of invite tokens including null/undefined cases, so non-null assertion on searchParams.get('t') is safe because error handling is centralized in checkInviteErrors.
Applied to files:
src/routes/(unauthenticated)/api/auth/token/+server.ts
🧬 Code graph analysis (1)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (1)
src/lib/server/bullmq/queues.ts (1)
getAuthConnection(90-93)
⏰ 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: checks / build-and-test
🔇 Additional comments (6)
src/routes/(unauthenticated)/api/auth/exchange/+server.ts (2)
33-41: LGTM! Proper immediate invalidation with resilient error handling.The code correctly invalidates both the code and token immediately after retrieval using a Redis pipeline, and gracefully handles any deletion errors without blocking the authentication flow.
26-29: Unable to verify the review comment due to repository access limitations. The verification requires examining:
- The actual
keyPrefixconfiguration insrc/lib/server/bullmq/queues.ts- The specific Redis connection setup for
getAuthConnection()- The token storage mechanism to confirm key format compatibility
Based on web research, ioredis provides transparent key prefixing and mget and del work fine with keyPrefix (only the KEYS command has known compatibility issues). The concern about requiring a trailing colon on keyPrefix appears to be more of a naming convention than a strict technical requirement, as ioredis applies the keyPrefix by concatenating it to key names.
However, the core concern—whether keys created by the token endpoint match the key format expected by the exchange endpoint—cannot be validated without examining the actual code.
src/routes/(unauthenticated)/api/auth/token/+server.ts (4)
20-29: LGTM! Clear documentation of authentication constraints.The expanded documentation clearly specifies the allowed redirect_uri patterns and the new scope parameter, making the API contract explicit for both desktop apps and BuildEngine integration.
56-58: LGTM! Proper authorization enforcement for admin scope.The admin scope check correctly requires SuperAdmin privileges when
scope=adminis specified, preventing unauthorized access to BuildEngine. The placement afterrequireAuthenticated()ensures the user is authenticated before checking admin privileges.
92-93: Redis key format depends on correct keyPrefix configuration.Similar to the exchange endpoint, these non-namespaced keys (
code:,token:) rely on the auth connection's keyPrefix to produce the final Redis keys. EnsuregetAuthConnection()insrc/lib/server/bullmq/queues.tsuseskeyPrefix: 'auth:'with a trailing colon.Without the colon, keys would be stored as
authcode:${code}instead ofauth:code:${code}, causing the exchange endpoint to fail when retrieving them.Based on the critical issue identified in
src/lib/server/bullmq/queues.tsat line 91.
44-46: Verify subdomain allowlist for BuildEngine authentication.The regex pattern
buildengine\.scriptoria\.io$permits any subdomain precedingbuildengine.scriptoria.io. Confirm whether:
- Production uses only
buildengine.scriptoria.io, or- Additional subdomains like
dev.buildengine.scriptoria.ioorstaging.buildengine.scriptoria.ioare legitimately usedIf only the exact hostname should be allowed, change to:
url.hostname === 'buildengine.scriptoria.io'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/(unauthenticated)/api/auth/token/+server.ts (1)
84-92: Consider validating the scope parameter against allowed values.The scope parameter is included directly in the JWT without validation. While the
adminscope correctly triggers SuperAdmin enforcement, arbitrary scope values (e.g.,?scope=superuser) would be embedded in the token without authorization checks.Consider adding a whitelist validation:
♻️ Suggested improvement
+const VALID_SCOPES = ['admin', 'default'] as const; +type Scope = typeof VALID_SCOPES[number]; + export const GET: RequestHandler = async ({ locals, url }) => { locals.security.requireNothing(); // validate url params before trying login const challenge = url.searchParams.get('challenge'); const redirectUri = url.searchParams.get('redirect_uri'); + const scope: Scope = VALID_SCOPES.includes(url.searchParams.get('scope') as Scope) + ? (url.searchParams.get('scope') as Scope) + : 'default'; if (!challenge || !redirectUri) error(400, 'Missing URL Search Params');Then use the validated
scopevariable throughout the handler:- if (url.searchParams.get('scope') === 'admin') { + if (scope === 'admin') { locals.security.requireSuperAdmin(); } else if (isBuildEngine) { error(400, 'Missing required scope'); }const token = await new SignJWT({ email: user.Email, - scope: url.searchParams.get('scope') ?? 'default' + scope })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(unauthenticated)/api/auth/token/+server.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-13T17:26:55.905Z
Learnt from: chrisvire
Repo: sillsdev/appbuilder-portal PR: 1343
File: src/routes/(unauthenticated)/api/auth/token/+server.ts:15-19
Timestamp: 2025-10-13T17:26:55.905Z
Learning: In src/routes/(unauthenticated)/api/auth/token/+server.ts, the GET handler correctly uses requireNothing() at the beginning because this endpoint initiates the authentication flow for AppBuilders. Auth establishment endpoints should allow unauthenticated access initially, as users may need to be redirected to login.
Applied to files:
src/routes/(unauthenticated)/api/auth/token/+server.ts
📚 Learning: 2025-10-13T19:53:32.811Z
Learnt from: FyreByrd
Repo: sillsdev/appbuilder-portal PR: 1343
File: src/routes/(unauthenticated)/api/auth/token/+server.ts:17-19
Timestamp: 2025-10-13T19:53:32.811Z
Learning: In src/routes/(unauthenticated)/api/auth/token/+server.ts, the challenge parameter is a SHA-256 hash encoded in Base64URL format (43 characters), not hex format. Validation should use the regex /^[A-Za-z0-9_-]{43}$/.
Applied to files:
src/routes/(unauthenticated)/api/auth/token/+server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function properly handles null/undefined invite tokens by accepting inviteToken?: string | null and having an explicit guard clause if (!inviteToken) return { error: 'not found' }, making the non-null assertion on searchParams.get('t')! safe since error handling is centralized.
Applied to files:
src/routes/(unauthenticated)/api/auth/token/+server.ts
📚 Learning: 2025-09-25T03:51:13.814Z
Learnt from: 7dev7urandom
Repo: sillsdev/appbuilder-portal PR: 1297
File: src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts:7-8
Timestamp: 2025-09-25T03:51:13.814Z
Learning: In the organization invitation flow in src/routes/(unauthenticated)/invitations/organization-membership/+page.server.ts, the checkInviteErrors function handles validation of invite tokens including null/undefined cases, so non-null assertion on searchParams.get('t') is safe because error handling is centralized in checkInviteErrors.
Applied to files:
src/routes/(unauthenticated)/api/auth/token/+server.ts
🧬 Code graph analysis (1)
src/routes/(unauthenticated)/api/auth/token/+server.ts (2)
src/lib/otel/index.ts (1)
error(46-53)src/lib/server/bullmq/queues.ts (1)
getAuthConnection(90-93)
⏰ 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). (2)
- GitHub Check: checks / build-and-test
- GitHub Check: check / build-and-test
🔇 Additional comments (4)
src/routes/(unauthenticated)/api/auth/token/+server.ts (4)
15-30: LGTM!The documentation clearly describes the redirect_uri restrictions and the new optional
scopeparameter with itsadminenforcement requirement.
40-50: LGTM!The BuildEngine hostname detection and URL validation logic correctly allows localhost, loopback, and buildengine.scriptoria.io hosts (including subdomains for different environments).
56-60: LGTM!The scope enforcement logic correctly ensures BuildEngine requests require
scope=admin, and theadminscope in turn requires SuperAdmin authentication. This properly gates BuildEngine access to SuperAdmin users only, as stated in the PR objectives.
94-99: LGTM! Key pattern is consistent across both endpoints.The Redis key naming change from explicit
auth:code:tocode:is correct sincegetAuthConnection()uses a keyPrefix of'auth', resulting in the same effective keys. The exchange endpoint at lines 27-28 and 36-37 uses identical key patterns (code:${code}andtoken:${code}), confirming consistency across all Redis operations.
In the future, BuildEngine will be authenticating directly through Scriptoria instead of using the whitelist policy.
See corresponding changes in sillsdev/appbuilder-buildengine-api#72.
Changes:
?scope=adminenforces this.redirect_uriAdditional Changes:
authas built-in prefix for Redis storage (resolves note from earlier implementation)Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.