diff --git a/.env.example b/.env.example index 22b8d81a..df33b119 100644 --- a/.env.example +++ b/.env.example @@ -73,21 +73,32 @@ DB_NAME=picpeak_prod #ADMIN_EMAIL=admin@yourdomain.com #ADMIN_PASSWORD=your_secure_admin_password_here -# Email Configuration +# Email Configuration — OPTIONAL, and normally left alone. +# SMTP is configured in the setup wizard / Settings -> Email and stored in the +# database (email_configs); that is what the mail queue actually sends with. +# These variables are a legacy path kept for config-as-code deployments: when +# SMTP_HOST is set, the initial migration seeds the database row from it. +# Developers running the `dev` compose profile want SMTP_HOST=mailhog here so +# that seed points at the mailhog container. # For Gmail: use app-specific password # For SendGrid: SMTP_USER=apikey, SMTP_PASS=your-api-key -SMTP_HOST=smtp.gmail.com -SMTP_PORT=587 -SMTP_SECURE=false -SMTP_USER=your-email@gmail.com -SMTP_PASS=your-app-specific-password -EMAIL_FROM=noreply@yourdomain.com +#SMTP_HOST=smtp.gmail.com +#SMTP_PORT=587 +#SMTP_SECURE=false +#SMTP_USER=your-email@gmail.com +#SMTP_PASS=your-app-specific-password +#EMAIL_FROM=noreply@yourdomain.com -# Application URLs +# Application URLs — OPTIONAL. Leave unset for the normal install. +# The public origin is captured by the setup wizard (it proposes the address +# you opened the browser at) and stored as the `general_site_url` setting, so +# you can change it later in Settings -> General without touching this file. +# Setting FRONTEND_URL here OVERRIDES that setting and makes the field +# read-only in the admin UI - use it only for config-as-code deployments. # Use full origin with scheme, no trailing slash. # Admin UI is served by the frontend at /admin. -FRONTEND_URL=https://yourdomain.com -ADMIN_URL=https://yourdomain.com +#FRONTEND_URL=https://yourdomain.com +#ADMIN_URL=https://yourdomain.com # Static HTML title + description used for social link previews when the # fetcher doesn't trigger the per-event OG endpoint — most notably the @@ -100,9 +111,10 @@ BRAND_TITLE=PicPeak BRAND_DESCRIPTION=Photo gallery shared with PicPeak. # API URL for email assets (logos, images in notification emails) -# This must be the publicly accessible URL where email recipients can load images. -# If not set, defaults to http://localhost:3001 which will show broken images in emails. -API_URL=https://yourdomain.com/api +# OPTIONAL: when unset this is derived from the resolved public origin + /api, +# so the wizard's answer covers it. Set it only for split-origin deployments +# where the API lives on a different host than the gallery. +#API_URL=https://yourdomain.com/api # Frontend API base # For pre-built images and production behind a reverse proxy, keep '/api'. diff --git a/Dockerfile.aio b/Dockerfile.aio index d97f7120..f2f4f472 100644 --- a/Dockerfile.aio +++ b/Dockerfile.aio @@ -113,15 +113,15 @@ ENV DATA_ROOT=/data \ LOG_DIR=/data/logs \ BACKUP_DIR=/data/backup -# Share links are absolute only when a base URL is known: getFrontendBaseUrl() -# reads FRONTEND_URL, falls back to the general_site_url setting, and otherwise -# returns "" — which makes share_url come out as a bare "/gallery//" -# in API responses, QR codes and emails. docker-compose.yml defaults this to -# http://localhost:3000, but the documented one-liner for this image passes only -# JWT_SECRET, so without a default here every fresh single-container install -# would hand out unusable links. Same default as compose; override with -# -e FRONTEND_URL=https://photos.example.com, or set the site URL in Settings. -ENV FRONTEND_URL=http://localhost:3000 +# FRONTEND_URL is deliberately NOT set here (#705). It used to default to +# http://localhost:3000 so share links would not come out relative, but a +# baked-in value OVERRIDES the general_site_url setting the setup wizard +# writes — so a single-container install could never configure its own public +# address, and the Settings field would show as env-pinned for everyone. +# getFrontendBaseUrl() now resolves the setting, then the origin the request +# arrived on, and getAbsoluteFrontendUrl() still ends at http://localhost:3000, +# so links stay absolute without pinning anything. Override with +# -e FRONTEND_URL=https://photos.example.com for config-as-code deployments. # /app/storage is a second entrance to the same volume. The business-document # writers (quoteService, invoice sending/reminders, contract signatures) build diff --git a/backend/migrations/core/001_init.js b/backend/migrations/core/001_init.js index 278e1fb9..36b99b62 100644 --- a/backend/migrations/core/001_init.js +++ b/backend/migrations/core/001_init.js @@ -146,11 +146,18 @@ Generated on: ${new Date().toISOString()} console.log('Default email templates created'); } - // Create default email config if none exists + // Seed an email config ONLY when the environment actually supplies a host + // (#705). This used to fall back to `mailhog`, the dev compose service, so + // every fresh install came up with a LIVE config pointing at a host that + // does not exist outside the dev stack — the setup wizard then showed empty + // SMTP fields (reading as "nothing configured") while mail silently failed. + // With no row at all, emailProcessor logs "No email configuration found" + // and the wizard's blank fields are the truth. The dev stack keeps mailhog + // by setting SMTP_HOST explicitly in docker-compose.yml. const emailConfig = await knex('email_configs').first(); - if (!emailConfig) { + if (!emailConfig && process.env.SMTP_HOST) { await knex('email_configs').insert({ - smtp_host: process.env.SMTP_HOST || 'mailhog', + smtp_host: process.env.SMTP_HOST, smtp_port: process.env.SMTP_PORT || 1025, smtp_secure: process.env.SMTP_SECURE === 'true', smtp_user: process.env.SMTP_USER || '', diff --git a/backend/server.js b/backend/server.js index cf205c9c..7e7ced7d 100644 --- a/backend/server.js +++ b/backend/server.js @@ -69,6 +69,11 @@ const compression = require('compression'); const cors = require('cors'); const path = require('path'); const { initializeDatabase, db } = require('./src/database/db'); +const { + getFrontendBaseUrlSync, + getAbsoluteFrontendUrl, + primeSiteUrlCache, +} = require('./src/utils/frontendUrl'); const { startFileWatcher } = require('./src/services/fileWatcher'); const { startExpirationChecker } = require('./src/services/expirationChecker'); const { startTransferCleanup } = require('./src/services/transferCleanupService'); @@ -212,8 +217,12 @@ app.use((req, res, next) => { // CORS configuration (apply only to API routes) const corsOptions = { origin: function (origin, callback) { + // getFrontendBaseUrlSync() resolves FRONTEND_URL, else the configured + // general_site_url (#705) — without it, an install that leaves the + // environment untouched and answers the setup wizard instead would have + // its own public origin missing from the allowlist. const allowedOrigins = [ - process.env.FRONTEND_URL || 'http://localhost:3005', + getFrontendBaseUrlSync() || 'http://localhost:3005', process.env.ADMIN_URL || 'http://localhost:3005' ]; @@ -499,7 +508,7 @@ app.use('/api/admin', sessionTimeoutMiddleware); const setCorsHeaders = (req, res, next) => { const origin = req.headers.origin; const staticAllowedOrigins = [ - process.env.FRONTEND_URL || 'http://localhost:3005', + getFrontendBaseUrlSync() || 'http://localhost:3005', process.env.ADMIN_URL || 'http://localhost:3005' ]; if (process.env.NODE_ENV === 'development') { @@ -637,7 +646,7 @@ app.get('/s/:shortSlug', async (req, res) => { // social platforms cache OG by URL, and the short URL is the // one operators actually share, so that's the cache key we // want them to stick with. - const base = (process.env.FRONTEND_URL || 'http://localhost:3000').replace(/\/$/, ''); + const base = await getAbsoluteFrontendUrl(req); meta.url = `${base}/s/${row.short_slug}`; res.set('Cache-Control', 'public, max-age=300'); res.set('Content-Type', 'text/html; charset=utf-8'); @@ -1016,6 +1025,12 @@ async function startServer() { // Initialize database await initializeDatabase(); + // Warm the public-origin cache so the SYNCHRONOUS resolver (CORS headers, + // secureImageMiddleware) can see the general_site_url setting. Best-effort: + // the async resolver reads through on its own, and a cold cache only means + // falling back to the environment. + await primeSiteUrlCache().catch(() => {}); + // Initialize storage backend (local fs or S3) — fail fast on misconfig const { initStorage } = require('./src/services/storage'); await initStorage(); diff --git a/backend/src/__tests__/frontendUrl.test.js b/backend/src/__tests__/frontendUrl.test.js new file mode 100644 index 00000000..24b14c7b --- /dev/null +++ b/backend/src/__tests__/frontendUrl.test.js @@ -0,0 +1,247 @@ +/** + * Unit tests for the public-origin resolver (#705). + * + * The zero-touch install depends on this precedence being exactly: + * 1. FRONTEND_URL, when it is not loopback + * 2. a purpose-specific override the caller passes (ADMIN_URL, APP_URL) + * 3. the `general_site_url` setting written by the setup wizard + * 4. the origin the request arrived on + * 5. whichever of those exists at all (a genuine localhost install) + * 6. '' — so callers that build RELATIVE urls keep doing so + * + * The loopback demotion in (1) matters because docker-compose used to inject + * FRONTEND_URL=http://localhost:3000 unconditionally: taking that literally + * sends every gallery link, QR code and reminder email to the RECIPIENT's own + * machine. The empty return in (5) is load-bearing too — shareLinkService and + * the SSO redirects in routes/auth treat it as "emit a relative url". + */ + +// `mock`-prefixed so jest's out-of-scope guard allows the factory to close +// over it; the same fn is reused across resetModules() so assertions on call +// counts survive a reload. +const mockDb = jest.fn(); +jest.mock('../database/db', () => ({ db: mockDb })); + +const db = mockDb; + +// Mimic knex's builder for: db('app_settings').where(...).select(...).first() +const settingRow = (value) => ({ + where: () => ({ select: () => ({ first: async () => (value === undefined ? undefined : { setting_value: JSON.stringify(value) }) }) }), +}); + +const fakeReq = (host, protocol = 'http') => ({ protocol, get: (h) => (h === 'host' ? host : undefined) }); + +// resetModules() clears the resolver's cached setting between cases; the +// jest.mock registration above survives it. +const loadModule = () => { + jest.resetModules(); + return require('../utils/frontendUrl'); +}; + +describe('getFrontendBaseUrl precedence', () => { + beforeEach(() => { + delete process.env.FRONTEND_URL; + delete process.env.API_URL; + db.mockReset(); + }); + + it('prefers a non-loopback FRONTEND_URL and never touches the database', async () => { + process.env.FRONTEND_URL = 'https://pinned.example.com'; + db.mockImplementation(() => settingRow('https://setting.example.com')); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl()).toBe('https://pinned.example.com'); + expect(db).not.toHaveBeenCalled(); + }); + + it('demotes a loopback FRONTEND_URL in favour of the configured setting', async () => { + process.env.FRONTEND_URL = 'http://localhost:3000'; + db.mockImplementation(() => settingRow('http://192.168.1.50:3000')); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl()).toBe('http://192.168.1.50:3000'); + }); + + it('uses the wizard setting when the environment is untouched', async () => { + db.mockImplementation(() => settingRow('https://gallery.example.com')); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl()).toBe('https://gallery.example.com'); + }); + + it('falls back to the origin the request arrived on', async () => { + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl(fakeReq('192.168.1.77:3000'))).toBe('http://192.168.1.77:3000'); + }); + + it('honours X-Forwarded-Proto via req.protocol', async () => { + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl(fakeReq('gallery.example.com', 'https'))).toBe('https://gallery.example.com'); + }); + + it('returns empty when nothing is configured, so callers can go relative', async () => { + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl()).toBe(''); + }); + + it('still resolves a genuine localhost install rather than returning empty', async () => { + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl(fakeReq('localhost:3000'))).toBe('http://localhost:3000'); + }); + + it('strips trailing slashes', async () => { + process.env.FRONTEND_URL = 'https://pinned.example.com///'; + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl()).toBe('https://pinned.example.com'); + }); +}); + +describe('getAbsoluteFrontendUrl', () => { + beforeEach(() => { + delete process.env.FRONTEND_URL; + delete process.env.API_URL; + db.mockReset(); + }); + + it('ends at localhost:3000 when nothing is configured and there is no request', async () => { + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getAbsoluteFrontendUrl()).toBe('http://localhost:3000'); + }); +}); + +describe('getApiBaseUrl', () => { + beforeEach(() => { + delete process.env.FRONTEND_URL; + delete process.env.API_URL; + db.mockReset(); + }); + + it('prefers an explicit API_URL for split-origin deployments', async () => { + process.env.API_URL = 'https://api.example.com'; + db.mockImplementation(() => settingRow('https://gallery.example.com')); + const m = loadModule(); + + expect(await m.getApiBaseUrl()).toBe('https://api.example.com'); + }); + + it('otherwise derives /api from the resolved origin', async () => { + db.mockImplementation(() => settingRow('https://gallery.example.com')); + const m = loadModule(); + + expect(await m.getApiBaseUrl()).toBe('https://gallery.example.com/api'); + }); +}); + +describe('purpose-specific env override (ADMIN_URL / APP_URL)', () => { + beforeEach(() => { delete process.env.FRONTEND_URL; db.mockReset(); }); + + it('beats the general_site_url setting, so a split-origin admin host wins', async () => { + db.mockImplementation(() => settingRow('https://gallery.example.com')); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl(null, { override: 'https://admin.example.com' })) + .toBe('https://admin.example.com'); + }); + + it('beats the request origin too', async () => { + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl(fakeReq('gallery.example.com', 'https'), { override: 'https://admin.example.com' })) + .toBe('https://admin.example.com'); + }); + + it('still sits below FRONTEND_URL, preserving the historic order', async () => { + process.env.FRONTEND_URL = 'https://pinned.example.com'; + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl(null, { override: 'https://admin.example.com' })) + .toBe('https://pinned.example.com'); + }); + + it('is demoted when it is loopback, like every other candidate', async () => { + db.mockImplementation(() => settingRow('https://gallery.example.com')); + const m = loadModule(); + + expect(await m.getFrontendBaseUrl(null, { override: 'http://localhost:3005' })) + .toBe('https://gallery.example.com'); + }); + + it('is used by getAbsoluteFrontendUrl ahead of the terminal fallback', async () => { + db.mockImplementation(() => settingRow(undefined)); + const m = loadModule(); + + expect(await m.getAbsoluteFrontendUrl(null, { override: 'https://admin.example.com' })) + .toBe('https://admin.example.com'); + }); +}); + +describe('isEnvPinned', () => { + beforeEach(() => { delete process.env.FRONTEND_URL; db.mockReset(); }); + + it('reports the environment override so the admin UI can go read-only', async () => { + db.mockImplementation(() => settingRow(undefined)); + let m = loadModule(); + expect(m.isEnvPinned()).toBe(false); + + process.env.FRONTEND_URL = 'https://pinned.example.com'; + m = loadModule(); + expect(m.isEnvPinned()).toBe(true); + }); + + // The upgrade case this whole PR exists for: an install still carrying the + // old compose default. The resolver demotes it, so the admin UI must NOT + // lock the Site URL field — otherwise that operator can never configure a + // public address anywhere (#1104). + it('does NOT report a loopback FRONTEND_URL as pinned', () => { + process.env.FRONTEND_URL = 'http://localhost:3000'; + const m = loadModule(); + + expect(m.isEnvPinned()).toBe(false); + expect(m.envPinnedBase()).toBe(''); + }); + + it('exposes the effective pinned value, normalised', () => { + process.env.FRONTEND_URL = 'https://pinned.example.com/// '; + const m = loadModule(); + + expect(m.envPinnedBase()).toBe('https://pinned.example.com'); + }); +}); + +describe('isLoopbackBase', () => { + it.each([ + ['http://localhost:3000', true], + ['http://127.0.0.1:8080', true], + ['http://0.0.0.0:3000', true], + ['http://[::1]:3000', true], + ['http://localhost', true], + ['http://localhost/gallery', true], + ['http://192.168.1.50:3000', false], + ['https://gallery.example.com', false], + // The host token must end at a boundary. These are real public hosts that + // merely START with a loopback name — demoting them would silently ignore + // the address the operator configured, since this predicate now gates the + // whole resolver rather than just the slideshow QR. + ['https://localhost-nas.example.com', false], + ['https://localhostings.io', false], + ['http://0.0.0.0.nip.io', false], + ['', false], + ])('%s → %s', (url, expected) => { + const m = loadModule(); + expect(m.isLoopbackBase(url)).toBe(expected); + }); +}); diff --git a/backend/src/middleware/secureImageMiddleware.js b/backend/src/middleware/secureImageMiddleware.js index 3ae5807a..3b604ec7 100644 --- a/backend/src/middleware/secureImageMiddleware.js +++ b/backend/src/middleware/secureImageMiddleware.js @@ -2,6 +2,7 @@ const { db } = require('../database/db'); const secureImageService = require('../services/secureImageService'); const logger = require('../utils/logger'); const { formatBoolean } = require('../utils/dbCompat'); +const { getFrontendBaseUrlSync } = require('../utils/frontendUrl'); /** * Enhanced secure image middleware with comprehensive protection @@ -272,7 +273,10 @@ class SecureImageMiddleware { 'X-Download-Policy': 'restricted', // CORS restrictions - 'Access-Control-Allow-Origin': process.env.FRONTEND_URL || '*', + // Deliberately NOT derived from the request: reflecting the caller's + // Origin would defeat the allowlist. Env, else the configured + // general_site_url (cached), else the previous wildcard behaviour. + 'Access-Control-Allow-Origin': getFrontendBaseUrlSync() || '*', 'Access-Control-Allow-Methods': 'GET', 'Access-Control-Allow-Headers': 'Authorization, Content-Type', 'Access-Control-Max-Age': '3600' diff --git a/backend/src/routes/adminDev.js b/backend/src/routes/adminDev.js index c81c3940..0aaaa621 100644 --- a/backend/src/routes/adminDev.js +++ b/backend/src/routes/adminDev.js @@ -128,7 +128,7 @@ router.get( }) ); -const FRONTEND_URL_FALLBACK = 'https://app.example.com'; +const { getAbsoluteFrontendUrl } = require('../utils/frontendUrl'); const DEV_TEST_DIR = () => path.join(getStoragePath(), 'business-docs', 'dev-test'); function fakeMoney(major, currency, locale = 'de') { @@ -440,7 +440,7 @@ router.post( throw new AppError(`Template "${req.body.templateKey}" not seeded yet — run migrations`, 409, 'TEMPLATE_MISSING'); } - const frontendUrl = (process.env.FRONTEND_URL || FRONTEND_URL_FALLBACK).replace(/\/$/, ''); + const frontendUrl = await getAbsoluteFrontendUrl(req); const payload = await buildPayloadFor(req.body.templateKey, req.admin.id, frontendUrl); await emailProcessor.queueEmail(null, admin.email, req.body.templateKey, payload); diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index 39882ec9..5dec245d 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -26,7 +26,7 @@ const { hasColumnCached } = require('../../utils/schemaCache'); const { requireEventOwnership } = require('../../middleware/ownership'); const { getAppSetting } = require('../../utils/appSettings'); const { clampIntOrUndefined } = require('../../utils/numericHelpers'); -const { getFrontendBaseUrl } = require('../../utils/frontendUrl'); +const { getFrontendBaseUrl, getAbsoluteFrontendUrl } = require('../../utils/frontendUrl'); const downloadZipService = require('../../services/downloadZipService'); const { validateHeroImageAnchor, getEventFieldRequirements, readBooleanSetting, getDownloadProtectionDefaults, getBrandingDefaults, getCustomerNameFromPayload, getCustomerEmailFromPayload, getCustomerPhoneFromPayload, isPhoneFieldEnabled, mapEventForApi, hasCustomerContactColumns, deleteEventCascade, SLIDESHOW_TRANSITIONS, SLIDESHOW_COLORFILTERS } = require('./helpers'); @@ -576,7 +576,12 @@ module.exports = (router) => { // Include client access info in email when enabled (#172) if (client_access_enabled && client_password) { const createdEvent = await db('events').where('id', eventId).first(); - const frontendUrl = process.env.FRONTEND_URL || process.env.APP_URL || ''; + // Same FRONTEND_URL-before-APP_URL order as before: APP_URL is + // passed as the override so it still outranks the general_site_url + // setting and the request origin. Chaining it after the resolver + // would make it dead code, because the resolver only returns falsy + // when NOTHING is configured (#1104). + const frontendUrl = await getAbsoluteFrontendUrl(req, { override: process.env.APP_URL }); emailData.client_link = `${frontendUrl}/gallery/${slug}/client-access?token=${createdEvent.client_share_token}`; emailData.client_password = client_password; } diff --git a/backend/src/routes/adminGuests.js b/backend/src/routes/adminGuests.js index 8b053121..91755c3b 100644 --- a/backend/src/routes/adminGuests.js +++ b/backend/src/routes/adminGuests.js @@ -9,8 +9,7 @@ const { requireEventOwnership } = require('../middleware/ownership'); const feedbackService = require('../services/feedbackService'); const logger = require('../utils/logger'); const { errorResponse } = require('../utils/routeHelpers'); - -const FRONTEND_URL = process.env.FRONTEND_URL || ''; +const { getAbsoluteFrontendUrl } = require('../utils/frontendUrl'); // ---------------------------------------------------------------------------- // Helpers @@ -185,10 +184,11 @@ router.get( ) .orderBy('guest_invites.created_at', 'desc'); + const baseUrl = await getAbsoluteFrontendUrl(req); const invites = rows.map((r) => ({ id: r.id, token: r.token, - url: `${FRONTEND_URL}/gallery/${event.slug}?invite=${r.token}`, + url: `${baseUrl}/gallery/${event.slug}?invite=${r.token}`, created_at: r.created_at, redeemed_at: r.redeemed_at, revoked_at: r.revoked_at, @@ -261,11 +261,12 @@ router.post( ); const event = await db('events').where({ id: eventId }).first(); + const baseUrl = await getAbsoluteFrontendUrl(req); res.json({ invite: { id: inviteId, token: inviteToken, - url: `${FRONTEND_URL}/gallery/${event.slug}?invite=${inviteToken}`, + url: `${baseUrl}/gallery/${event.slug}?invite=${inviteToken}`, status: 'pending', guest: { id: guestId, name, email: email || null }, }, diff --git a/backend/src/routes/adminSettings.js b/backend/src/routes/adminSettings.js index 714f8a7b..d3f0383f 100644 --- a/backend/src/routes/adminSettings.js +++ b/backend/src/routes/adminSettings.js @@ -3,6 +3,7 @@ const multer = require('multer'); const path = require('path'); const fs = require('fs').promises; const { body, validationResult } = require('express-validator'); +const validator = require('validator'); const { db, logActivity } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); const { adminAuth } = require('../middleware/auth'); @@ -21,6 +22,7 @@ const { const { sanitizeCss } = require('../utils/cssSanitizer'); const { upsertAppSetting } = require('../utils/appSettings'); const { clearShareLinkSettingsCache } = require('../services/shareLinkService'); +const { invalidateSiteUrlCache, isEnvPinned, envPinnedBase } = require('../utils/frontendUrl'); const { resetSecurityConfigCache } = require('../utils/authSecurity'); const { errorResponse } = require('../utils/routeHelpers'); const logger = require('../utils/logger'); @@ -57,7 +59,12 @@ const RESERVED_SETTING_KEYS = [ // would do neither, leaving galleries handing out archives at the old size. const isReservedSettingKey = (key) => RESERVED_SETTING_KEYS.includes(key) || key.startsWith('oidc_') - || key.startsWith('download_'); + || key.startsWith('download_') + // Derived, read-only fields the GET response adds for the General tab + // (#705). They are computed from the environment, never stored, so a + // round-trip of the GET payload must not create phantom setting rows. + || key === 'general_site_url_env_pinned' + || key === 'general_site_url_effective'; const stripReservedSettingKeys = (settings) => { for (const key of Object.keys(settings)) { if (isReservedSettingKey(key)) delete settings[key]; @@ -240,6 +247,14 @@ router.get('/', adminAuth, requirePermission('settings.view'), async (req, res) // any settings.view holder; setup_token is the first-run bootstrap secret. stripReservedSettingKeys(settingsObject); + // Surface whether FRONTEND_URL pins the public origin (#705). The env var + // OVERRIDES general_site_url, so without this the General tab would offer + // an editable field whose value is silently ignored at runtime. + settingsObject.general_site_url_env_pinned = isEnvPinned(); + if (settingsObject.general_site_url_env_pinned) { + settingsObject.general_site_url_effective = envPinnedBase(); + } + // Mask sensitive secrets before sending to client if (settingsObject.security_recaptcha_secret_key) { settingsObject.security_recaptcha_secret_key = '••••••••'; @@ -833,6 +848,15 @@ router.get('/:type', adminAuth, requirePermission('settings.view'), async (req, // any settings.view holder; setup_token is the first-run bootstrap secret. stripReservedSettingKeys(settingsObject); + // Same derived read-only fields as GET / (#705) — the General tab reads + // through this typed route, so the env-pinned hint must be here too. + if (type === 'general') { + settingsObject.general_site_url_env_pinned = isEnvPinned(); + if (settingsObject.general_site_url_env_pinned) { + settingsObject.general_site_url_effective = envPinnedBase(); + } + } + // Mask sensitive secrets before sending to client if (settingsObject.security_recaptcha_secret_key) { settingsObject.security_recaptcha_secret_key = '••••••••'; @@ -1368,6 +1392,38 @@ router.put('/general', adminAuth, requirePermission('settings.edit'), async (req // rejectUnauthorizedProtectedKeys (403s when a protected key is denied). if (await rejectUnauthorizedProtectedKeys(settings, req, res)) return; + // The public origin is no longer just an email link: it feeds the CORS + // allowlist (server.js) and the Access-Control-Allow-Origin header + // (secureImageMiddleware) since #705. A schemeless value like + // "gallery.example.com" therefore produces both links that don't resolve + // AND an allowlist entry no browser origin can ever match, so validate it + // server-side rather than trusting the input type (#1104). require_tld is + // off on purpose: LAN and NAS installs legitimately run on http://nas:3000 + // or a bare IP. Same validator as oidc_issuer_url above. + if (Object.prototype.hasOwnProperty.call(settings, 'general_site_url')) { + const siteUrl = typeof settings.general_site_url === 'string' + ? settings.general_site_url.trim().replace(/\/+$/, '') + : ''; + // allow_underscores for the same reason require_tld is off: this has to + // accept the addresses LAN and NAS installs actually run on. Browsers + // resolve http://my_nas.local happily and the client-side check accepts + // it, so rejecting it here only produced a mismatch between the two + // validators — and the wizard has no way to show a 400 it did not + // predict (#1104 review round 2). + const looksValid = validator.isURL(siteUrl, { + protocols: ['http', 'https'], + require_protocol: true, + require_tld: false, + allow_underscores: true, + }); + if (siteUrl && !looksValid) { + return res.status(400).json({ + error: 'general_site_url must be an absolute http(s) URL, for example https://gallery.example.com' + }); + } + settings.general_site_url = siteUrl; + } + const publicSiteKeysTouched = Object.keys(settings).some((key) => key.startsWith('general_public_site_')); if (Object.prototype.hasOwnProperty.call(settings, 'general_max_files_per_upload')) { @@ -1463,6 +1519,12 @@ router.put('/general', adminAuth, requirePermission('settings.edit'), async (req if (Object.prototype.hasOwnProperty.call(settings, 'general_short_gallery_urls')) { clearShareLinkSettingsCache(); } + // The public origin is cached (it now sits in per-request CORS paths and + // in a synchronous accessor); drop it immediately on write so a corrected + // site URL takes effect without waiting out the TTL. + if (Object.prototype.hasOwnProperty.call(settings, 'general_site_url')) { + invalidateSiteUrlCache(); + } // Toggling the original-filenames setting (#493) requires busting the // per-event pre-generated zips so the next download-all rebuilds with the // new entry names. Single-photo downloads pick up the change as soon as diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 69d4c1ec..f20b806c 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -501,7 +501,9 @@ const slideshowQrCache = new Map(); // eventId -> { url, dataUrl, at } // origin guests can reach — prefer that whenever the configured base is // missing or loopback. trust proxy is configured, so req.protocol respects // X-Forwarded-Proto behind the standard reverse-proxy setups. -const QR_LOCAL_BASE_RE = /^https?:\/\/(localhost|127\.|0\.0\.0\.0|\[::1\])/i; +// Centralised in utils/frontendUrl (#705) so the QR path and the public-origin +// resolver agree on what counts as a non-shareable base. +const QR_LOCAL_BASE_RE = { test: (v) => require('../utils/frontendUrl').isLoopbackBase(v) }; const QR_ORIGIN_RE = /^https?:\/\/[^\s/]+$/i; async function slideshowQrDataUrl(event, req) { try { diff --git a/backend/src/services/emailService.js b/backend/src/services/emailService.js deleted file mode 100644 index 23686222..00000000 --- a/backend/src/services/emailService.js +++ /dev/null @@ -1,66 +0,0 @@ -const nodemailer = require('nodemailer'); -const { db } = require('../database/db'); -const { emailTemplates } = require('./emailTemplates'); -const logger = require('../utils/logger'); - -// Create transporter -const transporter = nodemailer.createTransport({ - host: process.env.SMTP_HOST, - port: process.env.SMTP_PORT, - secure: process.env.SMTP_SECURE === 'true', - auth: { - user: process.env.SMTP_USER, - pass: process.env.SMTP_PASS - } -}); - -async function sendEmail(to, type, data) { - try { - const template = emailTemplates[type](data); - - const info = await transporter.sendMail({ - from: process.env.EMAIL_FROM, - to: to, - subject: template.subject, - html: template.html, - text: template.text - }); - - logger.info(`Email sent: ${info.messageId}`); - return info; - } catch (error) { - logger.error('Error sending email:', error); - throw error; - } -} - -// Process email queue -async function processEmailQueue() { - const pendingEmails = await db('email_queue') - .where('status', 'pending') - .where('retry_count', '<', 3) - .limit(10); - - for (const email of pendingEmails) { - try { - const emailData = JSON.parse(email.email_data); - await sendEmail(email.recipient_email, email.email_type, emailData); - - await db('email_queue').where('id', email.id).update({ - status: 'sent', - sent_at: new Date() - }); - } catch (error) { - await db('email_queue').where('id', email.id).update({ - retry_count: email.retry_count + 1, - error_message: error.message - }); - } - } -} - -// Start email queue processor -// DISABLED: Using emailProcessor.js instead to prevent duplicate connections -// setInterval(processEmailQueue, 60000); // Process every minute - -module.exports = { sendEmail, processEmailQueue }; diff --git a/backend/src/services/galleryOgService.js b/backend/src/services/galleryOgService.js index 141a528d..95a14c86 100644 --- a/backend/src/services/galleryOgService.js +++ b/backend/src/services/galleryOgService.js @@ -2,6 +2,7 @@ const { db } = require('../database/db'); const logger = require('../utils/logger'); const { ensureThumbnail } = require('./imageProcessor'); const { getStorage } = require('./storage'); +const { getAbsoluteFrontendUrl } = require('../utils/frontendUrl'); const SOCIAL_CRAWLER_PATTERNS = [ /facebookexternalhit/i, @@ -148,8 +149,8 @@ function absoluteUrl(maybeRelative, base) { } } -function frontendBase() { - return (process.env.FRONTEND_URL || 'http://localhost:3000').replace(/\/$/, ''); +async function frontendBase() { + return getAbsoluteFrontendUrl(); } // Render the event date for the OG preview card respecting the @@ -171,7 +172,7 @@ async function formatEventDate(value) { async function buildOgMetadata(slug, requestPath) { const event = await resolveSlug(slug); const branding = await fetchBranding(); - const base = frontendBase(); + const base = await frontendBase(); const siteName = branding.companyName || 'PicPeak'; const logoUrl = absoluteUrl(branding.logoUrl, base) || `${base}/picpeak-logo-transparent.png`; diff --git a/backend/src/services/invoice/payments.js b/backend/src/services/invoice/payments.js index 05b6fdba..81cf9d3b 100644 --- a/backend/src/services/invoice/payments.js +++ b/backend/src/services/invoice/payments.js @@ -7,6 +7,7 @@ const logger = require('../../utils/logger'); const { getAppSetting } = require('../../utils/appSettings'); const { AppError } = require('../../utils/errors'); const { formatShortDate } = require('../../utils/dateFormatter'); +const { getFrontendBaseUrl, DEFAULT_ABSOLUTE_BASE } = require('../../utils/frontendUrl'); const emailProcessor = require('../emailProcessor'); const { ensureInt } = require('../../utils/numericHelpers'); const { formatMajor } = require('./helpers'); @@ -249,9 +250,13 @@ async function queuePaymentCheckEmail(invoiceId, { skipThrottle = false } = {}) const nextLevel = (invoice.reminder_level || 0) + 1; const willChargeFee = reminderFeeMinor > 0 && nextLevel >= 2; - const baseUrl = process.env.FRONTEND_URL + // FRONTEND_URL -> general_site_url (the setup wizard's answer) -> the + // legacy app_frontend_url key -> localhost. Was defaulting to + // https://app.example.com, which shipped a dead placeholder domain into + // customer-facing payment-reminder emails (#705). + const baseUrl = (await getFrontendBaseUrl()) || (await getAppSetting('app_frontend_url')) - || 'https://app.example.com'; + || DEFAULT_ABSOLUTE_BASE; const buildUrl = (action) => `${baseUrl.replace(/\/$/, '')}/payment-check/${token}?action=${action}`; diff --git a/backend/src/services/transferService.js b/backend/src/services/transferService.js index f12fa1be..73d26165 100644 --- a/backend/src/services/transferService.js +++ b/backend/src/services/transferService.js @@ -40,8 +40,9 @@ const UPLOAD_TOKEN_LENGTH = 6; const DAY_MS = 24 * 60 * 60 * 1000; -function getFrontendUrl() { - return (process.env.FRONTEND_URL || 'http://localhost:3000').replace(/\/+$/, ''); +async function getFrontendUrl() { + const { getAbsoluteFrontendUrl } = require('../utils/frontendUrl'); + return getAbsoluteFrontendUrl(); } function generateDownloadToken() { @@ -871,7 +872,7 @@ async function sendTransferEmails(transferId, emails) { const fileCount = (Number(fileCountRow?.c) || 0) + (Number(extraCountRow?.c) || 0); const { sendTemplateEmail } = require('./emailProcessor'); - const downloadUrl = `${getFrontendUrl()}/transfer/${transfer.token}`; + const downloadUrl = `${await getFrontendUrl()}/transfer/${transfer.token}`; const vars = { transfer_title: transfer.title || `Transfer #${transferId}`, message: transfer.message || '', diff --git a/backend/src/services/updateNotificationService.js b/backend/src/services/updateNotificationService.js index 21c07a5b..30b45dec 100644 --- a/backend/src/services/updateNotificationService.js +++ b/backend/src/services/updateNotificationService.js @@ -7,6 +7,7 @@ const { db } = require('../database/db'); const { checkForUpdates } = require('./updateCheckService'); const { sendTemplateEmail, initializeTransporter } = require('./emailProcessor'); const logger = require('../utils/logger'); +const { getAbsoluteFrontendUrl } = require('../utils/frontendUrl'); /** * Get update notification settings from database @@ -128,7 +129,7 @@ async function checkAndNotifyUpdates() { await initializeTransporter(); // Send email to each recipient - const frontendUrl = process.env.FRONTEND_URL || 'http://localhost:3000'; + const frontendUrl = await getAbsoluteFrontendUrl(); const releaseNotesUrl = `https://github.com/PicPeak/picpeak/releases/tag/v${newVersion}`; const channelLabel = updateInfo.channel === 'beta' ? 'Beta' : 'Stable'; diff --git a/backend/src/services/userManagementService.js b/backend/src/services/userManagementService.js index 6fe19083..ed92af72 100644 --- a/backend/src/services/userManagementService.js +++ b/backend/src/services/userManagementService.js @@ -9,6 +9,7 @@ const { db, logActivity } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); const { generateReadablePassword } = require('../utils/passwordGenerator'); const { getBcryptRounds } = require('../utils/passwordValidation'); +const { getAbsoluteFrontendUrl } = require('../utils/frontendUrl'); const { queueEmail } = require('./emailProcessor'); const logger = require('../utils/logger'); const { ConflictError, NotFoundError, ValidationError, ForbiddenError } = require('../utils/errors'); @@ -63,7 +64,14 @@ async function createInvitation({ email, roleId, invitedById, inviterRoleName }) const id = invitationId?.id || invitationId; // Queue invitation email - const frontendUrl = process.env.FRONTEND_URL || process.env.ADMIN_URL || 'http://localhost:3005'; + // Keeps the original FRONTEND_URL-before-ADMIN_URL order: ADMIN_URL is the + // resolver's override, so a split-origin install that points it at a + // separate admin host still wins over the general_site_url setting - it + // must not sit AFTER the resolver, which only returns falsy when nothing at + // all is configured (#1104). Only the terminal fallback changes, from + // localhost:3005 - not even the frontend's port, so an unconfigured install + // mailed admin invites pointing nowhere - to the resolved origin (#705). + const frontendUrl = await getAbsoluteFrontendUrl(null, { override: process.env.ADMIN_URL }); await queueEmail(null, email, 'admin_invitation', { invite_link: `${frontendUrl}/invite/${token}`, role_name: role.display_name, diff --git a/backend/src/utils/frontendUrl.js b/backend/src/utils/frontendUrl.js index 19edd33b..93c7c705 100644 --- a/backend/src/utils/frontendUrl.js +++ b/backend/src/utils/frontendUrl.js @@ -1,27 +1,190 @@ const { db } = require('../database/db'); -const getFrontendBaseUrl = async () => { - let base = (process.env.FRONTEND_URL || '').trim().replace(/\/$/, ''); - if (base) return base; +// Terminal fallback for callers that need an ABSOLUTE url (emails, QR codes, +// payment links). Deliberately not the fallback of getFrontendBaseUrl(): some +// callers (shareLinkService, the SSO redirects in routes/auth) rely on an +// empty base to emit a RELATIVE url, which is the better answer for a +// same-origin redirect. +const DEFAULT_ABSOLUTE_BASE = 'http://localhost:3000'; +// A loopback base is treated as "not configured" so a better answer can win. +// Rationale (#705): docker-compose used to inject +// FRONTEND_URL=http://localhost:3000 unconditionally, so millions of installs +// have it baked into their environment; taking it literally means gallery +// links, QR codes and reminder emails point every recipient at THEIR OWN +// machine. The same guard already existed locally in routes/gallery.js for +// the slideshow QR (#848) and is centralised here. +// +// The host token has to end at a real boundary. Bare prefix matching (which is +// what routes/gallery.js did while this only gated the QR) also demotes +// https://localhost-nas.example.com — a legitimate public host, and now that +// this predicate decides "is this configured" for the entire resolver, being +// demoted means the operator's configured address is silently ignored. +// 127. stays a bare prefix on purpose: all of 127.0.0.0/8 is loopback. +const LOOPBACK_BASE_RE = /^https?:\/\/(localhost(?=[:/?#]|$)|127\.|0\.0\.0\.0(?=[:/?#]|$)|\[::1\])/i; + +const isLoopbackBase = (url) => !!url && LOOPBACK_BASE_RE.test(url); + +const normalise = (val) => (typeof val === 'string' ? val.trim().replace(/\/+$/, '') : ''); + +// Cached copy of the `general_site_url` setting. Two reasons: this now sits in +// per-request paths (CORS headers), and the sync accessor below has no other +// way to see the database. Invalidated explicitly when settings are written; +// the TTL is the backstop for writes that bypass that path (a restore, a +// direct SQL edit), so the documented worst case is CACHE_MS of staleness. +const CACHE_MS = 30_000; +let cache = { value: '', at: 0, primed: false }; + +const readSettingFromDb = async () => { try { const setting = await db('app_settings') .where('setting_key', 'general_site_url') .select('setting_value') .first(); - if (setting && setting.setting_value) { - let val = setting.setting_value; - if (typeof val === 'string') { - try { val = JSON.parse(val); } catch (_) {} - } - if (typeof val === 'string' && val.trim()) { - base = val.trim().replace(/\/$/, ''); - } + if (!setting || !setting.setting_value) return ''; + let val = setting.setting_value; + if (typeof val === 'string') { + try { val = JSON.parse(val); } catch (_) { /* stored as a bare string */ } } - } catch (_) {} - - return base; + return normalise(typeof val === 'string' ? val : ''); + } catch (_) { + return ''; + } }; -module.exports = { getFrontendBaseUrl }; +const getSiteUrlSetting = async () => { + const now = Date.now(); + if (cache.primed && now - cache.at < CACHE_MS) return cache.value; + const value = await readSettingFromDb(); + cache = { value, at: now, primed: true }; + return value; +}; + +// Expire the entry WITHOUT blanking it, then refresh in the background. +// Blanking would leave the synchronous accessor (the CORS allowlists) with no +// value at all until some async caller happened to re-read, which for +// Access-Control-Allow-Origin means falling back to '*' - a wider header than +// the operator configured. A briefly-stale origin is the safer trade. +const invalidateSiteUrlCache = () => { + cache = { ...cache, at: 0 }; + primeSiteUrlCache().catch(() => {}); +}; + +// The origin the request itself arrived on. `trust proxy` is configured in +// server.js, so req.protocol honours X-Forwarded-Proto behind the standard +// reverse proxies. Note frontend/nginx.conf forwards $host with the port +// stripped, so this can lose a non-default port on the split stack — which is +// why the setup wizard persists the browser's own window.location.origin +// instead of relying on this. It stays a last resort, and it is unavailable +// entirely to background jobs (reminder emails) that have no request. +const originFromRequest = (req) => { + if (!req || typeof req.get !== 'function') return ''; + const host = req.get('host'); + if (!host) return ''; + const proto = req.protocol || 'http'; + return normalise(`${proto}://${host}`); +}; + +const envBase = () => normalise(process.env.FRONTEND_URL); + +// The env value ONLY when it actually wins the resolution below — i.e. set and +// not loopback. A loopback FRONTEND_URL is demoted by getFrontendBaseUrl(), so +// reporting it as authoritative would lock the admin UI's Site URL field for +// exactly the operators this exists to unblock: an upgrade that still carries +// the old compose default FRONTEND_URL=http://localhost:3000 must be able to +// configure its public address through the wizard and Settings (#1104). +const envPinnedBase = () => { + const env = envBase(); + return env && !isLoopbackBase(env) ? env : ''; +}; + +// Is the public origin pinned by the environment? The admin UI uses this to +// show the Site URL field as read-only, so an operator never edits a setting +// that an env var is silently overriding. Mirrors the resolver exactly. +const isEnvPinned = () => !!envPinnedBase(); + +/** + * Resolve the public origin, best answer first: + * 1. FRONTEND_URL, when it is not loopback + * 2. `options.override` - a purpose-specific env var (ADMIN_URL, APP_URL) + * the caller passes in, when it is not loopback + * 3. the `general_site_url` setting, when it is not loopback + * 4. the origin this request arrived on, when it is not loopback + * 5. whichever of the above exists at all (covers a genuine localhost install) + * 6. '' - caller decides between a relative url and DEFAULT_ABSOLUTE_BASE + * + * (2) exists because split-origin deployments are supported (API_URL, #798): + * an operator who sets ADMIN_URL for a separate admin host has stated an + * explicit per-purpose intent, and that must beat a value derived from the + * database or from whichever request happened to trigger the email (#1104). + * It sits BELOW FRONTEND_URL to preserve the historic + * `FRONTEND_URL || ADMIN_URL` order those call sites used. + */ +const getFrontendBaseUrl = async (req, options = {}) => { + // Short-circuit before touching the database: a pinned, non-loopback + // FRONTEND_URL is the answer, and this runs in per-request paths. + const env = envBase(); + if (env && !isLoopbackBase(env)) return env; + + const override = normalise(options.override); + if (override && !isLoopbackBase(override)) return override; + + const setting = await getSiteUrlSetting(); + if (setting && !isLoopbackBase(setting)) return setting; + + const request = originFromRequest(req); + if (request && !isLoopbackBase(request)) return request; + + return env || override || setting || request || ''; +}; + +/** Same precedence, for callers that must produce an absolute url. */ +const getAbsoluteFrontendUrl = async (req, options = {}) => + (await getFrontendBaseUrl(req, options)) || DEFAULT_ABSOLUTE_BASE; + +/** + * Synchronous variant for call sites that cannot await (Express middleware + * setting a response header). Sees the environment and the last cached + * setting only - it never reaches the database - so prime the cache at boot + * via primeSiteUrlCache(). + */ +const getFrontendBaseUrlSync = (req) => { + const env = envBase(); + const setting = cache.primed ? cache.value : ''; + const request = originFromRequest(req); + + for (const candidate of [env, setting, request]) { + if (candidate && !isLoopbackBase(candidate)) return candidate; + } + return env || setting || request || ''; +}; + +const primeSiteUrlCache = async () => { + cache = { value: await readSettingFromDb(), at: Date.now(), primed: true }; + return cache.value; +}; + +/** + * Public API origin used for assets that email clients must load. Explicit + * API_URL wins (split-origin deployments); otherwise it is the resolved + * public origin + /api, so the wizard's single answer covers it. + */ +const getApiBaseUrl = async (req) => { + const explicit = normalise(process.env.API_URL); + if (explicit) return explicit; + return `${await getAbsoluteFrontendUrl(req)}/api`; +}; + +module.exports = { + getFrontendBaseUrl, + getAbsoluteFrontendUrl, + getFrontendBaseUrlSync, + getApiBaseUrl, + invalidateSiteUrlCache, + primeSiteUrlCache, + isLoopbackBase, + isEnvPinned, + envPinnedBase, + DEFAULT_ABSOLUTE_BASE, +}; diff --git a/docker-compose.yml b/docker-compose.yml index cf1ca216..540db892 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -59,7 +59,10 @@ services: - SMTP_USER=${SMTP_USER} - SMTP_PASS=${SMTP_PASS} - EMAIL_FROM=${EMAIL_FROM:-noreply@picpeak.local} - - FRONTEND_URL=${FRONTEND_URL:-http://localhost:3000} + # Unset by default (#705): an injected value would always win over the + # `general_site_url` admin setting, so the setup wizard could never + # take effect. Set this only to pin the origin from config-as-code. + - FRONTEND_URL=${FRONTEND_URL:-} # Public API origin for split-origin deployments (#798 SSO redirect_uri). # Empty = same origin as FRONTEND_URL (the standard proxied setup). - API_URL=${API_URL:-} @@ -68,7 +71,7 @@ services: # password login when the IdP is down while SSO-only mode is active. - OIDC_ENCRYPTION_KEY=${OIDC_ENCRYPTION_KEY:-} - OIDC_BREAK_GLASS=${OIDC_BREAK_GLASS:-} - - ADMIN_URL=${ADMIN_URL:-http://localhost:3001} + - ADMIN_URL=${ADMIN_URL:-} - TZ=${TZ:-UTC} - STORAGE_PATH=/app/storage # Watch-folder auto-import: max photos processed in parallel (default 2). diff --git a/docs/single-container.md b/docs/single-container.md index 02672f7f..d9f6a1cc 100644 --- a/docs/single-container.md +++ b/docs/single-container.md @@ -79,8 +79,8 @@ Only `JWT_SECRET` is required. Everything else has a working default. |---|---|---| | `JWT_SECRET` | — | **Required.** Long random string. | | `PORT` | `3000` | Listen port inside the container. | -| `FRONTEND_URL` | — | Public URL. Set it once you are behind a domain, so emails and share links point at the right host. | -| `SMTP_*` | — | Outbound email. Without it, PicPeak runs fine but sends nothing. | +| `FRONTEND_URL` | — | Optional override for the public URL. Normally you set this in the setup wizard instead (it proposes the address you opened), and it is editable later under Settings → General. Setting it here pins the value and makes that field read-only. | +| `SMTP_*` | — | Optional override for outbound email, which is normally configured in the setup wizard / Settings → Email. Without either, PicPeak runs fine but sends nothing. | | `DATABASE_CLIENT` | `sqlite3` | Set to `pg` to use an external PostgreSQL. Required — the image declares `sqlite3`, and the boot resolver treats a declared client as an explicit instruction, so `DB_*` alone will **not** switch engines. | | `DB_HOST`, `DB_USER`, `DB_PASSWORD`, `DB_NAME` | — | Connection details, used when `DATABASE_CLIENT=pg`. | @@ -100,8 +100,9 @@ migrations, exactly as the compose backend does. ## TLS None is included. Terminate TLS in front of it — your NAS's reverse proxy, -Caddy, nginx, or a Cloudflare Tunnel. Set `FRONTEND_URL` to the public -`https://…` address so generated links match. +Caddy, nginx, or a Cloudflare Tunnel. Put the public `https://…` address in +Settings → General (or re-run the setup wizard) so generated links match — +`FRONTEND_URL` does the same thing but pins it outside the admin UI. ## NAS notes diff --git a/frontend/src/components/admin/SetupConfigStep.tsx b/frontend/src/components/admin/SetupConfigStep.tsx index b6a3a200..1ecec084 100644 --- a/frontend/src/components/admin/SetupConfigStep.tsx +++ b/frontend/src/components/admin/SetupConfigStep.tsx @@ -7,9 +7,13 @@ import { Button, Input } from '../common'; import type { FeatureKey } from '../../services/featureFlags.service'; import { businessProfileService } from '../../services/businessProfile.service'; import { emailService, type EmailConfig } from '../../services/email.service'; +import { settingsService } from '../../services/settings.service'; +import { isAbsoluteHttpUrl } from '../../utils/url'; -// Features that need working SMTP to deliver anything. -const EMAIL_FEATURES: FeatureKey[] = ['reminderEmails', 'incomingMail', 'whatsapp', 'bills']; +// Email is NOT feature-gated (#705): a gallery-only install still mails the +// gallery link, guest invites and expiry warnings through the same +// email_configs row, so hiding SMTP behind the CRM-ish features left the most +// basic install unable to deliver anything. interface Props { selectedFeatures: Set; @@ -18,12 +22,13 @@ interface Props { // Lean per-feature config, shown after the "How will you use PicPeak?" step. // Only the sections a selected feature actually needs are rendered; everything -// else keeps its seeded defaults and is tunable later in Settings. Saving is -// best-effort per section — a failure never traps the user on setup. +// else keeps its seeded defaults and is tunable later in Settings. Every field +// is optional — "Skip for now" always leaves — but a section the user DID fill +// in is validated before it is posted, and a save that fails keeps them on the +// step with their input intact rather than advancing into a silent data loss. export const SetupConfigStep: React.FC = ({ selectedFeatures, onDone }) => { const { t } = useTranslation(); const showInvoicing = selectedFeatures.has('bills'); - const showEmail = EMAIL_FEATURES.some((f) => selectedFeatures.has(f)); const [saving, setSaving] = useState(false); const [inv, setInv] = useState({ @@ -33,14 +38,97 @@ export const SetupConfigStep: React.FC = ({ selectedFeatures, onDone }) = const [mail, setMail] = useState({ smtp_host: '', smtp_port: '587', smtp_user: '', smtp_pass: '', from_email: '', from_name: '', }); + // Prefilled with the address the admin actually reached the wizard on, which + // on a NAS or LAN install is the one thing no default can guess (#705). + const [siteUrl, setSiteUrl] = useState(window.location.origin.replace(/\/+$/, '')); + const [errors, setErrors] = useState>({}); const invField = (k: keyof typeof inv) => (e: React.ChangeEvent) => setInv((p) => ({ ...p, [k]: e.target.value })); const mailField = (k: keyof typeof mail) => (e: React.ChangeEvent) => setMail((p) => ({ ...p, [k]: e.target.value })); - const finish = async () => { + // Persist the public origin on BOTH paths — skipping the optional invoicing + // and SMTP sections must not also skip the address that every gallery link, + // QR code and reminder email is built from. + // + // Throws rather than swallowing: PUT /general applies its own URL check, and + // a rejection here means the wizard would otherwise finish reporting success + // with no public address stored at all — the silent misconfiguration this + // whole change exists to remove. Callers decide what to do with it. + const saveSiteUrl = async () => { + const value = siteUrl.trim().replace(/\/+$/, ''); + if (!value) return; + await settingsService.updateSettings({ general_site_url: value }); + }; + + const siteUrlRejected = () => t( + 'setup.config.siteUrlRejected', + 'The server rejected this address. Use the full origin, for example https://gallery.example.com or http://192.168.1.50:3000.', + ); + + // Blocking validation, run before anything is posted. Everything on this + // step is optional, but a value that IS filled in has to be usable: the + // public address feeds the CORS allowlist (#705), and /admin/email/config + // rejects a config whose from_email isn't a valid address — which used to + // surface as a generic warning while the wizard advanced anyway, throwing + // away every SMTP value including the password (#1104). + const validate = () => { + const next: Record = {}; + if (siteUrl.trim() && !isAbsoluteHttpUrl(siteUrl)) { + next.siteUrl = t('setup.config.siteUrlInvalid', 'Enter the full address including http:// or https://, for example https://gallery.example.com'); + } + if (mail.smtp_host.trim()) { + const from = mail.from_email.trim(); + if (!from) { + next.from_email = t('setup.config.fromEmailRequired', 'A From address is required when an SMTP host is set.'); + } else if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(from)) { + next.from_email = t('setup.config.fromEmailInvalid', 'Enter a valid email address.'); + } + const port = parseInt(mail.smtp_port, 10); + if (!Number.isInteger(port) || port < 1 || port > 65535) { + next.smtp_port = t('setup.config.smtpPortInvalid', 'Enter a port between 1 and 65535.'); + } + } + setErrors(next); + return Object.keys(next).length === 0; + }; + + const skip = async () => { + if (siteUrl.trim() && !isAbsoluteHttpUrl(siteUrl)) { + setErrors({ siteUrl: t('setup.config.siteUrlInvalid', 'Enter the full address including http:// or https://, for example https://gallery.example.com') }); + return; + } + setErrors({}); setSaving(true); + // "Skip for now" always leaves, by contract — but say so rather than + // dropping the address without a word. + try { + await saveSiteUrl(); + } catch { + toast.warn(siteUrlRejected()); + } + setSaving(false); + onDone(); + }; + + const finish = async () => { + if (!validate()) return; + setSaving(true); + let failed = false; + + // Separate from the block below so the message lands on the address field + // instead of the generic "some settings could not be saved" warning. The + // server check is stricter than isAbsoluteHttpUrl in places, so this is + // reachable even after validate() has passed. + try { + await saveSiteUrl(); + } catch { + setSaving(false); + setErrors((prev) => ({ ...prev, siteUrl: siteUrlRejected() })); + return; + } + try { // Invoicing: only persist if they actually started filling it in. if (showInvoicing && inv.companyName.trim()) { @@ -64,7 +152,7 @@ export const SetupConfigStep: React.FC = ({ selectedFeatures, onDone }) = } } // Email: only persist if a host was entered. - if (showEmail && mail.smtp_host.trim()) { + if (mail.smtp_host.trim()) { const port = parseInt(mail.smtp_port, 10) || 587; const config: EmailConfig = { smtp_host: mail.smtp_host.trim(), @@ -78,20 +166,39 @@ export const SetupConfigStep: React.FC = ({ selectedFeatures, onDone }) = }; await emailService.updateConfig(config); } - } catch (_) { - toast.warn(t('setup.config.saveFailed', 'Some settings could not be saved — you can finish them in Settings.')); + } catch { + // Stay on the step: advancing here discarded everything the user typed, + // the SMTP password included, with no way back to re-enter it (#1104). + failed = true; + toast.warn(t('setup.config.saveFailed', 'Some settings could not be saved — check the values below, or use “Skip for now” and finish in Settings.')); } finally { setSaving(false); - onDone(); } + if (!failed) onDone(); }; return (

