diff --git a/backend/migrations/1788200000000_add-social-identities.js b/backend/migrations/1788200000000_add-social-identities.js new file mode 100644 index 0000000..6bb3571 --- /dev/null +++ b/backend/migrations/1788200000000_add-social-identities.js @@ -0,0 +1,77 @@ +exports.up = (pgm) => { + pgm.sql(` + -- A sign-in that belongs to somebody else's identity provider (#340). + -- + -- A table rather than columns on customers, because one customer may hold + -- more than one: Google today, and Apple if #332 ever decides in its + -- favour. Columns would mean a second provider is a migration and a third + -- is an embarrassment. + -- + -- Same shape as customer_credentials, and for the same reason: a row that + -- links this account to something a third party can vouch for, deleted with + -- the customer because an identity that outlived its owner could + -- authenticate as a customer who no longer exists. + CREATE TABLE IF NOT EXISTS customer_identities ( + id SERIAL PRIMARY KEY, + customer_id INTEGER NOT NULL REFERENCES customers(id) ON DELETE CASCADE, + + -- 'google' today. Not a CHECK constraint: the values come from this + -- codebase rather than from a request, and the project has no enum types + -- elsewhere. + provider TEXT NOT NULL, + + -- The provider's subject claim, and **never the email**. + -- + -- This is the whole security posture of the table in one column. An email + -- is a display value that its owner can change and that a provider may + -- reassign; a subject is opaque, stable for the life of the account, and + -- means nothing outside the provider that issued it. Matching on the + -- email would strand a customer who changed theirs and, far worse, hand + -- their account to whoever inherited the old address. + provider_sub TEXT NOT NULL, + + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + + -- Null until first used, exactly as on a passkey. It is what tells two + -- entries apart on an account page where the names are similar. + last_used_at TIMESTAMPTZ, + + -- Unique across the pair, not on the subject alone. Two providers could + -- in principle issue the same opaque string and it would mean nothing — + -- but the same provider issuing one subject to two accounts here means + -- something has gone wrong rather than that two people share an identity. + UNIQUE (provider, provider_sub) + ); + + -- Listing what an account is linked to is the common read, and it is always + -- scoped by owner. + CREATE INDEX IF NOT EXISTS customer_identities_customer_id_idx + ON customer_identities (customer_id); + + -- The change with the widest blast radius in the whole project (#332). + -- + -- A customer who signed up through Google has no password and never will + -- unless they ask for one, so the column has to admit that. Making it + -- nullable is one line; what it costs is that every read of it is now a + -- question rather than a fact, and the three that compare against it with + -- bcrypt have been made to ask first. + -- + -- Nothing writes a NULL yet. The first accounts without a password arrive + -- with the sign-up path, and this is deliberately landed before them so the + -- schema change can be reviewed on its own. + ALTER TABLE customers ALTER COLUMN password_hash DROP NOT NULL; + `); +}; + +exports.down = (pgm) => { + pgm.sql(` + DROP TABLE IF EXISTS customer_identities; + + -- Deliberately not restored. Re-adding NOT NULL fails outright if any + -- passwordless customer exists by then, and a down migration that destroys + -- accounts to satisfy a constraint would be far worse than a column that is + -- merely more permissive than it needs to be. Reversing this properly means + -- deciding what happens to those customers, which is not a schema decision. + SELECT 1; + `); +}; diff --git a/backend/src/db-kysely/schema.ts b/backend/src/db-kysely/schema.ts index 77d9eed..8ee5dca 100644 --- a/backend/src/db-kysely/schema.ts +++ b/backend/src/db-kysely/schema.ts @@ -102,7 +102,7 @@ export interface Customers { marketing_consent: Generated; marketing_consent_at: Timestamp | null; marketing_consent_text: string | null; - password_hash: string; + password_hash: string | null; unsubscribe_token: string; } @@ -115,6 +115,15 @@ export interface CustomerEmailChanges { reason: string; } +export interface CustomerIdentities { + created_at: Generated; + customer_id: number; + id: Generated; + last_used_at: Timestamp | null; + provider: string; + provider_sub: string; +} + export interface CustomerSessions { created_at: Generated; customer_id: number; @@ -251,6 +260,7 @@ export interface DB { checkouts: Checkouts; customer_credentials: CustomerCredentials; customer_email_changes: CustomerEmailChanges; + customer_identities: CustomerIdentities; customer_sessions: CustomerSessions; customer_tokens: CustomerTokens; customers: Customers; diff --git a/backend/src/envValidation.ts b/backend/src/envValidation.ts index 345321e..cf08aa9 100644 --- a/backend/src/envValidation.ts +++ b/backend/src/envValidation.ts @@ -152,6 +152,57 @@ function checkMail(env: NodeJS.ProcessEnv): EnvValidation { return { errors, warnings }; } +/** + * Google sign-in is all or nothing (#340). + * + * Half-configured is the case worth naming. With neither value set the feature + * reports itself disabled and the button never appears, which is a legitimate + * environment. With one set, the token exchange fails at the moment a customer + * presses the button — the worst possible time to discover a typo in a stack + * variable. + * + * An error rather than a warning, matching the SMTP pair above: both refuse to + * start rather than serving something that is visibly offered and cannot work. + */ +function checkGoogleSignIn(env: NodeJS.ProcessEnv): EnvValidation { + const hasId = isPresent(env, 'GOOGLE_CLIENT_ID'); + const hasSecret = isPresent(env, 'GOOGLE_CLIENT_SECRET'); + + if (hasId && !hasSecret) { + return { + errors: ['GOOGLE_CLIENT_SECRET is required when GOOGLE_CLIENT_ID is set — set both or neither.'], + warnings: [] + }; + } + if (hasSecret && !hasId) { + return { + errors: ['GOOGLE_CLIENT_ID is required when GOOGLE_CLIENT_SECRET is set — set both or neither.'], + warnings: [] + }; + } + if (!hasId) { + return { + errors: [], + warnings: ['Google sign-in is not configured — the button will not be offered.'] + }; + } + + // Only meaningful once the credentials exist, and only a warning: a + // deployment with no PUBLIC_URL falls back to localhost, which is right for + // local development and wrong everywhere else in a way worth saying out loud. + if (!isPresent(env, 'PUBLIC_URL')) { + return { + errors: [], + warnings: [ + 'Google sign-in is configured but PUBLIC_URL is not, so the redirect URI falls back to ' + + 'localhost. Correct locally; anywhere else, Google will refuse the callback.' + ] + }; + } + + return { errors: [], warnings: [] }; +} + function checkAdminGate(env: NodeJS.ProcessEnv): string[] { if (isPresent(env, 'ADMIN_GATE_SECRET')) { return []; @@ -227,6 +278,7 @@ function checkUploadsOrigin(env: NodeJS.ProcessEnv): EnvValidation { export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation { const mail = checkMail(env); const uploads = checkUploadsOrigin(env); + const google = checkGoogleSignIn(env); return { errors: [ @@ -234,8 +286,16 @@ export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation { ...checkDemoMode(env), ...checkPayPal(env), ...mail.errors, - ...uploads.errors + ...uploads.errors, + ...google.errors ], - warnings: [...mail.warnings, ...checkAdminGate(env), ...uploads.warnings, ...checkDraftingKey(env), ...checkIntakeActionSecret(env)] + warnings: [ + ...mail.warnings, + ...checkAdminGate(env), + ...uploads.warnings, + ...checkDraftingKey(env), + ...checkIntakeActionSecret(env), + ...google.warnings + ] }; } diff --git a/backend/src/google/config.ts b/backend/src/google/config.ts new file mode 100644 index 0000000..810985b --- /dev/null +++ b/backend/src/google/config.ts @@ -0,0 +1,113 @@ +/** + * Who this application is, as far as Google is concerned (#340). + * + * ## Why the redirect URI is derived rather than written down + * + * It is registered in two places that must agree exactly: Google's console, and + * every authorization request this server sends. Google compares them as + * strings — scheme, host, port, path, trailing slash and case all count — and + * answers a mismatch with `redirect_uri_mismatch`, which is accurate and tells + * you nothing about which half is wrong. + * + * So it comes from `PUBLIC_URL`, the same value every customer-facing link is + * already built from, exactly as the WebAuthn Relying Party ID does (#37). One + * source, and it is the one that is already correct in any environment where + * mail works. + * + * ## The consequence worth stating plainly + * + * Google refuses a redirect URI whose host is not under an **authorized + * domain**, and a domain can only be authorized after ownership has been proved + * by DNS in Search Console. `localhost` is the sole exemption. + * + * `qa-redefined-designs.bermudalamb.synology.me` therefore **cannot ever be + * used**: Synology owns the registrable domain above it, so there is no record + * to add and nothing to prove. This is the same wall #285 hit with Cloudflare. + * + * | Environment | Redirect URI | Works | + * | --- | --- | --- | + * | Local | `http://localhost:3000/...` | Yes, by exemption | + * | QA on the Synology host | — | **No, and cannot** | + * | QA on `qa.redefined-designs.com` | `https://qa.redefined-designs.com/...` | After #313 | + * | Production | `https://redefined-designs.com/...` | After #313 | + * + * So this feature is built and exercised locally, and QA cannot see it until QA + * moves onto a subdomain of the real domain. That is a `PUBLIC_URL` change and + * one console entry, not a code change — this module follows `PUBLIC_URL` + * wherever it points. See #345. + */ + +/** The callback path. One constant, because it appears in two sentences. */ +export const GOOGLE_CALLBACK_PATH = '/api/auth/google/callback'; + +/** + * Local development, where `PUBLIC_URL` is legitimately unset. + * + * `envValidation` requires `PUBLIC_URL` only when SMTP is configured, so a local + * setup that cannot send mail does not have it. Falling back to the backend's + * own port rather than refusing keeps that setup working, and `localhost` is + * the one host Google will accept without an authorized domain — so the fallback + * is also the only value that could possibly work here. + */ +const LOCAL_ORIGIN = 'http://localhost:3000'; + +export interface GoogleConfig { + clientId: string; + clientSecret: string; + /** Absolute, and byte-identical to what is registered in Google's console. */ + redirectUri: string; + /** + * Whether to offer Google sign-in at all. + * + * False when either credential is missing, and the button is then **absent + * rather than disabled** — the same choice #41 made for a browser without + * WebAuthn. A developer without credentials gets a storefront that works and + * simply does not offer the option, rather than one that offers it and fails. + */ + enabled: boolean; +} + +/** + * The Google configuration for this environment. + * + * Takes the environment as an argument so it can be tested without touching + * `process.env`, and reads it on each call rather than at import time: the + * module would otherwise capture whatever was set when it was first required, + * which in tests is whatever the previous suite happened to leave behind. + * + * Throws on a `PUBLIC_URL` that is set but unparseable, for the reason + * `relyingParty` does: that is a deployment already producing broken links in + * every email, so failing here is not the first thing to go wrong — it is the + * first thing to say so. + */ +export function googleConfig(env: NodeJS.ProcessEnv = process.env): GoogleConfig { + const clientId = (env.GOOGLE_CLIENT_ID ?? '').trim(); + const clientSecret = (env.GOOGLE_CLIENT_SECRET ?? '').trim(); + const publicUrl = (env.PUBLIC_URL ?? '').trim(); + + let base = LOCAL_ORIGIN; + if (publicUrl !== '') { + try { + // `origin` normalises away any path, trailing slash or default port, + // which is what makes the result stable regardless of how PUBLIC_URL was + // written. A trailing slash there would otherwise produce a double slash + // here and a mismatch at Google. + base = new URL(publicUrl).origin; + } catch { + throw new Error( + `PUBLIC_URL is not a URL (${publicUrl}), so the Google redirect URI cannot be derived ` + + 'from it. Google compares that value as an exact string, so this is refused rather ' + + 'than guessed at.' + ); + } + } + + return { + clientId, + clientSecret, + redirectUri: `${base}${GOOGLE_CALLBACK_PATH}`, + // Both, or neither. One without the other cannot complete a token exchange, + // and offering a button that always fails is worse than offering none. + enabled: clientId !== '' && clientSecret !== '' + }; +} diff --git a/backend/src/passwordHashing.ts b/backend/src/passwordHashing.ts index 8d1a5b9..9aa1b9c 100644 --- a/backend/src/passwordHashing.ts +++ b/backend/src/passwordHashing.ts @@ -1,3 +1,5 @@ +import bcrypt from 'bcryptjs'; + /** * How expensive a password hash is, and why that differs under test. * @@ -47,3 +49,30 @@ export function hashRoundsFor(nodeEnv: string | undefined): number { /** Resolved once at import: NODE_ENV does not change while the process runs. */ export const PASSWORD_HASH_ROUNDS = hashRoundsFor(process.env.NODE_ENV); + +/** + * Whether a supplied password matches a stored hash that may not exist (#340). + * + * `customers.password_hash` became nullable when social sign-in arrived, and a + * null one is not an edge case to tidy away — it is a customer who signed up + * through Google and has never set a password. There is nothing to compare + * against, so the answer is no. + * + * This exists because the alternative is worse than a wrong answer. + * `bcrypt.compare` throws `Illegal arguments` on a null hash rather than + * returning false, so every call site that forgot the check would answer a + * sign-in attempt with a 500 instead of a refusal — and a 500 on the login route + * is also an oracle, since it happens for exactly the accounts that have no + * password. + * + * One function rather than a null check repeated at each call site, so the + * question is asked the same way in all three places and a fourth cannot forget + * to ask it. + */ +export async function passwordMatches( + supplied: unknown, + storedHash: string | null | undefined +): Promise { + if (!storedHash) return false; + return bcrypt.compare(String(supplied ?? ''), storedHash); +} diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index fe881c7..cf832af 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -1,6 +1,6 @@ import { Router, Request, Response } from 'express'; import bcrypt from 'bcryptjs'; -import { PASSWORD_HASH_ROUNDS } from '../passwordHashing'; +import { PASSWORD_HASH_ROUNDS, passwordMatches } from '../passwordHashing'; import crypto from 'node:crypto'; import { pool, requireRow } from '../db'; import { requireCustomer } from '../middleware/customerAuth'; @@ -57,7 +57,7 @@ interface CustomerRow { * Kept in step with the schema by hand; nothing checks this against Postgres. */ interface CustomerRecord extends CustomerRow { - password_hash: string; + password_hash: string | null; disabled_at: Date | null; unsubscribe_token: string; marketing_consent_at: Date | null; @@ -418,7 +418,7 @@ router.post('/login', asyncRoute(async (req: Request, res: Response) => { const { email, password } = req.body; const { rows } = await pool.query(`SELECT * FROM customers WHERE email = $1`, [String(email || '').toLowerCase().trim()]); const customer = rows[0]; - if (!customer || !(await bcrypt.compare(password || '', customer.password_hash))) { + if (!customer || !(await passwordMatches(password, customer.password_hash))) { return res.status(401).json({ error: 'invalid email or password' }); } // Only after the password checks out, so a wrong password still looks like a @@ -519,7 +519,7 @@ router.post('/change-password', requireCustomer, asyncRoute(async (req: Request, } const { rows } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [req.customerId]); const customer = requireRow(rows, 'the signed-in customer'); - if (!(await bcrypt.compare(currentPassword || '', customer.password_hash))) { + if (!(await passwordMatches(currentPassword, customer.password_hash))) { return res.status(401).json({ error: 'current password is incorrect' }); } const newHash = await bcrypt.hash(newPassword, PASSWORD_HASH_ROUNDS); @@ -552,7 +552,7 @@ router.put('/me/email', requireCustomer, asyncRoute(async (req: Request, res: Re const { rows } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [req.customerId]); const customer = requireRow(rows, 'the signed-in customer'); - if (!(await bcrypt.compare(String(currentPassword ?? ''), customer.password_hash))) { + if (!(await passwordMatches(currentPassword, customer.password_hash))) { return res.status(401).json({ error: 'current password is incorrect' }); } diff --git a/backend/tests/integration/schemaLoss.integration.test.ts b/backend/tests/integration/schemaLoss.integration.test.ts index f91a73d..09090b5 100644 --- a/backend/tests/integration/schemaLoss.integration.test.ts +++ b/backend/tests/integration/schemaLoss.integration.test.ts @@ -42,7 +42,7 @@ describe('when the database loses its schema', () => { // The count and a few names, not the whole list: the point is that a reader // can tell at a glance this is a missing schema rather than a logic bug. - await expect(assertSchemaPresent('in a test')).rejects.toThrow(/Missing 21 of 21 tables/); + await expect(assertSchemaPresent('in a test')).rejects.toThrow(/Missing 22 of 22 tables/); await expect(assertSchemaPresent('in a test')).rejects.toThrow(/items/); await migrate(); diff --git a/backend/tests/integration/setup/testDb.ts b/backend/tests/integration/setup/testDb.ts index 7004b8e..e20183e 100755 --- a/backend/tests/integration/setup/testDb.ts +++ b/backend/tests/integration/setup/testDb.ts @@ -39,6 +39,7 @@ const REQUIRED_TABLES = [ 'checkouts', 'customer_credentials', 'customer_email_changes', + 'customer_identities', 'customer_sessions', 'customer_tokens', 'customers', @@ -154,7 +155,7 @@ export async function resetDb(): Promise { await testPool.query(` TRUNCATE TABLE item_drafts, upload_links, orders, checkout_items, checkouts, shipping_addresses, cart_items, carts, customer_tokens, customer_sessions, favorites, - webauthn_challenges, customer_credentials, customer_email_changes, + webauthn_challenges, customer_credentials, customer_email_changes, customer_identities, customers, item_tags, item_images, items, tags, categories RESTART IDENTITY CASCADE `); diff --git a/backend/tests/unit/envValidation.test.ts b/backend/tests/unit/envValidation.test.ts index b87df83..f7733a9 100644 --- a/backend/tests/unit/envValidation.test.ts +++ b/backend/tests/unit/envValidation.test.ts @@ -251,4 +251,44 @@ describe('UPLOADS_BASE_URL', () => { expect(warnings.join(' ')).not.toMatch(/INTAKE_ACTION_SECRET/); }); }); + + // #340. All or nothing, and half-configured is the case worth catching: the + // failure would otherwise arrive when a customer presses the button. + describe('Google sign-in credentials', () => { + const BOTH = { GOOGLE_CLIENT_ID: 'id', GOOGLE_CLIENT_SECRET: 'shh', PUBLIC_URL: 'https://x.test' }; + + it('are not required', () => { + expect(validateEnv(MINIMAL).errors).toEqual([]); + }); + + it('warn when absent, so a missing button has a stated reason', () => { + expect(validateEnv(MINIMAL).warnings.join(' ')).toMatch(/Google sign-in is not configured/); + }); + + it('refuse to start with only the client id', () => { + const { errors } = validateEnv(withEnv({ GOOGLE_CLIENT_ID: 'id' })); + expect(errors.join(' ')).toMatch(/GOOGLE_CLIENT_SECRET is required/); + }); + + it('refuse to start with only the client secret', () => { + const { errors } = validateEnv(withEnv({ GOOGLE_CLIENT_SECRET: 'shh' })); + expect(errors.join(' ')).toMatch(/GOOGLE_CLIENT_ID is required/); + }); + + it('are accepted when both are set alongside PUBLIC_URL', () => { + const { errors, warnings } = validateEnv(withEnv(BOTH)); + expect(errors).toEqual([]); + expect(warnings.join(' ')).not.toMatch(/Google sign-in/); + }); + + it('warn when configured without PUBLIC_URL, because the callback falls back to localhost', () => { + // Correct locally and wrong everywhere else, which is precisely the shape + // that needs saying out loud rather than failing at Google later. + const { errors, warnings } = validateEnv( + withEnv({ GOOGLE_CLIENT_ID: 'id', GOOGLE_CLIENT_SECRET: 'shh' }) + ); + expect(errors).toEqual([]); + expect(warnings.join(' ')).toMatch(/redirect URI falls back to localhost/); + }); + }); }); diff --git a/backend/tests/unit/googleConfig.test.ts b/backend/tests/unit/googleConfig.test.ts new file mode 100644 index 0000000..bce4e8b --- /dev/null +++ b/backend/tests/unit/googleConfig.test.ts @@ -0,0 +1,80 @@ +import { googleConfig, GOOGLE_CALLBACK_PATH } from '../../src/google/config'; + +const CREDENTIALS = { GOOGLE_CLIENT_ID: 'id.apps.googleusercontent.com', GOOGLE_CLIENT_SECRET: 'shh' }; + +/** + * The redirect URI is compared by Google as an exact string, and a mismatch + * answers `redirect_uri_mismatch` — accurate, and silent about which half is + * wrong. So these assert the derivation itself rather than that the function + * returns something, and most of them are about what must *not* end up in it. + */ +describe('googleConfig', () => { + it('builds the redirect URI from PUBLIC_URL', () => { + const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'https://redefined-designs.com' }); + + expect(config.redirectUri).toBe(`https://redefined-designs.com${GOOGLE_CALLBACK_PATH}`); + }); + + it('normalises a trailing slash away, rather than producing a double slash', () => { + // The failure this prevents is worth naming: "https://x.com//api/..." is a + // different string from the one registered in the console, and it is an + // easy way to write PUBLIC_URL. + const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'https://redefined-designs.com/' }); + + expect(config.redirectUri).toBe(`https://redefined-designs.com${GOOGLE_CALLBACK_PATH}`); + expect(config.redirectUri).not.toContain('//api'); + }); + + it('discards a path on PUBLIC_URL', () => { + const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'https://redefined-designs.com/shop' }); + + expect(config.redirectUri).toBe(`https://redefined-designs.com${GOOGLE_CALLBACK_PATH}`); + }); + + it('keeps a non-default port, because the console entry carries one too', () => { + const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'http://localhost:5173' }); + + expect(config.redirectUri).toBe(`http://localhost:5173${GOOGLE_CALLBACK_PATH}`); + }); + + it('falls back to localhost when PUBLIC_URL is unset', () => { + // Local development legitimately has no PUBLIC_URL: envValidation only + // demands it alongside SMTP. localhost is also the one host Google accepts + // without an authorized domain, so the fallback is the only value that + // could work here anyway. + const config = googleConfig({ ...CREDENTIALS }); + + expect(config.redirectUri).toBe(`http://localhost:3000${GOOGLE_CALLBACK_PATH}`); + }); + + it('refuses a PUBLIC_URL that is not a URL', () => { + expect(() => googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'redefined-designs.com' })).toThrow( + /not a URL/ + ); + }); + + describe('enabled', () => { + it('is true only when both credentials are present', () => { + expect(googleConfig(CREDENTIALS).enabled).toBe(true); + }); + + it('is false when neither is set, which is a legitimate environment', () => { + // A developer without credentials gets a storefront that works and does + // not offer the button, rather than one that offers it and fails. + expect(googleConfig({}).enabled).toBe(false); + }); + + it.each([ + ['only the id', { GOOGLE_CLIENT_ID: 'id' }], + ['only the secret', { GOOGLE_CLIENT_SECRET: 'shh' }] + ])('is false with %s', (_label, env) => { + expect(googleConfig(env).enabled).toBe(false); + }); + + it('treats whitespace as absent', () => { + // A stack variable set to an empty string is the same as one never set, + // and it is the shape a copied-and-blanked entry takes. + expect(googleConfig({ GOOGLE_CLIENT_ID: ' ', GOOGLE_CLIENT_SECRET: 'shh' }).enabled).toBe(false); + }); + }); +}); diff --git a/backend/tests/unit/passwordMatches.test.ts b/backend/tests/unit/passwordMatches.test.ts new file mode 100644 index 0000000..8ef2cd5 --- /dev/null +++ b/backend/tests/unit/passwordMatches.test.ts @@ -0,0 +1,50 @@ +import bcrypt from 'bcryptjs'; +import { passwordMatches, TEST_ROUNDS } from '../../src/passwordHashing'; + +/** + * `customers.password_hash` became nullable in #340, and a null one is not an + * edge case to tidy away — it is a customer who signed up through Google and + * has never set a password. + * + * The reason this is a function rather than a null check at each call site is + * what the second test asserts: `bcrypt.compare` throws on a null hash instead + * of returning false, so a forgotten check answers a sign-in attempt with a 500. + * On the login route that is also an oracle, because it happens for exactly the + * accounts that have no password. + */ +describe('passwordMatches', () => { + it('matches a correct password against a real hash', async () => { + const hash = await bcrypt.hash('supersecret123', TEST_ROUNDS); + + await expect(passwordMatches('supersecret123', hash)).resolves.toBe(true); + }); + + it('refuses a wrong password against a real hash', async () => { + const hash = await bcrypt.hash('supersecret123', TEST_ROUNDS); + + await expect(passwordMatches('nope', hash)).resolves.toBe(false); + }); + + it.each([ + ['null', null], + ['undefined', undefined], + ['an empty string', ''] + ])('answers false rather than throwing when the stored hash is %s', async (_label, stored) => { + // The whole point. bcrypt.compare throws "Illegal arguments" here. + await expect(passwordMatches('anything', stored)).resolves.toBe(false); + }); + + it('answers false for a missing password rather than throwing', async () => { + const hash = await bcrypt.hash('supersecret123', TEST_ROUNDS); + + await expect(passwordMatches(undefined, hash)).resolves.toBe(false); + await expect(passwordMatches(null, hash)).resolves.toBe(false); + }); + + it('does not treat an empty password as matching an absent hash', async () => { + // Both sides missing is the combination that would be most tempting to call + // a match, and it would let anyone sign in as any social-only customer by + // submitting a blank password. + await expect(passwordMatches('', null)).resolves.toBe(false); + }); +}); diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index f4efaec..6181e02 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -104,6 +104,13 @@ # INTAKE_ACTION_SECRET Optional. Signs the regenerate and discard links in the # intake notification email (#224). Absent, the email # still sends and carries no shortcuts. +# GOOGLE_CLIENT_ID Optional, and all-or-nothing with the secret below — +# GOOGLE_CLIENT_SECRET setting one without the other refuses to boot (#340). +# Both unset means Google sign-in is simply not offered. +# The redirect URI is derived from PUBLIC_URL and must +# match what is registered in the Google Auth Platform +# exactly, so changing the domain means updating that +# console too (#313). # REMBG_URL Optional. The background-removal sidecar, e.g. # http://rembg-syn:7000. Unset turns the feature off. # @@ -260,6 +267,11 @@ services: # review queue without shortcuts. Rotating it revokes every outstanding # link, which is how a leaked one is dealt with. - INTAKE_ACTION_SECRET=${INTAKE_ACTION_SECRET:-} + # Both or neither. envValidation refuses to start on one without the + # other, because the failure would otherwise arrive at the moment a + # customer presses the button (#340). + - GOOGLE_CLIENT_ID=${GOOGLE_CLIENT_ID:-} + - GOOGLE_CLIENT_SECRET=${GOOGLE_CLIENT_SECRET:-} volumes: # Production's own uploads directory. QA writes to # /volume1/configs/redefined-designs-qa/uploads; sharing this one would diff --git a/docker-compose.qa.yml b/docker-compose.qa.yml index 963d25d..090f94c 100644 --- a/docker-compose.qa.yml +++ b/docker-compose.qa.yml @@ -195,6 +195,20 @@ services: # rotating it revokes every outstanding link, which is the intended way to # deal with a leak. - INTAKE_ACTION_SECRET=${QA_INTAKE_ACTION_SECRET:-} + # Deliberately left empty, and it is not an oversight (#340, #345). + # + # Google refuses a redirect URI whose host is not under a domain whose + # ownership has been proved by DNS, and nobody can prove ownership of + # *.bermudalamb.synology.me because Synology owns the registrable domain + # above it. Same wall as #285. So QA cannot run Google sign-in at all + # while it lives on this hostname, and setting these would only produce a + # button that fails at Google. + # + # It becomes possible when #313 moves QA to qa.redefined-designs.com: + # set both here, set PUBLIC_URL to the new host, and add the matching + # callback in the Google Auth Platform. No code change either way. + - GOOGLE_CLIENT_ID= + - GOOGLE_CLIENT_SECRET= volumes: # Separate uploads directory. Sharing production's would let a QA run # write into, and a QA teardown delete, real product images.