Skip to content

policy: server 3-state default-deny matrix (MR-723) - #105

Merged
aaltshuler merged 1 commit into
mainfrom
andrew/mr-723-default-deny-matrix
May 18, 2026
Merged

policy: server 3-state default-deny matrix (MR-723)#105
aaltshuler merged 1 commit into
mainfrom
andrew/mr-723-default-deny-matrix

Conversation

@aaltshuler

@aaltshuler aaltshuler commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

PR #5 of the policy chassis series. Closes the "tokens but no policy" trap. Pre-MR-723, an operator who configured bearer tokens and forgot to set policy.file got a server that required auth and then permitted every action — the illusion of protection. After this PR, that configuration is default-deny: only read actions succeed; every other action returns HTTP 403.

The state matrix

State Tokens Policy file Behavior
Open no no Every request is permitted. Refuses to start unless --unauthenticated or OMNIGRAPH_UNAUTHENTICATED=1 is set — the operator must explicitly opt in.
DefaultDeny yes no Every authenticated request for an action other than read is rejected with HTTP 403. Closes the trap.
PolicyEnabled any yes Every request is evaluated by Cedar against the configured policy. (Unchanged behavior.)

What landed

  • ServerConfig.allow_unauthenticated: bool + --unauthenticated flag on the omnigraph-server bin + OMNIGRAPH_UNAUTHENTICATED env var. load_server_settings honors both.
  • classify_server_runtime_state(has_tokens, has_policy, allow_unauthenticated) -> Result<ServerRuntimeState> — pure classifier function. serve() calls it before opening the engine and bails with a clear error when the operator hits the no-tokens-no-policy-no-flag cell.
  • authorize_request state-2 branch — when policy_engine() is None but the bearer-auth middleware delivered an authenticated actor, any action other than Read returns 403 with a "default-deny" message that names the misconfiguration.
  • AppState::with_policy_engine(self, engine) builder method — lets integration tests that need a custom workload still install a permit-all policy without proliferating constructors.
  • Test helper rewritesapp_for_loaded_repo_with_auth(token) and app_for_loaded_repo_with_auth_tokens(tokens) now install a permit-all policy alongside tokens (they previously represented the tokens-without-policy state that MR-723 makes default-deny, and tests that don't care about policy were inadvertently coupled to the loophole). A new app_for_repo_with_auth_tokens_only helper builds the State-2 fixture explicitly for the deny-path tests.

Test coverage

  • 3 unit tests on classify_server_runtime_state — every cell of the matrix, plus the refusal-to-start case.
  • 3 HTTP integration tests on the State-2 deny path — default_deny_mode_allows_read_for_authenticated_actor, default_deny_mode_rejects_change_with_forbidden, default_deny_mode_rejects_schema_apply_with_forbidden.
  • All 48 existing server tests still pass after the helper rewrite.
  • cargo test --workspace --locked green.

Docs

docs/user/policy.md gets a new "Server runtime states (MR-723)" section that documents the matrix and the explicit --unauthenticated opt-in.

Sequencing

This PR builds on PRs #102 (chassis core), #103 (writer fan-out), and #104 (CLI policy injection). One more chassis-series PR remains: PR #6 for MR-736 (per-rule severity warn/deny).

🤖 Generated with Claude Code


Open in Devin Review

Closes the "tokens but no policy" trap. Pre-MR-723, an operator who
configured bearer tokens and forgot to set policy.file got a server
that required auth and then permitted every action — the illusion of
protection. After MR-723, that configuration is default-deny: only
`read` actions succeed; every other action returns HTTP 403.

Three startup states, classified deterministically:

- **Open** — no tokens, no policy. Requires explicit
  `--unauthenticated` flag or `OMNIGRAPH_UNAUTHENTICATED=1`; otherwise
  `serve()` refuses to start. Forces the operator to opt in to
  "fully open dev mode" so it can't happen accidentally.
- **DefaultDeny** — tokens configured, no policy. `authorize_request`
  rejects every action except `Read` with 403. The warn-log on
  startup names the misconfiguration explicitly.
- **PolicyEnabled** — policy file configured. Cedar evaluates every
  request, unchanged from pre-MR-723.

What landed:

- `ServerConfig.allow_unauthenticated: bool` + `--unauthenticated` flag
  on the `omnigraph-server` bin + `OMNIGRAPH_UNAUTHENTICATED` env var
  (`load_server_settings` honors both).
- New `classify_server_runtime_state(has_tokens, has_policy,
  allow_unauthenticated) -> Result<ServerRuntimeState>` pure function.
  `serve()` calls it before opening the engine and bails with a clear
  error when the operator hits the no-tokens-no-policy-no-flag cell.
- `authorize_request` state-2 branch: when `policy_engine()` is None
  but the bearer-auth middleware delivered an authenticated actor, any
  action other than `Read` returns 403 with a message that names the
  misconfiguration.
- `AppState::with_policy_engine(self, engine)` builder method so
  integration tests that need a custom workload (`new_with_workload`)
  can still install a permit-all policy without a new constructor.
- `app_for_loaded_repo_with_auth(token)` and
  `app_for_loaded_repo_with_auth_tokens(tokens)` test helpers now
  install a permit-all policy alongside tokens — they previously
  represented the "tokens but no policy" state that MR-723 makes
  default-deny, and tests that don't care about policy were
  inadvertently coupled to the loophole.

Tests:

- `classify_*` unit tests (3) — every cell of the matrix.
- `default_deny_mode_allows_read_for_authenticated_actor` — GET
  /snapshot succeeds with bearer token + no policy.
- `default_deny_mode_rejects_change_with_forbidden` — POST /change
  rejected with 403 + "default-deny" message.
- `default_deny_mode_rejects_schema_apply_with_forbidden` — POST
  /schema/apply rejected with 403 + "default-deny" message.
- New `app_for_repo_with_auth_tokens_only(schema, tokens)` helper
  builds the State-2 fixture without policy. The pre-MR-723 helpers
  `app_for_loaded_repo_with_auth*` shift semantics to "tokens +
  permit-all" so existing tests retain their original intent.

docs/user/policy.md: new "Server runtime states (MR-723)" section
documents the matrix and the explicit `--unauthenticated` opt-in.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@aaltshuler
aaltshuler requested a review from ragnorc as a code owner May 18, 2026 01:14

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cubic analysis

2 issues found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="docs/user/policy.md">

<violation number="1" location="docs/user/policy.md:77">
P3: The PolicyEnabled row documents `Tokens = any`, but runtime startup requires at least one bearer token when a policy file is configured.

According to linked Linear issue MR-723, the policy-enabled state is the "tokens AND policy" shape, so this row should not advertise no-token policy mode as valid.</violation>
</file>

<file name="crates/omnigraph-server/src/lib.rs">

<violation number="1" location="crates/omnigraph-server/src/lib.rs:625">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**

Doc comment and classifier claim `PolicyEnabled` is reachable with policy but no tokens, but `open_with_bearer_tokens_and_policy` rejects this at startup.</violation>
</file>

Linked issue analysis

Linked issue: MR-723: Policy: reverse default-allow when tokens are configured

Status Acceptance criteria Notes
Add CLI flag/env to opt into fully-unauthenticated dev mode and have load_server_settings honor it main.rs adds --unauthenticated, load_server_settings parses OMNIGRAPH_UNAUTHENTICATED and returns ServerConfig.allow_unauthenticated.
Classify the three server runtime states and refuse to start when no tokens + no policy + no flag Pure function classify_server_runtime_state implements the 3‑state matrix and returns Err for the no-tokens/no-policy/no-flag cell; serve() calls it and will bail when Err is returned; unit tests exercise each cell.
When tokens are configured but no policy exists, deny every authenticated action except Read (default-deny) with HTTP 403 and explanatory message authorize_request has the DefaultDeny branch that returns forbidden for authenticated actors when action != Read; integration tests verify Read allowed and Change/SchemaApply return 403 with the expected error substring.
Log a startup warning when running in tokens-but-no-policy (DefaultDeny) or when running Open due to explicit unauthenticated opt-in serve() matches runtime_state and emits warn! messages for DefaultDeny and Open modes.
Add tests and test-helper changes to cover the three-state matrix and avoid accidental coupling to the previous loophole Unit tests added for the classifier; three HTTP integration tests pin the default-deny behavior; test helpers were updated to install permit-all policies where needed and a new helper builds the tokens-only (State-2) fixture.

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic

),
(false, false, true) => Ok(ServerRuntimeState::Open),
(true, false, _) => Ok(ServerRuntimeState::DefaultDeny),
(_, true, _) => Ok(ServerRuntimeState::PolicyEnabled),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Custom agent: Flag AI Slop and Fabricated Changes

Doc comment and classifier claim PolicyEnabled is reachable with policy but no tokens, but open_with_bearer_tokens_and_policy rejects this at startup.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/omnigraph-server/src/lib.rs, line 625:

<comment>Doc comment and classifier claim `PolicyEnabled` is reachable with policy but no tokens, but `open_with_bearer_tokens_and_policy` rejects this at startup.</comment>

<file context>
@@ -535,20 +555,77 @@ pub fn load_server_settings(
+        ),
+        (false, false, true) => Ok(ServerRuntimeState::Open),
+        (true, false, _) => Ok(ServerRuntimeState::DefaultDeny),
+        (_, true, _) => Ok(ServerRuntimeState::PolicyEnabled),
+    }
+}
</file context>

Comment thread docs/user/policy.md
|---|---|---|---|
| **Open** | no | no | Every request is permitted. Refuses to start unless `--unauthenticated` or `OMNIGRAPH_UNAUTHENTICATED=1` is set — the operator must explicitly opt in. |
| **DefaultDeny** | yes | no | Every authenticated request for an action other than `read` is rejected with HTTP 403. Closes the "tokens but forgot the policy file" trap — an operator who sets up auth and forgot to point at a policy file used to ship the illusion of protection. |
| **PolicyEnabled** | any | yes | Every request is evaluated by Cedar against the configured policy. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The PolicyEnabled row documents Tokens = any, but runtime startup requires at least one bearer token when a policy file is configured.

