Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 178 additions & 7 deletions crates/omnigraph-server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ use subtle::ConstantTimeEq;
use tokio::net::TcpListener;
use tokio::sync::mpsc;
use tower_http::trace::TraceLayer;
use tracing::{error, info};
use tracing::{error, info, warn};
use tracing_subscriber::EnvFilter;
use utoipa::OpenApi;
use utoipa::openapi::security::{Http, HttpAuthScheme, SecurityScheme};
Expand Down Expand Up @@ -114,6 +114,15 @@ pub struct ServerConfig {
pub uri: String,
pub bind: String,
pub policy_file: Option<PathBuf>,
/// Operator opt-in for fully-unauthenticated dev mode (MR-723).
/// When neither bearer tokens nor a policy file are configured,
/// `serve()` refuses to start unless this is true (set via
/// `--unauthenticated` or `OMNIGRAPH_UNAUTHENTICATED=1`). The
/// motivation is that "no tokens + no policy" looks like protection
/// (no Cedar errors at boot) but is actually fully open — operators
/// who set up auth and forgot the policy file would otherwise ship
/// the illusion of protection.
pub allow_unauthenticated: bool,
}

#[derive(Clone)]
Expand Down Expand Up @@ -246,6 +255,17 @@ impl AppState {
}
}

/// Install a `PolicyEngine` post-construction (MR-723). Used by
/// integration tests that need to thread custom workload limits
/// alongside a permit-all policy — the existing `new_with_*` and
/// `new_with_workload` constructors don't compose. Production
/// callers should use `open_with_bearer_tokens_and_policy` which
/// installs the policy on both the HTTP state and the engine.
pub fn with_policy_engine(mut self, engine: PolicyEngine) -> Self {
self.policy_engine = Some(Arc::new(engine));
self
Comment on lines +264 to +266

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.

}

pub async fn open(uri: impl Into<String>) -> Result<Self> {
Self::open_with_bearer_token(uri, None).await
}
Expand Down Expand Up @@ -535,20 +555,77 @@ pub fn load_server_settings(
cli_uri: Option<String>,
cli_target: Option<String>,
cli_bind: Option<String>,
cli_allow_unauthenticated: bool,
) -> Result<ServerConfig> {
let config = load_config(config_path)?;
let uri =
config.resolve_target_uri(cli_uri, cli_target.as_deref(), config.server_graph_name())?;
let bind = cli_bind.unwrap_or_else(|| config.server_bind().to_string());
let policy_file = config.resolve_policy_file();
// Either `--unauthenticated` or `OMNIGRAPH_UNAUTHENTICATED=1` flips
// this. Treat any non-empty, non-"0"/"false" string as truthy —
// standard 12-factor "any value is true" reading of the env var.
let env_unauth = std::env::var("OMNIGRAPH_UNAUTHENTICATED")
.ok()
.map(|v| {
let trimmed = v.trim();
!trimmed.is_empty() && trimmed != "0" && !trimmed.eq_ignore_ascii_case("false")
})
.unwrap_or(false);
let allow_unauthenticated = cli_allow_unauthenticated || env_unauth;

Ok(ServerConfig {
uri,
bind,
policy_file,
allow_unauthenticated,
})
}

/// MR-723 server runtime state, classified from the three-state matrix
/// of (bearer tokens configured) × (policy file configured) at startup.
///
/// * **Open** — neither tokens nor policy; requires explicit
/// `allow_unauthenticated`. Effectively a "trust the network" dev
/// mode. `serve()` refuses to start in this shape without the flag,
/// so the only way to reach this state at runtime is via deliberate
/// operator opt-in.
/// * **DefaultDeny** — tokens configured but no policy file. The
/// server requires a valid bearer token; once authenticated, every
/// action except `Read` is denied with 403. Closes the "tokens but
/// forgot the policy file" trap.
/// * **PolicyEnabled** — policy file configured. Cedar evaluates every
/// authenticated request. Tokens may also be configured (typical) or
/// not (unusual but valid — every request fails 401 without a
/// bearer, which is effectively "locked").
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub enum ServerRuntimeState {
Open,
DefaultDeny,
PolicyEnabled,
}

