feat(auth): MVP customer signup seam (TEC-60)#23
Merged
Conversation
- POST /api/v1/auth/sign-up endpoint with validation, duplicate-email rejection, structured logging - Extended auth repository contract (registerCustomer) with in-memory + Postgres implementations - Migration 0009 for users.full_name + users.password_hash columns - api-client SignUpRequestBody contract + createSignUpRequest helper - product-app create-account CTA wired to live signup with full-name input, loading/error states, authenticated handoff - Focused tests: signup success, validation failure, duplicate-email (platform-api + product-app) - README updated to reflect sign-up capability Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAuth sign-up support added end-to-end: product-app UI and client add sign-up flows; api-client gains sign-up request builder; platform-api adds sign-up controller, AuthService.signUp, repository registration methods, DB migration, and in-memory/Postgres implementations with password hashing/verification. Tests updated/added across layers. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Product App UI
participant Client as API Client (createSignUpRequest)
participant API as Platform API Controller
participant Service as AuthService
participant Repo as AuthSessionRepository
participant DB as Database
App->>Client: build POST /auth/sign-up {name,email,password}
Client->>API: HTTP POST /api/v1/auth/sign-up
API->>Service: signUp(body, correlationId)
Service->>Repo: registerCustomer({name, normalizedEmail, password})
Repo->>DB: INSERT users (email lowercased, full_name, password_hash) ON CONFLICT
alt inserted
Repo->>DB: INSERT sessions row
DB-->>Repo: session row with token
Repo-->>Service: AuthSession { token, userId, role, email }
Service-->>API: 200 { token, sessionState }
API-->>Client: 200 OK
Client-->>App: success response
else conflict
DB-->>Repo: conflict
Repo-->>Service: throw DuplicateEmailError
Service-->>API: ConflictException
API-->>Client: 409 Conflict
Client-->>App: error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/api-client/src/index.ts (1)
65-69: Make the sign-up payload required in the client contract.
createSignUpRequest({})currently type-checks even though this route only works with all three fields present. Makingname,passwordrequired will catch bad call sites before they become 400s at runtime.Suggested change
export type SignUpRequestBody = { - name?: string; - email?: string; - password?: string; + name: string; + email: string; + password: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/src/index.ts` around lines 65 - 69, The SignUpRequestBody type currently has optional fields (name?, email?, password?) which allows invalid calls like createSignUpRequest({}) to type-check; change SignUpRequestBody to require name, email, and password (remove the ?), update any related factory/function signatures such as createSignUpRequest to accept the stricter type, and fix call sites/tests that pass incomplete payloads to use a complete { name, email, password } object so callers are caught at compile time rather than producing 400s at runtime.apps/product-app/src/features/auth/auth-entry-actions.test.js (1)
5-88: Assert the outbound request shape as well.These tests only validate the mapped result. They would still pass if
createSignInRequestorcreateSignUpRequestpointed at the wrong path or serialized the wrong body becausefetchMockignores its inputs. AddingtoHaveBeenCalledWith(...)assertions would lock down the new API-client integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/product-app/src/features/auth/auth-entry-actions.test.js` around lines 5 - 88, Tests currently only assert return shapes; they should also assert the outbound fetch call shape so createSignInRequest/createSignUpRequest are exercised. After each signInWithCredentials/signUpWithCredentials call, add an assertion that fetchMock was called with the request produced by createSignInRequest(...) or createSignUpRequest(...) (and the same base URL used in the call), e.g. expect(fetchMock).toHaveBeenCalledWith(createSignInRequest({email...,password...,role...}, 'http://localhost:3100')) (and likewise for sign-up), or use expect.objectContaining on the fetch args to verify method, path and body match the expected request shape. Ensure you reference the same argument objects used in the test when calling createSignInRequest/createSignUpRequest so the assertion locks down the outbound request shape.apps/product-app/src/features/auth/auth-entry-actions.js (1)
31-39: Preserve request-descriptor headers inrunAuthRequest.This helper now consumes request objects from
@quickwerk/api-client, but it ignoresrequest.headers. That makes the abstraction incomplete and will break silently as soon as an auth route needs extra headers.Suggested change
const response = await fetch(`${baseUrl}${request.path}`, { method: request.method, - headers: { 'content-type': 'application/json' }, + headers: { + 'content-type': 'application/json', + ...(request.headers ?? {}), + }, body: JSON.stringify(request.body), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/product-app/src/features/auth/auth-entry-actions.js` around lines 31 - 39, runAuthRequest currently ignores request.headers from the `@quickwerk/api-client` request object, so preserve and merge them into the fetch call; update the headers passed to fetch in runAuthRequest to include request.headers (e.g., merge request.headers into the default {'content-type':'application/json'} so callers can override or add headers) and ensure you handle a missing request.headers safely (null/undefined) and still stringify request.body as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/product-app/app/auth.js`:
- Around line 45-64: The handler handleCreateAccount currently ignores the
user's provider selection and always creates a customer account; add a guard at
the start of handleCreateAccount (before calling signUpWithCredentials) that
checks the incoming provider value (the parameter passed to handleCreateAccount)
and if it indicates a non-customer/provider signup that isn’t supported, set an
appropriate error (via setError), reset loading (setLoading(false)) and return
early so no sign-up, setSession, or router.replace('/home-triage') occurs;
reference handleCreateAccount, signUpWithCredentials, setError, setLoading,
setSession, and router.replace to locate where to add this check.
In `@services/platform-api/src/auth/auth.service.ts`:
- Around line 103-111: The failure breadcrumb for auth.sign-up.write currently
includes the raw email in the details (logged via logStructuredBreadcrumb),
exposing PII; update the log call in the auth.sign-up.write failure path to
remove the raw email (or replace it with a non-reversible identifier such as a
salted hash or truncated hash) and ensure the details object only contains
reason: 'duplicate-email' and a non-PII correlation key if needed; modify the
logStructuredBreadcrumb invocation in the sign-up failure branch to stop
emitting the email field (or emit emailHash) so PII is not written to logs.
In
`@services/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.ts`:
- Around line 44-47: The in-memory registerCustomer method is checking
input.email without normalizing, causing case-sensitive duplicates that differ
from Postgres behavior; update registerCustomer to normalize the email (e.g.,
const email = input.email.toLowerCase()) before performing the
customersByEmail.has check, before throwing DuplicateEmailError, and use the
normalized email when storing the new customer/session so customersByEmail and
any lookups are consistent with the Postgres implementation (refer to
registerCustomer, customersByEmail, RegisterCustomerInput, and
DuplicateEmailError).
In
`@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts`:
- Around line 79-84: The SQL insert is storing input.password directly into the
password_hash column (variables userId, normalizedEmail, input.name,
input.password in postgres-auth-session.repository.ts); replace this by hashing
the plaintext password (e.g., await bcrypt.hash(input.password, saltRounds) or
argon2.hash) before building the query and pass the resulting hash instead of
input.password, and apply the same change to the in-memory implementation so
both storage layers write real hashes to password_hash; ensure async hashing is
awaited before the INSERT, update any tests that construct users to
supply/verify hashed passwords, and keep the column name password_hash as the
hashed value.
---
Nitpick comments:
In `@apps/product-app/src/features/auth/auth-entry-actions.js`:
- Around line 31-39: runAuthRequest currently ignores request.headers from the
`@quickwerk/api-client` request object, so preserve and merge them into the fetch
call; update the headers passed to fetch in runAuthRequest to include
request.headers (e.g., merge request.headers into the default
{'content-type':'application/json'} so callers can override or add headers) and
ensure you handle a missing request.headers safely (null/undefined) and still
stringify request.body as before.
In `@apps/product-app/src/features/auth/auth-entry-actions.test.js`:
- Around line 5-88: Tests currently only assert return shapes; they should also
assert the outbound fetch call shape so createSignInRequest/createSignUpRequest
are exercised. After each signInWithCredentials/signUpWithCredentials call, add
an assertion that fetchMock was called with the request produced by
createSignInRequest(...) or createSignUpRequest(...) (and the same base URL used
in the call), e.g.
expect(fetchMock).toHaveBeenCalledWith(createSignInRequest({email...,password...,role...},
'http://localhost:3100')) (and likewise for sign-up), or use
expect.objectContaining on the fetch args to verify method, path and body match
the expected request shape. Ensure you reference the same argument objects used
in the test when calling createSignInRequest/createSignUpRequest so the
assertion locks down the outbound request shape.
In `@packages/api-client/src/index.ts`:
- Around line 65-69: The SignUpRequestBody type currently has optional fields
(name?, email?, password?) which allows invalid calls like
createSignUpRequest({}) to type-check; change SignUpRequestBody to require name,
email, and password (remove the ?), update any related factory/function
signatures such as createSignUpRequest to accept the stricter type, and fix call
sites/tests that pass incomplete payloads to use a complete { name, email,
password } object so callers are caught at compile time rather than producing
400s at runtime.
🪄 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: b30a66ba-9df0-4875-84dc-d6a7f0d437e7
📒 Files selected for processing (15)
README.mdapps/product-app/app/auth.jsapps/product-app/src/features/auth/auth-entry-actions.jsapps/product-app/src/features/auth/auth-entry-actions.test.jsapps/product-app/src/features/auth/auth-entry-screen.jspackages/api-client/src/index.tsservices/platform-api/migrations/0009_signup_customer_profile_fields.sqlservices/platform-api/src/auth/auth.controller.tsservices/platform-api/src/auth/auth.service.test.tsservices/platform-api/src/auth/auth.service.tsservices/platform-api/src/auth/domain/auth-session.repository.tsservices/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.test.tsservices/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.tsservices/platform-api/src/auth/infrastructure/postgres-auth-session.repository.test.tsservices/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
services/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.ts (1)
94-100: Extract the password-hash format into shared auth code.The same
hashPassword()implementation now exists here and inpostgres-auth-session.repository.ts. Once verification or cost tuning lands, drift between adapters will make test and production hashes diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.ts` around lines 94 - 100, Extract the password hashing implementation currently duplicated in hashPassword (which uses scryptAsync, randomBytes, and the `scrypt$${salt}$${derivedKey}` format) into a shared auth utility module and replace the local function with an import; update both in-memory repository (in-memory-auth-session.repository.ts) and postgres-auth-session.repository.ts to call the shared function (e.g., export function hashPassword(...) in auth-utils or auth/crypto) so future verification/cost changes are centralized and both adapters use the exact same format and implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform-api/src/auth/auth.service.ts`:
- Around line 170-177: The signIn flow must use the same canonicalization as
signUp: replace any direct uses of raw input (e.g., input.email?.trim()) in
signIn() with a call to normalizeEmail(email) so both signUp() and signIn()
operate on the identical lowercased/trimmed email value; after updating signIn()
to call normalizeEmail, ensure repository lookups (used by signIn and signUp)
query by the normalized value and add or verify a case-insensitive uniqueness
constraint in storage so emails cannot be duplicated by case differences.
- Around line 71-85: The signup flow (signUp) currently stores a password but
the auth flow still creates sessions from email/role without verifying the
password; fix by either disabling password-based signup or wiring full
verification into sign-in: ensure registerCustomer continues to store a hashed
password (use normalizePassword to produce the raw/password input only), then
update signIn to fetch the stored password_hash and verify with a secure compare
(e.g., bcrypt.compare) before issuing a session, and change any session creation
logic that currently constructs sessions from email/role alone (e.g.,
sessionStore.createSession or similar) to only run after successful password
verification; alternatively, if you cannot implement signIn verification in this
release, reject password signups in signUp (throw or return an unsupported
error) so passwords are not stored without enforcement.
In
`@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts`:
- Around line 79-105: The users insert and sessions insert in the registration
flow (calls to this.postgresClient.query inserting into users and sessions,
returning createdUser and row) must be made atomic: wrap both statements in a
single DB transaction (BEGIN; INSERT users...; INSERT sessions...; COMMIT) using
your postgresClient transaction API or explicit BEGIN/COMMIT/ROLLBACK queries,
and on any error ensure you ROLLBACK; if you cannot use a transaction, implement
compensating cleanup by deleting the newly created user (using createdUser.id)
when session creation fails before throwing DuplicateEmailError or other errors;
ensure DuplicateEmailError is still thrown only when the users insert truly
conflicts.
---
Nitpick comments:
In
`@services/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.ts`:
- Around line 94-100: Extract the password hashing implementation currently
duplicated in hashPassword (which uses scryptAsync, randomBytes, and the
`scrypt$${salt}$${derivedKey}` format) into a shared auth utility module and
replace the local function with an import; update both in-memory repository
(in-memory-auth-session.repository.ts) and postgres-auth-session.repository.ts
to call the shared function (e.g., export function hashPassword(...) in
auth-utils or auth/crypto) so future verification/cost changes are centralized
and both adapters use the exact same format and implementation.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: e99372f4-9a82-4612-b807-01dba00093ec
📒 Files selected for processing (6)
apps/product-app/app/auth.jsservices/platform-api/src/auth/auth.service.tsservices/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.test.tsservices/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.tsservices/platform-api/src/auth/infrastructure/postgres-auth-session.repository.test.tsservices/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts
✅ Files skipped from review due to trivial changes (1)
- services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- services/platform-api/src/auth/infrastructure/in-memory-auth-session.repository.test.ts
Comment on lines
+71
to
+85
| async signUp( | ||
| input: { name?: string; email?: string; password?: string }, | ||
| context?: { correlationId?: string }, | ||
| ) { | ||
| const correlationId = context?.correlationId ?? 'corr-missing'; | ||
| const name = this.normalizeName(input.name); | ||
| const email = this.normalizeEmail(input.email); | ||
| const password = this.normalizePassword(input.password); | ||
|
|
||
| try { | ||
| const session = await this.sessionStore.registerCustomer({ | ||
| name, | ||
| email, | ||
| password, | ||
| }); |
Contributor
There was a problem hiding this comment.
Do not ship password-based signup until sign-in verifies the stored hash.
This path now stores a real password credential, but Line 46 still ignores password entirely and Line 50 creates sessions from email/role alone. Anyone who knows a registered email can still authenticate without the password, so the new password field does not actually protect the account. Please land password verification against password_hash in the same release, or keep signup disabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform-api/src/auth/auth.service.ts` around lines 71 - 85, The
signup flow (signUp) currently stores a password but the auth flow still creates
sessions from email/role without verifying the password; fix by either disabling
password-based signup or wiring full verification into sign-in: ensure
registerCustomer continues to store a hashed password (use normalizePassword to
produce the raw/password input only), then update signIn to fetch the stored
password_hash and verify with a secure compare (e.g., bcrypt.compare) before
issuing a session, and change any session creation logic that currently
constructs sessions from email/role alone (e.g., sessionStore.createSession or
similar) to only run after successful password verification; alternatively, if
you cannot implement signIn verification in this release, reject password
signups in signUp (throw or return an unsupported error) so passwords are not
stored without enforcement.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
… (#74) Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts (1)
235-239: Consider specifying explicit scrypt cost parameters.The current implementation uses Node's default scrypt parameters (
N=16384, r=8, p=1). While acceptable for MVP, consider making these explicit and tunable for future hardening:💡 Optional: Explicit cost parameters
+const SCRYPT_OPTIONS = { N: 16384, r: 8, p: 1, maxmem: 128 * 16384 * 8 * 2 }; + async function hashPassword(password: string): Promise<string> { const salt = randomBytes(16).toString('hex'); - const derivedKey = (await scryptAsync(password, salt, 64)) as Buffer; + const derivedKey = (await scryptAsync(password, salt, 64, SCRYPT_OPTIONS)) as Buffer; return `scrypt$${salt}$${derivedKey.toString('hex')}`; }This makes the parameters visible for future audits and easier to increase when hardening.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts` around lines 235 - 239, hashPassword currently relies on Node defaults for scrypt cost parameters; update the function to pass explicit scrypt options (e.g., { N: 16384, r: 8, p: 1, maxmem: ... }) when calling scryptAsync and preferably source those values from configurable constants or env/config so they can be tuned later (keep key length 64 as before). Modify the hashPassword function to accept/use a config-backed SCRYPT_OPTIONS constant and pass it as the options argument to scryptAsync, and ensure the salt/derivedKey handling and return format (`scrypt$${salt}$${derivedKey.toString('hex')}`) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts`:
- Around line 241-250: The equality check in verifyPassword is vulnerable to
timing attacks; change the final comparison to use crypto.timingSafeEqual by
converting the hex storedHash to a Buffer and comparing it to the derivedKey
Buffer (ensure lengths match before calling timingSafeEqual). Update function
verifyPassword to import/require crypto if needed and replace the direct string
comparison between derivedKey.toString('hex') and storedHash with a
length-checked, timingSafeEqual Buffer comparison using derivedKey and
Buffer.from(storedHash, 'hex').
---
Nitpick comments:
In
`@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts`:
- Around line 235-239: hashPassword currently relies on Node defaults for scrypt
cost parameters; update the function to pass explicit scrypt options (e.g., { N:
16384, r: 8, p: 1, maxmem: ... }) when calling scryptAsync and preferably source
those values from configurable constants or env/config so they can be tuned
later (keep key length 64 as before). Modify the hashPassword function to
accept/use a config-backed SCRYPT_OPTIONS constant and pass it as the options
argument to scryptAsync, and ensure the salt/derivedKey handling and return
format (`scrypt$${salt}$${derivedKey.toString('hex')}`) remain unchanged.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2548c577-9d16-437b-a3b1-c0060a01272c
📒 Files selected for processing (4)
services/platform-api/src/auth/auth.service.tsservices/platform-api/src/auth/domain/auth-session.repository.tsservices/platform-api/src/auth/infrastructure/postgres-auth-session.repository.test.tsservices/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- services/platform-api/src/auth/auth.service.ts
- services/platform-api/src/auth/domain/auth-session.repository.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts (2)
235-239: Recommend explicit scrypt cost parameters for future hardening.The
scryptcall uses Node.js default parameters (N=16384, r=8, p=1). While functional for MVP, these are on the lower end of OWASP recommendations. Since the PR notes password hashing hardening is deferred, consider adding a TODO or documenting the current parameters:♻️ Suggested improvement with explicit parameters
async function hashPassword(password: string): Promise<string> { const salt = randomBytes(16).toString('hex'); - const derivedKey = (await scryptAsync(password, salt, 64)) as Buffer; + // TODO: Increase N to 32768 or higher during hardening phase + const derivedKey = (await scryptAsync(password, salt, 64, { N: 16384, r: 8, p: 1 })) as Buffer; return `scrypt$${salt}$${derivedKey.toString('hex')}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts` around lines 235 - 239, The scrypt call in hashPassword currently relies on Node's defaults (via scryptAsync) which hides cost parameters; update hashPassword to either explicitly pass scrypt cost options (N, r, p) to scryptAsync or add a clear TODO/comment documenting the current parameters (N=16384, r=8, p=1) and that they must be hardened later; ensure you reference the hashPassword function and the scryptAsync invocation so the change targets the correct place.
37-55: Consider strengthening type safety for the password-based authentication path.The runtime check
'password' in input && input.passwordbypasses TypeScript's type system. IfCreateAuthSessionInputdoesn't include apasswordfield, this works at runtime but loses compile-time guarantees.Consider creating a discriminated union type or a separate method for password-based authentication to make the contract explicit:
type CreateAuthSessionInput = | { email: string; role: string; password?: never } | { email: string; password: string };The generic error message "Invalid email or password" is correctly used for both cases (missing user and wrong password), which is good for preventing user enumeration attacks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts` around lines 37 - 55, Change the runtime-only password check to a type-safe contract: define a discriminated union for CreateAuthSessionInput (e.g., { email: string; role?: never } | { email: string; password: string }) or extract a dedicated method for password auth and use an explicit PasswordAuthInput type, then narrow the input before calling verifyPassword and querying users in PostgresAuthSessionRepository (the method containing the current `'password' in input && input.password` logic). Update the method signature or add an overload so TypeScript enforces the presence of password for the password-auth path and eliminate the runtime-only type check while keeping the existing generic error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts`:
- Around line 235-239: The scrypt call in hashPassword currently relies on
Node's defaults (via scryptAsync) which hides cost parameters; update
hashPassword to either explicitly pass scrypt cost options (N, r, p) to
scryptAsync or add a clear TODO/comment documenting the current parameters
(N=16384, r=8, p=1) and that they must be hardened later; ensure you reference
the hashPassword function and the scryptAsync invocation so the change targets
the correct place.
- Around line 37-55: Change the runtime-only password check to a type-safe
contract: define a discriminated union for CreateAuthSessionInput (e.g., {
email: string; role?: never } | { email: string; password: string }) or extract
a dedicated method for password auth and use an explicit PasswordAuthInput type,
then narrow the input before calling verifyPassword and querying users in
PostgresAuthSessionRepository (the method containing the current `'password' in
input && input.password` logic). Update the method signature or add an overload
so TypeScript enforces the presence of password for the password-auth path and
eliminate the runtime-only type check while keeping the existing generic error
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46f822f5-201d-4d5d-af77-c25a09c4483b
📒 Files selected for processing (1)
services/platform-api/src/auth/infrastructure/postgres-auth-session.repository.ts
Co-Authored-By: Paperclip <noreply@paperclip.ing>
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
1 participant
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.
Thinking Path
QuickWerk had no
POST /sign-upendpoint and the product-app hard-rejected account creation. Closes the top-of-funnel growth blocker identified in Phase 11 planning (TEC-59 plan).The signup seam follows the existing session-based auth pattern already established for sign-in, extending it minimally with registration validation and a new DB migration for customer profile fields.
What Changed
POST /api/v1/auth/sign-up: validates name/email/password, rejects duplicates, returns session on success; structured logging consistent with existing auth actionsregisterCustomercontract added to domain interface + implemented in in-memory and Postgres adaptersfull_name+password_hashcolumns onusersSignUpRequestBodytype +createSignUpRequesthelperauth.jsremoves hard-coded rejection; auth entry screen adds full-name input, loading/error states, authenticated handoff on successVerification
Platform-api auth tests cover: successful registration, duplicate-email 409, missing-field 400.
Product-app auth-entry-actions tests cover: signup dispatch, error state, loading state.
Risks
Model Used
claude-sonnet-4-6 (CEO orchestration + branch isolation) / gpt-5.3-codex (CodexCoder implementation)
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores