Repository navigation
Pull request from trinm/mob-22-implement-apple-sign-in-authentication-api - #1
Merged
nusminhtv merged 9 commits intoJul 29, 2026
Conversation
…tokens Implement complete Apple identity-token verification flow under `POST /api/v1/auth/apple`. Add `jose` dependency for RS256 Apple token verification against remote JWKS and HS256 RoomScan JWT issuance. Create `User` Prisma model with `(provider, providerId)` unique constraint and committed `add_apple_auth` migration. Add infrastructure adapters for Apple verification, user repository with atomic upsert, and RoomScan token
…roxy support Implement two-tier rate limiting: general API (120 req/60s) and Apple auth (20 req/15m) using `express-rate-limit` with process-local MemoryStore. Add `TRUST_PROXY` and four `RATE_LIMIT_*` environment variables with validation for hop count, IP/CIDR lists and window bounds. Rejected requests return 429 with `RATE_LIMIT_EXCEEDED`, draft-8 `RateLimit`/`RateLimit-Policy`/`Retry-After` headers and `x-request-id`. Exempt
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR adds Apple Sign-In with JWKS verification, Prisma user upsert, JWT issuance, and a new authentication endpoint. It also adds process-local API rate limiting, runtime configuration, application wiring, tests, and operational documentation. ChangesApple Auth & Rate Limiting
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RateLimiter
participant AuthRouter
participant AuthService
participant AppleIdentityTokenVerifier
participant UserRepository
participant TokenIssuer
Client->>RateLimiter: POST /api/v1/auth/apple
RateLimiter->>AuthRouter: Forward request within quota
AuthRouter->>AuthService: signInWithApple(identityToken, nonce)
AuthService->>AppleIdentityTokenVerifier: Verify Apple token
AuthService->>UserRepository: Upsert Apple user
AuthService->>TokenIssuer: Issue access and refresh tokens
AuthRouter-->>Client: Return authentication response
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
nus-trinm
requested review from
nus-trinm and
nusminhtv
and removed request for
nus-trinm
July 24, 2026 02:47
![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
🧹 Nitpick comments (8)
src/config/env.ts (1)
94-122: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider enforcing distinct access/refresh token secrets.
AUTH_ACCESS_TOKEN_SECRETandAUTH_REFRESH_TOKEN_SECRETare validated independently but nothing prevents them from being set to the same value. Using one secret for both token types removes a layer of defense against token-type confusion (a refresh token being accepted where an access token is expected, or vice versa) if the issuer/verifier don't otherwise strictly scope tokens by claim. A cheap schema-level guard would close this gap regardless of downstream implementation details.♻️ Suggested addition to the existing superRefine
.superRefine((environment, context) => { if (environment.AUTH_REFRESH_TOKEN_TTL_SECONDS <= environment.AUTH_ACCESS_TOKEN_TTL_SECONDS) { context.addIssue({ code: 'custom', path: ['AUTH_REFRESH_TOKEN_TTL_SECONDS'], message: 'AUTH_REFRESH_TOKEN_TTL_SECONDS must be greater than access token TTL', }); } + + if (environment.AUTH_ACCESS_TOKEN_SECRET === environment.AUTH_REFRESH_TOKEN_SECRET) { + context.addIssue({ + code: 'custom', + path: ['AUTH_REFRESH_TOKEN_SECRET'], + message: 'AUTH_REFRESH_TOKEN_SECRET must differ from AUTH_ACCESS_TOKEN_SECRET', + }); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/env.ts` around lines 94 - 122, Update the existing superRefine on environmentSchema to reject configurations where AUTH_ACCESS_TOKEN_SECRET and AUTH_REFRESH_TOKEN_SECRET are identical. Add a validation issue associated with the refresh-token secret while preserving the existing TTL validation and secret length requirements.prisma/schema.prisma (1)
21-22: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider
@db.TimestamptzforcreatedAt/updatedAt.Without an explicit type, these map to PostgreSQL
TIMESTAMP(3)(no timezone), as confirmed by the migration SQL. For a new auth-related table, storing timezone-aware timestamps up front avoids ambiguity issues later — retrofitting this after data exists is a more invasive migration.♻️ Suggested schema change
- createdAt DateTime `@default`(now()) - updatedAt DateTime `@updatedAt` + createdAt DateTime `@default`(now()) `@db.Timestamptz`(3) + updatedAt DateTime `@updatedAt` `@db.Timestamptz`(3)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/schema.prisma` around lines 21 - 22, Update the createdAt and updatedAt fields in the relevant Prisma model to use the PostgreSQL timezone-aware `@db.Timestamptz` type while preserving their existing defaults and automatic update behavior.test/env.test.ts (1)
6-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
AppConfig-shaped test fixtures across files.The same literal config values (Apple client id, 32+ char secrets, TTLs, rate limits) are hand-duplicated here and in
test/logger.test.ts'smakeConfig(and, per referenced context, intest/rate-limit.test.ts/test/app.test.ts). Extracting a sharedAppConfigtest fixture/builder would reduce the risk of drift the next time a field is added or renamed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/env.test.ts` around lines 6 - 15, Create a shared AppConfig test fixture or builder for the duplicated values currently defined in validEnvironment and test logger makeConfig, then reuse it from the related rate-limit and app tests. Update validEnvironment and makeConfig to derive from this shared helper so fields, secrets, TTLs, and rate limits remain consistent when the configuration shape changes.compose.yml (1)
40-49: 🔒 Security & Privacy | 🔵 TrivialConfirm TRUST_PROXY is documented as required behind a reverse proxy.
TRUST_PROXYdefaults to empty (disabled). If thisapiservice sits behind a load balancer/reverse proxy in production, leaving it unset will cause the app to use the proxy's IP for rate limiting andreq.ip, effectively pooling all clients under one bucket. Worth calling out in deployment docs/runbook that operators must setTRUST_PROXYexplicitly for their topology.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compose.yml` around lines 40 - 49, Document TRUST_PROXY as a required deployment setting when the api service runs behind a reverse proxy or load balancer, including that operators must set it explicitly according to their proxy topology; preserve the existing empty default for deployments without a proxy.src/common/middleware/rate-limit.ts (1)
46-69: 🧹 Nitpick | 🔵 TrivialFail-open behavior on store errors — confirm monitoring is in place.
passOnStoreError: truelets traffic through unlimited if the rate-limit store becomes unavailable. This is presumably intentional (per PR summary "fail-open behavior"), but it means rate-limit protection silently disappears during a store outage. Recommend alerting onRateLimiterloggererror/warncalls (fromcreateRateLimitLogger) so store outages are visible to operators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/middleware/rate-limit.ts` around lines 46 - 69, Confirm that the logger supplied to rateLimit in the surrounding middleware, created via createRateLimitLogger, emits error or warning events to an operationally monitored destination for store failures. Add or connect alerting for those logger events without changing the intentional passOnStoreError: true fail-open behavior.src/infrastructure/auth/apple-identity-verifier.ts (1)
51-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve original error as
causefor observability.
AppleIdentityProviderUnavailableErrorandInvalidAppleIdentityTokenErrorare thrown without chaining the original jose/network error viacause. Since these failures are intentionally hidden from API responses (pertest/app.test.ts"without exposing verification details"), the underlying reason (expired vs. bad signature vs. network timeout) becomes unrecoverable from server-side logs too.♻️ Suggested fix: thread `cause` through
- throw new AppleIdentityProviderUnavailableError(); + throw new AppleIdentityProviderUnavailableError({ cause: error });- if ( - error instanceof InvalidAppleIdentityTokenError || - error instanceof errors.JOSEError || - error instanceof ZodError - ) { - throw new InvalidAppleIdentityTokenError(); - } + if ( + error instanceof InvalidAppleIdentityTokenError || + error instanceof errors.JOSEError || + error instanceof ZodError + ) { + throw new InvalidAppleIdentityTokenError({ cause: error }); + }(Requires adding an optional
causeparam to the error constructors inauth.errors.ts.)Also applies to: 108-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/infrastructure/auth/apple-identity-verifier.ts` around lines 51 - 61, Preserve the original verification error when translating failures in the Apple identity verifier. Add an optional cause parameter to the AppleIdentityProviderUnavailableError and InvalidAppleIdentityTokenError constructors in auth.errors.ts, then pass the caught error when throwing those errors while keeping the existing public error behavior and rethrows unchanged.src/server.ts (2)
2-10: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove
dotenv/configto the first import.
import 'dotenv/config'is listed last among the imports; every module imported above it is evaluated before.envis loaded intoprocess.env. Currently no listed module readsprocess.envat top level, but this ordering is fragile — any future top-level env read in one of these (or their transitive imports) will silently see undefined values.♻️ Proposed fix
+import 'dotenv/config'; import { createRateLimiters } from './common/middleware/rate-limit.js'; import { loadConfig } from './config/env.js'; import { AppleIdentityTokenVerifier } from './infrastructure/auth/apple-identity-verifier.js'; import { JoseAuthTokenIssuer } from './infrastructure/auth/jwt-token-issuer.js'; import { createPrismaClient, PrismaDatabase } from './infrastructure/database/prisma.js'; import { PrismaAppleUserRepository } from './infrastructure/database/prisma-user-repository.js'; import { createLogger } from './infrastructure/logging/logger.js'; import { AuthService } from './modules/auth/auth.service.js'; -import 'dotenv/config';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 2 - 10, Move the side-effect import for dotenv/config to the first import position in server.ts, before createRateLimiters, loadConfig, and all other module imports, so environment variables are initialized before any imported module is evaluated.
18-23: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNew token secrets aren't covered by log redaction.
accessTokenSecret/refreshTokenSecretare new sensitive config values butlogger.ts's redact list (per graph context) only coversdatabaseUrl/DATABASE_URLand header auth/cookie fields, not these secrets. Ifconfigis ever logged directly, they'd leak in plaintext.Consider adding
config.accessTokenSecret,config.refreshTokenSecret,accessTokenSecret, andrefreshTokenSecretto the redact paths increateLogger.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 18 - 23, Add the token secret paths accessTokenSecret and refreshTokenSecret, including their config-prefixed variants, to the redaction configuration in createLogger. Preserve the existing database URL and header/cookie redaction entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@document/architecture.md`:
- Around line 51-54: Update the Apple sign-in flow documentation and
implementation scope to address nonce binding: generate and transport a nonce
through authorization, then validate it against the returned identity token
before accepting authentication. If nonce binding cannot be implemented in this
change, record an explicit security exception documenting the replay risk and
limitation.
In `@document/development.md`:
- Around line 29-37: Update the development setup instructions in
document/development.md to run the appropriate Prisma migration command after
starting PostgreSQL and before yarn dev, ensuring fresh databases have the
required schema. Synchronize the corresponding setup sequence in the README so
both documents include the migration step in the same position.
In `@src/infrastructure/auth/apple-identity-verifier.ts`:
- Around line 81-124: Update the Apple identity verification flow around verify
and its callers to accept the client nonce, require the token’s nonce claim
through appleClaimsSchema, and compare it before returning
VerifiedAppleIdentity. Reject missing or mismatched nonces as
InvalidAppleIdentityTokenError, and thread the nonce through the endpoint before
session creation.
---
Nitpick comments:
In `@compose.yml`:
- Around line 40-49: Document TRUST_PROXY as a required deployment setting when
the api service runs behind a reverse proxy or load balancer, including that
operators must set it explicitly according to their proxy topology; preserve the
existing empty default for deployments without a proxy.
In `@prisma/schema.prisma`:
- Around line 21-22: Update the createdAt and updatedAt fields in the relevant
Prisma model to use the PostgreSQL timezone-aware `@db.Timestamptz` type while
preserving their existing defaults and automatic update behavior.
In `@src/common/middleware/rate-limit.ts`:
- Around line 46-69: Confirm that the logger supplied to rateLimit in the
surrounding middleware, created via createRateLimitLogger, emits error or
warning events to an operationally monitored destination for store failures. Add
or connect alerting for those logger events without changing the intentional
passOnStoreError: true fail-open behavior.
In `@src/config/env.ts`:
- Around line 94-122: Update the existing superRefine on environmentSchema to
reject configurations where AUTH_ACCESS_TOKEN_SECRET and
AUTH_REFRESH_TOKEN_SECRET are identical. Add a validation issue associated with
the refresh-token secret while preserving the existing TTL validation and secret
length requirements.
In `@src/infrastructure/auth/apple-identity-verifier.ts`:
- Around line 51-61: Preserve the original verification error when translating
failures in the Apple identity verifier. Add an optional cause parameter to the
AppleIdentityProviderUnavailableError and InvalidAppleIdentityTokenError
constructors in auth.errors.ts, then pass the caught error when throwing those
errors while keeping the existing public error behavior and rethrows unchanged.
In `@src/server.ts`:
- Around line 2-10: Move the side-effect import for dotenv/config to the first
import position in server.ts, before createRateLimiters, loadConfig, and all
other module imports, so environment variables are initialized before any
imported module is evaluated.
- Around line 18-23: Add the token secret paths accessTokenSecret and
refreshTokenSecret, including their config-prefixed variants, to the redaction
configuration in createLogger. Preserve the existing database URL and
header/cookie redaction entries.
In `@test/env.test.ts`:
- Around line 6-15: Create a shared AppConfig test fixture or builder for the
duplicated values currently defined in validEnvironment and test logger
makeConfig, then reuse it from the related rate-limit and app tests. Update
validEnvironment and makeConfig to derive from this shared helper so fields,
secrets, TTLs, and rate limits remain consistent when the configuration shape
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6acfbcaa-e92f-4d0e-8aa2-ebada066c324
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.env.exampleREADME.mdcompose.ymldocument/api-conventions.mddocument/architecture.mddocument/development.mdpackage.jsonprisma/migrations/20260723094350_add_apple_auth/migration.sqlprisma/migrations/migration_lock.tomlprisma/schema.prismasrc/app.tssrc/common/middleware/rate-limit.tssrc/config/env.tssrc/infrastructure/auth/apple-identity-verifier.tssrc/infrastructure/auth/jwt-token-issuer.tssrc/infrastructure/database/prisma-user-repository.tssrc/infrastructure/database/prisma.tssrc/modules/auth/auth.errors.tssrc/modules/auth/auth.openapi.tssrc/modules/auth/auth.routes.tssrc/modules/auth/auth.schemas.tssrc/modules/auth/auth.service.tssrc/modules/auth/auth.types.tssrc/openapi/document.tssrc/server.tstest/app.test.tstest/apple-identity-verifier.test.tstest/auth.service.test.tstest/env.test.tstest/jwt-token-issuer.test.tstest/logger.test.tstest/prisma-user-repository.test.tstest/rate-limit.test.ts
…k prevention Add optional `nonce` field to Apple auth request (max 512 bytes). When provided, `AppleIdentityTokenVerifier` validates the identity token's `nonce` claim matches exactly, rejecting mismatches or missing claims as `InvalidAppleIdentityTokenError`. When omitted, nonce validation is skipped for backward compatibility. Thread nonce through `AuthService.signInWithApple`, route handler, and all type signatures. Update documentation
nusminhtv
requested changes
Jul 24, 2026
…fore body parsing Mount Apple auth rate limiter at `/api/v1/auth/apple` alongside general API limiter, before body parsers, so malformed JSON consumes Apple quota. Remove limiter from auth router dependencies and handler chain. Update architecture documentation to clarify both limiters apply before parsing. Add test verifying malformed JSON counts against Apple policy.
Update `AUTH_REFRESH_TOKEN_TTL_SECONDS` default from 604800 (7 days) to 2592000 (30 days) in `.env.example` and `compose.yml` fallback value to reduce re-authentication frequency for mobile users.
Update `AUTH_ACCESS_TOKEN_TTL_SECONDS` default from 900 (15 minutes) to 3600 (60 minutes) in schema, README, and all test fixtures to reduce re-authentication frequency while maintaining reasonable session security.
nusminhtv
deleted the
trinm/mob-22-implement-apple-sign-in-authentication-api
branch
July 29, 2026 02:16
This was referenced Jul 29, 2026
nusminhtv
pushed a commit
that referenced
this pull request
Jul 30, 2026
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
POST /api/v1/auth/apple, supporting optional nonce verification, Apple user upsert, and access/refresh JWT issuance.429responses.Configuration
.env.example) for proxy trust, rate-limiting, Apple credentials, token secrets, and token TTLs.Documentation
Database
Tests