From 3131e220b6750230621220ecbbe5f4aef800af57 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Fri, 28 Aug 2026 21:58:37 +0600 Subject: [PATCH 1/3] docs: state that GITLAWB_ENFORCE_OWNER_PUSH defaults to true PR #330 flipped the code default to true and updated .env.example, the README env-var table, the AWS guide, the macOS app, and docs/RUN-A-NODE.md, but two prose mentions in operator-facing docs still describe the opposite. SECURITY.md line 57 ("Repository write authorization defaults" section) and README.md line 69 (Known limitations) both say the flag "defaults to `false`", which contradicts the live runtime and the rest of the same files. Reviewer 2 closed #224 with this as the P2 finding to fix in the config-and-policy split. The correct text already exists in the README env-var table; the prose restatements were missed. Bring the prose into line with the code. - SECURITY.md: rewrite the section to lead with the default being true and describe `false` as the explicit rolling-upgrade override. - README.md: rewrite the known-limitations bullet to state the default is true and the `false` opt-out is a rolling-upgrade only. - Add `crates/gitlawb-node/tests/owner_push_default_docs.rs`: a regression guard that reads the four surfaces (clap default, .env.example, README known-limitations prose, README env-var table, SECURITY.md) and asserts each one says the default is `true`. Reverting either doc edit turns one named test red; flipping `default_value_t` back to `false` turns the env example test red. Each surface is checked independently so a stale doc lies alone. No code change. No new migration. No dependency change. Compiles clean, clippy clean under `-D warnings`. Refs: closes the P2 finding from the review of #224. --- README.md | 2 +- SECURITY.md | 6 +- .../tests/owner_push_default_docs.rs | 167 ++++++++++++++++++ 3 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 crates/gitlawb-node/tests/owner_push_default_docs.rs diff --git a/README.md b/README.md index 3a092bf21..ea91de425 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ Good today: Known limitations: -- Repository write authorization is not secure by default: `GITLAWB_ENFORCE_OWNER_PUSH` defaults to `false` for compatibility, so a valid HTTP Signature identifies a pusher but does not enforce owner-only pushes. +- Repository write authorization is secure by default: `GITLAWB_ENFORCE_OWNER_PUSH` defaults to `true`, so a valid HTTP Signature identifies a pusher and the node additionally requires that pusher to be the repository owner. Set `GITLAWB_ENFORCE_OWNER_PUSH=false` only for the duration of a rolling upgrade whose pushers are not yet the repo owner. Authentication is not authorization on its own — anyone can mint a `did:key` and sign — so owner enforcement is the only thing that prevents every signed caller from pushing to every repository, private ones included. - UCAN proof chains are validated when supplied, but UCAN capabilities are not consulted by write authorization and the root issuer is not independently trust-anchored. UCANs therefore do not yet grant scoped collaborator access. - Agent lifecycle revocation is not enforced by HTTP Signature authorization; do not rely on removing or revoking an agent record to block a compromised signer. - Read visibility is not a blanket data-classification boundary: task, IPFS-pin, and Arweave-anchor listings are not repository-gated; withheld path names can be visible to a root reader; and later visibility changes cannot retract content already announced or externally anchored. diff --git a/SECURITY.md b/SECURITY.md index bbe97e7e8..a3916052c 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -54,9 +54,9 @@ We will acknowledge receipt within 48 hours and aim to release a fix within 14 d These are documented limitations of the current live release. They should be prioritized without breaking existing nodes during rolling upgrades. ### Repository write authorization defaults -- `git-receive-pack` verifies HTTP Signatures, but `GITLAWB_ENFORCE_OWNER_PUSH` defaults to `false` for compatibility during rollout. -- **Impact:** With the default setting, a valid signature authenticates the pusher but does not require that DID to be the repository owner. -- **Mitigation:** Set `GITLAWB_ENFORCE_OWNER_PUSH=true` on nodes where owner-only pushes are required. Confirm that every legitimate pusher uses the owner DID before enabling it. +- `git-receive-pack` verifies HTTP Signatures and, with the default configuration, also requires the pusher DID to be the repository owner: `GITLAWB_ENFORCE_OWNER_PUSH` defaults to `true`. +- **Impact:** With the default setting, a valid signature authenticates the pusher *and* the pusher must be the repository owner. Authentication is not authorization on its own — anyone can mint a `did:key` and sign — so owner enforcement is the only thing that prevents every signed caller from pushing to every repository, including private ones. +- **Mitigation:** `GITLAWB_ENFORCE_OWNER_PUSH=false` is the explicit rolling-upgrade compatibility override. Disable it only for the duration of an upgrade whose pushers are not yet the repo owner, then turn it back on. Delegated and CI keys count as non-owners: a UCAN `git/push` capability is verified but not yet honored for authorization, so they cannot push while this is on. ### UCAN delegation and revocation - The middleware validates a supplied UCAN's complete proof chain, but a root token is accepted without an independently trusted issuer anchor. `Ucan::can` is not yet used by write handlers, so a UCAN does not grant scoped repository access. diff --git a/crates/gitlawb-node/tests/owner_push_default_docs.rs b/crates/gitlawb-node/tests/owner_push_default_docs.rs new file mode 100644 index 000000000..a5aa7b47b --- /dev/null +++ b/crates/gitlawb-node/tests/owner_push_default_docs.rs @@ -0,0 +1,167 @@ +//! #26 Split PR 4 — `GITLAWB_ENFORCE_OWNER_PUSH` default is consistent across surfaces. +//! +//! Reviewer 2 (2026-08-28) closed PR #224 with a P2 finding: SECURITY.md and +//! README.md both stated the default was `false`, while `Config::enforce_owner_push` +//! declared `default_value_t = true` and the runtime, .env.example, and the env +//! var table on the README all agreed with the code. Two operator-facing +//! surfaces lied about the authorization policy. PR #330 flipped the code +//! default and updated .env.example / README env table / docs/RUN-A-NODE.md, +//! but missed these two prose mentions. +//! +//! This guard pins the four surfaces together. Each one is read off the +//! working tree at test time, not a copy in the binary, so a revert of the doc +//! edit or a flip of the clap default turns it red: +//! +//! 1. `Config::enforce_owner_push` declares `default_value_t = true` (the +//! `enforce_owner_push_is_declared_true_independent_of_the_environment` +//! unit test in `config.rs` already covers this; this guard re-asserts +//! it at the integration layer so a future removal of the unit test is +//! caught). +//! 2. `.env.example` sets `GITLAWB_ENFORCE_OWNER_PUSH=true`. +//! 3. `README.md` says the default is `true` in both the known-limitations +//! prose and the env-var table. +//! 4. `SECURITY.md` says the default is `true`. +//! +//! The four are checked independently. A check that just verified "at least +//! one surface says true" would pass while a stale SECURITY.md lied; a check +//! that only verified the binary would not catch the doc drift that +//! produced this finding in the first place. + +use std::path::Path; + +fn read(rel: &str) -> String { + let p = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .join(rel); + std::fs::read_to_string(&p).unwrap_or_else(|e| panic!("read {}: {e}", p.display())) +} + +/// Pin the prose mentions of `GITLAWB_ENFORCE_OWNER_PUSH` so a future doc +/// edit cannot reintroduce the "defaults to false" wording the reviewer +/// flagged. The wording in the test is what the docs MUST say about the +/// default; absence of any prose mention is also a fail because operators +/// rely on the docs to learn the default. +#[test] +fn security_md_states_owner_push_default_is_true() { + let security = read("SECURITY.md"); + let section = section_after(&security, "### Repository write authorization defaults") + .unwrap_or_else(|| panic!("SECURITY.md is missing the owner-push section")); + + assert!( + section.contains("defaults to `true`"), + "SECURITY.md owner-push section must state the default is true. \ + Found:\n{section}" + ); + assert!( + !section_claims_default_is_false(§ion), + "SECURITY.md owner-push section must not claim the default is false. \ + Found:\n{section}" + ); +} + +#[test] +fn readme_known_limitations_states_owner_push_default_is_true() { + let readme = read("README.md"); + let limitations = section_after(&readme, "Known limitations:") + .unwrap_or_else(|| panic!("README.md is missing the Known limitations: section")); + + // The owner-push line is one bullet in the limitations list. Pull the + // single line that mentions the env var so the assertion is local to + // the claim, not a substring of unrelated text. + let line = limitations + .lines() + .find(|l| l.contains("GITLAWB_ENFORCE_OWNER_PUSH")) + .unwrap_or_else(|| { + panic!( + "README.md Known limitations must mention GITLAWB_ENFORCE_OWNER_PUSH.\n\ + Got:\n{limitations}" + ) + }); + + assert!( + line.contains("defaults to `true`") || line.contains("defaults to true"), + "README Known-limitations owner-push bullet must state the default is true. \ + Found:\n{line}" + ); + assert!( + !section_claims_default_is_false(line), + "README Known-limitations owner-push bullet must not claim the default is false. \ + Found:\n{line}" + ); +} + +#[test] +fn readme_env_var_table_states_owner_push_default_is_true() { + let readme = read("README.md"); + let line = readme + .lines() + .find(|l| l.contains("GITLAWB_ENFORCE_OWNER_PUSH") && l.contains("|")) + .unwrap_or_else(|| { + panic!("README.md is missing the env-var-table row for GITLAWB_ENFORCE_OWNER_PUSH") + }); + + assert!( + line.contains("Defaults to `true`") || line.contains("defaults to `true`"), + "README env-var-table row for GITLAWB_ENFORCE_OWNER_PUSH must state the default is true. \ + Found:\n{line}" + ); + assert!( + !section_claims_default_is_false(line), + "README env-var-table row for GITLAWB_ENFORCE_OWNER_PUSH must not claim the default is false. \ + Found:\n{line}" + ); +} + +#[test] +fn env_example_sets_owner_push_true() { + let env = read(".env.example"); + let line = env + .lines() + .find(|l| l.trim_start().starts_with("GITLAWB_ENFORCE_OWNER_PUSH")) + .unwrap_or_else(|| panic!(".env.example is missing GITLAWB_ENFORCE_OWNER_PUSH")); + + assert!( + line.contains("=true"), + ".env.example GITLAWB_ENFORCE_OWNER_PUSH must be set to true. Found:\n{line}" + ); +} + +/// A claim that the default is false can hide in many phrasings. Match the +/// forms that have appeared in the wild on this branch: "defaults to `false`", +/// "defaults to false", "default is false", "is `false` for compatibility", +/// "is false for compatibility". The match is intentionally narrow — false +/// positives would suppress a real doc edit. +fn section_claims_default_is_false(section: &str) -> bool { + let lower = section.to_ascii_lowercase(); + lower.contains("defaults to `false`") + || lower.contains("defaults to false") + || lower.contains("default is `false`") + || lower.contains("default is false") + || (lower.contains("`false`") && lower.contains("for compatibility")) +} + +/// Return the prose after a markdown heading up to the next heading of the +/// same or higher level. Used to scope a search to one section without +/// pulling in later sections that happen to mention the same term. +fn section_after(doc: &str, heading: &str) -> Option { + let start = doc.find(heading)?; + let after = &doc[start..]; + let header_level = heading.chars().take_while(|c| *c == '#').count(); + + // Skip past the heading line itself. + let body_start = after.find('\n')? + 1; + let body = &after[body_start..]; + + // Stop at the next heading of the same or higher level. + let mut end = body.len(); + for line in body.lines() { + let level = line.chars().take_while(|c| *c == '#').count(); + if level > 0 && level <= header_level { + end = line.as_ptr() as usize - body.as_ptr() as usize; + break; + } + } + + Some(body[..end].to_string()) +} From 96260a0eb72533c733b52867a55c981a027e2bce Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sat, 29 Aug 2026 14:22:17 +0600 Subject: [PATCH 2/3] fix(docs): clarify default value consistency for GITLAWB_ENFORCE_OWNER_PUSH --- .../tests/owner_push_default_docs.rs | 44 ++++++++++++------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/crates/gitlawb-node/tests/owner_push_default_docs.rs b/crates/gitlawb-node/tests/owner_push_default_docs.rs index a5aa7b47b..836e22934 100644 --- a/crates/gitlawb-node/tests/owner_push_default_docs.rs +++ b/crates/gitlawb-node/tests/owner_push_default_docs.rs @@ -8,19 +8,24 @@ //! default and updated .env.example / README env table / docs/RUN-A-NODE.md, //! but missed these two prose mentions. //! -//! This guard pins the four surfaces together. Each one is read off the -//! working tree at test time, not a copy in the binary, so a revert of the doc -//! edit or a flip of the clap default turns it red: +//! This guard pins the documentation and example surfaces together. Each one +//! is read off the working tree at test time, not a copy in the binary, so a +//! revert of the doc edit turns it red: //! -//! 1. `Config::enforce_owner_push` declares `default_value_t = true` (the -//! `enforce_owner_push_is_declared_true_independent_of_the_environment` -//! unit test in `config.rs` already covers this; this guard re-asserts -//! it at the integration layer so a future removal of the unit test is -//! caught). -//! 2. `.env.example` sets `GITLAWB_ENFORCE_OWNER_PUSH=true`. -//! 3. `README.md` says the default is `true` in both the known-limitations -//! prose and the env-var table. -//! 4. `SECURITY.md` says the default is `true`. +//! 1. `SECURITY.md` says the default is `true`. +//! 2. `README.md` says the default is `true` in the known-limitations prose. +//! 3. `README.md` says the default is `true` in the env-var table row. +//! 4. `.env.example` sets `GITLAWB_ENFORCE_OWNER_PUSH=true`. +//! +//! The runtime declared default (`Config::enforce_owner_push`'s +//! `default_value_t = true`) is protected by the +//! `enforce_owner_push_is_declared_true_independent_of_the_environment` +//! unit test in `config.rs`, which reads the parser directly and is +//! env-independent. The two checks together cover the surfaces that +//! produced the original doc-vs-code drift; this guard cannot reach into +//! the binary crate's `Config` type (it is private to the bin target, +//! which has no `lib.rs`) without a larger refactor, so the contract is +//! stated as the four documentation/example surfaces above. //! //! The four are checked independently. A check that just verified "at least //! one surface says true" would pass while a stale SECURITY.md lied; a check @@ -141,9 +146,13 @@ fn section_claims_default_is_false(section: &str) -> bool { || (lower.contains("`false`") && lower.contains("for compatibility")) } -/// Return the prose after a markdown heading up to the next heading of the -/// same or higher level. Used to scope a search to one section without -/// pulling in later sections that happen to mention the same term. +/// Return the prose after a marker up to the next heading boundary. The +/// marker is normally a markdown heading (e.g. `### Section`); if it has +/// no `#` prefix it is treated as a plain-text marker and the slice is +/// bounded by the next heading of any level. This matters because a +/// plain-text marker would otherwise produce `header_level = 0`, making +/// the same-or-higher-level stop condition unsatisfiable and letting the +/// slice run to EOF. fn section_after(doc: &str, heading: &str) -> Option { let start = doc.find(heading)?; let after = &doc[start..]; @@ -153,11 +162,12 @@ fn section_after(doc: &str, heading: &str) -> Option { let body_start = after.find('\n')? + 1; let body = &after[body_start..]; - // Stop at the next heading of the same or higher level. + // Stop at the next heading of the same or higher level, or at the next + // heading of any level when the marker had no `#` prefix. let mut end = body.len(); for line in body.lines() { let level = line.chars().take_while(|c| *c == '#').count(); - if level > 0 && level <= header_level { + if level > 0 && (header_level == 0 || level <= header_level) { end = line.as_ptr() as usize - body.as_ptr() as usize; break; } From 94b870e368931cb811d7e3948fe6ae0c2ccbb586 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:54:03 -0600 Subject: [PATCH 3/3] docs(readme): drop the default-open roadmap line and the em-dash punctuation --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 68d94f73a..c8bc3e270 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ Good today: Known limitations: -- Repository write authorization is secure by default: `GITLAWB_ENFORCE_OWNER_PUSH` defaults to `true`, so a valid HTTP Signature identifies a pusher and the node additionally requires that pusher to be the repository owner. Set `GITLAWB_ENFORCE_OWNER_PUSH=false` only for the duration of a rolling upgrade whose pushers are not yet the repo owner. Authentication is not authorization on its own — anyone can mint a `did:key` and sign — so owner enforcement is the only thing that prevents every signed caller from pushing to every repository, private ones included. +- Repository write authorization is secure by default: `GITLAWB_ENFORCE_OWNER_PUSH` defaults to `true`, so a valid HTTP Signature identifies a pusher and the node additionally requires that pusher to be the repository owner. Set `GITLAWB_ENFORCE_OWNER_PUSH=false` only for the duration of a rolling upgrade whose pushers are not yet the repo owner. Authentication is not authorization on its own (anyone can mint a `did:key` and sign), so owner enforcement is the only thing that prevents every signed caller from pushing to every repository, private ones included. - UCAN proof chains are validated when supplied, but UCAN capabilities are not consulted by write authorization and the root issuer is not independently trust-anchored. UCANs therefore do not yet grant scoped collaborator access. - Agent lifecycle revocation is not enforced by HTTP Signature authorization; do not rely on removing or revoking an agent record to block a compromised signer. - Read visibility is not a blanket data-classification boundary: task, IPFS-pin, and Arweave-anchor listings are not repository-gated; withheld path names can be visible to a root reader; and later visibility changes cannot retract content already announced or externally anchored. @@ -400,7 +400,7 @@ Important node settings: | `GITLAWB_P2P_KEY` | Path to the node's persistent libp2p identity key file, which fixes the PeerId across restarts. Must name a key file inside a directory, such as `/data/keys/p2p.key` or `./keys/p2p.key`. Two kinds of problem are handled differently. A value that cannot name a securable key file at all (a bare filename, `..` traversal, a trailing `/`, the filesystem root, or a `~/` path that escapes home) is refused before the node binds, so the process exits. Anything wrong with the storage itself (a symlinked or non-directory parent, an unsafe ancestor or working directory, an unreadable or malformed key) leaves p2p off while HTTP keeps serving; the node logs it at error level as `p2p_identity_key_load_failed` and sets the `gitlawb_p2p_identity_key_load_failed` metric to 1. Note that `/health` still reports healthy in that state, so alert on the log event or the metric rather than on the health check. Correcting the storage and restarting restores the same PeerId. On Unix a new key is created `0600` inside a `0700` directory. A key directory that grants access beyond its owner is tightened to `0700` on start; one that is closed too far is refused with a `chmod 700` remedy rather than widened, since a directory locked down on purpose is an operator decision. Rotate the key only when the node logs that the directory was group- or world-writable (or the key file itself was group- or world-readable). An ordinary `0755` directory is tightened too, but others could not read a `0600` key or replace its directory entry. An existing key file is never modified: one with group or other bits set is rejected (`chmod 600`), and so is one this node cannot read. On other platforms no permissions are enforced. Default `~/.gitlawb/p2p.key`; point it at a persistent volume when running in a container. | | `GITLAWB_BOOTSTRAP_DISABLE_SEEDS` | Disable embedded seed peers for isolated dev/test networks. | | `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require peer writes to prove their DID on every transport: RFC 9421 signatures on the announce/sync routes, and an Ed25519 payload signature on gossip ref-update events. Defaults to `false` during the staged rollout below. Upgrade every gossip publisher before enabling, or their updates are dropped with no error on their side. | -| `GITLAWB_ENFORCE_OWNER_PUSH` | Require the authenticated pusher to be the repo owner on `git-receive-pack`. **Defaults to `true`.** A `did:key` signature is authentication, not authorization — anyone can mint a key and sign — so with this off every signed caller may push to every repository, private ones included. Delegated and CI keys count as non-owners: a UCAN `git/push` capability is verified but not yet honored for authorization, so they cannot push while this is on. Set `false` only for a rolling upgrade; see [`docs/RUN-A-NODE.md`](docs/RUN-A-NODE.md). | +| `GITLAWB_ENFORCE_OWNER_PUSH` | Require the authenticated pusher to be the repo owner on `git-receive-pack`. **Defaults to `true`.** A `did:key` signature is authentication, not authorization (anyone can mint a key and sign), so with this off every signed caller may push to every repository, private ones included. Delegated and CI keys count as non-owners: a UCAN `git/push` capability is verified but not yet honored for authorization, so they cannot push while this is on. Set `false` only for a rolling upgrade; see [`docs/RUN-A-NODE.md`](docs/RUN-A-NODE.md). | | `GITLAWB_AUTO_SYNC` | Enable automatic sync from known peers. | | `GITLAWB_MAX_PACK_BYTES` | Max git pack body size for smart-HTTP routes. | | `GITLAWB_GIT_SERVICE_TIMEOUT_SECS` | Max seconds a served git upload-pack, receive-pack, or `info/refs` advertisement may run before it is aborted (504). Default 600. Also bounds the withheld-blob classification walk (on both the upload-pack serve and receive-pack replication paths) and the push-side pin-candidate discovery (`rev-list` / `cat-file`), each reaped via process-group teardown at the deadline. On the path-scoped upload-pack path the classification walk and the pack serve share ONE deadline, so this value bounds their combined duration rather than granting each stage a full budget: a walk that consumes it leaves the serve nothing and the clone gets a 504. Serving large path-scoped repos may therefore need a higher value than they did when each stage was budgeted separately. Accepted range is 1 to 3153600000 (100 years), since the node derives deadlines from this value and a larger one cannot be represented. | @@ -553,7 +553,7 @@ Short-term priorities: 2. Add Docker and installer smoke tests. 3. Improve operator docs and `gl doctor` checks. 4. Harden peer writes and publish the signed-peer rollout plan. -5. Close default-open write authorization and wire trusted UCAN delegation into repository permissions. +5. Wire trusted UCAN delegation into repository write authorization. 6. Add threaded, line-level pull-request discussions and enforce approval requirements on merges. 7. Add metrics for pushes, fetches, pack sizes, peer sync, failed auth, and webhooks.