According to linked Linear issue MR-723, the policy-enabled state is the "tokens AND policy" shape, so this row should not advertise no-token policy mode as valid.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/user/policy.md, line 77:

<comment>The PolicyEnabled row documents `Tokens = any`, but runtime startup requires at least one bearer token when a policy file is configured.

According to linked Linear issue MR-723, the policy-enabled state is the "tokens AND policy" shape, so this row should not advertise no-token policy mode as valid.</comment>

<file context>
@@ -63,6 +63,25 @@ is a strict no-op; when one is installed and the call site forgets to
+|---|---|---|---|
+| **Open** | no | no | Every request is permitted. Refuses to start unless `--unauthenticated` or `OMNIGRAPH_UNAUTHENTICATED=1` is set — the operator must explicitly opt in. |
+| **DefaultDeny** | yes | no | Every authenticated request for an action other than `read` is rejected with HTTP 403. Closes the "tokens but forgot the policy file" trap — an operator who sets up auth and forgot to point at a policy file used to ship the illusion of protection. |
+| **PolicyEnabled** | any | yes | Every request is evaluated by Cedar against the configured policy. |
+
+The classifier is `classify_server_runtime_state` in
</file context>
Suggested change
| **PolicyEnabled** | any | yes | Every request is evaluated by Cedar against the configured policy. |
| **PolicyEnabled** | yes | yes | Every authenticated request is evaluated by Cedar against the configured policy. |

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines +264 to +266
pub fn with_policy_engine(mut self, engine: PolicyEngine) -> Self {
self.policy_engine = Some(Arc::new(engine));
self

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 with_policy_engine installs policy on HTTP layer only, silently bypassing engine-layer enforcement

with_policy_engine sets self.policy_engine on AppState but does not install the PolicyChecker on the inner Arc<Omnigraph> engine (compare with new_with_bearer_tokens_and_policy at crates/omnigraph-server/src/lib.rs:217-222 which calls db.with_policy(checker)). Since the engine is already wrapped in Arc<Omnigraph>, there's no way to install the checker after construction.

This means every engine-layer enforce() call — wired into mutate_as (crates/omnigraph/src/exec/mutation.rs:700), ingest_as (crates/omnigraph/src/loader/mod.rs:100,191), branch_create_as (crates/omnigraph/src/db/omnigraph.rs:1005), branch_create_from_as (:1046), branch_delete_as (:1112), branch_merge_as (crates/omnigraph/src/exec/merge.rs:1072), and apply_schema_as — becomes a silent no-op because the engine's policy field remains None. The method is pub with no #[cfg(test)] guard, so any caller (not just the test that currently uses it) will get a state where HTTP-layer policy appears to work but the engine-layer defense-in-depth backstop is silently disabled.

Prompt for agents
The method `with_policy_engine` on `AppState` (crates/omnigraph-server/src/lib.rs:264-266) sets `self.policy_engine` on the HTTP-layer AppState but does not install the policy checker on the inner `Omnigraph` engine. Compare with `new_with_bearer_tokens_and_policy` (same file, lines 217-222) which calls `db.with_policy(checker)` before wrapping the engine in `Arc::new(db)`.

Since `self.engine` is `Arc<Omnigraph>` and `Omnigraph::with_policy` takes `mut self` (ownership), you cannot install the checker after the engine is already wrapped in Arc. The fix options are:

1. Restructure `with_policy_engine` to rebuild the engine: extract the inner Omnigraph via `Arc::try_unwrap` (fails if cloned), call `.with_policy(checker)`, re-wrap in Arc. This is fragile if the Arc has been cloned.

2. Add an `install_policy` method on `Omnigraph` that takes `&self` and writes to the `Option<Arc<dyn PolicyChecker>>` field via interior mutability (e.g. `OnceLock` or `Mutex`). This is the cleanest long-term fix.

3. Gate `with_policy_engine` with `#[cfg(test)]` to prevent production misuse, and document that it intentionally omits engine-layer enforcement for test convenience. At minimum, the method should panic or warn if called outside tests.

4. Remove the method entirely and instead add a new `new_with_workload_and_policy` constructor that composes workload + policy in one call, installing the policy on both AppState and the engine.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +19 to +24
/// Run without bearer tokens and without a policy file (MR-723).
/// Required when neither is configured — otherwise the server
/// refuses to start to prevent shipping the illusion of protection.
/// Equivalent to setting `OMNIGRAPH_UNAUTHENTICATED=1`.
#[arg(long)]
unauthenticated: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 docs/user/server.md not updated for new --unauthenticated flag and changed startup behavior (AGENTS.md rule 1 & 6 violation)

This PR introduces the --unauthenticated CLI flag and OMNIGRAPH_UNAUTHENTICATED env var, and changes startup behavior so the server now refuses to start without bearer tokens unless the flag is set. However, docs/user/server.md:82 still reads: "If no tokens configured, server runs unauthenticated (local dev)" — which is now factually wrong.

AGENTS.md rule 1 requires: "New endpoint, query function, CLI flag, env var, constant, schema construct, or invariant: update both the source code and the doc in the same change. Never split documentation drift into a follow-up." AGENTS.md rule 6 requires: "Don't lie. If a section becomes wrong but you can't rewrite it fully right now, replace the wrong line with (stale — needs update after <change>) rather than leaving silently incorrect text."

The --unauthenticated flag, the OMNIGRAPH_UNAUTHENTICATED env var, and the three-state server runtime matrix should be documented in docs/user/server.md (the canonical server docs page). The new docs/user/policy.md section covers the runtime states but server.md still contradicts it.

Prompt for agents
This PR introduces the `--unauthenticated` CLI flag (crates/omnigraph-server/src/main.rs:19-24) and the `OMNIGRAPH_UNAUTHENTICATED` env var (crates/omnigraph-server/src/lib.rs:568), and changes server startup behavior via `classify_server_runtime_state`. Per AGENTS.md rules 1 and 6, the following docs need updating in the same PR:

1. `docs/user/server.md` line 82: Replace 'If no tokens configured, server runs unauthenticated (local dev) and `/openapi.json` strips the security scheme.' with an accurate description of the three-state matrix (Open/DefaultDeny/PolicyEnabled) and the requirement for `--unauthenticated` when neither tokens nor policy are configured. Reference the new section in `docs/user/policy.md` for details.

2. `docs/user/server.md` Auth model section: Add documentation for the `--unauthenticated` CLI flag and `OMNIGRAPH_UNAUTHENTICATED` env var.

3. `docs/user/constants.md`: Add an entry for `OMNIGRAPH_UNAUTHENTICATED` in the constants/tunables cheat sheet.

4. `docs/user/deployment.md` line 112: Also mentions 'The server can run unauthenticated for local development' which should note the new `--unauthenticated` requirement.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@aaltshuler
aaltshuler merged commit f3f2a05 into main May 18, 2026
7 checks passed
@aaltshuler
aaltshuler deleted the andrew/mr-723-default-deny-matrix branch May 18, 2026 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant