Per-config access roles (user/MCP) with MCP exclusion#41
Merged
Conversation
CP1 of per-config access roles (#40). Hard-coded viewer/operator/admin matrix with channel-resolved confirm (user prompts yes-<config>, mcp collapses to deny), guarded() display helper, and classifyStatements read/write/ddl generalized from isReadOnlyStatement — fail closed: unparseable, EXEC/CALL, and SELECT INTO all classify ddl. Unwired until CP3.
CP2 of per-config access roles (#40). Config/ConfigInput/ConfigSummary carry access; protected becomes derived-at-load and is never persisted; v2 state migration maps legacy protected (true -> operator/viewer, false -> admin/admin) via single-source resolveLegacyAccess; stage protected:true becomes an access ceiling clamp in both resolver paths (replaces the stored-wins override block, which let stage protection be silently overridden); settings rules match guarded(access). Review round fixed a fail-open where DEFAULTS.access pre-empted the legacy mapping.
CP3 of per-config access roles (#40). Every RpcCommand declares a required permission; run_noorm_cmd checks policy before any handler runs, closing the gap where change_run/change_ff/change_revert/run_file/run_build bypassed protection on MCP. sql escalates via classifyStatements (read/write/ddl). Configs with access.mcp=false vanish: filtered from list_configs, connect fails with the byte-identical unknown-config error (single-sourced via configNotFoundMessage and pinned by an unmocked end-to-end test). SessionManager carries the channel; absent access fails closed. rpc/protection.ts absorbed into core/policy.
CP4 of per-config access roles (#40). createContext gains channel (default user); SDK guards consult checkPolicy per operation — truncate/teardown/reset/dt.import map to new db:reset permission (viewer deny, operator confirm, admin allow: preserves pre-migration behavior for open configs), revert/rewind to change:revert, db:destroy reserved for dropping databases. CLI: db drop policy-gated (covered by new subprocess tests against a real SQLite target), config list shows access levels, ci init writes OPEN_ACCESS. Fail-closed absent-access check consolidated to a single checkConfigPolicy in core/policy; SDK re-exports Channel/ConfigAccess/Role so the public surface closes under its own references.
CP5 of per-config access roles (#40). SmartConfirm/ProtectedConfirm consume the policy check (requiresConfirmation + confirmationPhrase, single-sourced); gated screens compute checkConfigPolicy and render a blocked panel when denied, with keyboard handlers unreachable while denied (incl. the retry-past-deny and teardown/truncate viewer gaps found in review); config add/edit expose user/mcp role selects with fail-closed fallbacks; list shows access levels; import maps legacy protected via resolveLegacyAccess; guarded() drives the yellow-border styling. Matrix is the enforcement floor: teardown/truncate keep unconditional phrase-confirm.
CP6 of per-config access roles (#40). Config/ConfigSummary lose protected (access required); orphaned checkProtection module deleted; ConfigInput.protected survives as parse-time legacy input only; export payload keeps a computed legacy field for one version. Root-cause fix uncovered by review: the schemaVersion migration system (core/version/state, where v2 registers) was dead wiring — StateManager.load() now runs it before the semver migration, with a single raw-boundary access backfill that persists its repairs (previously a legacy state file loaded access:undefined and crashed config list/edit/run screens). End-to-end tests pin the legacy load path and the durable repair. BREAKING CHANGE: Config.protected removed from the object model; stored state auto-migrates on load.
CP7 of per-config access roles (#40). 20+ surfaces move from the protected boolean to the access model (matrix, mcp:false invisibility, SQL classification, confirm semantics, migration notes); changeset minor for @noormdev/cli and @noormdev/sdk. Doc claim-verification surfaced a real fail-open riding along: noorm config import cast raw JSON past the schema, so imported legacy protected:true configs landed admin/admin — import now parses through the real zod path and the load-boundary backfill reads the raw legacy key, both pinned by red-first tests.
…t the core seam Post challenge-swarm hardening (#40). Classifier: data-modifying CTEs (WITH ... AS (DELETE ...) SELECT ...) and a denylist of side-effecting functions (pg_terminate_backend, dblink_exec, query_to_xml family; bare or schema-qualified) now classify as writes — a confirmed critical where a viewer could mutate data through the read-allowed sql path. Also fixed a keyword-fallback over-denial on CTEs whose final statement contains a subquery. Enforcement: user-channel policy moved to the core seam (runner, change, transfer, sql-terminal) so SDK, TUI, and CLI all inherit one gate — previously only MCP and 6 SDK ops were gated, leaving run/transfer/ad-hoc-SQL open on the user channel. transferData gates the destination (db:reset). The raw-SQL gate is mandatory by construction: executeRawSql requires a gate, executeRawSqlUnchecked is the grep-able test-only path. Policy assertion single-sourced as assertPolicy in core/policy. Deferred to tracked follow-ups: state.enc atomicity/locking, denial observability, legacy-input removal trigger, downgrade guard, and a configurable per-config function allow/deny policy.
SDK deny path now surfaces the policy blockedReason (role + permission) instead of a generic message; ProtectedConfigError names the actual method invoked (changes.rewind, dt.importFile). Confirmation phrase single-sourced as confirmationPhraseFor. Deleted the dead stageEnforcesProtected (its JSDoc still asserted the pre-clamp stored-wins rule) and marked the surviving settings-stage protected vocabulary in code. New table-driven test pins every RPC command's permission value against the real registry (a too-weak permission now fails the build). Reworked two false-confidence tests that passed vacuously. getContext invisibility documented as a connect-time invariant.
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
1 participant
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.
Closes #40.
Replaces the single
protected: booleanper config with per-channel access roles, enforced by one policy matrix, and closes an MCP enforcement gap where several commands reached the database unchecked.What ships
access: { user, mcp }, eachviewer | operator | admin(ormcp: falseto hide a config from agents entirely).viewerreads,operatorwrites with confirmation,adminis frictionless. Stored state auto-migrates fromprotectedon load.change_run/change_ff/change_revert/run_file/run_buildbypass).mcp: falseconfigs are absent fromlist_configsandconnectfails with the byte-identical unknown-config error.confirm-tier permissions collapse to a hard deny on the MCP channel.sql, MCPsql, TUI terminal) is gated by what the statement does (read/write/ddl). Data-modifying CTEs and a denylist of side-effecting functions (bare or schema-qualified) classify as writes so aviewercannot mutate through the read path. Fail-closed on unparseable /EXEC/CALL.createContext({ channel }), exportedChannel/ConfigAccess/Role.Breaking
Config.protectedis removed from the exported types;accessis now required onConfig/ConfigSummary. Stored state migrates automatically. See the changeset for theNOORM_YESbehavior note and the downgrade caveat.Verification
Design + spec under
docs/design/anddocs/spec/. A challenge-swarm (6 independent expert lenses) reviewed the completed implementation; it caught a confirmed critical (viewer SQL-write via CTE/functions), now fixed and adversarially re-verified, plus the enforcement-scope gap now closed at the core seam. Build, both typechecks, lint, and all four CI-mirrored test groups are green (3,317 tests, 0 failures). Deferred items (state.enc atomicity, denial observability, legacy-input removal trigger, configurable function policy) are tracked as follow-ups.