- {t('setup.config.intro', 'A few details for the features you picked. Anything you skip keeps its default and can be set later in Settings.')} + {t('setup.config.intro', 'A few details to finish setting up. Anything you skip keeps its default and can be set later in Settings.')}

+
+

+ {t('setup.config.siteUrl', 'Public address')} +

+

+ {t('setup.config.siteUrlHint', 'Where your clients will reach this gallery. Prefilled with the address you opened right now — change it if you will put PicPeak behind a domain or reverse proxy. You can update this any time in Settings → General.')} +

+ setSiteUrl(e.target.value)} + error={errors.siteUrl} + /> +
+ {showInvoicing && (

{t('setup.config.invoicing', 'Invoicing details')}

@@ -119,27 +226,33 @@ export const SetupConfigStep: React.FC = ({ selectedFeatures, onDone }) =
)} - {showEmail && ( -
-

{t('setup.config.email', 'Email delivery (SMTP)')}

-

{t('setup.config.emailHint', 'Required to send reminders, invoices and notifications.')}

+
+

{t('setup.config.email', 'Email delivery (SMTP)')}

+

{t('setup.config.emailHint', 'Used to send gallery links to your clients, plus guest invites, expiry warnings and any reminders or invoices you enable. Leave blank to set it up later in Settings → Email.')}

- +
- +
-
- )} +
-