feat(security): no-issue: add security.requireHttps setting to reject non-HTTPS clients#10154
feat(security): no-issue: add security.requireHttps setting to reject non-HTTPS clients#10154passcod wants to merge 5 commits into
Conversation
… non-HTTPS clients Adds a boolean `security.requireHttps` setting at both the central and per-facility scopes (default false). When enabled, middleware on the central and facility servers rejects any request that did not arrive over HTTPS — detected via `X-Forwarded-Proto`, which is honoured through the existing loopback trust-proxy config — with a 403. The index/health route stays accessible over HTTP. To avoid locking everyone out, the admin settings endpoint refuses to enable `security.requireHttps` unless the request enabling it is itself over HTTPS, proving HTTPS reaches the server before it becomes mandatory. This is the server-side mitigation companion to the cleartext token-storage fallback used by HTTP-only deployments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1c18005. Configure here.
…al/scoped setting Adds `security.requireHttps` at the global scope and drops the hard `false` default from the central and facility scopes, so the setting is unset by default and resolves through the normal settings cascade. The global value acts as the default for every server (one toggle to require HTTPS everywhere), while a central or facility value overrides it in either direction for the complex per-site case. Unset everywhere means HTTPS is not required. Also adds a spec describing the subsystem under specs/administration/settings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dd enforcement tests The enable-guard used a strict `=== true` check, which let a truthy non-boolean (e.g. `1`) slip past while the middleware would still enforce on it. Switch to a truthy check so the guard matches enforcement. Add a facility-server integration test that enables the setting and confirms plain-HTTP requests are rejected while HTTPS requests pass, and strengthen the central admin test to read back the persisted value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary Below consensus threshold (9 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
…andshakes WebSocket upgrades attach to the HTTP server directly and never pass through the Express middleware stack, so the requireHttps middleware did not cover them — a cleartext client could still open a WebSocket while ordinary HTTP requests were rejected. Add a Socket.IO allowRequest guard on both servers that refuses a handshake which did not arrive over HTTPS when security.requireHttps is in effect. Protocol detection reuses the server's compiled Express trust-proxy function, so X-Forwarded-Proto is honoured exactly as it is for HTTP requests (and spoofed values from untrusted peers are ignored). Fails closed if the setting cannot be read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cover central enforcement Cap the per-facility setting reads in the requireHttps check with a small async-pool concurrency limit instead of an unbounded Promise.all, per the project's finite-DB-pool convention. The all-or-nothing semantics are kept deliberately, so a settings-read failure still fails closed rather than letting a request through over plain HTTP. Add a central-server enforcement test block (global scope) confirming the middleware returns 403 for non-HTTPS requests and allows HTTPS requests, mirroring the facility-server coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Changes
🤖 Server-side mitigation companion to #9942 (cleartext token-storage fallback for HTTP-only deployments). For deployments where HTTPS is available, this lets operators forbid non-HTTPS client access.
Setting:
security.requireHttps(boolean), defined at the global, central, and per-facility scopes with no default — unset everywhere means HTTPS is not required.Cascade / override semantics: the effective value resolves through the normal settings cascade, so a central or facility value overrides the global value in either direction.
Enforcement: when in effect, a request that did not arrive over HTTPS is rejected with
403before reaching any route. Detection is viaX-Forwarded-Proto(honoured through the existing loopbacktrust proxyconfig); the index/health route stays reachable over HTTP. A facility server hosting multiple facilities enforces if the requirement is in effect for any hosted facility.Anti-lockout guard: the admin settings endpoint refuses to enable
security.requireHttpsunless the enabling request is itself HTTPS — proving HTTPS reaches the server before it becomes mandatory. (Uses a truthy check so a non-boolean truthy value cannot slip past the guard yet still trigger enforcement.)Spec: adds
specs/administration/settings/require-https.mddescribing the subsystem.Operational note (in the setting description): enforcement relies on
proxy.trustedbeing configured (defaultloopback) and the TLS-terminating proxy forwardingX-Forwarded-Proto.Tests: unit tests for the middleware decision logic (both
req.settingsshapes, enforce-if-any, error propagation); a facility-server integration test that enables the setting and confirms HTTP is rejected / HTTPS passes; and central admin guard tests (enable blocked over HTTP, allowed + persisted over HTTPS). Unit + settings-schema tests pass locally; the server integration tests were not run in my local worktree (symlinked deps / unbuilt shared dist) and will run in CI.Auto-Deploy
Options
Tests
Review Hero
.github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.Remember to...