fix(core): security hardening — auth fail-closed, arg injection, secret file perms - #28
Merged
Merged
Conversation
…erms Security batch (audit findings C1, C2, H1, H2, H3, H6, H7, M4): - C1 auth: empty whitelist now FAILS CLOSED (denies all) instead of authorizing everyone. Opt into open access with the new auth.allowAllSenders flag (local dev only). Paired users are still authorized via access_control even with an empty whitelist. BREAKING: V0 configs relying on an empty whitelist for open access must now set auth.allowAllSenders=true. - C2 email: strip path components from attacker-controlled MIME attachment filenames (basename + ./.. guard) to block arbitrary file write via crafted .eml. - H1 auth: checkAccessControl fails closed — a getAccess DB error now denies instead of granting access, and an unknown/corrupt role falls back to the most restrictive (viewer) instead of unrestricted. The /role and /approve handlers default an un-provisioned caller to viewer, not owner. - H2 agent-runner: neutralize CLI argument injection — a prompt passed as a positional argv element that begins with '-' is prefixed with a space so it can never be parsed as a flag (e.g. a message of "--dangerously-skip-permissions" or "--mcp-config ..."). - H3 agent-runner: an unknown spawn profile now falls back to read-only tools instead of leaving the worker unrestricted. - H6 secrets: write the per-worker MCP temp config and the global .openbridge/mcp-config.json with mode 0600 (they hold raw MCP tokens). - H7 email: enable IMAP TLS certificate verification by default; opt out explicitly via options.allowInsecureTLS. - M4 cli: write config.json with mode 0600 (holds bot tokens / MCP env). Tests updated to assert the new fail-closed behavior.
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
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
Security hardening batch from a full-project audit. Fixes the critical fail-open paths and the quick-win HIGH findings. Each item was verified against the actual code path; tests updated to assert the new fail-closed behavior.
auth.whitelistnow fails closed (denies all) instead of authorizing everyone. New opt-inauth.allowAllSendersflag for local dev. Paired users still authorized viaaccess_control.basename+./..guard) — blocks arbitrary file write via crafted.eml.checkAccessControlfails closed: agetAccessDB error denies (was: allow); an unknown/corrupt role falls back toviewer(was: unrestricted)./role&/approvedefault an un-provisioned caller toviewer, notowner.-is space-prefixed so it can't be parsed as a flag (e.g.--dangerously-skip-permissions,--mcp-config)..openbridge/mcp-config.jsonwritten with mode0600(they hold raw MCP tokens).options.allowInsecureTLS.config.jsonwritten with mode0600(holds bot tokens / MCP env).Breaking change
V0 configs that relied on an empty whitelist for open access will now deny all senders. Either add entries to
auth.whitelist, or setauth.allowAllSenders: true(intended for local/console dev only). A loud startup error explains this when the whitelist is empty.Testing
npm run typecheck,npm run lint,npm run build— pass.npm run test— 5518 pass. Updated 7 tests that encoded the old insecure behavior; added opt-in/fail-closed assertions.develop(date-dependentnaming-seriesyear-rollover; flakypdf-processorOCR timeout) — not touched by this PR.Not included (larger redesigns, follow-up)
C3 (FileServer auth + public-tunnel exposure), H5 (OpenAPI SSRF allowlist), H8 (WebChat
0.0.0.0default + token-in-URL), plus medium items M1–M7. Happy to tackle these in separate PRs.