/// Compute the [`ServerRuntimeState`] from the configured inputs.
/// Pulled out as a pure function so the 3-state matrix is unit-testable
/// without standing up the full server.
pub fn classify_server_runtime_state(
has_tokens: bool,
has_policy: bool,
allow_unauthenticated: bool,
) -> Result<ServerRuntimeState> {
match (has_tokens, has_policy, allow_unauthenticated) {
(false, false, false) => bail!(
"server has no bearer tokens and no policy file configured. This is a fully \
open server — pass `--unauthenticated` (or set OMNIGRAPH_UNAUTHENTICATED=1) \
if you actually want that, otherwise configure bearer tokens (see \
docs/user/server.md) and/or `policy.file` in omnigraph.yaml."
),
(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>

}
}

pub fn build_app(state: AppState) -> Router {
let protected = Router::new()
.route("/snapshot", get(server_snapshot))
Expand Down Expand Up @@ -586,9 +663,28 @@ pub fn build_app(state: AppState) -> Router {
pub async fn serve(config: ServerConfig) -> Result<()> {
let token_source = resolve_token_source().await?;
info!(source = token_source.name(), "loaded bearer token source");
let tokens = token_source.load().await?;
let runtime_state = classify_server_runtime_state(
!tokens.is_empty(),
config.policy_file.is_some(),
config.allow_unauthenticated,
)?;
match runtime_state {
ServerRuntimeState::Open => warn!(
"running with --unauthenticated: no bearer tokens, no policy file, all \
requests permitted. This is for local dev only — do not expose to a \
network you don't fully trust."
),
ServerRuntimeState::DefaultDeny => warn!(
"bearer tokens are configured but no policy file is set — running in \
default-deny mode (only `read` actions are permitted for authenticated \
actors). Configure `policy.file` in omnigraph.yaml to enable Cedar rules."
),
ServerRuntimeState::PolicyEnabled => {}
}
let state = AppState::open_with_bearer_tokens_and_policy(
config.uri.clone(),
token_source.load().await?,
tokens,
config.policy_file.as_ref(),
)
.await?;
Expand Down Expand Up @@ -708,6 +804,27 @@ fn authorize_request(
mut request: PolicyRequest,
) -> std::result::Result<(), ApiError> {
let Some(engine) = state.policy_engine() else {
// MR-723 default-deny path. We're here when no PolicyEngine is
// installed. Two startup-validated shapes can reach this:
//
// * **Open mode** (`--unauthenticated`): no tokens, no policy.
// `require_bearer_auth` short-circuits before this is called,
// but defense in depth — if a future change makes the
// middleware call here for an unauthenticated request, we
// want every action to remain Ok rather than 403. The
// operator opted in.
// * **DefaultDeny mode**: tokens configured but no policy. The
// request went through bearer auth, so `actor` is Some and
// identifies a known actor. Only `Read` is permitted; every
// other action returns 403. This closes the "configured auth
// but forgot the policy file" trap from MR-723.
if actor.is_some() && request.action != PolicyAction::Read {
return Err(ApiError::forbidden(
"server runs in default-deny mode (bearer tokens configured but no \
policy file). Only `read` actions are permitted; configure \
`policy.file` in omnigraph.yaml to enable other actions.",
));
}
return Ok(());
};
let Some(actor) = actor else {
Expand Down Expand Up @@ -1641,8 +1758,8 @@ fn server_bearer_tokens_from_env() -> Result<Vec<(String, String)>> {
#[cfg(test)]
mod tests {
use super::{
hash_bearer_token, load_server_settings, normalize_bearer_token, parse_bearer_tokens_json,
server_bearer_tokens_from_env,
ServerRuntimeState, classify_server_runtime_state, hash_bearer_token, load_server_settings,
normalize_bearer_token, parse_bearer_tokens_json, server_bearer_tokens_from_env,
};
use std::env;
use std::fs;
Expand Down Expand Up @@ -1695,7 +1812,7 @@ server:
)
.unwrap();

let settings = load_server_settings(Some(&config), None, None, None).unwrap();
let settings = load_server_settings(Some(&config), None, None, None, false).unwrap();
assert_eq!(settings.uri, "/tmp/demo.omni");
assert_eq!(settings.bind, "0.0.0.0:9090");
}
Expand All @@ -1722,6 +1839,7 @@ server:
Some("/tmp/override.omni".to_string()),
None,
Some("0.0.0.0:9999".to_string()),
false,
)
.unwrap();
assert_eq!(settings.uri, "/tmp/override.omni");
Expand All @@ -1748,16 +1866,69 @@ server:
.unwrap();

let settings =
load_server_settings(Some(&config), None, Some("dev".to_string()), None).unwrap();
load_server_settings(Some(&config), None, Some("dev".to_string()), None, false)
.unwrap();
assert_eq!(settings.uri, "http://127.0.0.1:8080");
}

#[test]
fn server_settings_require_uri_from_cli_or_config() {
let error = load_server_settings(None, None, None, None).unwrap_err();
let error = load_server_settings(None, None, None, None, false).unwrap_err();
assert!(error.to_string().contains("URI must be provided"));
}

#[test]
fn classify_open_requires_explicit_unauthenticated_flag() {
// State 1: no tokens, no policy, no flag → refuse to start.
let error = classify_server_runtime_state(false, false, false).unwrap_err();
let msg = error.to_string();
assert!(
msg.contains("--unauthenticated"),
"expected refusal message mentioning --unauthenticated, got: {msg}"
);

// Same matrix cell but with the flag set → Open mode permitted.
assert_eq!(
classify_server_runtime_state(false, false, true).unwrap(),
ServerRuntimeState::Open
);
}

#[test]
fn classify_tokens_without_policy_is_default_deny() {
// State 2: tokens configured, no policy → DefaultDeny regardless
// of the flag (the flag opts into the fully-open dev mode; it
// doesn't downgrade default-deny back to open).
assert_eq!(
classify_server_runtime_state(true, false, false).unwrap(),
ServerRuntimeState::DefaultDeny
);
assert_eq!(
classify_server_runtime_state(true, false, true).unwrap(),
ServerRuntimeState::DefaultDeny
);
}

#[test]
fn classify_policy_enabled_always_wins() {
// State 3: any setup with a policy file → PolicyEnabled. The
// flag doesn't matter and tokens-or-not doesn't matter (no
// tokens + policy is unusual but valid — every request fails
// 401 without a bearer, which is effectively "locked").
assert_eq!(
classify_server_runtime_state(true, true, false).unwrap(),
ServerRuntimeState::PolicyEnabled
);
assert_eq!(
classify_server_runtime_state(false, true, false).unwrap(),
ServerRuntimeState::PolicyEnabled
);
assert_eq!(
classify_server_runtime_state(true, true, true).unwrap(),
ServerRuntimeState::PolicyEnabled
);
}

#[test]
fn normalize_bearer_token_trims_and_filters_blank_values() {
assert_eq!(normalize_bearer_token(None), None);
Expand Down
15 changes: 13 additions & 2 deletions crates/omnigraph-server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ struct Cli {
config: Option<PathBuf>,
#[arg(long)]
bind: Option<String>,
/// 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,
Comment on lines +19 to +24

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.

}

#[tokio::main]
Expand All @@ -24,7 +30,12 @@ async fn main() -> Result<()> {
init_tracing();

let cli = Cli::parse();
let settings: ServerConfig =
load_server_settings(cli.config.as_ref(), cli.uri, cli.target, cli.bind)?;
let settings: ServerConfig = load_server_settings(
cli.config.as_ref(),
cli.uri,
cli.target,
cli.bind,
cli.unauthenticated,
)?;
serve(settings).await
}
Loading