Repository navigation
feat: move auth to Coolify Postgres - #88
Conversation
Strix Security ReviewAll previously reported security findings have been resolved. 4 resolved findings
Review summaryRe-reviewed the full diff with focus on the app-owned email/password auth introduced by this PR. The previously open rate-limit finding is now resolved: Updated for Reviewed by Strix |
| import { randomBytes, scrypt as scryptCallback, timingSafeEqual, type ScryptOptions } from 'node:crypto'; | ||
|
|
||
| const KEY_LENGTH = 64; | ||
| const SCRYPT_PARAMS = { N: 16384, r: 8, p: 1 } as const; |
There was a problem hiding this comment.
Weak scrypt password-hashing parameters (N=16384) below OWASP recommendation
Resolved — no longer flagged as of bca647b.
| export async function POST(request: Request) { | ||
| const body = await readJson(request); | ||
| const result = await signInWithPassword(stringField(body, 'email'), stringField(body, 'password')); | ||
|
|
||
| if (!result.ok) { | ||
| return NextResponse.json({ ok: false, error: result.error }, { status: result.status ?? 401 }); | ||
| } | ||
|
|
||
| return okWithSession({ user: result.user }, result.token, result.expiresAt); | ||
| } |
There was a problem hiding this comment.
Sign-in/sign-up throttling bypassable via spoofed X-Forwarded-For and per-process state
Resolved — no longer flagged as of 29948b0.
|
@strix-security please rerun the security review. Addressed both findings: raised scrypt to N=131072 with increased maxmem and added fixed-window throttling for sign-in/sign-up attempts. |
| if (!user || !(await verifyPassword(password, user.passwordHash))) { | ||
| return { ok: false, error: 'Invalid email or password', status: 401 }; | ||
| } |
There was a problem hiding this comment.
Timing-based account enumeration on password sign-in endpoint
Resolved — no longer flagged as of 29948b0.
|
@strix-security please rerun again. Follow-up fixes: moved auth throttling into Postgres-backed counters keyed without X-Forwarded-For, added auth_rate_limits migration/table, and added dummy scrypt verification for missing users to reduce sign-in timing enumeration. |
| @@ -0,0 +1,75 @@ | |||
| import 'server-only'; | |||
|
|
|||
| import { eq } from 'drizzle-orm'; | |||
There was a problem hiding this comment.
Non-atomic rate-limit counter allows concurrent requests to bypass the sign-in brute-force throttle
Resolved — no longer flagged as of bdaf2c1.
| export async function recordAuthRateLimitAttempt({ | ||
| key, | ||
| kind, | ||
| windowMs, | ||
| }: Omit<AuthRateLimitOptions, 'maxAttempts'>) { | ||
| const [entry] = await db.select().from(authRateLimits).where(eq(authRateLimits.key, key)).limit(1); | ||
| const windowStart = new Date(); | ||
|
|
||
| if (!entry || nowMs() - entry.windowStart.getTime() >= windowMs) { | ||
| await db | ||
| .insert(authRateLimits) | ||
| .values({ key, kind, count: 1, windowStart, updatedAt: windowStart }) | ||
| .onConflictDoUpdate({ | ||
| target: authRateLimits.key, | ||
| set: { kind, count: 1, windowStart, updatedAt: windowStart }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| await db | ||
| .update(authRateLimits) | ||
| .set({ count: entry.count + 1, updatedAt: windowStart }) | ||
| .where(eq(authRateLimits.key, key)); | ||
| } |
There was a problem hiding this comment.
Non-atomic rate-limit counter allows concurrent requests to bypass the sign-in brute-force throttle
Resolved — no longer flagged as of bdaf2c1.
|
@strix-security please rerun once more. Fixed the remaining atomicity finding by changing rate-limit recording to a single Postgres upsert that increments count in the database. |
Summary
auth_usersandauth_sessionstables plus first-party auth API routes..env.examplefor local/Coolify Postgres setup.Coolify
clawproxy-postgresin the existingclawproxyproduction project.clawproxy.ioappDATABASE_URLenv var to the new internal Coolify DB URL.running:healthyand not public.Test Plan
yarn lintyarn test:runDATABASE_URL='postgresql://user:pass@localhost:5432/clawproxy' ENCRYPTION_KEY='0000000000000000000000000000000000000000000000000000000000000000' yarn buildNotes