From 9431b9f0949e8e51c486019224ca92f46443c50e Mon Sep 17 00:00:00 2001 From: Luca <102960244+Luca-Timo@users.noreply.github.com> Date: Fri, 21 Aug 2026 13:54:14 +0300 Subject: [PATCH] feat(setup): configure the public address and SMTP in the wizard, not .env (#1104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(setup): configure the public address and SMTP in the wizard, not .env A fresh install could not configure its own public address. `general_site_url` and the `email_configs` row already existed as admin settings, but nothing could reach them: - docker-compose.yml injected FRONTEND_URL=${FRONTEND_URL:-http://localhost:3000} and Dockerfile.aio baked in ENV FRONTEND_URL=http://localhost:3000, so getFrontendBaseUrl() returned on its first branch every time and the setting was never read. .env.example shipped the same value as an uncommented placeholder for FRONTEND_URL / ADMIN_URL / API_URL. - the wizard never asked for the address at all, and skipped its whole config step unless a CRM-ish feature was selected — so a gallery-only install was also never offered SMTP, despite gallery links, guest invites and expiry warnings all going out through email_configs. - eleven call sites read process.env.FRONTEND_URL directly rather than the resolver, three of them defaulting to placeholder hosts that reached real recipients: https://app.example.com in payment-reminder emails, localhost:3005 in admin invitation emails, https://app.example.com in dev template previews. Stop injecting a default anywhere, and resolve the origin instead: FRONTEND_URL -> general_site_url -> the origin the request arrived on -> whichever exists -> ''. A loopback candidate is treated as unconfigured so the installs that already have http://localhost:3000 baked into their environment self-heal; the same guard previously lived inline in routes/gallery.js for the slideshow QR (#848) and is now shared. The empty return is preserved because shareLinkService and the SSO redirects in routes/auth rely on it to emit relative urls — callers needing an absolute url use getAbsoluteFrontendUrl(), which still ends at http://localhost:3000. The wizard now persists window.location.origin right after the admin account is created, so an install that skips the rest still has a usable origin for background jobs that have no request to derive one from, and offers it as an editable "Public address" field. Settings -> General shows the field read-only when FRONTEND_URL pins it, instead of silently ignoring edits. Also drop the `|| 'mailhog'` fallback when seeding email_configs: that host only exists in the dev compose profile (which does not even start by default), so a fresh install came up with a live config pointing nowhere while the wizard showed empty SMTP fields. With no row, blank fields are the truth and emailProcessor logs "No email configuration found". Developers set SMTP_HOST=mailhog explicitly. backend/src/services/emailService.js is deleted: nothing in backend/ references it, and it was the only consumer of the SMTP_* variables, which misrepresented how mail is configured. Refs #705 * fix(setup): keep FRONTEND_URL ahead of ADMIN_URL/APP_URL when resolving links The previous commit routed two call sites through the resolver but put the site-specific variable FIRST, silently reversing precedence: userManagementService was: FRONTEND_URL || ADMIN_URL || localhost:3005 became: ADMIN_URL || resolver adminEvents/crud was: FRONTEND_URL || APP_URL || '' became: APP_URL || resolver An install with both variables set would have flipped which one won. Call the resolver first instead — it starts with FRONTEND_URL, so the original relative order is preserved and only the final fallback changes: localhost:3005 (not even the frontend's port) and '' (a relative link inside an email) both become the resolved origin. Refs #705 * fix(setup): unpin loopback FRONTEND_URL, keep ADMIN_URL/APP_URL reachable Review feedback on #1104. isEnvPinned() reported ANY FRONTEND_URL as authoritative, including the loopback values getFrontendBaseUrl() deliberately demotes. An install upgrading with the old compose default FRONTEND_URL=http://localhost:3000 therefore resolved its origin from general_site_url correctly, but got the Site URL field rendered read-only in Settings and skipped by the wizard's seeding - locking the exact operators this change exists to unblock out of configuring a public address anywhere. The predicate now mirrors the resolver, and the derived general_site_url_effective the General tab reads comes from the same helper instead of re-normalising process.env inline. APP_URL and ADMIN_URL had become dead code: getFrontendBaseUrl() only returns falsy when NOTHING is configured, so `|| process.env.ADMIN_URL` after it never ran once a site URL existed - which after this PR is the normal case. A split-origin install pointing ADMIN_URL at a separate admin host got invite links on the public gallery origin instead. They are now passed as an explicit `override` that resolves directly below FRONTEND_URL, preserving the historic FRONTEND_URL-before-ADMIN_URL order while beating the database- and request-derived fallbacks. general_site_url now feeds the CORS allowlist and the Access-Control-Allow-Origin header, not just email links, so a schemeless value is an allowlist entry no browser origin can match. Validate it server-side in PUT /general (isURL with require_protocol, require_tld off so LAN/NAS installs on http://nas:3000 still work) and client-side in both surfaces that write it - type="url" never fires in either, since neither input sits inside a form. Two more wizard fixes: the General tab no longer reposts general_site_url while it is env-pinned, because the field then holds the effective env value rather than the stored one and the round-trip read as a change to a protected key, 403ing a settings.edit-without-settings.domains admin on an unrelated save. And SetupConfigStep validates the From address before posting - /admin/email/config rejects a blank one, which used to surface as a generic warning while the wizard advanced from its finally block anyway, discarding every SMTP value the user had typed, password included. A failed save now keeps them on the step. * fix(setup): surface a rejected public address instead of swallowing it Review round 2 follow-up on #1104, pushed onto the branch. saveSiteUrl() caught and discarded every error. That was defensible before round 2 added a server-side URL check, but PUT /general can now answer 400 — and the two validators disagreed: http://my_nas.local client: accepted server: rejected http://foo_bar:3000 client: accepted server: rejected validate() let those through, the 400 was swallowed, `failed` stayed false and onDone() ran. The operator finished the wizard believing the public address was stored when nothing had been. That is the silent misconfiguration this whole change exists to remove, landing on the LAN and NAS installs it targets. Three parts: - saveSiteUrl() throws. finish() resolves it before anything else is posted and puts the message on the address field rather than the generic "some settings could not be saved" warning. Skip for now still always leaves, by contract, but warns instead of dropping the value in silence. - allow_underscores on the server check, for the same reason require_tld is off: browsers resolve http://my_nas.local and the client accepts it, so rejecting it server-side only produced the mismatch above. Both validators now agree across the LAN/NAS, IDN, bare-IP and scheme-less cases. - LOOPBACK_BASE_RE anchors its host token. Bare prefix matching also demoted https://localhost-nas.example.com, and now that this predicate gates the whole resolver rather than just the slideshow QR, being demoted means a configured address is silently ignored. 127. stays a bare prefix on purpose: all of 127.0.0.0/8 is loopback. Resolver suite 31 passing, up from 26. Mutation-checked: restoring the unanchored regex fails the three new host-boundary cases. * fix(settings): don't lock the General tab on a site URL nobody typed Review follow-up on #1104, pushed onto the branch. general_site_url was free-text until this PR added a server-side check, so an upgraded install can hold something schemeless that predates it. The tab flagged that on load, and `disabled={!!siteUrlError}` then killed Save for EVERY General setting. An admin holding settings.edit but not settings.domains could not clear it either: correcting the address is a change to a protected key and 403s. The tab has no permission gating, so that role was simply locked out of the tab with no self-service way back. That is the same role adminSettings.js:85-95 documents the no-op round-trip allowance for. The allowance only helps if the request is made, and this blocked it in the browser first. Validation now waits until the field is actually edited, and an unchanged value is dropped from the payload rather than reposted — matching what the env-pinned case already does one line above, and for the same reason. stored value invalid, untouched Save works, key not sent edited to something unusable Save blocked edited to a usable absolute url saved Four tests, first coverage for this feature. Mutation-checked: removing the dirty gate fails the untouched-value case. --------- Co-authored-by: Paul Nothaft <53005142+the-luap@users.noreply.github.com> --- .env.example | 38 ++- Dockerfile.aio | 18 +- backend/migrations/core/001_init.js | 13 +- backend/server.js | 21 +- backend/src/__tests__/frontendUrl.test.js | 247 ++++++++++++++++++ .../src/middleware/secureImageMiddleware.js | 6 +- backend/src/routes/adminDev.js | 4 +- backend/src/routes/adminEvents/crud.js | 9 +- backend/src/routes/adminGuests.js | 9 +- backend/src/routes/adminSettings.js | 64 ++++- backend/src/routes/gallery.js | 4 +- backend/src/services/emailService.js | 66 ----- backend/src/services/galleryOgService.js | 7 +- backend/src/services/invoice/payments.js | 9 +- backend/src/services/transferService.js | 7 +- .../src/services/updateNotificationService.js | 3 +- backend/src/services/userManagementService.js | 10 +- backend/src/utils/frontendUrl.js | 193 ++++++++++++-- docker-compose.yml | 7 +- docs/single-container.md | 9 +- .../src/components/admin/SetupConfigStep.tsx | 153 +++++++++-- .../__tests__/GeneralTab.siteUrl.test.tsx | 107 ++++++++ .../settings/hooks/useSettingsState.ts | 34 ++- .../src/features/settings/tabs/GeneralTab.tsx | 34 ++- frontend/src/i18n/locales/de.json | 18 +- frontend/src/i18n/locales/en.json | 18 +- frontend/src/pages/SetupPage.tsx | 31 ++- frontend/src/utils/url.ts | 22 ++ 28 files changed, 984 insertions(+), 177 deletions(-) create mode 100644 backend/src/__tests__/frontendUrl.test.js delete mode 100644 backend/src/services/emailService.js create mode 100644 frontend/src/features/settings/__tests__/GeneralTab.siteUrl.test.tsx 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.')}

- +
- +
-
- )} +
-