Skip to content

fix(auth): equalise signin response timing across rejection paths #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JOY (JOY) merged 1 commit into from
Sep 22, 2026
Merged

fix(auth): equalise signin response timing across rejection paths #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/auth/server/routes/email-password.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
isEmailDomainAllowedForSignup,
isSigninEnabledForProvider,
isSignupEnabledForProvider,
TIMING_SAFE_DUMMY_PASSWORD_HASH,
} from '@documenso/lib/constants/auth';
import { EMAIL_VERIFICATION_STATE } from '@documenso/lib/constants/email';
import { AppError } from '@documenso/lib/errors/app-error';
Expand Down Expand Up @@ -102,6 +103,10 @@ export const emailPasswordRoute = new Hono<HonoAuthContext>()
// same INVALID_CREDENTIALS error as a wrong password, so probing the
// endpoint cannot reveal which emails are on the allowlist.
if (!isSigninEnabledForProvider('email') && !isBreakGlassEmail(email)) {
// Equalise timing with the real compare below so measuring response
// time cannot reveal allowlist membership either.
await compare(password, TIMING_SAFE_DUMMY_PASSWORD_HASH);

throw new AppError(AuthenticationErrorCode.InvalidCredentials, {
message: 'Invalid email or password',
});
Expand All @@ -118,6 +123,10 @@ export const emailPasswordRoute = new Hono<HonoAuthContext>()
});

if (!user || !user.password) {
// Equalise timing with the real compare below: unknown users and
// passwordless accounts must not be distinguishable by response time.
await compare(password, TIMING_SAFE_DUMMY_PASSWORD_HASH);

throw new AppError(AuthenticationErrorCode.InvalidCredentials, {
message: 'Invalid email or password',
});
Comment on lines 125 to 132

@gemini-code-assist gemini-code-assist Bot Sep 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

While this PR successfully equalises the CPU-heavy bcrypt comparison cost, two timing discrepancies remain due to database I/O operations:\n\n1. Audit Log Write Latency: When an existing user with a password enters an incorrect password, the application performs and awaits a database write (prisma.userSecurityAuditLog.create on line 138), adding database insertion latency (typically 5–50ms) to the response time. However, when a user does not exist, or is a passwordless/SSO-only user (entering this !user || !user.password block), no audit log is written. This allows an attacker to distinguish between existing accounts with passwords and non-existent or passwordless accounts.\n\n2. Database Query Latency: In the disabled-signin path (lines 105-113), the application performs a dummy comparison and throws immediately without querying the database. In contrast, the normal sign-in path queries the database (prisma.user.findFirst on line 119). This difference in database query latency (~5-50ms) can allow an attacker to distinguish whether an email is on the break-glass allowlist.\n\n### Suggested Mitigation\n- Log failed attempts for passwordless users: If user exists but has no password, write a SIGN_IN_FAIL audit log for them before throwing.\n- Make audit log writes non-blocking: Consider not awaiting the prisma.userSecurityAuditLog.create promise directly in the main request-response cycle, or use a background task runner/ctx.executionCtx.waitUntil if running in a serverless environment.\n- Simulate DB latency or query anyway: For the disabled-signin path, consider performing a dummy database query or always querying the database to ensure database query latency is uniform across all paths.\n\nAdditionally, note that the legacy/deleted service account check at lines 115-117 returns a FORBIDDEN 403 response immediately without performing any bcrypt comparison. This creates a massive timing difference (a few milliseconds vs ~250ms) and a different status code, making these service accounts easily identifiable.

Expand Down
19 changes: 18 additions & 1 deletion packages/lib/constants/auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { afterEach, describe, expect, it, vi } from 'vitest';

import { getBreakGlassEmails, isBreakGlassEmail, isBreakGlassSigninEnabled } from './auth';
import {
getBreakGlassEmails,
isBreakGlassEmail,
isBreakGlassSigninEnabled,
SALT_ROUNDS,
TIMING_SAFE_DUMMY_PASSWORD_HASH,
} from './auth';

describe('break-glass password signin allowlist', () => {
afterEach(() => {
Expand Down Expand Up @@ -33,3 +39,14 @@ describe('break-glass password signin allowlist', () => {
expect(isBreakGlassEmail('')).toBe(false);
});
});

describe('timing-safe dummy password hash', () => {
it('is a bcrypt hash at the same cost factor as real password hashing', () => {
// bcrypt format: $<variant>$<cost>$<22-char salt><31-char hash>
const parts = TIMING_SAFE_DUMMY_PASSWORD_HASH.split('$');

expect(parts[1]).toMatch(/^2[aby]$/);
expect(Number(parts[2])).toBe(SALT_ROUNDS);
expect(TIMING_SAFE_DUMMY_PASSWORD_HASH).toHaveLength(60);
});
});
10 changes: 10 additions & 0 deletions packages/lib/constants/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@ import { NEXT_PUBLIC_WEBAPP_URL } from './app';

export const SALT_ROUNDS = 12;

/**
* Precomputed bcrypt hash (cost = SALT_ROUNDS) of an unguessable string.
* Compared against the submitted password on early-rejected signin attempts
* (signin disabled suite-wide, unknown user, user without a password) so
* every failing path pays the same bcrypt cost and the endpoint cannot be
* probed by measuring verification time. Never validates anything: any
* compare against it returns false.
*/
export const TIMING_SAFE_DUMMY_PASSWORD_HASH = '$2y$12$eAJ6CR54acEljh1J/AN.peIPUd19yidDQRKhynGQriewBgzF1bQdm';

export const IDENTITY_PROVIDER_NAME: Record<string, string> = {
DOCUMENSO: 'Documenso',
GOOGLE: 'Google',
Expand Down
Loading