mirror of
https://github.com/bretzel-app/crumbs.git
synced 2026-10-12 12:06:57 +02:00
feat(wu-001): add password generator and reset-eligibility utilities
Groundwork for issue #77 (admin cannot reset other users' passwords). Two pure, colocated-tested modules shared by the API route and the settings UI that follow. password.ts — generatePassword() over a charset pinned at exactly 64 characters. 64 is a power of two, so nanoid's customAlphabet takes its exactly-uniform masking path (256 % 64 === 0) rather than the rejection branch; 120 bits of entropy at the default length of 20. Visually ambiguous characters (0 O o 1 l I) are excluded so a password survives being read aloud or retyped. Class coverage is guaranteed by discarding and regenerating a whole password that is missing a group, which stays uniform over exactly the set of valid passwords — rather than forcing characters into positions and shuffling, which would need its own unbiased index source. auth-methods.ts — canResetPassword() and passwordResetBlockedReason(), the single source of truth for eligibility so the endpoint and the component cannot drift. Password login is gated on authProvider (verifyPassword matches only rows where it equals 'password'), so writing a hash for any other provider stores a credential that can never be used to sign in. The reason lookup is a Map, not an object literal: an object inherits Object.prototype, so a lookup for 'toString' or '__proto__' resolves to an inherited member and the nullish fallback never fires — returning a function where the signature promises string | null. A Map has no such keys, so totality holds by construction instead of by a guard a later refactor could drop. DoD items verified: - [x] 1. generatePassword() returns 20 chars; explicit length honoured - [x] 2. always >=1 lowercase, uppercase, digit and symbol - [x] 3. never contains 0 O o 1 l I - [x] 4. randomness from nanoid customAlphabet; no Math.random - [x] 5. throws for length < 8 (the server minimum) - [x] 6. retry bounded at 100 with an unconditional throw (inspection) - [x] 7. canResetPassword true only for 'password' - [x] 8. totality: every blocked input yields a non-empty string - [x] 9. exact reason copy per provider; null when allowed - [x] 10. colocated tests pass Verified independently by the orchestrator: pnpm check 0 errors / 14 pre-existing warnings; pnpm test:unit 306 passed / 26 files (from 288 / 24); coverage 45.52 lines, 45.52 statements, 54.01 functions, 84.87 branches — every metric above both its .coverage-thresholds.json floor and the previous baseline, new files at 100%. Reviewed-by: adversarial-review (PASS, second reviewer; first found a prototype-chain totality violation, since fixed and re-reviewed fresh) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbqJcFofZSbhbGwUwZP3wU
This commit is contained in:
Frederic Cabassut
committed by
Fred Cabassut
Fred Cabassut
co-authored by
Claude Opus 5
Claude Opus 5
parent
0d21f7efda
commit
51dd844f96
@@ -0,0 +1,91 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { canResetPassword, passwordResetBlockedReason } from './auth-methods.js';
|
||||
|
||||
const NON_PASSWORD_PROVIDERS = ['none', 'google', 'github', 'oidc'];
|
||||
|
||||
describe('canResetPassword', () => {
|
||||
it('allows a reset for a password account', () => {
|
||||
expect(canResetPassword({ authProvider: 'password' })).toBe(true);
|
||||
});
|
||||
|
||||
it('blocks a reset for every non-password provider', () => {
|
||||
// verifyPassword() filters on authProvider === 'password', so a hash
|
||||
// written for any other provider could never be used to sign in.
|
||||
for (const authProvider of NON_PASSWORD_PROVIDERS) {
|
||||
expect(canResetPassword({ authProvider }), authProvider).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it('blocks a reset for an unenumerated provider', () => {
|
||||
expect(canResetPassword({ authProvider: 'saml' })).toBe(false);
|
||||
expect(canResetPassword({ authProvider: '' })).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('passwordResetBlockedReason', () => {
|
||||
it('returns null for a password account', () => {
|
||||
expect(passwordResetBlockedReason({ authProvider: 'password' })).toBeNull();
|
||||
});
|
||||
|
||||
it('names the provider in the reason', () => {
|
||||
expect(passwordResetBlockedReason({ authProvider: 'none' })).toBe(
|
||||
'This account has no password — it signs in via OAuth.'
|
||||
);
|
||||
expect(passwordResetBlockedReason({ authProvider: 'google' })).toBe(
|
||||
'Managed by Google — password login is disabled for this account.'
|
||||
);
|
||||
expect(passwordResetBlockedReason({ authProvider: 'github' })).toBe(
|
||||
'Managed by GitHub — password login is disabled for this account.'
|
||||
);
|
||||
expect(passwordResetBlockedReason({ authProvider: 'oidc' })).toBe(
|
||||
'Managed by SSO — password login is disabled for this account.'
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to a generic reason for an unenumerated provider', () => {
|
||||
expect(passwordResetBlockedReason({ authProvider: 'saml' })).toBe(
|
||||
'Password login is not available for this account.'
|
||||
);
|
||||
});
|
||||
|
||||
it('always explains a blocked reset', () => {
|
||||
// authProvider is typed `string`, so no missing branch can be caught by
|
||||
// the compiler. A null reason here would render a disabled control with
|
||||
// no explanation — the confusion issue #77 reported.
|
||||
//
|
||||
// The Object.prototype keys are the interesting cases: with a plain object
|
||||
// literal as the lookup table they resolve to inherited members, so a
|
||||
// nullish fallback never fires and the function returns a function or an
|
||||
// object in place of its declared string. Asserting typeof rather than
|
||||
// truthiness is what catches that.
|
||||
const providers = [
|
||||
...NON_PASSWORD_PROVIDERS,
|
||||
'saml',
|
||||
'',
|
||||
'PASSWORD',
|
||||
'ldap',
|
||||
'unknown',
|
||||
'__proto__',
|
||||
'toString',
|
||||
'constructor',
|
||||
'hasOwnProperty',
|
||||
'valueOf'
|
||||
];
|
||||
for (const authProvider of providers) {
|
||||
const user = { authProvider };
|
||||
expect(canResetPassword(user), authProvider).toBe(false);
|
||||
const reason = passwordResetBlockedReason(user);
|
||||
expect(typeof reason, authProvider).toBe('string');
|
||||
expect(reason, authProvider).not.toBe('');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('auth-methods module', () => {
|
||||
it('does not use Math.random', () => {
|
||||
// The reset-eligibility rule must be pure and deterministic.
|
||||
const source = readFileSync(new URL('./auth-methods.ts', import.meta.url), 'utf8');
|
||||
expect(source).not.toContain('Math.random');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,44 @@
|
||||
import type { User } from '$lib/types/index.js';
|
||||
|
||||
/**
|
||||
* Human-readable reason a password reset is unavailable, keyed by auth provider.
|
||||
* Anything absent from this map falls back to BLOCKED_REASON_FALLBACK, so the
|
||||
* blocked path can never be left unexplained.
|
||||
*
|
||||
* A Map, not an object literal: an object inherits Object.prototype, so a lookup
|
||||
* for 'toString' or '__proto__' would resolve to an inherited member instead of
|
||||
* missing, and the fallback below would never fire. A Map has no such keys, so
|
||||
* totality holds by construction rather than by a guard that could be dropped.
|
||||
*/
|
||||
const BLOCKED_REASONS = new Map<string, string>([
|
||||
['none', 'This account has no password — it signs in via OAuth.'],
|
||||
['google', 'Managed by Google — password login is disabled for this account.'],
|
||||
['github', 'Managed by GitHub — password login is disabled for this account.'],
|
||||
['oidc', 'Managed by SSO — password login is disabled for this account.']
|
||||
]);
|
||||
|
||||
const BLOCKED_REASON_FALLBACK = 'Password login is not available for this account.';
|
||||
|
||||
/**
|
||||
* Whether an admin may set a new password for this account.
|
||||
*
|
||||
* Password login is gated on `authProvider` — verifyPassword() only matches rows
|
||||
* where it equals 'password' — so writing a hash for any other provider stores a
|
||||
* credential that can never be used to sign in.
|
||||
*/
|
||||
export function canResetPassword(user: Pick<User, 'authProvider'>): boolean {
|
||||
return user.authProvider === 'password';
|
||||
}
|
||||
|
||||
/**
|
||||
* Why a password reset is blocked, or null when it is allowed.
|
||||
*
|
||||
* `authProvider` is typed `string`, so the compiler cannot enforce exhaustiveness
|
||||
* here. The fallback guarantees the invariant this module exists for: whenever
|
||||
* `canResetPassword` is false, this returns a non-empty explanation, so the UI
|
||||
* never shows a disabled control without saying why.
|
||||
*/
|
||||
export function passwordResetBlockedReason(user: Pick<User, 'authProvider'>): string | null {
|
||||
if (canResetPassword(user)) return null;
|
||||
return BLOCKED_REASONS.get(user.authProvider) ?? BLOCKED_REASON_FALLBACK;
|
||||
}
|
||||
@@ -0,0 +1,84 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { generatePassword, PASSWORD_ALPHABET } from './password.js';
|
||||
|
||||
const LOWERCASE = /[abcdefghijkmnpqrstuvwxyz]/;
|
||||
const UPPERCASE = /[ABCDEFGHJKLMNPQRSTUVWXYZ]/;
|
||||
const DIGIT = /[23456789]/;
|
||||
const SYMBOL = /[!@#$%^&*]/;
|
||||
|
||||
/** Visually ambiguous characters the charset deliberately excludes. */
|
||||
const AMBIGUOUS = ['0', 'O', 'o', '1', 'l', 'I'];
|
||||
|
||||
describe('PASSWORD_ALPHABET', () => {
|
||||
it('is exactly 64 distinct characters', () => {
|
||||
// 256 % 64 === 0, which is what keeps nanoid's masking exactly uniform.
|
||||
// A charset edit that breaks this silently biases every password.
|
||||
expect(PASSWORD_ALPHABET).toHaveLength(64);
|
||||
expect(new Set(PASSWORD_ALPHABET).size).toBe(64);
|
||||
});
|
||||
|
||||
it('excludes every visually ambiguous character', () => {
|
||||
for (const char of AMBIGUOUS) {
|
||||
expect(PASSWORD_ALPHABET.includes(char), char).toBe(false);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('generatePassword', () => {
|
||||
it('returns a 20-character password by default', () => {
|
||||
expect(generatePassword()).toHaveLength(20);
|
||||
});
|
||||
|
||||
it('honours an explicit length', () => {
|
||||
expect(generatePassword(8)).toHaveLength(8);
|
||||
expect(generatePassword(32)).toHaveLength(32);
|
||||
expect(generatePassword(64)).toHaveLength(64);
|
||||
});
|
||||
|
||||
it('always includes at least one character from each of the four groups', () => {
|
||||
// Not a statistical bet: the function only ever returns a class-complete
|
||||
// password or throws, so every sample must pass.
|
||||
for (let i = 0; i < 20; i++) {
|
||||
const password = generatePassword();
|
||||
expect(password, `sample ${i}`).toMatch(LOWERCASE);
|
||||
expect(password, `sample ${i}`).toMatch(UPPERCASE);
|
||||
expect(password, `sample ${i}`).toMatch(DIGIT);
|
||||
expect(password, `sample ${i}`).toMatch(SYMBOL);
|
||||
}
|
||||
});
|
||||
|
||||
it('never emits a visually ambiguous character', () => {
|
||||
// 0 O o 1 l I survive neither being read aloud nor retyped.
|
||||
for (let i = 0; i < 20; i++) {
|
||||
const password = generatePassword();
|
||||
for (const char of AMBIGUOUS) {
|
||||
expect(password.includes(char), `sample ${i} contains ${char}`).toBe(false);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
it('draws only from the pinned 64-character charset', () => {
|
||||
const charset = /^[abcdefghijkmnpqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789!@#$%^&*]+$/;
|
||||
for (let i = 0; i < 20; i++) {
|
||||
expect(generatePassword(), `sample ${i}`).toMatch(charset);
|
||||
}
|
||||
});
|
||||
|
||||
it('produces a different password on each call', () => {
|
||||
const passwords = new Set(Array.from({ length: 20 }, () => generatePassword()));
|
||||
expect(passwords.size).toBe(20);
|
||||
});
|
||||
|
||||
it('throws below the 8-character server minimum', () => {
|
||||
expect(() => generatePassword(7)).toThrow(/8/);
|
||||
expect(() => generatePassword(0)).toThrow(/8/);
|
||||
expect(() => generatePassword(-1)).toThrow(/8/);
|
||||
});
|
||||
|
||||
it('does not use Math.random anywhere in the module', () => {
|
||||
// Randomness must come from nanoid's CSPRNG, not Math.random.
|
||||
const source = readFileSync(new URL('./password.ts', import.meta.url), 'utf8');
|
||||
expect(source).not.toContain('Math.random');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,59 @@
|
||||
import { customAlphabet } from 'nanoid';
|
||||
|
||||
// The excluded characters — 0 O o 1 l I — are visually ambiguous, so a generated
|
||||
// password survives being read aloud or retyped from a screenshot.
|
||||
const LOWERCASE = 'abcdefghijkmnpqrstuvwxyz'; // no l, no o
|
||||
const UPPERCASE = 'ABCDEFGHJKLMNPQRSTUVWXYZ'; // no I, no O
|
||||
const DIGITS = '23456789'; // no 0, no 1
|
||||
const SYMBOLS = '!@#$%^&*';
|
||||
|
||||
/**
|
||||
* Pinned at exactly 64 characters. 64 is a power of two, so nanoid's masking
|
||||
* takes its exactly-uniform fast path (256 % 64 === 0) and no character is
|
||||
* favoured over another. At 64 symbols a 20-character password carries 120 bits
|
||||
* of entropy — 119.79 once conditioned on the class-completeness retry below.
|
||||
* Do not change the size without understanding both consequences.
|
||||
*/
|
||||
export const PASSWORD_ALPHABET = LOWERCASE + UPPERCASE + DIGITS + SYMBOLS;
|
||||
|
||||
const GROUPS = [LOWERCASE, UPPERCASE, DIGITS, SYMBOLS];
|
||||
|
||||
/** The server rejects anything shorter. */
|
||||
const MIN_LENGTH = 8;
|
||||
|
||||
/**
|
||||
* Per-attempt failure is 13.5% at the default length of 20 and 61.9% at the
|
||||
* minimum of 8, so exhausting the cap has probability ~1e-87 and ~1e-21
|
||||
* respectively — unreachable in practice. The bound exists so a future charset
|
||||
* edit cannot turn the retry into an infinite loop.
|
||||
*/
|
||||
const MAX_ATTEMPTS = 100;
|
||||
|
||||
const randomString = customAlphabet(PASSWORD_ALPHABET);
|
||||
|
||||
function containsAnyOf(password: string, group: string): boolean {
|
||||
return [...password].some((char) => group.includes(char));
|
||||
}
|
||||
|
||||
/**
|
||||
* Generates a random password containing at least one lowercase letter, one
|
||||
* uppercase letter, one digit and one symbol.
|
||||
*
|
||||
* A password missing a group is discarded whole and regenerated, which keeps the
|
||||
* result uniform over exactly the set of valid passwords. Forcing characters
|
||||
* into chosen positions and shuffling would need its own unbiased index source.
|
||||
*/
|
||||
export function generatePassword(length = 20): string {
|
||||
if (length < MIN_LENGTH) {
|
||||
throw new Error(`Password length must be at least ${MIN_LENGTH}, got ${length}`);
|
||||
}
|
||||
|
||||
for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
|
||||
const password = randomString(length);
|
||||
if (GROUPS.every((group) => containsAnyOf(password, group))) return password;
|
||||
}
|
||||
|
||||
throw new Error(
|
||||
`Failed to generate a password with all character groups after ${MAX_ATTEMPTS} attempts`
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user