From 286975dc52acf72809476e49ec0393021dd467cb Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Thu, 2 Jul 2026 12:44:39 +0200 Subject: [PATCH] =?UTF-8?q?fix(setup):=20address=20PR=20#714=20review=20?= =?UTF-8?q?=E2=80=94=20password=20UX,=20script=20token,=20race,=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blockers: - SetupPage now mirrors the server password rule (>=8 with upper/lower/digit) so a green client isn't bounced by the server; server errors carry a `field` (routes/setup.js) that the client maps to a translated key instead of rendering raw English. New i18n: setup.invalidToken, setup.passwordRequirements. - picpeak-setup.sh: the ADMIN_CREDENTIALS.txt block no longer dead-ends on the wizard path — when no legacy admin was seeded it prints the one-time setup token (from data/SETUP_TOKEN / docker compose logs) and points at /setup. Concern: - createInitialAdmin creates the admin + burns the token in ONE transaction, atomically claiming the token (null-if-present, expect 1 row) so a double-submit can't create two super_admins. Cross-DB (whereNotNull, trx-only writes). Added a concurrency test. Nits: - SetupPage redirects to /login when /setup/status errors (no form flash on a configured instance). - Dropped the unused DATABASE_URL from docker-compose.yml. - Documented why secrets are chmod 644 (three different reader users). --- .../integration/setupService.test.js | 12 +++++ backend/src/routes/setup.js | 4 +- backend/src/services/setupService.js | 45 +++++++++++++------ docker-compose.production.yml | 3 ++ docker-compose.yml | 4 +- frontend/src/i18n/locales/de.json | 2 + frontend/src/i18n/locales/en.json | 2 + frontend/src/pages/SetupPage.tsx | 37 ++++++++++----- scripts/picpeak-setup.sh | 39 +++++++++++----- 9 files changed, 110 insertions(+), 38 deletions(-) diff --git a/backend/__tests__/integration/setupService.test.js b/backend/__tests__/integration/setupService.test.js index d27a211d..2953d7af 100644 --- a/backend/__tests__/integration/setupService.test.js +++ b/backend/__tests__/integration/setupService.test.js @@ -113,6 +113,18 @@ describe('setupService (first-run bootstrap)', () => { ).rejects.toMatchObject({ statusCode: 409 }); }); + it('serialises a double-submit — two concurrent valid-token calls create only one admin', async () => { + const token = await setupService.ensureSetupToken(); + const results = await Promise.allSettled([ + setupService.createInitialAdmin({ token, email: 'a@example.com', password: VALID_PW }), + setupService.createInitialAdmin({ token, email: 'b@example.com', password: VALID_PW }), + ]); + const fulfilled = results.filter((r) => r.status === 'fulfilled'); + expect(fulfilled).toHaveLength(1); // the atomic token claim lets exactly one win + const count = await db('admin_users').count({ c: '*' }).first(); + expect(Number(count.c)).toBe(1); + }); + it('ensureSetupToken clears any stale token once an admin exists', async () => { const token = await setupService.ensureSetupToken(); await setupService.createInitialAdmin({ token, email: 'first@example.com', password: VALID_PW }); diff --git a/backend/src/routes/setup.js b/backend/src/routes/setup.js index 537093a6..c8defa43 100644 --- a/backend/src/routes/setup.js +++ b/backend/src/routes/setup.js @@ -45,7 +45,9 @@ router.post('/admin', [ res.status(201).json({ user: result.user }); } catch (err) { if (err.statusCode) { - return res.status(err.statusCode).json({ error: err.message }); + // `field` (token/email/password) lets the client show a translated + // message instead of rendering the raw English error verbatim. + return res.status(err.statusCode).json({ error: err.message, field: err.details || undefined }); } logger.error('[setup] createInitialAdmin failed', { error: err.message }); return res.status(500).json({ error: 'Setup failed' }); diff --git a/backend/src/services/setupService.js b/backend/src/services/setupService.js index 4c971a0f..8f199f6c 100644 --- a/backend/src/services/setupService.js +++ b/backend/src/services/setupService.js @@ -104,22 +104,39 @@ async function createInitialAdmin({ token, email, password, ip }) { if (!role) { throw new ConflictError('super_admin role missing — database not initialised'); } - const passwordHash = await bcrypt.hash(password, getBcryptRounds()); - const inserted = await db('admin_users').insert({ - username: cleanEmail, - email: cleanEmail, - password_hash: passwordHash, - role_id: role.id, - is_active: formatBoolean(true), - must_change_password: formatBoolean(false), - created_at: new Date(), - updated_at: new Date(), - }).returning('id'); - const id = inserted[0]?.id || inserted[0]; - // Burn the one-time token (DB + file) — the endpoint is now permanently closed. - await clearSetupToken(); + // Create the admin and burn the token ATOMICALLY. The claim (null the token + // row expecting exactly one match) serialises concurrent valid-token submits, + // so a double-submit can't create two super_admins. All writes use `trx` + // (never the global db) to avoid the SQLite in-transaction deadlock. + const id = await db.transaction(async (trx) => { + const claimed = await trx('app_settings') + .where({ setting_key: SETUP_TOKEN_KEY }) + .whereNotNull('setting_value') + .update({ setting_value: null, updated_at: new Date() }); + if (claimed !== 1) { + throw new ConflictError('Setup already completed — an admin account exists'); + } + const cnt = await trx('admin_users').count({ c: '*' }).first(); + if (Number(cnt?.c || 0) !== 0) { + throw new ConflictError('Setup already completed — an admin account exists'); + } + const inserted = await trx('admin_users').insert({ + username: cleanEmail, + email: cleanEmail, + password_hash: passwordHash, + role_id: role.id, + is_active: formatBoolean(true), + must_change_password: formatBoolean(false), + created_at: new Date(), + updated_at: new Date(), + }).returning('id'); + return inserted[0]?.id || inserted[0]; + }); + + // DB token cleared inside the tx; remove the on-disk file too (best-effort). + try { fs.unlinkSync(setupTokenFilePath()); } catch (_) { /* best-effort */ } logger.info(`[setup] Initial super_admin created (id=${id}, email=${cleanEmail})`); const authToken = jwt.sign( diff --git a/docker-compose.production.yml b/docker-compose.production.yml index 9245a63f..4b1992e9 100644 --- a/docker-compose.production.yml +++ b/docker-compose.production.yml @@ -28,6 +28,9 @@ services: if [ -n "$$REDIS_PASSWORD" ]; then printf '%s' "$$REDIS_PASSWORD" > /run/secrets/redis_password; else tr -dc A-Za-z0-9 < /dev/urandom | head -c 48 > /run/secrets/redis_password; fi fi + # 644: the readers run as three different users (postgres, redis, nodejs), + # so a non-root reader must be able to read them. The volume is private to + # these containers and never host-exposed. chmod 644 /run/secrets/jwt_secret /run/secrets/db_password /run/secrets/redis_password volumes: - picpeak-secrets:/run/secrets diff --git a/docker-compose.yml b/docker-compose.yml index 8724a54f..786c6652 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -24,6 +24,9 @@ services: if [ -n "$$REDIS_PASSWORD" ]; then printf '%s' "$$REDIS_PASSWORD" > /run/secrets/redis_password; else tr -dc A-Za-z0-9 < /dev/urandom | head -c 48 > /run/secrets/redis_password; fi fi + # 644: the readers run as three different users (postgres, redis, nodejs), + # so a non-root reader must be able to read them. The volume is private to + # these containers and never host-exposed. chmod 644 /run/secrets/jwt_secret /run/secrets/db_password /run/secrets/redis_password volumes: - picpeak-secrets:/run/secrets @@ -43,7 +46,6 @@ services: - ADMIN_EMAIL=${ADMIN_EMAIL:-admin@example.com} - ADMIN_PASSWORD=${ADMIN_PASSWORD:-} - DATABASE_CLIENT=pg - - DATABASE_URL=postgresql://${DB_USER}:${DB_PASSWORD:-}@postgres:5432/${DB_NAME} - DB_TYPE=postgresql - DB_HOST=postgres - DB_PORT=5432 diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index cc0e8277..d2bdaedf 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -3470,6 +3470,8 @@ "tokenHint": "Wird beim ersten Start in den Server-Logs ausgegeben (auch in data/SETUP_TOKEN gespeichert).", "tokenRequired": "Der Setup-Token ist erforderlich", "tokenLocationHint": "Nicht gefunden? Führen Sie aus: docker compose logs backend | grep -i \"setup token\"", + "invalidToken": "Dieser Setup-Token ist ungültig.", + "passwordRequirements": "Verwenden Sie mindestens 8 Zeichen mit einem Groß- und einem Kleinbuchstaben sowie einer Ziffer.", "emailLabel": "E-Mail-Adresse", "emailPlaceholder": "sie@beispiel.de", "emailRequired": "E-Mail ist erforderlich", diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 704b8f9c..8f6ed7ab 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -3366,6 +3366,8 @@ "tokenHint": "Printed to the server logs on first start (also saved to data/SETUP_TOKEN).", "tokenRequired": "The setup token is required", "tokenLocationHint": "Can't find it? Run: docker compose logs backend | grep -i \"setup token\"", + "invalidToken": "That setup token is not valid.", + "passwordRequirements": "Use at least 8 characters with an upper-case letter, a lower-case letter and a number.", "emailLabel": "Email address", "emailPlaceholder": "you@example.com", "emailRequired": "Email is required", diff --git a/frontend/src/pages/SetupPage.tsx b/frontend/src/pages/SetupPage.tsx index 896bd882..3164c4a3 100644 --- a/frontend/src/pages/SetupPage.tsx +++ b/frontend/src/pages/SetupPage.tsx @@ -19,7 +19,7 @@ export const SetupPage: React.FC = () => { const navigate = useNavigate(); const { login } = useAdminAuth(); - const { data: status, isLoading: statusLoading } = useQuery({ + const { data: status, isLoading: statusLoading, isError: statusError } = useQuery({ queryKey: ['setup-status'], queryFn: setupService.getSetupStatus, retry: false, @@ -34,8 +34,10 @@ export const SetupPage: React.FC = () => { if (statusLoading) { return ; } - // Setup already done → nothing to bootstrap here. - if (status && !status.needsAdmin) { + // Setup already done, OR the status couldn't be read (e.g. a transient 500) → + // go to login rather than flashing the create-admin form on a configured + // instance. Only render the wizard when we know an admin is genuinely missing. + if (statusError || !status?.needsAdmin) { return ; } @@ -50,7 +52,9 @@ export const SetupPage: React.FC = () => { if (!form.email) next.email = t('setup.emailRequired'); else if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(form.email)) next.email = t('setup.invalidEmail'); if (!form.password) next.password = t('setup.passwordRequired'); - else if (form.password.length < 8) next.password = t('setup.passwordMinLength'); + // Mirror the server's rule (validatePassword): >=8 chars with upper, lower + // and a digit — so the user isn't bounced by the server after a green client. + else if (!/^(?=.*[a-z])(?=.*[A-Z])(?=.*\d).{8,}$/.test(form.password)) next.password = t('setup.passwordRequirements'); if (form.confirm !== form.password) next.confirm = t('setup.passwordMismatch'); setErrors(next); return Object.keys(next).length === 0; @@ -81,18 +85,27 @@ export const SetupPage: React.FC = () => { toast.success(t('setup.success')); navigate('/admin/dashboard', { replace: true }); } catch (error: any) { - const apiErrors = error.response?.data?.errors; - if (error.response?.status === 429) { + const httpStatus = error.response?.status; + const data = error.response?.data; + // Map the server's field back to a translated message instead of + // rendering its raw English error verbatim. + const fieldKey: Record = { + token: 'setup.invalidToken', + email: 'setup.invalidEmail', + password: 'setup.passwordRequirements', + }; + if (httpStatus === 429) { toast.error(t('setup.tooManyAttempts')); - } else if (Array.isArray(apiErrors) && apiErrors.length) { - setErrors({ form: apiErrors[0]?.msg || t('setup.genericError') }); - } else if (error.response?.status === 409) { + } else if (httpStatus === 409) { // Someone else finished setup first — send to login. navigate('/admin/login', { replace: true }); - } else if (error.response?.data?.error) { - setErrors({ form: error.response.data.error }); + } else if (data?.field && fieldKey[data.field]) { + setErrors({ [data.field]: t(fieldKey[data.field]) }); + } else if (Array.isArray(data?.errors) && data.errors.length) { + const p = data.errors[0]?.path || data.errors[0]?.param; + setErrors(p && fieldKey[p] ? { [p]: t(fieldKey[p]) } : { form: t('setup.genericError') }); } else { - toast.error(t('setup.genericError')); + setErrors({ form: t('setup.genericError') }); } } finally { setIsSubmitting(false); diff --git a/scripts/picpeak-setup.sh b/scripts/picpeak-setup.sh index 7cb0bb21..cdd8619d 100755 --- a/scripts/picpeak-setup.sh +++ b/scripts/picpeak-setup.sh @@ -563,24 +563,43 @@ EOF fi fi - # Always surface the admin credentials file (#427: iSchumi reported - # admins couldn't find the generated password — the migration writes it - # to the in-container path and we never copied it to the host unless - # --reset-admin-password was used). Best-effort: a missing file just - # means the migration ran on a pre-existing DB and didn't generate one. + # Surface how to finish setup. Two paths: + # - Legacy: if ADMIN_PASSWORD was set, migration seeded an admin and wrote + # data/ADMIN_CREDENTIALS.txt (#427) — print those credentials. + # - Default (no ADMIN_PASSWORD): no admin is seeded; the app shows a + # first-run wizard at /setup guarded by a one-time token + # (data/SETUP_TOKEN). Print the token and point the operator there. + local login_base="${DOMAIN_NAME:+https://$DOMAIN_NAME}${DOMAIN_NAME:-http://YOUR_HOST_IP:3000}" if docker compose cp backend:/app/data/ADMIN_CREDENTIALS.txt "$app_dir/data/ADMIN_CREDENTIALS.txt" 2>/dev/null; then chown "$host_uid":"$host_gid" "$app_dir/data/ADMIN_CREDENTIALS.txt" 2>/dev/null || true chmod 600 "$app_dir/data/ADMIN_CREDENTIALS.txt" 2>/dev/null || true log_step "Admin credentials saved to: $app_dir/data/ADMIN_CREDENTIALS.txt" - # Show the password in the install output so the operator can log - # in immediately. The file remains as a backup record. echo echo "--------------------------------------------------" grep -E '^Email:|^Password:' "$app_dir/data/ADMIN_CREDENTIALS.txt" 2>/dev/null || true echo "--------------------------------------------------" - echo " Login URL: ${DOMAIN_NAME:+https://$DOMAIN_NAME}${DOMAIN_NAME:-http://YOUR_HOST_IP:3000}/admin" - echo " Full credentials file: $app_dir/data/ADMIN_CREDENTIALS.txt" - echo " Delete the file after recording the password." + echo " Login URL: $login_base/admin" + echo " Delete the credentials file after recording the password." + echo + else + # First-run wizard path — surface the one-time setup token. + docker compose cp backend:/app/data/SETUP_TOKEN "$app_dir/data/SETUP_TOKEN" 2>/dev/null || true + chown "$host_uid":"$host_gid" "$app_dir/data/SETUP_TOKEN" 2>/dev/null || true + local setup_token + setup_token="$(cat "$app_dir/data/SETUP_TOKEN" 2>/dev/null)" + [ -z "$setup_token" ] && setup_token="$(docker compose logs backend 2>/dev/null | grep -i 'setup token:' | tail -1 | sed -E 's/.*setup token: *([A-Za-z0-9_-]+).*/\1/')" + echo + echo "--------------------------------------------------" + echo " Finish setup in your browser — create the admin account:" + echo " 1. Open $login_base/admin" + echo " 2. Enter this one-time setup token:" + if [ -n "$setup_token" ]; then + echo " $setup_token" + else + echo " (run: cd $app_dir && docker compose logs backend | grep -i \"setup token\")" + fi + echo " 3. Set your admin email and password." + echo "--------------------------------------------------" echo fi