From e8b6fcb3fc2c17345962e0889c15e1a6fb65ebac Mon Sep 17 00:00:00 2001 From: NGPixel Date: Sun, 6 Sep 2026 06:47:41 -0400 Subject: [PATCH] fix: make trustProxy changes instant + clarify security settings requiring restart --- backend/api/assets.ts | 4 ++-- backend/api/system.ts | 4 ++-- backend/index.ts | 34 ++++++++++++++++++++++++---- backend/locales/en.json | 4 ++-- backend/models/security.ts | 9 +++++--- frontend/src/pages/AdminSecurity.vue | 26 ++++++++++++++++++--- 6 files changed, 64 insertions(+), 17 deletions(-) diff --git a/backend/api/assets.ts b/backend/api/assets.ts index 1819a0b14..88dfdaf60 100644 --- a/backend/api/assets.ts +++ b/backend/api/assets.ts @@ -53,8 +53,8 @@ async function routes(app: FastifyInstance) { // parses, so the JSON routes below are unaffected. // // The limit is read once, here, because a route's body limit is fixed when it is registered — - // changing it in the admin area takes effect on the next restart, as the rest of the security - // settings do. + // changing it in the admin area takes effect on the next restart, which is why that field is + // one of the ones the security view marks as needing one. app.addContentTypeParser( '*', { parseAs: 'buffer', bodyLimit: WIKI.config.security?.uploadMaxFileSize ?? 10485760 }, diff --git a/backend/api/system.ts b/backend/api/system.ts index f6668484d..894c82ba6 100644 --- a/backend/api/system.ts +++ b/backend/api/system.ts @@ -314,7 +314,7 @@ async function routes(app: FastifyInstance) { schema: { summary: 'Get the security configuration', description: - 'Most of this is applied when the HTTP server starts, so changing it takes effect on the next restart.', + 'Response headers, CSP, HSTS, CORS and `uploadMaxFileSize` are applied when the HTTP server starts, so changing those takes effect on the next restart. The rest is read per request.', tags: ['System'], response: { 200: { $ref: 'SecurityConfig#' } @@ -338,7 +338,7 @@ async function routes(app: FastifyInstance) { schema: { summary: 'Update the security configuration', description: - 'Accepts any subset of the fields. Header, CORS and proxy settings are read when the HTTP server starts and therefore apply after a restart.', + 'Accepts any subset of the fields. Response headers, CSP, HSTS, CORS and `uploadMaxFileSize` are read when the HTTP server starts and therefore apply after a restart; `trustProxy`, `forceAssetDownload` and the rate limit are read per request and apply at once.', tags: ['System'], body: { $ref: 'SecurityConfig#' }, response: { diff --git a/backend/index.ts b/backend/index.ts index 021c14751..fcc0caa0b 100644 --- a/backend/index.ts +++ b/backend/index.ts @@ -304,9 +304,31 @@ async function initHTTPServer() { logger: { level: 'error' }, - // -> `securityTrustProxy` was the 2.x name: the setting is `trustProxy`, so this read never - // matched and the option was permanently off no matter what the admin area showed - trustProxy: WIKI.config.security.trustProxy ?? false, + /* + A function rather than the boolean itself, so that the setting is answered per request instead + of being baked in here: the value fastify is handed at construction can never change, and this + is the one security setting whose effect an administrator checks immediately — an audit entry + or a rate limit recorded against the proxy rather than the visitor. Turning it on and being + told to restart before it means anything is how a wrong address gets read as a bug. + + Fastify only asks whether a given hop is trusted, and it asks per hop per request, so + returning false for all of them is exactly the `false` behaviour: `proxy-addr` truncates the + chain at the socket, and `req.ip` / `req.host` / `req.protocol` come from the connection and + the Host header as they would with the option off. The truthiness of the function is what + matters at construction time, not the setting. + + Read through the `WIKI` global on every call, and that is load-bearing rather than incidental: + an HA instance that did not serve the save learns about it from the `reloadConfig` event, whose + handler REPLACES `WIKI.config` with a merged copy rather than mutating it. Hoisting this to a + captured `WIKI.config.security` would keep working on the instance the administrator happened to + hit and silently freeze on every other one. Verified across two instances sharing a database. + + Note that `true` trusts the whole chain and therefore takes the LEFTMOST `X-Forwarded-For` + entry, which a client can put anything it likes into. That is the meaning of the setting as it + stands; a deployment where clients can reach the wiki without passing the proxy needs a hop + count or a CIDR list, which this setting cannot express yet. + */ + trustProxy: () => WIKI.config.security?.trustProxy === true, routerOptions: { ignoreTrailingSlash: true } @@ -354,8 +376,10 @@ async function initHTTPServer() { // Security // ---------------------------------------- - // -> Every setting below comes from the admin area's security view. They are read once, here, so a - // change takes effect on the next restart — the view says as much. + // -> Every setting below comes from the admin area's security view, and every one of them is read + // once, here, so a change takes effect on the next restart. Not every setting in that view is: + // `trustProxy` above and `forceAssetDownload` and the rate limit elsewhere are read per request, + // which is why the view marks the restart on the individual options rather than on the page. const security = WIKI.config.security app.register(fastifyHelmet, { diff --git a/backend/locales/en.json b/backend/locales/en.json index 51afa0412..7cade8c6c 100644 --- a/backend/locales/en.json +++ b/backend/locales/en.json @@ -929,7 +929,7 @@ "admin.security.rateLimitRecommended": "Keeping rate limiting enabled is highly recommended: without it, passwords, second factors and page passwords can be guessed as fast as the server will answer.", "admin.security.rateLimitWindow": "Time Window", "admin.security.rateLimitWindowHint": "How long attempts are counted over, as a duration — 30s, 5m, 2h, 1d. Going over the limit within it earns a ban.", - "admin.security.restartRequired": "Header, CORS and proxy settings are applied when the server starts, so they take effect after a restart.", + "admin.security.restartRequired": "Takes effect after a server restart.", "admin.security.saveFailed": "Failed to save the security configuration.", "admin.security.saveSuccess": "Security configuration updated successfully.", "admin.security.scanSVG": "Scan and Sanitize SVG Uploads", @@ -941,7 +941,7 @@ "admin.security.trustProxyHint": "Should be enabled when using a reverse-proxy like nginx, apache, CloudFlare, etc in front of Wiki.js. Turn off otherwise.", "admin.security.uploads": "Uploads", "admin.security.uploadsInfo": "These settings only affect Wiki.js. If you're using a reverse-proxy (e.g. nginx, Apache, Cloudflare), you must also change its settings to match.", - "admin.security.uploadsNotEnforced": "These limits are saved but not enforced yet: uploading is not implemented.", + "admin.security.uploadsNotEnforced": "The batch limit and the SVG scan are saved but not enforced yet. The maximum file size is.", "admin.security.warn": "Make sure to understand the implications before turning on / off a security feature.", "admin.sites.activate": "Activate Site", "admin.sites.activateConfirm": "Are you sure you want activate site {siteTitle}? The site will become accessible to users with read access.", diff --git a/backend/models/security.ts b/backend/models/security.ts index 53badf67b..6a15039cb 100644 --- a/backend/models/security.ts +++ b/backend/models/security.ts @@ -28,9 +28,12 @@ const DURATION_PATTERN = /^\d+[smhdwy]$/ /** * Security model * - * The admin area's security view, which is exactly the `security` settings blob. Most of it is read - * when the HTTP server starts — see the `Security` section of `index.ts` — so saving here takes - * effect on the next restart. + * The admin area's security view, which is exactly the `security` settings blob. Saving does not mean + * the same thing for all of it, which is why the view marks the restart per option rather than once + * at the top: the response headers, CSP, HSTS and CORS are read by the `Security` section of + * `index.ts` when the HTTP server starts, and `uploadMaxFileSize` by the upload route's body limit + * when it is registered, so those take effect on the next restart. `trustProxy`, the rate limit + * fields and `forceAssetDownload` are read per request and apply as soon as they are saved. */ class Security { /** diff --git a/frontend/src/pages/AdminSecurity.vue b/frontend/src/pages/AdminSecurity.vue index 751c3966f..e247bafe8 100644 --- a/frontend/src/pages/AdminSecurity.vue +++ b/frontend/src/pages/AdminSecurity.vue @@ -5,7 +5,9 @@
-
{{ t('admin.security.title') }}
+
+ {{ t('admin.security.title') }} +
{{ t('admin.security.subtitle') }}
@@ -57,8 +59,6 @@
{{ t('admin.security.warn') }}
- -
{{ t('admin.security.restartRequired') }}
 @@ -69,6 +69,10 @@ {{ t(`admin.security.disallowIframe`) }} {{ t(`admin.security.disallowIframeHint`) }} + + + {{ t('admin.security.restartRequired') }} + {{ t(`admin.security.enforceSameOriginReferrerPolicyHint`) }} + + + {{ t('admin.security.restartRequired') }} + {{ t(`admin.security.enforceHsts`) }} {{ t(`admin.security.enforceHstsHint`) }} + + + {{ t('admin.security.restartRequired') }} + {{ t(`admin.security.maxUploadSize`) }} {{ t(`admin.security.maxUploadSizeHint`) }} + + + {{ t('admin.security.restartRequired') }} + {{ t(`admin.security.corsMode`) }} {{ t(`admin.security.corsModeHint`) }} + + + {{ t('admin.security.restartRequired') }} +