feat(security): production hardening for security, audit trails and testing - #222
Merged
Conversation
…esting
Adds the enterprise controls the offline desktop app was missing, without
touching any Epic, FHIR, HL7, SMART-on-FHIR or server-tier module. Changes to
shared infrastructure that Epic traffic passes through were kept additive and
are each covered by a test asserting Epic payloads still flow unchanged.
Controls added:
- Electron hardening: sandbox on the main window (config now passed via
additionalArguments since a sandboxed preload cannot require local modules),
permission denial handlers, webview/popup blocking, stricter CSP directives
- IPC trust boundary: sender validation (trusted window/frame/origin) and
argument validation (prototype pollution, depth, size). FHIR/HL7 channels
deliberately get no per-channel schema so Epic payloads pass untouched
- Audit trail tamper-evidence: keyed HMAC-SHA256 alongside the existing hash
chain, with the key in OS secure storage; the test-key override is gated to
NODE_ENV=test and unpackaged builds and fails closed everywhere else
- Database-enforced immutability for access_justification_logs and
electronic_signatures; PRAGMA secure_delete = ON
- Inspection-ready audit export (CSV/HTML) with an embedded integrity verdict
- Secure deletion wired into every site that deletes a database file or
credential, and local tampering detection over critical main-process files
- OS screen-lock/suspend now ends the session immediately, closing the gap
where a deliberate workstation lock left a live session and rendered PHI
behind the lock screen for the rest of the idle window
Pre-existing defects fixed:
- Audit chain verification reported false tampering: writer and verifier
disagreed on the canonical payload and ordered rows by UUID. Both now share
auditCanonical.cjs
- hasPermission() threw on inherited property names ('constructor') instead of
failing closed
- Deleting a user could abort or launder Part 11 signature records; users with
attribution records are now deactivated rather than hard-deleted
- The audit key upgrade path was unreachable, silently losing the HMAC key when
a keyring appeared after first run
- Backup rotation orphaned -wal/-shm sidecars, leaving database pages on disk
after the backup had aged out of retention
- The plaintext database wipe during encryption migration missed its WAL
sidecar, retaining plaintext PHI
Testing: 12 new suites (259 assertions) plus 25 Playwright assertions against
the running app. npm test now runs a suite runner reporting per-suite results
instead of a 34-command && chain. CI runs the security and hardening groups as
a named step so a compliance failure is distinguishable from a functional one.
Verified: 37/37 Node suites, 39/39 Playwright assertions (including the 14
pre-existing login/patient/audit/backup/restore tests), lint and typecheck clean.
Controls, Epic impact statement and residual risk are documented in
docs/security/PRODUCTION_READINESS_HARDENING.md.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged three high-severity js/file-system-race alerts (CWE-367) in the new audit key loader. It branched on fs.existsSync(keyPath) and then read or wrote that path, so the file could be swapped for a symlink or an attacker-chosen key between the check and the operation. For an audit HMAC key that is decisive: substituting the key lets an attacker re-HMAC a forged chain. All key file I/O now goes through descriptors: - the read opens 'r' and treats ENOENT as the "no key yet" signal rather than checking existence first - creation uses 'wx' (O_CREAT|O_EXCL) with the mode applied by the open, so it cannot clobber a key another process just wrote and needs no separate chmod - losing the O_EXCL race adopts the winner's key instead of overwriting it, which would have orphaned the audit rows that process had already written Two further defects surfaced while fixing this: - the first attempt re-sealed the legacy key by truncating in place, so a crash between truncate and write would have emptied the file and lost the audit key permanently, leaving every existing row HMAC-unverifiable. The re-seal now writes a temp file and renames it over the target, which is atomic - the re-seal has to run after the read descriptor is closed, because Windows refuses to rename over a file with an open handle and the upgrade would have silently failed on the primary deployment platform Adds 3 assertions covering the race-safety properties, the concurrent-creation adoption path, and that no temp file survives a re-seal. Verified: 37/37 Node suites (auditKeyGating now 39 assertions), 39/39 Playwright assertions against the running app, lint and typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the enterprise security, audit-trail, testing and operational-readiness controls the offline desktop app was missing, and fixes six latent defects found along the way.
No Epic, FHIR, HL7, SMART-on-FHIR or
server/module was modified. Five changes touch shared infrastructure that Epic traffic passes through; each was kept additive and is covered by a test asserting Epic payloads still flow unchanged. The full impact statement is indocs/security/PRODUCTION_READINESS_HARDENING.md§1.Controls added
sandbox: trueon the main window (config now arrives viaadditionalArguments, since a sandboxed preload cannotrequire()local modules), permission denial handlers, webview/popup blocking, stricter CSP directives.NODE_ENV=teston unpackaged builds and fails closed everywhere else.access_justification_logsandelectronic_signatures;PRAGMA secure_delete = ON.Pre-existing defects fixed
hasPermission()threw on inherited property names ('constructor') instead of failing closed-wal/-shmsidecarsTesting
12 new suites (259 assertions) plus 25 Playwright assertions against the running app.
npm testnow uses a suite runner reporting per-suite results instead of a 34-command&&chain, and fails if a listed suite is missing from disk. CI runs the security and hardening groups as a named step so a compliance failure is distinguishable from a functional regression.Test plan
npm test— 37/37 Node suites passnpm run test:e2e— 39/39 Playwright assertions pass against the launched app, including the 14 pre-existing login → patient → audit → encrypted backup → restore tests (the strongest evidence no core workflow regressed, and they exercise the paths where secure deletion was newly wired in)npm run test:securityandnpm run test:hardeningnpm run lintandnpx tsc -p jsconfig.json --noEmitcleanpowerMonitordoes not deliverlock-screenon every Linux desktopNotes for the reviewer
record_hmactoaudit_logs) and writes a fresh integrity baseline manifest. Both are automatic; the monitor rebaselines on version change rather than alarming.connect-srcin the static<meta>CSP was deliberately left wide to preserve Epic remote mode. The main-process response header is the binding narrow policy in a packaged build. Rationale and the regression test that detects the header being dropped are in §1.errorLogger.cjsis intentionally not secure-deleted — those files are non-PHI by design and enforced as such by existing tests.