From 5b7e34eb67d35aa91dd3fe3818525caa42cfeca5 Mon Sep 17 00:00:00 2001 From: Jamie Curnow Date: Mon, 9 Nov 2026 23:39:26 +1000 Subject: [PATCH] Fix second round of review findings from PR #6029 Security - Check advanced_config permissions before creating a certificate on host update (proxy, redirection and 404 hosts). A refused request previously requested a Let's Encrypt certificate first, leaving an orphan certificate and using up rate limit. - Only trust X-NPM-Peer-Addr when it comes with a secret generated at container start. The admin nginx sends both headers. Any proxy host or stream forwarding to 127.0.0.1:3000 was also a loopback peer, so it could forge the header and get a fresh rate-limit bucket on every request. Without the secret (dev, or a mounted production.conf), the header is ignored. - Add a login limit per IP across all accounts (30 failed attempts a minute) alongside the per-account one. Before this, one address could spray passwords over any number of accounts. Behind a shared proxy, everyone using that proxy shares this limit. - Stop trusting X-Real-IP from loopback. It was only there for an older version of the rate limiter, and it let a stream host forwarding to a local proxy host forge the client IP and get past IP access lists. Loopback now behaves as it does on master. Correctness - Refuse to delete a certificate that hosts still use (400, with counts per host type). Their configs include its files, and a successful revoke deleted them anyway, which broke nginx -t. Otherwise the certificate is revoked and its files are removed even if the revoke fails. - Let non-admins clear a location's advanced config. Deleting the location was already allowed and has the same effect, and a location deleted and re-added at the same path arrives with an empty config, which was refused. - Clear proxy host grants (meta.proxy_host_ids) when visibility isn't "user" or proxy hosts are hidden, and drop ids of deleted hosts. Before, hidden grants came back when proxy hosts were shown again. The backend enforces this; the permissions modal also stops sending them. - 404 host and redirection host modals now wait for the current user to load, so an admin's advanced config change isn't dropped if they submit before it loads. Tests - NonAdminHosts.cy.js: refused change doesn't create a certificate, location rename and clear rules, grant clearing. - Certificates.cy.js: deleting a certificate that is in use. --- backend/internal/certificate.js | 37 ++-- backend/internal/dead-host.js | 5 +- backend/internal/host.js | 16 +- backend/internal/proxy-host.js | 5 +- backend/internal/redirection-host.js | 5 +- backend/internal/user.js | 34 +++- backend/lib/express/rate-limit.js | 30 ++- backend/routes/tokens.js | 19 +- .../nginx/certificates/certID/delete.json | 20 ++ docker/rootfs/etc/nginx/conf.d/dev.conf | 4 +- .../etc/nginx/conf.d/production.conf.template | 4 +- docker/rootfs/etc/nginx/nginx.conf | 4 - .../s6-rc.d/prepare/45-admin-port.sh | 13 +- frontend/src/modals/DeadHostModal.tsx | 12 +- frontend/src/modals/PermissionsModal.tsx | 8 +- frontend/src/modals/RedirectionHostModal.tsx | 12 +- test/cypress/e2e/api/Certificates.cy.js | 73 +++++++ test/cypress/e2e/api/NonAdminHosts.cy.js | 184 ++++++++++++++++++ 18 files changed, 423 insertions(+), 62 deletions(-) create mode 100644 test/cypress/e2e/api/NonAdminHosts.cy.js diff --git a/backend/internal/certificate.js b/backend/internal/certificate.js index 56be24e47..0c3166a60 100644 --- a/backend/internal/certificate.js +++ b/backend/internal/certificate.js @@ -566,7 +566,24 @@ const internalCertificate = { throw new error.ItemNotFoundError(data.id); } - const inUse = hostRelations.some((rel) => row[rel]?.length); + // A certificate that hosts still use can't be deleted: their rendered configs include its + // files, and revoking it would leave them serving a revoked certificate. Hosts may belong + // to other users, so only counts are reported. + const labels = { + proxy_hosts: "proxy host", + redirection_hosts: "redirection host", + dead_hosts: "404 host", + streams: "stream", + }; + const usage = hostRelations + .map((rel) => ({ rel, count: row[rel]?.length || 0 })) + .filter((u) => u.count > 0) + .map((u) => `${u.count} ${labels[u.rel]}${u.count === 1 ? "" : "s"}`); + if (usage.length) { + throw new error.ValidationError( + `Certificate is in use by ${usage.join(", ")}. Remove it from them before deleting it.`, + ); + } for (const rel of hostRelations) { delete row[rel]; } @@ -586,20 +603,10 @@ const internalCertificate = { }); if (row.provider === "letsencrypt") { - // Revoke the cert - const revoked = await internalCertificate.revokeLetsEncryptSsl(row, true).then( - () => true, - () => false, - ); - // Certbot leaves the files behind when it can't revoke (e.g. a broken renewal config). - // Remove them anyway, unless a host still uses this certificate: its rendered config - // includes these files, and removing them would fail nginx -t on the next reload. - if (!revoked && inUse) { - logger.warn( - `Certificate #${row.id} was deleted but could not be revoked, and hosts still use it; its files in /etc/letsencrypt were kept`, - ); - return true; - } + // Revoke the cert. Errors are logged by revokeLetsEncryptSsl. + await internalCertificate.revokeLetsEncryptSsl(row); + // Certbot leaves the files behind when it can't revoke (e.g. a broken renewal config), + // so remove them either way. No host uses them; that was checked above. // The row is already deleted and audited, so leftover files are logged, not thrown: // an error here would report a completed delete as failed, and a retry would 404. const leftover = internalCertificate.removeLetsEncryptFiles(row.id); diff --git a/backend/internal/dead-host.js b/backend/internal/dead-host.js index 41291ed79..5e249de59 100644 --- a/backend/internal/dead-host.js +++ b/backend/internal/dead-host.js @@ -132,6 +132,9 @@ const internalDeadHost = { ); } + // Before the certificate is created, so a refused request doesn't leave one behind + internalHost.assertAdvancedConfigAllowed(access, data, row); + if (createCertificate) { const cert = await internalCertificate.createQuickCertificate(access, { domain_names: data.domain_names || row.domain_names, @@ -153,8 +156,6 @@ const internalDeadHost = { thisData = internalHost.cleanSslHstsData(thisData, row); - internalHost.assertAdvancedConfigAllowed(access, data, row); - // do the row update await deadHostModel.query().where({ id: data.id }).patch(data); diff --git a/backend/internal/host.js b/backend/internal/host.js index 76329b43c..7bdaa7837 100644 --- a/backend/internal/host.js +++ b/backend/internal/host.js @@ -45,10 +45,12 @@ const internalHost = { }, /** - * For non-admins: every location's advanced_config must be one that already exists on this - * host. Matching is by value, not path, so renaming or reordering a location that carries - * admin-written config is allowed, but new or edited config is not. Each existing config can - * be used once (no duplicating it onto other paths), and same-path matches are taken first. + * For non-admins: every location's advanced_config must be empty or one that already exists + * on this host. Matching is by value, not path, so renaming or reordering a location that + * carries admin-written config is allowed, but new or edited config is not. Each existing + * config can be used once (no duplicating it onto other paths), and same-path matches are + * taken first. Clearing a config is allowed: deleting the location is, and has the same + * effect, and a location deleted and re-added at the same path arrives with an empty config. * A location that omits advanced_config keeps the one at its path, unless another location * explicitly carries it. Mutates `locations`. * @@ -86,10 +88,10 @@ const internalHost = { } } - // Pass 2: renamed/moved locations may carry an existing config not already claimed. - // Clearing a config is not allowed, as before. + // Pass 2: renamed/moved locations may carry an existing config not already claimed, or + // none at all for (const loc of unresolved) { - if (!loc.advanced_config || !take(loc.advanced_config)) { + if (loc.advanced_config && !take(loc.advanced_config)) { deny(); } } diff --git a/backend/internal/proxy-host.js b/backend/internal/proxy-host.js index 5065e082e..4682a94fa 100644 --- a/backend/internal/proxy-host.js +++ b/backend/internal/proxy-host.js @@ -157,6 +157,9 @@ const internalProxyHost = { ); } + // Before the certificate is created, so a refused request doesn't leave one behind + internalHost.assertAdvancedConfigAllowed(access, thisData, row); + if (createCertificate) { return internalCertificate .createQuickCertificate(access, { @@ -185,8 +188,6 @@ const internalProxyHost = { thisData = internalHost.cleanSslHstsData(thisData, row); - internalHost.assertAdvancedConfigAllowed(access, thisData, row); - return proxyHostModel .query() .where({ id: thisData.id }) diff --git a/backend/internal/redirection-host.js b/backend/internal/redirection-host.js index f7ad6be33..f5945a6b8 100644 --- a/backend/internal/redirection-host.js +++ b/backend/internal/redirection-host.js @@ -157,6 +157,9 @@ const internalRedirectionHost = { ); } + // Before the certificate is created, so a refused request doesn't leave one behind + internalHost.assertAdvancedConfigAllowed(access, thisData, row); + if (createCertificate) { return internalCertificate .createQuickCertificate(access, { @@ -185,8 +188,6 @@ const internalRedirectionHost = { thisData = internalHost.cleanSslHstsData(thisData, row); - internalHost.assertAdvancedConfigAllowed(access, thisData, row); - return redirectionHostModel .query() .where({ id: thisData.id }) diff --git a/backend/internal/user.js b/backend/internal/user.js index e27ac2111..0c0313b44 100644 --- a/backend/internal/user.js +++ b/backend/internal/user.js @@ -3,6 +3,7 @@ import _ from "lodash"; import errs from "../lib/error.js"; import utils from "../lib/utils.js"; import authModel from "../models/auth.js"; +import proxyHostModel from "../models/proxy_host.js"; import userModel from "../models/user.js"; import userPermissionModel from "../models/user_permission.js"; import internalAuditLog from "./audit-log.js"; @@ -460,7 +461,8 @@ const internalUser = { .query() .where("user_id", user.id) .first() - .then((existing_auth) => { + .then(async (existing_auth) => { + permData.meta = await internalUser.normalisePermissionMeta(existing_auth, permData); if (existing_auth) { // patch return userPermissionModel @@ -489,6 +491,36 @@ const internalUser = { }); }, + /** + * The permission meta to store. Proxy host grants (meta.proxy_host_ids) only apply while + * visibility is "user" and proxy hosts are not hidden, so they're cleared otherwise; kept, + * they would silently come back when proxy hosts are shown again. Ids of hosts that no + * longer exist are dropped. `permData` may be partial, so the stored values fill the gaps. + * + * @param {Object} [existing] the stored user_permission row + * @param {Object} permData incoming permission fields + * @returns {Promise} + */ + normalisePermissionMeta: async (existing, permData) => { + const effective = _.assign( + { visibility: "user", proxy_hosts: "hidden" }, + existing || {}, + _.omit(permData, ["meta"]), + ); + const meta = _.assign({}, existing?.meta || {}, permData.meta || {}); + const requested = Array.isArray(meta.proxy_host_ids) ? _.uniq(meta.proxy_host_ids) : []; + + if (effective.visibility !== "user" || effective.proxy_hosts === "hidden" || !requested.length) { + meta.proxy_host_ids = []; + return meta; + } + + const rows = await proxyHostModel.query().select("id").where("is_deleted", 0).whereIn("id", requested); + const found = new Set(rows.map((r) => r.id)); + meta.proxy_host_ids = requested.filter((id) => found.has(id)); + return meta; + }, + /** * @param {Access} access * @param {Object} data diff --git a/backend/lib/express/rate-limit.js b/backend/lib/express/rate-limit.js index 512519df9..7512f7485 100644 --- a/backend/lib/express/rate-limit.js +++ b/backend/lib/express/rate-limit.js @@ -28,21 +28,37 @@ const cleanupTimer = setInterval(() => { }, 120000); cleanupTimer.unref(); +// Generated at container start (prepare/45-admin-port.sh) and sent by the admin nginx as +// X-NPM-Peer-Secret alongside X-NPM-Peer-Addr. Unset (development, or a mounted production.conf) +// means the header is never trusted. +const peerSecret = Buffer.from(process.env.NPM_PEER_SECRET || ""); + +const hasPeerSecret = (value) => { + if (!peerSecret.length || typeof value !== "string") { + return false; + } + const given = Buffer.from(value); + return given.length === peerSecret.length && crypto.timingSafeEqual(given, peerSecret); +}; + /** * The address to rate limit on: the TCP peer of the admin nginx, which it passes as - * X-NPM-Peer-Addr ($realip_remote_addr), only honoured when the socket is loopback (that nginx, - * in this container). The realip-resolved client address is deliberately not used: nginx trusts - * X-Real-IP from private ranges, so a client on the LAN or Docker network could pick a new - * address, and a fresh bucket, on every request. Behind another proxy this resolves to that - * proxy's address, which is why limiters also key on the target account. + * X-NPM-Peer-Addr ($realip_remote_addr). That header is only honoured from a loopback socket + * carrying the startup secret, so it can only have come from the admin nginx: a proxy host or + * stream forwarding to the backend port is also a loopback peer, but can't know the secret. + * The realip-resolved client address is deliberately not used: nginx trusts X-Real-IP from + * private ranges, so a client on the LAN or Docker network could pick a new address, and a fresh + * bucket, on every request. Behind another proxy (including an NPM proxy host in front of the + * admin UI) this resolves to that proxy's address. * * @param {Object} req * @returns {String} */ export const getClientIp = (req) => { const socketAddr = req.socket?.remoteAddress || "unknown"; - const isTrustedLocalProxy = socketAddr === "127.0.0.1" || socketAddr === "::1" || socketAddr === "::ffff:127.0.0.1"; - const peerAddr = isTrustedLocalProxy ? req.headers["x-npm-peer-addr"]?.trim() : null; + const isLoopback = socketAddr === "127.0.0.1" || socketAddr === "::1" || socketAddr === "::ffff:127.0.0.1"; + const trusted = isLoopback && hasPeerSecret(req.headers["x-npm-peer-secret"]); + const peerAddr = trusted ? req.headers["x-npm-peer-addr"]?.trim() : null; return peerAddr || socketAddr; }; diff --git a/backend/routes/tokens.js b/backend/routes/tokens.js index 7879cb9c1..f56daa058 100644 --- a/backend/routes/tokens.js +++ b/backend/routes/tokens.js @@ -6,10 +6,9 @@ import apiValidator from "../lib/validator/api.js"; import { debug, express as logger } from "../logger.js"; import { getValidationSchema } from "../schema/index.js"; -// Keyed on IP + target account rather than IP alone: when the admin UI sits behind another -// proxy every client can resolve to the same IP, and an IP-only bucket would let anyone lock -// out every user with a handful of bad logins. Brute force against one account is still -// capped per connecting address (see getClientIp). +// Two limits apply to logins. Per IP + target account: when the admin UI sits behind another +// proxy every client can resolve to the same IP, and an IP-only bucket at this size would let +// anyone lock out every user with a handful of bad logins. const tokenRateLimiter = createRateLimiter({ windowMs: 60 * 1000, max: 10, @@ -21,6 +20,16 @@ const tokenRateLimiter = createRateLimiter({ }, }); +// And per IP across all accounts, so one address can't spray passwords over many accounts at +// 10 a minute each. Only failures count. Behind a shared proxy this cap is shared by everyone +// using it, which is why it's higher than the per-account one. +const tokenIpRateLimiter = createRateLimiter({ + windowMs: 60 * 1000, + max: 30, + message: "Too many login attempts. Please try again in 1 minute.", + skipSuccessfulRequests: true, +}); + // Keyed on the user the challenge belongs to, not the challenge token itself, so fetching // fresh challenge tokens doesn't reset the code-guessing budget. The payload is read without // verifying the signature; verify2FA does that. A forged token only affects buckets for the @@ -79,7 +88,7 @@ router * * Create a new Token */ - .post(tokenRateLimiter, async (req, res, next) => { + .post(tokenIpRateLimiter, tokenRateLimiter, async (req, res, next) => { try { const data = await apiValidator(getValidationSchema("/tokens", "post"), req.body); const result = await internalToken.getTokenFromEmail(data); diff --git a/backend/schema/paths/nginx/certificates/certID/delete.json b/backend/schema/paths/nginx/certificates/certID/delete.json index a99f619d6..2c38c388c 100644 --- a/backend/schema/paths/nginx/certificates/certID/delete.json +++ b/backend/schema/paths/nginx/certificates/certID/delete.json @@ -35,6 +35,26 @@ } } } + }, + "400": { + "description": "400 response", + "content": { + "application/json": { + "examples": { + "default": { + "value": { + "error": { + "code": 400, + "message": "Certificate is in use by 1 proxy host. Remove it from them before deleting it." + } + } + } + }, + "schema": { + "$ref": "../../../../components/error.json" + } + } + } } } } diff --git a/docker/rootfs/etc/nginx/conf.d/dev.conf b/docker/rootfs/etc/nginx/conf.d/dev.conf index c54f2a46e..57008f9a0 100644 --- a/docker/rootfs/etc/nginx/conf.d/dev.conf +++ b/docker/rootfs/etc/nginx/conf.d/dev.conf @@ -17,8 +17,8 @@ server { proxy_set_header X-Forwarded-Scheme $scheme; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Forwarded-For $remote_addr; - # The TCP peer before realip processing; the backend rate limits on this because - # X-Real-IP can be set by any client in the trusted private ranges + # The TCP peer before realip processing. In development there is no peer secret + # (see production.conf.template), so the backend ignores this and limits on the socket proxy_set_header X-NPM-Peer-Addr $realip_remote_addr; proxy_pass http://127.0.0.1:3000/; diff --git a/docker/rootfs/etc/nginx/conf.d/production.conf.template b/docker/rootfs/etc/nginx/conf.d/production.conf.template index 83293ebd3..be7aca196 100644 --- a/docker/rootfs/etc/nginx/conf.d/production.conf.template +++ b/docker/rootfs/etc/nginx/conf.d/production.conf.template @@ -18,8 +18,10 @@ server { proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Forwarded-For $remote_addr; # The TCP peer before realip processing; the backend rate limits on this because - # X-Real-IP can be set by any client in the trusted private ranges + # X-Real-IP can be set by any client in the trusted private ranges. The secret, generated + # at startup, is how the backend knows this header came from here. proxy_set_header X-NPM-Peer-Addr $realip_remote_addr; + proxy_set_header X-NPM-Peer-Secret "{{NPM_PEER_SECRET}}"; proxy_pass http://127.0.0.1:3000/; proxy_read_timeout 15m; diff --git a/docker/rootfs/etc/nginx/nginx.conf b/docker/rootfs/etc/nginx/nginx.conf index 608c1c1d8..1b25cb5fc 100644 --- a/docker/rootfs/etc/nginx/nginx.conf +++ b/docker/rootfs/etc/nginx/nginx.conf @@ -83,10 +83,6 @@ http { # Real IP Determination - # Loopback: lets an NPM proxy host forwarding to 127.0.0.1 (e.g. the admin UI on :81) - # pass the real client IP through instead of every request appearing as 127.0.0.1 - set_real_ip_from 127.0.0.1; - set_real_ip_from ::1; # Local subnets: set_real_ip_from 10.0.0.0/8; set_real_ip_from 172.16.0.0/12; # Includes Docker subnet diff --git a/docker/rootfs/etc/s6-overlay/s6-rc.d/prepare/45-admin-port.sh b/docker/rootfs/etc/s6-overlay/s6-rc.d/prepare/45-admin-port.sh index 3807c7d76..32252c727 100644 --- a/docker/rootfs/etc/s6-overlay/s6-rc.d/prepare/45-admin-port.sh +++ b/docker/rootfs/etc/s6-overlay/s6-rc.d/prepare/45-admin-port.sh @@ -16,7 +16,16 @@ if ! [[ "$NPM_ADMIN_PORT" =~ ^[0-9]+$ ]]; then fi PRODFILE="/etc/nginx/conf.d/production.conf" -SED_REGEX="s/\{\{NPM_ADMIN_PORT\}\}/${NPM_ADMIN_PORT}/g" + +# A new secret on every start. The admin nginx sends it with X-NPM-Peer-Addr, and the backend +# only trusts that header when the secret matches, so a proxy host or stream forwarding to the +# backend port can't pass a forged one through. +NPM_PEER_SECRET="$(head -c 32 /dev/urandom | od -An -tx1 | tr -d ' \n')" +if [ "${#NPM_PEER_SECRET}" -ne 64 ]; then + log_fatal "Failed to generate the backend peer secret" +fi + +SED_REGEX="s/\{\{NPM_ADMIN_PORT\}\}/${NPM_ADMIN_PORT}/g;s/\{\{NPM_PEER_SECRET\}\}/${NPM_PEER_SECRET}/g" if is_mounted "$PRODFILE"; then echo "WARNING: skipping ${PRODFILE} — mounted file" >&2 @@ -24,6 +33,8 @@ elif [ -f "$PRODFILE.template" ]; then if sed -E "$SED_REGEX" "$PRODFILE.template" > "$PRODFILE" && [ -s "$PRODFILE" ]; then # success log_info "Generated ${PRODFILE} from template" + # Hand the secret to the backend; with-contenv loads this directory into its environment + printf '%s' "$NPM_PEER_SECRET" > /var/run/s6/container_environment/NPM_PEER_SECRET else log_fatal "Failed to generate ${PRODFILE} from template" fi diff --git a/frontend/src/modals/DeadHostModal.tsx b/frontend/src/modals/DeadHostModal.tsx index ed375952e..3c432e976 100644 --- a/frontend/src/modals/DeadHostModal.tsx +++ b/frontend/src/modals/DeadHostModal.tsx @@ -25,7 +25,7 @@ interface Props extends InnerModalProps { id: number | "new"; } const DeadHostModal = EasyModal.create(({ id, visible, remove }: Props) => { - const { data: currentUser } = useUser("me"); + const { data: currentUser, isLoading: userIsLoading, error: userError } = useUser("me"); const { data, isLoading, error } = useDeadHost(id); const { mutate: setDeadHost } = useSetDeadHost(); const [errorMsg, setErrorMsg] = useState(null); @@ -63,17 +63,17 @@ const DeadHostModal = EasyModal.create(({ id, visible, remove }: Props) => { return ( <> - {(isLoading || (error && !data)) && ( + {(isLoading || userIsLoading || ((error || userError) && (!data || !currentUser))) && ( - {!isLoading && error && ( + {!isLoading && (error || userError) && ( - {error?.message || "Unknown error"} + {error?.message || userError?.message || "Unknown error"} )} - {isLoading && } + {(isLoading || userIsLoading) && } )} - {!isLoading && data && ( + {!isLoading && !userIsLoading && data && currentUser && ( { setIsSubmitting(true); setErrorMsg(null); try { - await setPermissions(id, values); + // Proxy host grants only apply to "user" visibility with proxy hosts shown. Don't + // submit stale ones; the backend clears them too. + const grantsApply = values.visibility === "user" && values.proxyHosts !== "hidden"; + await setPermissions(id, { + ...values, + meta: { ...values.meta, proxy_host_ids: grantsApply ? values.meta?.proxy_host_ids || [] : [] }, + }); remove(); queryClient.invalidateQueries({ queryKey: ["users"] }); queryClient.invalidateQueries({ queryKey: ["user"] }); diff --git a/frontend/src/modals/RedirectionHostModal.tsx b/frontend/src/modals/RedirectionHostModal.tsx index 78134c454..d83a81bcf 100644 --- a/frontend/src/modals/RedirectionHostModal.tsx +++ b/frontend/src/modals/RedirectionHostModal.tsx @@ -27,7 +27,7 @@ interface Props extends InnerModalProps { id: number | "new"; } const RedirectionHostModal = EasyModal.create(({ id, visible, remove }: Props) => { - const { data: currentUser } = useUser("me"); + const { data: currentUser, isLoading: userIsLoading, error: userError } = useUser("me"); const { data, isLoading, error } = useRedirectionHost(id); const { mutate: setRedirectionHost } = useSetRedirectionHost(); const [errorMsg, setErrorMsg] = useState(null); @@ -65,17 +65,17 @@ const RedirectionHostModal = EasyModal.create(({ id, visible, remove }: Props) = return ( <> - {(isLoading || (error && !data)) && ( + {(isLoading || userIsLoading || ((error || userError) && (!data || !currentUser))) && ( - {!isLoading && error && ( + {!isLoading && (error || userError) && ( - {error?.message || "Unknown error"} + {error?.message || userError?.message || "Unknown error"} )} - {isLoading && } + {(isLoading || userIsLoading) && } )} - {!isLoading && data && ( + {!isLoading && !userIsLoading && data && currentUser && ( { }); }); + it('Certificate in use by a host cannot be deleted', () => { + cy.task('backendApiPost', { + token: token, + path: '/api/nginx/certificates', + data: { + provider: 'other', + nice_name: 'In Use Certificate', + }, + }).then((cert) => { + cy.task('backendApiPostFiles', { + token: token, + path: `/api/nginx/certificates/${cert.id}/upload`, + files: { + certificate: certFile, + certificate_key: keyFile, + }, + }).then(() => { + cy.task('backendApiPost', { + token: token, + path: '/api/nginx/proxy-hosts', + data: { + domain_names: ['cert-in-use.example.com'], + forward_scheme: 'http', + forward_host: '1.1.1.1', + forward_port: 80, + access_list_id: '0', + certificate_id: cert.id, + meta: { dns_challenge: false }, + advanced_config: '', + locations: [], + block_exploits: false, + caching_enabled: false, + allow_websocket_upgrade: false, + http2_support: false, + hsts_enabled: false, + hsts_subdomains: false, + ssl_forced: false, + }, + }).then((host) => { + cy.task('backendApiDelete', { + token: token, + path: `/api/nginx/certificates/${cert.id}`, + returnOnError: true, + }).then((data) => { + cy.validateSwaggerSchema('delete', 400, '/nginx/certificates/{certID}', data); + expect(data.error.code).to.equal(400); + expect(data.error.message).to.contain('in use by 1 proxy host'); + }); + + // Still there + cy.task('backendApiGet', { + token: token, + path: `/api/nginx/certificates/${cert.id}`, + }).then((data) => { + expect(data.id).to.equal(cert.id); + }); + + // Once the host no longer uses it, it can be deleted + cy.task('backendApiDelete', { + token: token, + path: `/api/nginx/proxy-hosts/${host.id}`, + }); + cy.task('backendApiDelete', { + token: token, + path: `/api/nginx/certificates/${cert.id}`, + }).then((data) => { + expect(data).to.be.equal(true); + }); + }); + }); + }); + }); + it('Request Certificate - CVE-2024-46256/CVE-2024-46257', () => { cy.task('backendApiPost', { token: token, diff --git a/test/cypress/e2e/api/NonAdminHosts.cy.js b/test/cypress/e2e/api/NonAdminHosts.cy.js new file mode 100644 index 000000000..c05eca842 --- /dev/null +++ b/test/cypress/e2e/api/NonAdminHosts.cy.js @@ -0,0 +1,184 @@ +/// + +describe('Non-admin host and permission rules', () => { + const ADMIN_CONFIG = 'add_header X-Admin-Only "1";'; + const nonAdmin = { + email: 'nonadmin-hosts@example.com', + secret: 'changeme', + }; + + let token; + let userId; + let userToken; + let hostId; + + const hostPayload = (overrides) => Object.assign({ + domain_names: ['nonadmin-hosts.example.com'], + forward_scheme: 'http', + forward_host: '1.1.1.1', + forward_port: 80, + access_list_id: '0', + certificate_id: 0, + meta: { dns_challenge: false }, + advanced_config: '', + locations: [], + block_exploits: false, + caching_enabled: false, + allow_websocket_upgrade: false, + http2_support: false, + hsts_enabled: false, + hsts_subdomains: false, + ssl_forced: false, + }, overrides || {}); + + const location = (path, advancedConfig) => ({ + path: path, + forward_scheme: 'http', + forward_host: '1.1.1.1', + forward_port: 8080, + advanced_config: advancedConfig, + }); + + const getPermissions = () => cy.task('backendApiGet', { + token: token, + path: `/api/users/${userId}?expand=permissions`, + }).then((user) => user.permissions); + + before(() => { + cy.resetUsers(); + cy.getToken().then((tok) => { + token = tok; + + // Admin-owned host with an admin-written location config + cy.task('backendApiPost', { + token: token, + path: '/api/nginx/proxy-hosts', + data: hostPayload({ locations: [location('/api', ADMIN_CONFIG)] }), + }).then((host) => { + hostId = host.id; + + cy.task('backendApiPost', { + token: token, + path: '/api/users', + data: { + name: 'Non Admin', + nickname: 'NonAdmin', + email: nonAdmin.email, + roles: [], + auth: { type: 'password', secret: nonAdmin.secret }, + }, + }).then((user) => { + userId = user.id; + cy.task('backendApiPut', { + token: token, + path: `/api/users/${userId}/permissions`, + data: { + visibility: 'user', + proxy_hosts: 'manage', + certificates: 'manage', + meta: { proxy_host_ids: [hostId] }, + }, + }); + cy.getToken(null, { identity: nonAdmin.email, secret: nonAdmin.secret }).then((tok2) => { + userToken = tok2; + }); + }); + }); + }); + }); + + it('Refused advanced config change does not request a certificate first', () => { + cy.task('backendApiGet', { token: token, path: '/api/nginx/certificates' }).then((before) => { + cy.task('backendApiPut', { + token: userToken, + path: `/api/nginx/proxy-hosts/${hostId}`, + data: { certificate_id: 'new', advanced_config: 'return 418;' }, + returnOnError: true, + }).then((data) => { + expect(data).to.have.property('error'); + expect(data.error.code).to.equal(403); + cy.task('backendApiGet', { token: token, path: '/api/nginx/certificates' }).then((after) => { + expect(after.length).to.equal(before.length); + }); + }); + }); + }); + + it('Non-admin cannot add new location advanced config', () => { + cy.task('backendApiPut', { + token: userToken, + path: `/api/nginx/proxy-hosts/${hostId}`, + data: { locations: [location('/api', ADMIN_CONFIG), location('/new', 'return 418;')] }, + returnOnError: true, + }).then((data) => { + expect(data).to.have.property('error'); + expect(data.error.code).to.equal(403); + }); + }); + + it('Non-admin can rename a location and keep its admin config', () => { + cy.task('backendApiPut', { + token: userToken, + path: `/api/nginx/proxy-hosts/${hostId}`, + data: { locations: [location('/v2', ADMIN_CONFIG), location('/api', '')] }, + }).then((data) => { + const byPath = Object.fromEntries(data.locations.map((l) => [l.path, l.advanced_config])); + expect(byPath['/v2']).to.equal(ADMIN_CONFIG); + expect(byPath['/api']).to.equal(''); + }); + }); + + it('Non-admin can delete and re-add a location at the same path', () => { + // The location form sends advanced_config "" for a new row + cy.task('backendApiPut', { + token: userToken, + path: `/api/nginx/proxy-hosts/${hostId}`, + data: { locations: [location('/v2', '')] }, + }).then((data) => { + expect(data.locations).to.have.length(1); + expect(data.locations[0].advanced_config).to.equal(''); + }); + }); + + it('Proxy host grants are cleared when proxy hosts are hidden', () => { + // Unknown ids are dropped + cy.task('backendApiPut', { + token: token, + path: `/api/users/${userId}/permissions`, + data: { proxy_hosts: 'manage', meta: { proxy_host_ids: [hostId, 999999] } }, + }); + getPermissions().then((perms) => { + expect(perms.meta.proxy_host_ids).to.deep.equal([hostId]); + }); + + cy.task('backendApiPut', { + token: token, + path: `/api/users/${userId}/permissions`, + data: { proxy_hosts: 'hidden' }, + }); + getPermissions().then((perms) => { + expect(perms.meta.proxy_host_ids).to.deep.equal([]); + }); + + // Showing proxy hosts again does not bring the old grant back + cy.task('backendApiPut', { + token: token, + path: `/api/users/${userId}/permissions`, + data: { proxy_hosts: 'manage' }, + }); + getPermissions().then((perms) => { + expect(perms.meta.proxy_host_ids).to.deep.equal([]); + }); + }); + + it('Proxy host grants are cleared when visibility is all', () => { + cy.task('backendApiPut', { + token: token, + path: `/api/users/${userId}/permissions`, + data: { visibility: 'all', proxy_hosts: 'manage', meta: { proxy_host_ids: [hostId] } }, + }); + getPermissions().then((perms) => { + expect(perms.meta.proxy_host_ids).to.deep.equal([]); + }); + }); +});