Files
redefined-designs/backend/src/envValidation.ts
T
synAdminandClaude Opus 5 9cc82002b8
Linting / lint (pull_request) Successful in 3m19s
SonarQube Analysis / sonarqube (pull_request) Failing after 27m40s
feat(auth): groundwork for signing in with Google (#340)
Nothing a customer can see. The schema change and the configuration land on their own so the widest-reaching edit in the project can be reviewed for what it is rather than buried inside a feature.

The password hash becomes nullable. That is one line and it is not the work; the work is that every read of the column is now a question rather than a fact. Three places compared against it with bcrypt, and all three now ask first through one shared function.

That function exists because the alternative is worse than a wrong answer. bcrypt.compare throws on a null hash rather than returning false, so any call site that forgot the check would answer a sign-in attempt with a 500 instead of a refusal. On the login route that is also an oracle, because it would happen for exactly the accounts that have no password. One function rather than a null check repeated three times means the question is asked identically everywhere and a fourth site cannot forget to ask it.

Nothing writes a null yet. The first accounts without a password arrive with the sign-up path, which is why this is landed ahead of them.

The identities table is a table rather than columns on customers, because one customer may eventually hold more than one. Columns would make a second provider a migration and a third an embarrassment.

Its important column is the provider subject, and the comment on it is the whole security posture of the feature in one place: never the email. An email is a display value its owner can change and a provider may reassign; a subject is opaque and stable for the life of the account. Matching on the email would strand a customer who changed theirs and, far worse, hand their account to whoever inherited the old address. Unique across the provider and subject together, not the subject alone.

The down migration drops the table and deliberately does not restore the NOT NULL. Re-adding it fails outright once a passwordless customer exists, and a down migration that destroys accounts to satisfy a constraint is far worse than a column that is merely more permissive than it needs to be.

The redirect URI is derived from PUBLIC_URL, the same single source the WebAuthn Relying Party ID uses and for the same reason: Google compares it as an exact string and answers a mismatch with a message that says nothing about which half is wrong. Deriving it means the value is correct by construction anywhere the email links already are. The tests are mostly about what must not end up in it, since a trailing slash on PUBLIC_URL is an easy way to produce a URI that is one character from the registered one.

The config also reports whether it is enabled at all, so a developer without credentials gets a storefront that works and simply does not offer the button, rather than one that offers it and fails. Absent rather than disabled, the same choice made for a browser without WebAuthn.

Environment validation refuses to boot on one credential without the other, matching how the SMTP pair is handled. Half-configured is the case worth catching because the failure otherwise arrives at the moment a customer presses the button.

The QA compose file sets both to empty, and the comment there says why at length rather than leaving it to look like an oversight. 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 anything under bermudalamb.synology.me because Synology owns the registrable domain above it. That is the same wall #285 hit with Cloudflare. So QA cannot run this at all until #313 moves it to a subdomain of the real domain, at which point it is two stack variables and one console entry, with no code change either way.

Also corrects the record in #332, which lists account deletion as confirming with a password. It does not; the route takes none and the confirmation is a modal in the account page. Deletion needed no change here.

Verified: backend tsc clean for src and tests, 550 unit tests pass including new coverage of the config derivation, the null-hash comparison and the environment rules; lint clean apart from warnings that predate this branch. The integration suite needs a database this machine has no Docker for.

Closes #340

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-10 08:18:51 -05:00

302 lines
11 KiB
TypeScript

/**
* Boot-time configuration checks.
*
* The backend reads environment variables in a couple of dozen places, and a
* missing or misspelled one used to be `undefined` until the first line of code
* that happened to need it — which could be a very long time after the
* container reported healthy. Several of those failures are silent and
* customer-visible: mail containing `undefined` in a link, or a shop that
* quietly stops charging anyone.
*
* The container already refuses to start on a failed migration rather than
* serving against a schema it does not match. This is the same argument applied
* to configuration.
*
* Kept a pure function of the environment it is handed, rather than reading
* `process.env` itself, so it can be tested exhaustively without booting a
* server or mutating global state. `server.ts` calls it; `app.ts` deliberately
* does not, because the integration suite imports `app` directly and would
* otherwise become a configuration exercise.
*/
export interface EnvValidation {
/** Configuration that must be fixed. The process should not start. */
errors: string[];
/** Working, but worth saying out loud — usually a capability that is off. */
warnings: string[];
}
// Without these the process cannot do its job at all.
//
// Exported so tests/unit/composeEnvironment.test.ts can assert the deploying
// environment actually sets them. #107 happened because this list grew and
// docker-compose.qa.yml did not: the check has to read this list rather than a
// copy of it, or the next variable added here goes unguarded in exactly the
// same way.
export const ALWAYS_REQUIRED = [
'PGHOST',
'PGPORT',
'PGUSER',
'PGPASSWORD',
'PGDATABASE',
// No reprieve for this one despite having a fallback: '/app/uploads' is
// correct inside the container and wrong everywhere else, so inheriting it
// silently writes uploads somewhere nobody is looking.
'UPLOADS_DIR'
] as const;
// Only meaningful once real payments are switched on. QA runs with none of
// these on purpose, which is why the requirement is conditional rather than
// absolute.
const PAYPAL_REQUIRED = [
'PAYPAL_CLIENT_ID',
'PAYPAL_CLIENT_SECRET',
'PAYPAL_WEBHOOK_ID',
'PAYPAL_ENV'
] as const;
// A variable set to spaces is a configuration mistake, not a value.
function isPresent(env: NodeJS.ProcessEnv, name: string): boolean {
const value = env[name];
return typeof value === 'string' && value.trim() !== '';
}
// One function per rule, at module level rather than nested. Each is small
// enough to read on its own, and cognitive complexity counts everything
// declared inside a function — so keeping these out of validateEnv is what
// keeps the composition below flat.
function checkAlwaysRequired(env: NodeJS.ProcessEnv): string[] {
return ALWAYS_REQUIRED.filter((name) => !isPresent(env, name)).map(
(name) => `${name} is required and is not set.`
);
}
// Strict rather than truthy. This used to be read as "demo unless the value is
// exactly 'false'", so DEMO_MODE=False, 0, or any typo meant demo mode was on —
// a configuration slip that stopped the shop taking money and said nothing.
function checkDemoMode(env: NodeJS.ProcessEnv): string[] {
const demoMode = env.DEMO_MODE;
if (demoMode === undefined || demoMode.trim() === '') {
return [
"DEMO_MODE is required and must be exactly 'true' or 'false'. It decides whether real " +
'payments are taken, so it has to be stated rather than inherited.'
];
}
if (demoMode !== 'true' && demoMode !== 'false') {
return [
`DEMO_MODE must be exactly 'true' or 'false', but is '${demoMode}'. Anything else used to ` +
'be read as demo mode, which meant a typo here quietly stopped the shop charging anyone.'
];
}
return [];
}
// Conditional rather than absolute: QA runs with no PayPal credentials on
// purpose, so requiring them unconditionally would be wrong.
function checkPayPal(env: NodeJS.ProcessEnv): string[] {
if (env.DEMO_MODE !== 'false') {
return [];
}
return PAYPAL_REQUIRED.filter((name) => !isPresent(env, name)).map(
(name) => `${name} is required when DEMO_MODE=false, because real payments are enabled.`
);
}
// SMTP is all or nothing, and two other variables hang off whether it is set.
function checkMail(env: NodeJS.ProcessEnv): EnvValidation {
const errors: string[] = [];
const warnings: string[] = [];
const hasUser = isPresent(env, 'SMTP_USER');
const hasPassword = isPresent(env, 'SMTP_PASSWORD');
// Half-configured is worse than absent: the mailer only skips when both are
// missing, so setting one produces a connection that fails at send time
// instead of a clean "mail is off".
if (hasUser && !hasPassword) {
errors.push('SMTP_PASSWORD is required when SMTP_USER is set — set both or neither.');
}
if (hasPassword && !hasUser) {
errors.push('SMTP_USER is required when SMTP_PASSWORD is set — set both or neither.');
}
if (!hasUser || !hasPassword) {
warnings.push(
'SMTP is not configured — no email will be sent. Verification, password reset, favorite ' +
'alerts and cart reminders will all be skipped with a warning.'
);
return { errors, warnings };
}
// Demanded only alongside SMTP. Its sole job is building links in email, so a
// local environment that cannot send mail does not need it, and requiring it
// there would break every existing local setup to prevent nothing.
if (!isPresent(env, 'PUBLIC_URL')) {
errors.push(
'PUBLIC_URL is required when SMTP is configured, or every link in a verification, ' +
'password-reset, favorite-alert or cart-reminder email reads "undefined".'
);
}
if (!isPresent(env, 'MAIL_ALLOWLIST')) {
warnings.push(
'MAIL_ALLOWLIST is not set while SMTP is configured — this environment can email real ' +
'customers. That is correct for production and a hazard anywhere else.'
);
}
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 [];
}
return [
'ADMIN_GATE_SECRET is not set — /api/admin is protected only by the reverse proxy. ' +
'Anything able to reach this container directly can administer the store.'
];
}
// Optional on purpose, and unlike the two above, unset costs nothing in
// safety. A submission still arrives, keeps its photos and waits in the queue
// undrafted (#223). It is a warning rather than an error because the photos are
// often the only copy of an item no longer in the sender's hands, so losing a
// consignment to an expired key would be far worse than an item arriving
// without its description written. Silence would be the wrong answer too: an
// operator who believes drafting is on and finds every item undrafted has
// nothing to tell them why.
// Optional, like the drafting key below. Absent, the notification still sends
// with its review link and simply carries no shortcuts — being told an item
// arrived matters far more than being able to discard it in one click.
function checkIntakeActionSecret(env: NodeJS.ProcessEnv): string[] {
if (isPresent(env, 'INTAKE_ACTION_SECRET')) {
return [];
}
return [
'INTAKE_ACTION_SECRET is not set — intake notifications will link to the review queue ' +
'but carry no regenerate or discard shortcuts.'
];
}
function checkDraftingKey(env: NodeJS.ProcessEnv): string[] {
if (isPresent(env, 'ANTHROPIC_API_KEY')) {
return [];
}
return [
'ANTHROPIC_API_KEY is not set — submitted items will arrive undrafted and wait in the ' +
'review queue for someone to write them up by hand.'
];
}
// Optional, and the same shape as the admin gate above: unset is a working
// configuration with one defence switched off, which is worth saying out loud
// rather than leaving to be discovered. Set, it has to be an absolute origin —
// a value missing its scheme joins into a relative path and silently breaks
// every image on the site, which is a worse outcome than either extreme.
function checkUploadsOrigin(env: NodeJS.ProcessEnv): EnvValidation {
if (!isPresent(env, 'UPLOADS_BASE_URL')) {
return {
errors: [],
warnings: [
'UPLOADS_BASE_URL is not set — uploaded files are served from this application on its ' +
'own origin, so anything reaching the uploads directory shares an origin with the site.'
]
};
}
const value = (env.UPLOADS_BASE_URL ?? '').trim();
if (!value.startsWith('https://') && !value.startsWith('http://')) {
return {
errors: [
'UPLOADS_BASE_URL must be an absolute origin including the scheme, such as ' +
'https://uploads.example.com. Without one it joins into a relative path and every ' +
'image on the site breaks.'
],
warnings: []
};
}
return { errors: [], warnings: [] };
}
export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation {
const mail = checkMail(env);
const uploads = checkUploadsOrigin(env);
const google = checkGoogleSignIn(env);
return {
errors: [
...checkAlwaysRequired(env),
...checkDemoMode(env),
...checkPayPal(env),
...mail.errors,
...uploads.errors,
...google.errors
],
warnings: [
...mail.warnings,
...checkAdminGate(env),
...uploads.warnings,
...checkDraftingKey(env),
...checkIntakeActionSecret(env),
...google.warnings
]
};
}