test(gl,node): pin LC_ALL=C on git-subprocess stderr assertions

* feat(node): persist upstream mirror transition state

Origin-Session: local-d6a143 | Codex | 12 prompts

* fix(node): reject null mirror transition state

Origin-Session: local-d6a143 | Codex | 14 prompts

* fix(node): address mirror state review findings

Origin-Session: local-d6a143 | Codex | 16 prompts

* fix(node): close mirror review blockers

Origin-Session: local-d6a143 | Codex | 20 prompts

* fix(node): align peer URL validation message

Origin-Session: local-d6a143 | Codex | 20 prompts

* fix(node): close remaining mirror review findings

Origin-Session: local-d6a143 | Codex | 21 prompts

* test(node): track the Db pool field change in the mirror-state migration tests

* fix(node): revalidate stored webhook URLs at delivery and cover the subdomain repeated-dot case

* docs(node): document the invalid_url webhook delivery outcome

---------

Co-authored-by: KRATOS <[email protected]>
Co-authored-by: Kevin Codex <[email protected]>

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.
@@ -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.
 

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.

crates/gitlawb-attest/src/attestation.rs

@@ -504,6 +504,23 @@ mod tests {
         assert!(matches!(err, Error::Did(_)));
     }
 
+    #[test]
+    fn verify_rejects_oversized_method_id_before_decoding() {
+        let sk = fresh();
+        let mut att = dummy_attestation(&sk, sample_cert_hash());
+        // A well-formed prefix on an absurdly long id must fail the length
+        // bound, not the decoder: base58 decode cost is quadratic in input
+        // length, so reaching it at all is the defect.
+        att.signer = format!("did:key:z{}", "1".repeat(256));
+        let err = att.verify_signature(sample_cert_hash()).unwrap_err();
+        // Match the bound's own message: any other Did error (a decode or
+        // length mismatch downstream) means the oversized input was decoded.
+        match err {
+            Error::Did(m) => assert!(m.contains("too long"), "unexpected: {m}"),
+            e => panic!("expected Error::Did, got {e:?}"),
+        }
+    }
+
     #[test]
     fn verify_rejects_bad_signature_base64() {
         let sk = fresh();

crates/gitlawb-attest/src/verifier.rs

@@ -150,23 +150,48 @@ impl Registry {
     }
 
     /// Verify a batch, then enforce `RequireAll`.
+    ///
+    /// Under [`Policy::RequireAll`] this never short-circuits: an attestation
+    /// that fails to verify — bad signature, cert-hash mismatch, or malformed
+    /// payload, the actual `Err` paths of [`Registry::verify`] — is dropped
+    /// from the result rather than aborting the batch. A valid attestation of
+    /// a type with NO registered verifier is not one of those: under
+    /// `RequireAll` it verifies to `Ok` with `fully_verified: false` and stays
+    /// in the result, preserving the distinction between *untrusted* and
+    /// *invalid* (see `require_all_is_lenient_on_unknown_types_in_the_batch`).
+    /// Any third party can attach an attestation, so a hard error here would
+    /// let an unrelated `attacker/spam/v1` deny-of-service the whole cert (the
+    /// DoS vector the module docstring warns about). The `required_types`
+    /// check below is the real gate, and it only counts `fully_verified`
+    /// entries — so a required type represented solely by a malformed
+    /// attestation still fails with [`Error::RequiredMissing`].
+    ///
+    /// Under `AcceptKnown` and `RejectUnknown` the batch stays strict: the
+    /// first verification error is surfaced to the caller.
     pub fn verify_all(
         &self,
         attestations: &[Attestation],
         expected_cert_hash: [u8; 32],
     ) -> Result<Vec<VerifiedAttestation>> {
-        let mut verified = Vec::with_capacity(attestations.len());
-        for a in attestations {
-            verified.push(self.verify(a, expected_cert_hash)?);
+        if self.policy != Policy::RequireAll {
+            let mut verified = Vec::with_capacity(attestations.len());
+            for a in attestations {
+                verified.push(self.verify(a, expected_cert_hash)?);
+            }
+            return Ok(verified);
         }
-        if self.policy == Policy::RequireAll {
-            for required in &self.required_types {
-                let present = verified
-                    .iter()
-                    .any(|v| &v.type_ == required && v.fully_verified);
-                if !present {
-                    return Err(Error::RequiredMissing(required.clone()));
-                }
+
+        // RequireAll: drop entries that fail to verify instead of aborting.
+        let verified: Vec<VerifiedAttestation> = attestations
+            .iter()
+            .filter_map(|a| self.verify(a, expected_cert_hash).ok())
+            .collect();
+        for required in &self.required_types {
+            let present = verified
+                .iter()
+                .any(|v| &v.type_ == required && v.fully_verified);
+            if !present {
+                return Err(Error::RequiredMissing(required.clone()));
             }
         }
         Ok(verified)
@@ -324,6 +349,38 @@ mod tests {
             .any(|v| v.type_ == "other/v1" && !v.fully_verified));
     }
 
+    /// The strict policies must surface a batch member's verification error
+    /// through `verify_all`, not launder it into a partial result. This binds
+    /// the POLICY BOUNDARY itself: only the `RequireAll` arm may drop failing
+    /// entries, and an apparently harmless cleanup that applies its
+    /// `filter_map(Result::ok)` shape to the strict arms would silently turn
+    /// the fail-closed contracts into fail-open — with every per-attestation
+    /// `verify` test still green, because none of them go through the batch
+    /// path. The failing entry is a wrong-cert-hash attestation (signed over a
+    /// different hash than the batch expects), a genuine `Err` under every
+    /// policy.
+    #[test]
+    fn strict_policies_propagate_a_batch_members_error() {
+        let sk = fresh();
+        let cert_hash = sample_hash();
+        let mut wrong_hash = sample_hash();
+        wrong_hash[0] ^= 0xff;
+
+        for policy in [Policy::AcceptKnown, Policy::RejectUnknown] {
+            let mut reg = Registry::new().with_policy(policy);
+            reg.register(DemoVerifier);
+            let good = signed_demo(&sk, cert_hash, "ok");
+            let bad = signed_demo(&sk, wrong_hash, "ok");
+            let err = reg
+                .verify_all(&[good, bad], cert_hash)
+                .expect_err("a strict policy must fail the whole batch");
+            assert!(
+                matches!(err, Error::Signature(_) | Error::CertHashMismatch),
+                "expected the underlying verification error, got {err:?}"
+            );
+        }
+    }
+
     /// A required type that is present but unverified (no verifier registered
     /// even though the type appears in `required_types`) must fail
     /// `verify_all` with `RequiredMissing`. Catches a misconfiguration where
@@ -341,6 +398,65 @@ mod tests {
         assert!(matches!(err, Error::RequiredMissing(t) if t == "demo/v1"));
     }
 
+    /// A cert-hash that does not match `sample_hash()`, so an attestation
+    /// signed against it fails `verify_signature` when the batch is checked
+    /// against `sample_hash()`.
+    fn other_hash() -> [u8; 32] {
+        let mut h = sample_hash();
+        h[0] ^= 0xff;
+        h
+    }
+
+    /// Under `RequireAll`, a malformed attestation (here: signed against the
+    /// wrong cert hash, so it fails signature verification) attached alongside a
+    /// valid required one must not abort the batch. The junk is dropped and the
+    /// cert still verifies. Before the fix, `verify_all` propagated the error
+    /// via `?` and the whole batch failed — the DoS vector the docstring warns
+    /// about.
+    #[test]
+    fn require_all_drops_a_malformed_extra_and_still_accepts() {
+        let sk = fresh();
+        let cert_hash = sample_hash();
+        let mut reg = Registry::new()
+            .with_policy(Policy::RequireAll)
+            .require_types(["demo/v1"]);
+        reg.register(DemoVerifier);
+
+        // Signed against `other_hash()`, so it will not verify under `cert_hash`.
+        let malformed = signed_demo(&sk, other_hash(), "boom");
+        let real = signed_demo(&sk, cert_hash, "ok");
+
+        let verified = reg
+            .verify_all(&[malformed, real], cert_hash)
+            .expect("a malformed extra must not abort the batch under RequireAll");
+        assert_eq!(verified.len(), 1, "the malformed entry should be dropped");
+        assert!(verified
+            .iter()
+            .any(|v| v.type_ == "demo/v1" && v.fully_verified));
+    }
+
+    /// Leniency must not open a hole: if the *only* attestation of a required
+    /// type is malformed, dropping it leaves the required type absent and
+    /// `verify_all` must still fail with `RequiredMissing`. Guards against a fix
+    /// that skips errors so eagerly it lets an unverified required type pass.
+    #[test]
+    fn require_all_rejects_when_only_a_malformed_required_type_is_present() {
+        let sk = fresh();
+        let cert_hash = sample_hash();
+        let mut reg = Registry::new()
+            .with_policy(Policy::RequireAll)
+            .require_types(["demo/v1"]);
+        reg.register(DemoVerifier);
+
+        // The sole demo/v1 is bound to the wrong cert hash → fails verification.
+        let malformed_required = signed_demo(&sk, other_hash(), "ok");
+
+        let err = reg
+            .verify_all(&[malformed_required], cert_hash)
+            .unwrap_err();
+        assert!(matches!(err, Error::RequiredMissing(t) if t == "demo/v1"));
+    }
+
     #[test]
     fn payload_check_failure_rejects() {
         let sk = fresh();

crates/gitlawb-node/src/api/bounties.rs

@@ -285,11 +285,22 @@ pub async fn submit_bounty(
     Path(id): Path<String>,
     Json(req): Json<SubmitBountyRequest>,
 ) -> Result<Json<BountyRecord>> {
-    let bounty = state
-        .db
-        .get_bounty(&id)
-        .await?
-        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;
+    let not_found = || AppError::NotFound(format!("bounty {id} not found"));
+
+    let bounty = state.db.get_bounty(&id).await?.ok_or_else(not_found)?;
+
+    if crate::api::authorize_repo_read(
+        &state,
+        &bounty.repo_owner,
+        &bounty.repo_name,
+        Some(auth.0.as_str()),
+        "/",
+    )
+    .await
+    .is_err()
+    {
+        return Err(not_found());
+    }
 
     if bounty.status != "claimed" {
         return Err(AppError::BadRequest(format!(
@@ -325,11 +336,22 @@ pub async fn approve_bounty(
     Path(id): Path<String>,
     Json(req): Json<ApproveBountyRequest>,
 ) -> Result<Json<BountyRecord>> {
-    let bounty = state
-        .db
-        .get_bounty(&id)
-        .await?
-        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;
+    let not_found = || AppError::NotFound(format!("bounty {id} not found"));
+
+    let bounty = state.db.get_bounty(&id).await?.ok_or_else(not_found)?;
+
+    if crate::api::authorize_repo_read(
+        &state,
+        &bounty.repo_owner,
+        &bounty.repo_name,
+        Some(auth.0.as_str()),
+        "/",
+    )
+    .await
+    .is_err()
+    {
+        return Err(not_found());
+    }
 
     if bounty.status != "submitted" {
         return Err(AppError::BadRequest(format!(
@@ -372,11 +394,22 @@ pub async fn cancel_bounty(
     Extension(auth): Extension<AuthenticatedDid>,
     Path(id): Path<String>,
 ) -> Result<Json<BountyRecord>> {
-    let bounty = state
-        .db
-        .get_bounty(&id)
-        .await?
-        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;
+    let not_found = || AppError::NotFound(format!("bounty {id} not found"));
+
+    let bounty = state.db.get_bounty(&id).await?.ok_or_else(not_found)?;
+
+    if crate::api::authorize_repo_read(
+        &state,
+        &bounty.repo_owner,
+        &bounty.repo_name,
+        Some(auth.0.as_str()),
+        "/",
+    )
+    .await
+    .is_err()
+    {
+        return Err(not_found());
+    }
 
     if bounty.status != "open" {
         return Err(AppError::BadRequest(format!(
@@ -407,11 +440,22 @@ pub async fn dispute_bounty(
     Extension(auth): Extension<AuthenticatedDid>,
     Path(id): Path<String>,
 ) -> Result<Json<BountyRecord>> {
-    let bounty = state
-        .db
-        .get_bounty(&id)
-        .await?
-        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;
+    let not_found = || AppError::NotFound(format!("bounty {id} not found"));
+
+    let bounty = state.db.get_bounty(&id).await?.ok_or_else(not_found)?;
+
+    if crate::api::authorize_repo_read(
+        &state,
+        &bounty.repo_owner,
+        &bounty.repo_name,
+        Some(auth.0.as_str()),
+        "/",
+    )
+    .await
+    .is_err()
+    {
+        return Err(not_found());
+    }
 
     if bounty.status != "claimed" && bounty.status != "submitted" {
         return Err(AppError::BadRequest(format!(
@@ -456,21 +500,68 @@ pub async fn dispute_bounty(
     Ok(Json(updated))
 }
 
+/// Resolve the set of `(owner_did, name)` pairs for repos an anonymous caller
+/// can list.  Mirrors the `stats()` pattern in `server.rs` (#104): batch-load
+/// all deduped repos, batch-load their visibility rules, keep only those that
+/// pass `listable_at_root`.  Pure I/O after the two DB round-trips — no
+/// per-repo authorization queries.
+///
+/// Both bounty stats handlers mount behind `optional_signature` (see
+/// `bounty_read_routes` in server.rs), so the caller is always `None` for
+/// anonymous access.  Every caller, signed or not, currently receives the
+/// anonymous-scoped aggregates (matching the #104 pattern); per-caller
+/// visibility can be threaded through here when that work lands.
+///
+/// A database error is returned to the caller, never collapsed to an empty
+/// set: an empty set would render as HTTP 200 with zero counts, which a client
+/// reads as confirmed absence of activity.
+async fn visible_repo_pairs(state: &AppState) -> Result<Vec<(String, String)>> {
+    let rows = state.db.list_all_repos_deduped().await?;
+    let ids: Vec<String> = rows.iter().map(|r| r.id.clone()).collect();
+    let rules_by_repo = state.db.list_visibility_rules_for_repos(&ids).await?;
+    let pairs = rows
+        .iter()
+        .filter(|r| {
+            let rules = rules_by_repo.get(&r.id).map(Vec::as_slice).unwrap_or(&[]);
+            // mirror-rows-handled: a mirror with a canonical twin is deduped to the
+            // canonical row and its rules; a mirror-only repo counts as public, the
+            // same answer authorize_repo_read and /stats give. Pinned by
+            // bounty_stats_mirror_rows_follow_canonical_twin_or_count_when_alone.
+            crate::visibility::listable_at_root(rules, r.is_public, &r.owner_did, None)
+        })
+        .map(|r| {
+            (
+                crate::db::normalize_owner_key(&r.owner_did).to_string(),
+                r.name.clone(),
+            )
+        })
+        .collect();
+    Ok(pairs)
+}
+
 /// GET /api/v1/bounties/stats
+///
+/// Aggregates are restricted to anonymously-listable repos so private-repo
+/// bounty activity is not exposed (#477).  The visible-repo set is resolved
+/// once per request via `visible_repo_pairs` (two SQL round-trips), then
+/// passed into the filtered aggregate queries.
 pub async fn bounty_stats(State(state): State<AppState>) -> Result<Json<BountyStatsResponse>> {
-    let open = state.db.count_bounties_by_status("open").await.unwrap_or(0);
+    let visible = visible_repo_pairs(&state).await?;
+
+    let open = state
+        .db
+        .count_bounties_by_status_visible("open", &visible)
+        .await?;
     let claimed = state
         .db
-        .count_bounties_by_status("claimed")
-        .await
-        .unwrap_or(0);
+        .count_bounties_by_status_visible("claimed", &visible)
+        .await?;
     let completed = state
         .db
-        .count_bounties_by_status("completed")
-        .await
-        .unwrap_or(0);
+        .count_bounties_by_status_visible("completed", &visible)
+        .await?;
 
-    let leaders = state.db.bounty_leaderboard(10).await.unwrap_or_default();
+    let leaders = state.db.bounty_leaderboard_visible(10, &visible).await?;
     let leaderboard = leaders
         .into_iter()
         .map(|(did, cnt, total)| AgentBountyEntry {
@@ -489,11 +580,14 @@ pub async fn bounty_stats(State(state): State<AppState>) -> Result<Json<BountySt
 }
 
 /// GET /api/v1/agents/{did}/bounties
+///
+/// Per-agent earnings restricted to anonymously-listable repos (#477).
 pub async fn agent_bounty_stats(
     State(state): State<AppState>,
     Path(did): Path<String>,
 ) -> Result<Json<serde_json::Value>> {
-    let (count, total) = state.db.agent_bounty_stats(&did).await.unwrap_or((0, 0));
+    let visible = visible_repo_pairs(&state).await?;
+    let (count, total) = state.db.agent_bounty_stats_visible(&did, &visible).await?;
     Ok(Json(serde_json::json!({
         "did": did,
         "completed_bounties": count,

crates/gitlawb-node/src/api/labels.rs

@@ -14,14 +14,13 @@ pub struct LabelRequest {
     pub label: String,
 }
 
-/// POST /api/v1/repos/:owner/:repo/labels
-pub async fn add_label(
-    State(state): State<AppState>,
-    Extension(auth): Extension<AuthenticatedDid>,
-    Path((owner, name)): Path<(String, String)>,
-    Json(req): Json<LabelRequest>,
-) -> Result<(StatusCode, Json<serde_json::Value>)> {
-    let label = req.label.trim().to_lowercase();
+/// Canonicalize and validate a label string. `add_label` and `remove_label`
+/// must agree on the form stored in the DB: `add_label` already trims and
+/// lowercases before storing, so `remove_label` has to do the same to find
+/// the row. Validation is shared so neither handler can drift on charset or
+/// length (#344).
+fn canonicalize_label(raw: &str) -> std::result::Result<String, AppError> {
+    let label = raw.trim().to_lowercase();
     if label.is_empty() || label.len() > 50 {
         return Err(AppError::BadRequest("label must be 1–50 characters".into()));
     }
@@ -33,6 +32,17 @@ pub async fn add_label(
             "label must contain only alphanumeric characters, hyphens, and colons".into(),
         ));
     }
+    Ok(label)
+}
+
+/// POST /api/v1/repos/:owner/:repo/labels
+pub async fn add_label(
+    State(state): State<AppState>,
+    Extension(auth): Extension<AuthenticatedDid>,
+    Path((owner, name)): Path<(String, String)>,
+    Json(req): Json<LabelRequest>,
+) -> Result<(StatusCode, Json<serde_json::Value>)> {
+    let label = canonicalize_label(&req.label)?;
 
     let record = state
         .db
@@ -59,6 +69,8 @@ pub async fn remove_label(
     Extension(auth): Extension<AuthenticatedDid>,
     Path((owner, name, label)): Path<(String, String, String)>,
 ) -> Result<Json<serde_json::Value>> {
+    let label = canonicalize_label(&label)?;
+
     let record = state
         .db
         .get_repo(&owner, &name)
@@ -66,7 +78,10 @@ pub async fn remove_label(
         .ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
     crate::api::require_repo_owner(&record, &auth.0)?;
 
-    state.db.remove_label(&record.id, &label).await?;
+    let removed = state.db.remove_label(&record.id, &label).await?;
+    if !removed {
+        return Err(AppError::NotFound(format!("label '{label}'")));
+    }
     Ok(Json(serde_json::json!({ "label": label, "removed": true })))
 }
 

crates/gitlawb-node/src/api/peers.rs

@@ -70,6 +70,8 @@ pub struct PeerResponse {
     pub reachable: bool,
 }
 
+pub(crate) const PUBLIC_HTTP_URL_REQUIREMENT: &str = "must be a public http(s) URL (no loopback, private, localhost, .localhost, .internal, or .local hosts)";
+
 /// Extract an IPv4 address embedded in an IPv6 literal across the transition
 /// formats that carry one: IPv4-mapped (`::ffff:a.b.c.d`), IPv4-compatible
 /// (`::a.b.c.d`), 6to4 (`2002:WWXX:YYZZ::/16`), and the NAT64 well-known prefix
@@ -104,8 +106,8 @@ fn embedded_ipv4(v6: std::net::Ipv6Addr) -> Option<std::net::Ipv4Addr> {
 
 /// Whether a peer `http_url` is a public http(s) endpoint safe to register.
 /// Rejects non-http(s) schemes, loopback/unspecified/private/link-local IPs,
-/// and `localhost` / `.local` / `.internal` hostnames. Used at announce time
-/// and by the boot-time prune of already-poisoned rows.
+/// and `localhost` / `.localhost` / `.local` / `.internal` hostnames. Used at
+/// announce time and by the boot-time prune of already-poisoned rows.
 pub fn is_public_http_url(raw: &str) -> bool {
     let url = match reqwest::Url::parse(raw) {
         Ok(u) => u,
@@ -125,6 +127,7 @@ pub fn is_public_http_url(raw: &str) -> bool {
     host.truncate(host.trim_end_matches('.').len());
     if host.is_empty()
         || host == "localhost"
+        || host.ends_with(".localhost")
         || host.ends_with(".local")
         || host.ends_with(".internal")
     {
@@ -260,9 +263,9 @@ pub async fn announce(
     // and turn our outbound sync-notify fan-out into an SSRF probe — and bury
     // the real peers under junk so node-origin repos stop replicating.
     if !is_public_http_url(&req.http_url) {
-        return Err(AppError::BadRequest(
-            "http_url must be a public http(s) URL (no loopback, private, or .internal/.local hosts)".into(),
-        ));
+        return Err(AppError::BadRequest(format!(
+            "http_url {PUBLIC_HTTP_URL_REQUIREMENT}"
+        )));
     }
 
     // Reject self-announcements: a peer row whose http_url is our own public
@@ -541,6 +544,7 @@ mod tests {
     fn rejects_loopback_private_and_internal() {
         for bad in [
             "http://localhost:7545",
+            "http://node.localhost:7545",
             "http://127.0.0.1:5432/",
             "http://localhost:22/",
             "http://0.0.0.0:7545",
@@ -620,11 +624,14 @@ mod tests {
     #[test]
     fn rejects_repeated_trailing_root_dots() {
         // Stripping a single root dot leaves `localhost.`, which matches
-        // neither the equality nor the suffix check. The subdomain form
-        // (`node.localhost..`) is not asserted here: `.localhost` is not yet a
-        // rejected suffix on this base, so that case belongs to whichever
-        // change adds it rather than to this one.
-        for bad in ["http://localhost../", "http://localhost...:7545"] {
+        // neither the equality nor the suffix check. The subdomain form now
+        // exercises the `.localhost` suffix rejection together with the extra
+        // trailing dot.
+        for bad in [
+            "http://localhost../",
+            "http://localhost...:7545",
+            "http://node.localhost../",
+        ] {
             assert!(!is_public_http_url(bad), "{bad:?} must be rejected");
         }
     }
@@ -1582,6 +1589,31 @@ mod tests {
         )
     }
 
+    #[sqlx::test]
+    async fn announce_rejects_localhost_subdomains_with_an_accurate_error(pool: PgPool) {
+        let state = test_state(pool).await;
+        let did = Keypair::generate().did().to_string();
+        let resp = announce_only(state.clone())
+            .oneshot(announce_as(
+                &did,
+                &announce_body(&did, "https://node.localhost:7545"),
+            ))
+            .await
+            .unwrap();
+
+        let (status, error, message) = status_and_error(resp).await;
+        assert_eq!(status, StatusCode::BAD_REQUEST);
+        assert_eq!(error, "bad_request");
+        assert_eq!(
+            message,
+            "http_url must be a public http(s) URL (no loopback, private, localhost, .localhost, .internal, or .local hosts)"
+        );
+        assert!(
+            snapshot(&state.db, &did).await.is_none(),
+            "a rejected .localhost announce must leave no row behind"
+        );
+    }
+
     /// U4 scenario 1, and the first test the keyid branch has ever had: a caller
     /// who proved control of one DID must not announce another's. Kills
     /// neutralizing the handler's keyid comparison, and kills demoting this

crates/gitlawb-node/src/api/repos.rs

@@ -3508,6 +3508,17 @@ pub struct ForkRepoRequest {
 }
 
 /// POST /api/v1/repos/:owner/:repo/fork
+/// Argv for the fork's mirror clone, with `--` before the first positional (#374).
+///
+/// Both positionals here are server-derived paths rather than caller-supplied
+/// URLs, so this is defence in depth rather than a reachable injection. It is
+/// still the same argv contract as the other clone sites, and it is extracted for
+/// the same reason: so the delimiter's placement is pinned by a test instead of
+/// resting on review.
+fn fork_clone_args<'a>(source: &'a str, dest: &'a str) -> [&'a str; 5] {
+    ["clone", "--mirror", "--", source, dest]
+}
+
 pub async fn fork_repo(
     State(state): State<AppState>,
     Extension(auth): Extension<AuthenticatedDid>,
@@ -3593,12 +3604,10 @@ pub async fn fork_repo(
 
     // Clone the source repo as a mirror
     let output = std::process::Command::new("git")
-        .args([
-            "clone",
-            "--mirror",
+        .args(fork_clone_args(
             source_path.to_str().unwrap_or(""),
             disk_path.to_str().unwrap_or(""),
-        ])
+        ))
         .output()
         .map_err(|e| AppError::Git(format!("git clone --mirror failed: {e}")))?;
 
@@ -4065,6 +4074,30 @@ fn dedupe_canonical_repos(rows: Vec<(RepoRecord, i64)>) -> Vec<(RepoRecord, i64)
 #[cfg(test)]
 mod tests {
     use super::*;
+
+    /// #374: the fork path builds its own `git clone --mirror` argv. Driven
+    /// through real git so a missing or misplaced `--` fails, matching the
+    /// coverage on the `gl clone`, `gl mirror`, and sync sinks.
+    #[test]
+    fn fork_clone_never_parses_the_source_as_a_git_option() {
+        let td = tempfile::TempDir::new().unwrap();
+        let dest = td.path().join("fork.git");
+        let injected = "--upload-pack=false";
+
+        let out = std::process::Command::new("git")
+            .env("LC_ALL", "C")
+            .args(fork_clone_args(injected, dest.to_str().unwrap()))
+            .output()
+            .unwrap();
+        let stderr = String::from_utf8_lossy(&out.stderr);
+
+        assert!(!out.status.success(), "an option-shaped source must fail");
+        assert!(
+            stderr.contains(&format!("repository '{injected}'")),
+            "git must name the source as the repository, not consume it as an option: {stderr}"
+        );
+    }
+
     use crate::auth::caller_authorized_to_push;
     use crate::error::AppError;
     use gitlawb_core::identity::Keypair;
@@ -8289,7 +8322,7 @@ mod tests {
         let add = server
             .mock("POST", mockito::Matcher::Any)
             .with_status(200)
-            .with_body(r#"{"Hash":"QmShouldNotHappen"}"#)
+            .with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
             .expect(0)
             .create_async()
             .await;

crates/gitlawb-node/src/api/webhooks.rs

@@ -46,9 +46,10 @@ pub async fn create_webhook(
     // endpoints (SSRF). Delivery runs on the shared no-redirect client
     // (main.rs), which closes the 3xx-to-internal bounce.
     if !crate::api::peers::is_public_http_url(&req.url) {
-        return Err(AppError::BadRequest(
-            "webhook URL must be a public http(s) URL (no loopback, private, or .internal/.local hosts)".into(),
-        ));
+        return Err(AppError::BadRequest(format!(
+            "webhook URL {}",
+            crate::api::peers::PUBLIC_HTTP_URL_REQUIREMENT
+        )));
     }
 
     let events = req.events.unwrap_or_else(|| vec!["*".into()]);
@@ -141,7 +142,19 @@ pub async fn delete_webhook(
 
 #[cfg(test)]
 mod tests {
-    use crate::api::peers::is_public_http_url;
+    use axum::extract::{Extension, Path, State};
+    use axum::Json;
+    use chrono::Utc;
+    use gitlawb_core::identity::Keypair;
+    use sqlx::PgPool;
+    use uuid::Uuid;
+
+    use super::{create_webhook, CreateWebhookRequest};
+    use crate::api::peers::{is_public_http_url, PUBLIC_HTTP_URL_REQUIREMENT};
+    use crate::auth::AuthenticatedDid;
+    use crate::db::RepoRecord;
+    use crate::error::AppError;
+    use crate::test_support::test_state;
 
     // create_webhook gates req.url through is_public_http_url. Pin the exact
     // SSRF targets from issue #81 so the webhook path can never regress to the
@@ -152,6 +165,7 @@ mod tests {
             "http://127.0.0.1:5432/",
             "http://169.254.169.254/latest/meta-data/",
             "http://localhost/",
+            "http://node.localhost/",
             "http://10.0.0.5/",
             "http://[::1]/",
             // IPv6 transition encodings smuggling loopback v4 (6to4 / NAT64).
@@ -169,4 +183,53 @@ mod tests {
         assert!(is_public_http_url("https://hooks.example.com/gitlawb"));
         assert!(is_public_http_url("http://203.0.113.10:7545/"));
     }
+
+    #[sqlx::test]
+    async fn webhook_localhost_rejection_uses_shared_public_url_contract(pool: PgPool) {
+        let state = test_state(pool).await;
+        let owner = Keypair::generate().did().to_string();
+        let repo_id = Uuid::new_v4().to_string();
+        state
+            .db
+            .create_repo(&RepoRecord {
+                id: repo_id.clone(),
+                name: "webhook-contract".to_string(),
+                owner_did: owner.clone(),
+                description: None,
+                is_public: true,
+                default_branch: "main".to_string(),
+                created_at: Utc::now(),
+                updated_at: Utc::now(),
+                disk_path: "/tmp/webhook-contract.git".to_string(),
+                forked_from: None,
+                machine_id: None,
+            })
+            .await
+            .unwrap();
+
+        let result = create_webhook(
+            State(state.clone()),
+            Extension(AuthenticatedDid(owner.clone())),
+            Path((owner, "webhook-contract".to_string())),
+            Json(CreateWebhookRequest {
+                url: "https://node.localhost/hook".to_string(),
+                secret: None,
+                events: None,
+            }),
+        )
+        .await;
+
+        match result {
+            Err(AppError::BadRequest(message)) => assert_eq!(
+                message,
+                format!("webhook URL {PUBLIC_HTTP_URL_REQUIREMENT}")
+            ),
+            Err(error) => panic!("unexpected webhook rejection: {error}"),
+            Ok(_) => panic!("a .localhost webhook must be rejected"),
+        }
+        assert!(
+            state.db.list_webhooks(&repo_id).await.unwrap().is_empty(),
+            "a rejected .localhost webhook must leave no row behind"
+        );
+    }
 }

crates/gitlawb-node/src/git/issues.rs

@@ -141,19 +141,10 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result<Option<S
         return Ok(None);
     }
 
-    // Try exact match first — fast path for callers passing the full UUID.
+    // List all refs that start with the given string; the glob also matches the exact
+    // ref. The id handed back is always git's own refname, never the request input.
     let exact_ref = format!("refs/gitlawb/issues/{id_or_prefix}");
-    let check = Command::new("git")
-        .args(["cat-file", "-e", &exact_ref])
-        .current_dir(repo_path)
-        .output()
-        .context("failed to run git cat-file -e")?;
-    if check.status.success() {
-        return Ok(Some(id_or_prefix.to_string()));
-    }
-
-    // Prefix search: list all refs that start with the given string.
-    let prefix_glob = format!("refs/gitlawb/issues/{id_or_prefix}*");
+    let prefix_glob = format!("{exact_ref}*");
     let list = Command::new("git")
         .args(["for-each-ref", "--format=%(refname)", &prefix_glob])
         .current_dir(repo_path)
@@ -165,25 +156,44 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result<Option<S
     }
 
     let output = String::from_utf8_lossy(&list.stdout);
-    let matches: Vec<&str> = output.lines().filter(|l| !l.trim().is_empty()).collect();
+    let matches: Vec<&str> = output
+        .lines()
+        .map(str::trim)
+        .filter(|l| !l.is_empty())
+        .collect();
 
-    match matches.len() {
-        0 => Ok(None),
-        1 => {
-            // Strip the "refs/gitlawb/issues/" prefix to get the bare ID.
-            let full_id = matches[0]
-                .trim()
-                .strip_prefix("refs/gitlawb/issues/")
-                .unwrap_or(matches[0].trim())
-                .to_string();
-            Ok(Some(full_id))
+    // An exact id wins over longer ids that share it as a prefix, but only if its
+    // object exists: for-each-ref also lists a ref whose blob is missing, so a
+    // dangling exact ref falls through to the prefix rules below.
+    let exact = match matches.iter().find(|m| **m == exact_ref) {
+        Some(m) => {
+            let check = Command::new("git")
+                .args(["cat-file", "-e", m])
+                .current_dir(repo_path)
+                .output()
+                .context("failed to run git cat-file -e")?;
+            check.status.success().then_some(*m)
         }
-        _ => anyhow::bail!(
-            "ambiguous issue prefix '{}': matches {} issues",
-            id_or_prefix,
-            matches.len()
-        ),
-    }
+        None => None,
+    };
+    let found = match exact {
+        Some(m) => m,
+        None => match matches.as_slice() {
+            [] => return Ok(None),
+            [m] => *m,
+            _ => anyhow::bail!(
+                "ambiguous issue prefix '{}': matches {} issues",
+                id_or_prefix,
+                matches.len()
+            ),
+        },
+    };
+    // Strip the "refs/gitlawb/issues/" prefix to get the bare ID.
+    let full_id = found
+        .strip_prefix("refs/gitlawb/issues/")
+        .unwrap_or(found)
+        .to_string();
+    Ok(Some(full_id))
 }
 
 /// Close an issue by updating its status to "closed" in the git ref.
@@ -267,6 +277,43 @@ mod tests {
         assert_eq!(resolved, Some(full_id.to_string()));
     }
 
+    #[test]
+    fn test_resolve_exact_id_wins_over_longer_id() {
+        let dir = TempDir::new().unwrap();
+        init_repo(&dir);
+        create_issue(dir.path(), "abc12345", r#"{"status":"open"}"#).unwrap();
+        create_issue(dir.path(), "abc12345-0000", r#"{"status":"open"}"#).unwrap();
+        let resolved = resolve_issue_id(dir.path(), "abc12345").unwrap();
+        assert_eq!(resolved, Some("abc12345".to_string()));
+    }
+
+    #[test]
+    fn test_resolve_dangling_exact_id_does_not_win_over_longer_id() {
+        let dir = TempDir::new().unwrap();
+        init_repo(&dir);
+        create_issue(dir.path(), "abc12345", r#"{"status":"open","n":1}"#).unwrap();
+        create_issue(dir.path(), "abc12345-0000", r#"{"status":"open","n":2}"#).unwrap();
+        // Dangle the exact ref: the ref stays listed by for-each-ref, but its blob is gone.
+        let out = Command::new("git")
+            .args(["rev-parse", "refs/gitlawb/issues/abc12345"])
+            .current_dir(dir.path())
+            .output()
+            .unwrap();
+        let hash = String::from_utf8(out.stdout).unwrap();
+        let hash = hash.trim();
+        std::fs::remove_file(
+            dir.path()
+                .join(".git/objects")
+                .join(&hash[..2])
+                .join(&hash[2..]),
+        )
+        .unwrap();
+
+        let result = resolve_issue_id(dir.path(), "abc12345");
+        assert!(result.unwrap_err().to_string().contains("ambiguous"));
+        assert!(close_issue(dir.path(), "abc12345").is_err());
+    }
+
     #[test]
     fn test_resolve_prefix_matches_unique() {
         let dir = TempDir::new().unwrap();

crates/gitlawb-node/src/ipfs_pin.rs

@@ -1508,9 +1508,6 @@ pub async fn pin_git_object(
         return Ok(String::new());
     }
 
-    // Compute the expected CIDv1 from the content bytes
-    let expected_cid = Cid::from_git_object_bytes(data).to_string();
-
     let url = format!(
         "{}/api/v0/add?cid-version=1&raw-leaves=true&pin=true",
         ipfs_api.trim_end_matches('/')
@@ -1557,6 +1554,10 @@ pub async fn pin_git_object(
         .text()
         .await
         .map_err(|e| anyhow::anyhow!("IPFS add response body read failed: {e}"))?;
+    // A 200 without a parseable Hash is not a successful pin. Fabricating the
+    // caller-computed CID would record an address the backend never confirmed;
+    // above its chunk threshold that raw CID is not even a stored block, so
+    // every later cat fails permanently.
     let cid = body
         .lines()
         .filter(|l| !l.trim().is_empty())
@@ -1565,7 +1566,17 @@ pub async fn pin_git_object(
             v["Hash"].as_str().map(|s| s.to_string())
         })
         .next_back()
-        .unwrap_or(expected_cid.clone());
+        .ok_or_else(|| {
+            anyhow::anyhow!(
+                "IPFS /api/v0/add returned 200 without a usable Hash: {:.200}",
+                body
+            )
+        })?;
+    // The string still has to be a CID. A garbage Hash would be recorded and
+    // later cat-ed the same way a missing one was, so reject it here.
+    Cid::from_str(&cid).map_err(|e| {
+        anyhow::anyhow!("IPFS /api/v0/add returned a Hash that is not a CID ({cid:.200}): {e}")
+    })?;
 
     tracing::debug!(sha256 = %sha256_hex, %cid, "pinned git object to IPFS");
     Ok(cid)
@@ -2223,16 +2234,16 @@ mod tests {
         endpoint
     }
 
-    /// A sleeping-but-live endpoint. Answers `200` with an empty body after
-    /// `delays[i]` for the i-th request it accepts (the last entry repeats), so
-    /// a test can make one add slow and the next fast. Drains the full request,
-    /// headers plus the declared `Content-Length` body, before sleeping: exactly
-    /// as in `rejecting_endpoint`, answering early and closing would surface as
-    /// a write failure on the client and turn a slow-but-healthy add into a
-    /// different failure shape.
+    /// A sleeping-but-live endpoint. Answers `200` with a conformant NDJSON add
+    /// response after `delays[i]` for the i-th request it accepts (the last
+    /// entry repeats), so a test can make one add slow and the next fast.
+    /// Drains the full request, headers plus the declared `Content-Length`
+    /// body, before sleeping: exactly as in `rejecting_endpoint`, answering
+    /// early and closing would surface as a write failure on the client and
+    /// turn a slow-but-healthy add into a different failure shape.
     ///
-    /// An empty body is a successful pin: `pin_git_object` falls back to the CID
-    /// it computed from the bytes when the response carries no `Hash`.
+    /// The Hash is a fixed, real CID: the timing tests assert on pin counts,
+    /// not on the recorded address.
     async fn delaying_endpoint(delays: Vec<Duration>) -> String {
         use tokio::io::{AsyncReadExt, AsyncWriteExt};
         let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
@@ -2269,9 +2280,14 @@ mod tests {
                         }
                     }
                     tokio::time::sleep(delay).await;
-                    let _ = sock
-                        .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n")
-                        .await;
+                    let body = br#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4","Size":"12"}
+"#;
+                    let resp = format!(
+                        "HTTP/1.1 200 OK\r\nContent-Type: application/x-ndjson\r\nContent-Length: {}\r\n\r\n",
+                        body.len()
+                    );
+                    let _ = sock.write_all(resp.as_bytes()).await;
+                    let _ = sock.write_all(body).await;
                     let _ = sock.flush().await;
                 });
             }
@@ -2373,6 +2389,71 @@ mod tests {
         );
     }
 
+    /// A 200 add response with no parseable Hash must surface as an add
+    /// failure. Falling back to the caller-computed CID records an
+    /// unverifiable address: above the backend's chunk threshold that raw
+    /// CID is not a stored block, so every later cat 500s.
+    #[tokio::test]
+    async fn pin_git_object_rejects_a_200_without_a_hash() {
+        let mut server = mockito::Server::new_async().await;
+        let _m = server
+            .mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
+            .with_status(200)
+            .with_body(r#"{"Name":"object","Size":"19"}"#)
+            .create_async()
+            .await;
+        let result = pin_git_object(&server.url(), "deadbeef", b"some object bytes\n", None).await;
+        assert!(
+            result.is_err(),
+            "a 200 without a Hash must be an add failure, not a fabricated CID: {result:?}"
+        );
+    }
+
+    /// A Hash that is a string but not a CID must also fail. It would be
+    /// recorded into `encrypted_blobs.cid` and every later cat would miss.
+    #[tokio::test]
+    async fn pin_git_object_rejects_a_malformed_hash() {
+        let mut server = mockito::Server::new_async().await;
+        let _m = server
+            .mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
+            .with_status(200)
+            .with_body(r#"{"Hash":"not-a-cid","Size":"19"}"#)
+            .create_async()
+            .await;
+        let result = pin_git_object(&server.url(), "deadbeef", b"some object bytes\n", None).await;
+        assert!(
+            result.is_err(),
+            "a Hash that is not a CID must be an add failure: {result:?}"
+        );
+    }
+
+    /// The Hash text goes into the error and `encrypted_pin` logs that error,
+    /// so a backend answering with a huge invalid Hash must not produce an
+    /// unbounded log entry.
+    #[tokio::test]
+    async fn pin_git_object_bounds_an_oversized_invalid_hash_in_the_error() {
+        let long = "x".repeat(5000);
+        let mut server = mockito::Server::new_async().await;
+        let _m = server
+            .mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
+            .with_status(200)
+            .with_body(format!(r#"{{"Hash":"{long}","Size":"19"}}"#))
+            .create_async()
+            .await;
+        let result = pin_git_object(&server.url(), "deadbeef", b"some object bytes\n", None).await;
+        let err = result.expect_err("an invalid Hash must be an add failure");
+        let msg = err.to_string();
+        assert!(
+            msg.len() < 1000,
+            "the error must bound the echoed Hash, got {} bytes",
+            msg.len()
+        );
+        assert!(
+            !msg.contains(&long),
+            "the error must not carry the full invalid Hash"
+        );
+    }
+
     /// The permit-hold bound. `pin_new_objects` runs under a deferring
     /// `pin_semaphore`, so without a batch deadline the hold is O(N) with N
     /// chosen by the pusher. Five objects against an endpoint that takes 2s

crates/gitlawb-node/src/metrics.rs

@@ -314,7 +314,9 @@ pub fn record_sync_processed(status: &str) {
 }
 
 /// Record one webhook delivery attempt outcome. `result` ∈
-/// {ok, http_error, network_error, skipped}.
+/// {ok, http_error, network_error, skipped, invalid_url}.
+/// `invalid_url` is a policy skip: the stored hook URL failed the public-URL
+/// check at delivery time and the POST was never sent.
 pub fn record_webhook_delivery(result: &str) {
     if let Some(c) = WEBHOOK_DELIVERIES.get() {
         c.with_label_values(&[result]).inc();

crates/gitlawb-node/src/sync.rs

@@ -701,6 +701,7 @@ async fn clone_repo(remote_url: &str, local_path: &Path, mode: MirrorMode) -> an
     if mode == MirrorMode::Promisor {
         args.push("--filter=blob:limit=10g");
     }
+    args.push("--");
     args.push(remote_url);
     args.push(local_str);
 
@@ -729,7 +730,10 @@ async fn clone_repo(remote_url: &str, local_path: &Path, mode: MirrorMode) -> an
 async fn fetch_repo(local_path: &Path, remote_url: &str, mode: MirrorMode) -> anyhow::Result<()> {
     let local_str = local_path.to_str().unwrap_or(".");
 
-    git_run(&["-C", local_str, "remote", "set-url", "origin", remote_url]).await?;
+    git_run(&[
+        "-C", local_str, "remote", "set-url", "origin", "--", remote_url,
+    ])
+    .await?;
 
     match mode {
         MirrorMode::Promisor => {
@@ -1006,6 +1010,82 @@ mod tests {
         assert_eq!(probe, PromisorProbe::NotPromisor);
     }
 
+    /// #374: `clone_repo` interpolates the peer's origin URL into a `git
+    /// clone --mirror` argument list. Without a `--` delimiter git parses a
+    /// remote beginning with `-` as an option instead of a repository.
+    ///
+    /// The delimiter is what forces the same string to be a repository, and
+    /// git says so in its own words: it reports the whole argument as a
+    /// repository it cannot find. Undelimited, git consumes it as an option
+    /// and fails against the *destination* instead (`fatal: repository '<dest>' does not exist`),
+    /// never naming the injected string — which is why this assertion is RED
+    /// before the fix and GREEN after.
+    ///
+    /// The payload is deliberately free of `:` and spaces: a colon would make
+    /// git read the argument as an ssh-style `host:path` URL and report a
+    /// hostname rather than a repository, which is a different code path.
+    #[tokio::test]
+    async fn clone_never_parses_a_remote_as_a_git_option() {
+        let td = TempDir::new().unwrap();
+        let dest = td.path().join("mirror.git");
+        let injected = "--upload-pack=false";
+
+        let err = clone_repo(injected, &dest, MirrorMode::Plain)
+            .await
+            .expect_err("a remote that is not a repository must fail the clone")
+            .to_string();
+
+        assert!(
+            err.contains(injected),
+            "git must report the argument as a repository, not consume it as an option: {err}"
+        );
+    }
+
+    /// #374: `fetch_repo` interpolates the peer's origin URL into
+    /// `git remote set-url`, a second argv construction with the same contract as
+    /// the clone. It needs its own proof: the clone test cannot reach this path.
+    ///
+    /// The assertion is on stored state rather than on the error string, and that
+    /// is deliberate. `git_run` formats its failure as `git {args:?} failed:
+    /// {stderr}`, so the injected value is in the message via the debug-printed
+    /// argv whether or not the delimiter is present — asserting on containment
+    /// would pass with `--` deleted. Delimited, `set-url` succeeds and stores the
+    /// value verbatim; undelimited, git exits 129 with `unknown option` and leaves
+    /// the previous URL in place, so this goes red without the fix.
+    ///
+    /// The fetch that follows the rewrite fails, because the stored URL is not a
+    /// repository. That is expected and ignored: the property under test is what
+    /// `set-url` wrote, not whether the subsequent fetch could succeed.
+    #[tokio::test]
+    async fn fetch_never_parses_a_remote_as_a_git_option() {
+        let td = TempDir::new().unwrap();
+        let local = td.path().join("mirror.git");
+        let local_str = local.to_str().unwrap();
+        let placeholder = "http://example.invalid/x.git";
+
+        assert!(Command::new("git")
+            .args(["init", "-q", "--bare", local_str])
+            .status()
+            .unwrap()
+            .success());
+        assert!(Command::new("git")
+            .args(["-C", local_str, "remote", "add", "origin", placeholder])
+            .status()
+            .unwrap()
+            .success());
+
+        let injected = "--upload-pack=false";
+        let _ = fetch_repo(&local, injected, MirrorMode::Plain).await;
+
+        assert_eq!(
+            git_config_get(local_str, "remote.origin.url")
+                .await
+                .as_deref(),
+            Some(injected),
+            "set-url must store an option-shaped remote verbatim, not reject it as an option"
+        );
+    }
+
     #[tokio::test]
     async fn probe_reports_unknown_on_git_error() {
         // A path git cannot resolve as a repo at all (exit 128) is an indeterminate

crates/gitlawb-node/src/webhooks.rs

@@ -79,6 +79,19 @@ async fn fire_event_async(
         let delivery_id = uuid::Uuid::new_v4().to_string();
 
         tokio::spawn(async move {
+            // Policy skip, not a delivery failure: the stored URL predates the
+            // create-time gate. The "invalid_url" label is documented on
+            // record_webhook_delivery in metrics.rs.
+            if !crate::api::peers::is_public_http_url(&hook.url) {
+                crate::metrics::record_webhook_delivery("invalid_url");
+                tracing::warn!(
+                    url = %hook.url,
+                    event = %event_name,
+                    "webhook delivery skipped: stored URL is not a public http(s) target"
+                );
+                return;
+            }
+
             let signature = hook.secret.as_deref().map(|s| sign_payload(s, &bytes));
 
             let mut req = client

crates/gitlawb-node/tests/owner_push_default_docs.rs

@@ -0,0 +1,177 @@
+//! #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 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. `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
+//! 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(&section),
+        "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 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<String> {
+    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, 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 && (header_level == 0 || level <= header_level) {
+            end = line.as_ptr() as usize - body.as_ptr() as usize;
+            break;
+        }
+    }
+
+    Some(body[..end].to_string())
+}

crates/gl/src/agent.rs

@@ -88,7 +88,7 @@ async fn cmd_list(node: String, capability: Option<String>) -> Result<()> {
         let short = did
             .split(':')
             .next_back()
-            .map(|s| &s[..s.len().min(16)])
+            .map(|s| crate::text::truncate(s, 16))
             .unwrap_or("?");
         let trust = agent["trust_score"].as_f64().unwrap_or(0.0);
         let caps = agent["capabilities"]

crates/gl/src/bounty.rs

@@ -275,7 +275,7 @@ async fn cmd_list(
                 let title = b["title"].as_str().unwrap_or("?");
                 let amount = b["amount"].as_i64().unwrap_or(0);
                 let st = b["status"].as_str().unwrap_or("?");
-                let short_id = &id[..8.min(id.len())];
+                let short_id = crate::text::truncate(id, 8);
                 println!("{short_id}  {st:<10}  {amount:>12} $GITLAWB  {title}");
             }
         }
@@ -403,7 +403,7 @@ async fn cmd_stats(node: String) -> Result<()> {
                 let did = entry["did"].as_str().unwrap_or("?");
                 let count = entry["completed"].as_i64().unwrap_or(0);
                 let earned = entry["total_earned"].as_i64().unwrap_or(0);
-                let short = &did[did.len().saturating_sub(8)..];
+                let short = crate::text::truncate_suffix(did, 8);
                 println!(
                     "  {}. ...{short}  {count} bounties  {earned} $GITLAWB",
                     i + 1

crates/gl/src/cert.rs

@@ -117,7 +117,10 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
         let id = cert["id"].as_str().unwrap_or("?");
         let ref_name = cert["ref_name"].as_str().unwrap_or("?");
         let new_sha = cert["new_sha"].as_str().unwrap_or("?");
-        let issued_at = cert["issued_at"].as_str().map(|s| &s[..19]).unwrap_or("?");
+        let issued_at = cert["issued_at"]
+            .as_str()
+            .map(|s| crate::text::truncate(s, 19))
+            .unwrap_or("?");
         println!("  {id:.8}  {issued_at}  {ref_name}  {new_sha:.12}");
     }
     Ok(())

crates/gl/src/changelog.rs

@@ -75,14 +75,14 @@ pub async fn run(args: ChangelogArgs) -> Result<()> {
 
     for event in &events {
         let ts = event["timestamp"].as_str().unwrap_or("?");
-        let date = &ts[..ts.len().min(10)];
+        let date = crate::text::truncate(ts, 10);
 
         match event["type"].as_str().unwrap_or("") {
             "commit" => {
                 let sha = event["sha"].as_str().unwrap_or("?");
                 let msg = event["message"].as_str().unwrap_or("?");
                 let first_line = msg.lines().next().unwrap_or(msg);
-                let short_sha = &sha[..sha.len().min(8)];
+                let short_sha = crate::text::truncate(sha, 8);
                 println!("  {date}  commit      {short_sha}  {first_line}");
             }
             "pr_merged" => {

crates/gl/src/clone.rs

@@ -163,8 +163,8 @@ pub fn setup_partial_clone(
 
     if withheld_globs.is_empty() {
         match branch {
-            Some(b) => git_global(&["clone", "-q", "--branch", b, remote_url, dest_str])?,
-            None => git_global(&["clone", "-q", remote_url, dest_str])?,
+            Some(b) => git_global(&["clone", "-q", "--branch", b, "--", remote_url, dest_str])?,
+            None => git_global(&["clone", "-q", "--", remote_url, dest_str])?,
         }
         return Ok(());
     }
@@ -174,6 +174,7 @@ pub fn setup_partial_clone(
         "-q",
         "--filter=blob:none",
         "--no-checkout",
+        "--",
         remote_url,
         dest_str,
     ])?;
@@ -388,7 +389,7 @@ async fn recover_encrypted_blobs(
         let plaintext = match open_blob(&envelope, keypair) {
             Ok(p) => p,
             Err(e) => {
-                eprintln!("warning: could not decrypt {oid}: {e}");
+                emit_warning(&format!("warning: could not decrypt {oid}: {e}"));
                 continue;
             }
         };
@@ -406,7 +407,9 @@ async fn recover_encrypted_blobs(
                 recovered.push(p.clone());
             }
         } else {
-            eprintln!("warning: recovered blob {oid} hashed to {written}; discarding");
+            emit_warning(&format!(
+                "warning: recovered blob {oid} hashed to {written}; discarding"
+            ));
         }
     }
     Ok(recovered)
@@ -804,7 +807,9 @@ async fn recover_from_arweave(
                 recovered.push(p.clone());
             }
         } else {
-            eprintln!("warning: recovered blob {oid} hashed to {written}; discarding");
+            emit_warning(&format!(
+                "warning: recovered blob {oid} hashed to {written}; discarding"
+            ));
         }
     }
     Ok(recovered)
@@ -1090,6 +1095,85 @@ mod tests {
         );
     }
 
+    #[test]
+    fn setup_partial_clone_plain_without_withheld_paths() {
+        let (td, url) = bare_remote(&[("file.txt", b"hello\n")]);
+        let dest = td.path().join("dest");
+        setup_partial_clone(&dest, &url, &[], &[], None).unwrap();
+        assert!(dest.join("file.txt").exists());
+    }
+
+    #[test]
+    fn setup_partial_clone_with_branch_without_withheld_paths() {
+        let (td, url) = bare_remote(&[("file.txt", b"hello\n")]);
+        let branch_out = Command::new("git")
+            .args([
+                "-C",
+                td.path().join("origin").to_str().unwrap(),
+                "branch",
+                "--show-current",
+            ])
+            .output()
+            .unwrap();
+        let branch = String::from_utf8(branch_out.stdout)
+            .unwrap()
+            .trim()
+            .to_string();
+        let dest = td.path().join("dest");
+        setup_partial_clone(&dest, &url, &[], &[], Some(&branch)).unwrap();
+        assert!(dest.join("file.txt").exists());
+    }
+
+    /// #374: every `setup_partial_clone` shape interpolates `remote_url` into a
+    /// `git clone` argv. The `--` delimiter is what forces an option-shaped
+    /// remote to be read as a repository rather than parsed as an option.
+    ///
+    /// The assertion is on git's own words — `repository '<value>'`, single-quoted
+    /// — and not merely on the value appearing somewhere in the error. `git_global`
+    /// formats its failure as `git {args:?} failed: {stderr}`, so the injected
+    /// string is already present in the debug-printed argv whether or not the
+    /// delimiter is there; asserting on containment alone would pass with `--`
+    /// deleted. Undelimited, git consumes the value as an option, leaving the
+    /// destination as the only positional, and names *that* instead
+    /// (`repository '<dest>' does not exist`) — so this goes red without the fix.
+    ///
+    /// The payload carries no `:` or space: a colon would make git read the
+    /// argument as an ssh-style `host:path` URL and report a hostname, which is a
+    /// different parse.
+    fn assert_option_shaped_remote_is_a_repository(withheld: &[String], branch: Option<&str>) {
+        let td = TempDir::new().unwrap();
+        let dest = td.path().join("dest");
+        let injected = "--upload-pack=false";
+
+        let err = setup_partial_clone(&dest, injected, withheld, &[], branch)
+            .expect_err("an option-shaped remote is not a repository and must fail")
+            .to_string();
+
+        assert!(
+            err.contains(&format!("repository '{injected}'")),
+            "git must name the argument as the repository, not consume it as an option: {err}"
+        );
+    }
+
+    #[test]
+    fn plain_clone_never_parses_a_remote_as_a_git_option() {
+        assert_option_shaped_remote_is_a_repository(&[], None);
+    }
+
+    #[test]
+    fn branch_clone_never_parses_a_remote_as_a_git_option() {
+        // Pins that `--branch <b>` stays ahead of the delimiter: if `--` were moved
+        // before `--branch`, git would read the branch name as a positional instead.
+        assert_option_shaped_remote_is_a_repository(&[], Some("main"));
+    }
+
+    #[test]
+    fn sparse_clone_never_parses_a_remote_as_a_git_option() {
+        // The withheld-globs arm is a separate argv construction
+        // (`--filter=blob:none --no-checkout`) and needs its own proof.
+        assert_option_shaped_remote_is_a_repository(&["/secret/**".to_string()], None);
+    }
+
     #[test]
     fn sparse_patterns_subtree_and_exact() {
         assert_eq!(sparse_patterns("/secret/**"), vec!["/secret/".to_string()]);

crates/gl/src/ipfs_cmd.rs

@@ -131,11 +131,7 @@ async fn cmd_list(node: String, dir: Option<PathBuf>) -> Result<()> {
         let sha = pin["sha256_hex"].as_str().unwrap_or("?");
         let pinned_at = pin["pinned_at"].as_str().unwrap_or("?");
         // Trim pinned_at to date+time without subseconds
-        let ts = if pinned_at.len() >= 19 {
-            &pinned_at[..19]
-        } else {
-            pinned_at
-        };
+        let ts = crate::text::truncate(pinned_at, 19);
         println!("  {cid}");
         println!("    sha256: {sha}");
         println!("    pinned: {ts}");

crates/gl/src/issue.rs

@@ -251,7 +251,7 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
         let status = issue["status"].as_str().unwrap_or("?");
         let created = issue["created_at"]
             .as_str()
-            .map(|s| &s[..10])
+            .map(|s| crate::text::truncate(s, 10))
             .unwrap_or("?");
         let icon = match status {
             "open" => "○",
@@ -365,10 +365,13 @@ async fn cmd_issue_comments(
         let author_short = author
             .split(':')
             .next_back()
-            .map(|s| &s[..s.len().min(8)])
+            .map(|s| crate::text::truncate(s, 8))
             .unwrap_or("?");
         let cbody = c["body"].as_str().unwrap_or("");
-        let created = c["created_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
+        let created = c["created_at"]
+            .as_str()
+            .map(|s| crate::text::truncate(s, 10))
+            .unwrap_or("?");
         println!("  · {author_short} ({created})");
         println!("    {cbody}");
         println!();
@@ -439,6 +442,34 @@ mod tests {
         .unwrap();
     }
 
+    #[tokio::test]
+    async fn test_cmd_list_survives_malformed_timestamps() {
+        let dir = TempDir::new().unwrap();
+        write_identity(&dir);
+
+        let mut server = mockito::Server::new_async().await;
+        // A short timestamp used to panic on `&s[..10]`; a timestamp whose
+        // byte 10 is mid-char panics even with a len() guard.
+        let _m = server
+            .mock(
+                "GET",
+                mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/issues$".to_string()),
+            )
+            .with_status(200)
+            .with_header("content-type", "application/json")
+            .with_body(r#"{"issues":[{"id":"a","title":"short ts","status":"open","created_at":"2026"},{"id":"b","title":"mb ts","status":"open","created_at":"2026-08-1é5T00:00:00Z"},{"id":"c","title":"empty ts","status":"open","created_at":""}]}"#)
+            .create_async()
+            .await;
+
+        cmd_list(
+            "myrepo".to_string(),
+            server.url(),
+            Some(dir.path().to_path_buf()),
+        )
+        .await
+        .unwrap();
+    }
+
     #[tokio::test]
     async fn test_cmd_create_success() {
         let dir = TempDir::new().unwrap();

crates/gl/src/main.rs

@@ -30,6 +30,7 @@ mod star;
 mod status;
 mod sync;
 mod task;
+mod text;
 mod ucan_cmd;
 mod visibility;
 mod webhook;

crates/gl/src/mirror.rs

@@ -41,6 +41,17 @@ pub struct MirrorArgs {
     pub dir: Option<PathBuf>,
 }
 
+/// Argv for the mirror clone, with `--` immediately before the first positional
+/// so an option-shaped `source` cannot be parsed as a git option (#374).
+///
+/// This is a separate function purely so the delimiter's placement can be driven
+/// through real git by a test. The production call keeps `.status()` rather than
+/// `.output()`: a mirror clone of a large repo streams git's progress to the
+/// user's terminal, and capturing it to assert on stderr here would swallow that.
+fn mirror_clone_args(source: &str) -> [&str; 4] {
+    ["clone", "--mirror", "--", source]
+}
+
 pub async fn run(args: MirrorArgs) -> Result<()> {
     let source = args.source.trim_end_matches('/').to_string();
 
@@ -145,7 +156,7 @@ fn clone_mirror(source: &str, dest: &std::path::Path) -> Result<()> {
     // bounded-runner rule governs node request handlers. Same spawn as before,
     // only relocated into this helper.
     let status = Command::new("git")
-        .args(["clone", "--mirror", source])
+        .args(mirror_clone_args(source))
         .arg(dest)
         .status()
         .context("failed to run git clone — is git installed?")?;
@@ -186,6 +197,43 @@ pub fn extract_repo_name(url: &str) -> Option<String> {
 mod tests {
     use super::*;
 
+    /// #374: `gl mirror` interpolates the user-supplied source into a
+    /// `git clone --mirror` argv. `--` must sit immediately before the first
+    /// positional so an option-shaped source is read as a repository.
+    ///
+    /// This drives the production argv through real git rather than asserting on
+    /// the array's contents, so it fails on a delimiter that is missing *or*
+    /// misplaced. It cannot go through `run()`: that loads an identity keypair and
+    /// contacts a node before reaching the clone, and its bail message interpolates
+    /// `{source}` unconditionally, so an assertion there would pass with `--` gone.
+    ///
+    /// Asserting on git's single-quoted `repository '<value>'` is what makes this
+    /// red without the delimiter — undelimited, git consumes the value as an option
+    /// and names the destination as the repository instead.
+    #[test]
+    fn mirror_clone_never_parses_the_source_as_a_git_option() {
+        let td = tempfile::TempDir::new().unwrap();
+        let dest = td.path().join("mirror.git");
+        let injected = "--upload-pack=false";
+
+        let out = Command::new("git")
+            .env("LC_ALL", "C")
+            .args(mirror_clone_args(injected))
+            .arg(&dest)
+            .output()
+            .unwrap();
+        let stderr = String::from_utf8_lossy(&out.stderr);
+
+        assert!(
+            !out.status.success(),
+            "an option-shaped source is not a repository and must fail the clone"
+        );
+        assert!(
+            stderr.contains(&format!("repository '{injected}'")),
+            "git must name the source as the repository, not consume it as an option: {stderr}"
+        );
+    }
+
     #[test]
     fn test_extract_github_url() {
         assert_eq!(
@@ -330,7 +378,8 @@ mod tests {
             .create_async()
             .await;
 
-        // We can't easily test the git subprocess calls, but we can test the
+        // The git subprocess argv is pinned separately by
+        // `mirror_clone_never_parses_the_source_as_a_git_option`; this covers the
         // API error path by calling the create step directly via NodeClient.
         let kp2 = gitlawb_core::identity::Keypair::generate();
         let client = NodeClient::new(server.url(), Some(kp2));

crates/gl/src/node.rs

@@ -413,7 +413,7 @@ async fn cmd_status(node: String, dir: Option<PathBuf>) -> Result<()> {
                     let ref_name = ev["ref"].as_str().unwrap_or("?");
                     let ts = ev["timestamp"]
                         .as_str()
-                        .map(|s| &s[..10.min(s.len())])
+                        .map(|s| crate::text::truncate(s, 10))
                         .unwrap_or("?");
                     println!("  {ts}  {repo}  {ref_name}");
                 }

crates/gl/src/peer.rs

@@ -97,7 +97,7 @@ async fn cmd_list(node: String) -> Result<()> {
         let reachable = peer["reachable"].as_bool().unwrap_or(false);
         let last_seen = peer["last_seen"]
             .as_str()
-            .map(|s| &s[..10])
+            .map(|s| crate::text::truncate(s, 10))
             .unwrap_or("never");
         let status = if reachable { "✓" } else { "✗" };
         println!("  {status} {url}");

crates/gl/src/pr.rs

@@ -304,7 +304,7 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
         let author_short = author
             .split(':')
             .next_back()
-            .map(|s| &s[..s.len().min(8)])
+            .map(|s| crate::text::truncate(s, 8))
             .unwrap_or("?");
         let status_icon = match status {
             "open" => "○",
@@ -374,7 +374,7 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option<PathBuf>)
             let reviewer_short = reviewer
                 .split(':')
                 .next_back()
-                .map(|s| &s[..s.len().min(8)])
+                .map(|s| crate::text::truncate(s, 8))
                 .unwrap_or("?");
             let rstatus = r["status"].as_str().unwrap_or("?");
             let rbody = r["body"].as_str().unwrap_or("");
@@ -414,10 +414,13 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option<PathBuf>)
             let author_short = author
                 .split(':')
                 .next_back()
-                .map(|s| &s[..s.len().min(8)])
+                .map(|s| crate::text::truncate(s, 8))
                 .unwrap_or("?");
             let cbody = c["body"].as_str().unwrap_or("");
-            let created = c["created_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
+            let created = c["created_at"]
+                .as_str()
+                .map(|s| crate::text::truncate(s, 10))
+                .unwrap_or("?");
             println!("  · {author_short} ({created})");
             println!("    {cbody}");
         }
@@ -464,7 +467,7 @@ async fn cmd_merge(repo: String, number: u64, node: String, dir: Option<PathBuf>
 
     let sha = result["merge_sha"].as_str().unwrap_or("?");
     println!("✓ Merged PR #{number}");
-    println!("  Merge commit: {}", &sha[..sha.len().min(12)]);
+    println!("  Merge commit: {}", crate::text::truncate(sha, 12));
     Ok(())
 }
 
@@ -555,10 +558,13 @@ async fn cmd_comments(repo: String, number: u64, node: String, dir: Option<PathB
         let author_short = author
             .split(':')
             .next_back()
-            .map(|s| &s[..s.len().min(8)])
+            .map(|s| crate::text::truncate(s, 8))
             .unwrap_or("?");
         let cbody = c["body"].as_str().unwrap_or("");
-        let created = c["created_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
+        let created = c["created_at"]
+            .as_str()
+            .map(|s| crate::text::truncate(s, 10))
+            .unwrap_or("?");
         println!("  · {author_short} ({created})");
         println!("    {cbody}");
         println!();

crates/gl/src/repo.rs

@@ -337,7 +337,10 @@ async fn cmd_list(node: String, dir: Option<PathBuf>) -> Result<()> {
         let name = r["name"].as_str().unwrap_or("?");
         let desc = r["description"].as_str().unwrap_or("");
         let public = r["is_public"].as_bool().unwrap_or(true);
-        let updated = r["updated_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
+        let updated = r["updated_at"]
+            .as_str()
+            .map(|s| crate::text::truncate(s, 10))
+            .unwrap_or("?");
         let vis = if public { "public" } else { "private" };
         println!("  {name:<24}  {vis:<8}  {updated}  {desc}");
     }
@@ -514,7 +517,7 @@ async fn cmd_replicas(repo: String, node: String, dir: Option<PathBuf>) -> Resul
             let url = r["replica_url"].as_str().unwrap_or("?");
             let registered = r["registered_at"]
                 .as_str()
-                .map(|s| &s[..10.min(s.len())])
+                .map(|s| crate::text::truncate(s, 10))
                 .unwrap_or("?");
             println!("  {registered}  {did}  →  {url}");
         }
@@ -556,7 +559,7 @@ pub(crate) async fn cmd_commits(
             .or_else(|| c["sha"].as_str())
             .or_else(|| c["oid"].as_str())
             .unwrap_or("?");
-        let short_sha = &sha[..sha.len().min(10)];
+        let short_sha = crate::text::truncate(sha, 10);
         let msg = c["message"].as_str().unwrap_or("(no message)");
         let first_line = msg.lines().next().unwrap_or(msg);
         let author = c["author_name"]
@@ -566,7 +569,7 @@ pub(crate) async fn cmd_commits(
         let date = c["date"]
             .as_str()
             .or_else(|| c["committer_date"].as_str())
-            .map(|s| &s[..10.min(s.len())])
+            .map(|s| crate::text::truncate(s, 10))
             .unwrap_or("?");
         println!("  {short_sha}  {date}  {first_line}  ({author})");
     }

crates/gl/src/text.rs

@@ -0,0 +1,63 @@
+//! Display helpers for strings that come off the wire unchecked.
+
+/// Truncate `s` to at most `max` bytes, cutting at a char boundary.
+///
+/// `&s[..max]` panics when `s` is shorter than `max` bytes and when `max`
+/// lands inside a multi-byte char; `s.len().min(max)` guards only the first
+/// case. Node-supplied timestamps and ids hit both.
+pub(crate) fn truncate(s: &str, max: usize) -> &str {
+    let mut end = max.min(s.len());
+    while !s.is_char_boundary(end) {
+        end -= 1;
+    }
+    &s[..end]
+}
+
+/// Keep at most the last `max` bytes of `s`, cutting at a char boundary.
+///
+/// `&s[s.len().saturating_sub(max)..]` panics when the start index lands
+/// inside a multi-byte char; node-supplied ids hit that case.
+pub(crate) fn truncate_suffix(s: &str, max: usize) -> &str {
+    let mut start = s.len().saturating_sub(max);
+    while !s.is_char_boundary(start) {
+        start += 1;
+    }
+    &s[start..]
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{truncate, truncate_suffix};
+
+    #[test]
+    fn cuts_ascii_at_max() {
+        assert_eq!(truncate("2026-08-15T12:34:56Z", 10), "2026-08-15");
+    }
+
+    #[test]
+    fn short_and_empty_inputs_return_whole() {
+        assert_eq!(truncate("2026", 10), "2026");
+        assert_eq!(truncate("", 10), "");
+    }
+
+    #[test]
+    fn multibyte_cut_does_not_panic() {
+        // Byte index 10 lands inside 'é'; the cut must land on the boundary
+        // before it (9 bytes) rather than panic or exceed the byte limit.
+        assert_eq!(truncate("2026-08-1\u{e9}5T12:34:56Z", 10), "2026-08-1");
+        // A string that is entirely multi-byte and shorter than max.
+        assert_eq!(truncate("\u{e9}\u{e9}", 10), "\u{e9}\u{e9}");
+    }
+
+    #[test]
+    fn suffix_cut_does_not_panic() {
+        // 'é' is two bytes; a start index of len-8 lands inside it.
+        assert_eq!(truncate_suffix("did:key:\u{e9}1234567", 8), "1234567");
+    }
+
+    #[test]
+    fn suffix_short_input_returns_whole() {
+        assert_eq!(truncate_suffix("short", 8), "short");
+        assert_eq!(truncate_suffix("", 8), "");
+    }
+}

crates/icaptcha-client/src/lib.rs

@@ -88,20 +88,81 @@ fn sanitize_excerpt(s: &str) -> String {
     out
 }
 
-/// Whether `u` parses as a trusted URL.
+/// Fixed warning when `GITLAWB_ICAPTCHA_INSECURE` is present but not an
+/// explicit truthy value. Does not include the raw env value (arbitrary
+/// deployment input — secrets / terminal controls).
+fn warn_icaptcha_insecure_non_truthy() {
+    // One literal line on purpose: a continuation escape here keeps emitting
+    // correct prose (Rust strips the next line's leading whitespace), but the
+    // single-line form makes that property visible without knowing the escape
+    // semantics, which is what the review asked for.
+    tracing::warn!(
+        "GITLAWB_ICAPTCHA_INSECURE is set but not truthy (expected 1 or true); loopback HTTP trust relaxation remains disabled"
+    );
+}
+
+/// Parse a possibly-present `GITLAWB_ICAPTCHA_INSECURE` string.
 ///
-/// Production trusts only `https`.  When `GITLAWB_ICAPTCHA_INSECURE` is set (a
-/// runtime escape hatch for integration tests) the function also trusts
-/// `http://127.0.0.1` and `http://localhost` so the full iCaptcha retry path
-/// can be exercised against a local mockito server.
-fn is_https(u: &str) -> bool {
+/// Only an explicit truthy value (`1` / `true`, case-insensitive, after trim)
+/// enables the loopback-HTTP trust relaxation. Presence alone is not enough:
+/// `=0`, `=false`, or an empty value leave it off. A non-empty non-truthy
+/// value emits [`warn_icaptcha_insecure_non_truthy`].
+fn icaptcha_insecure_from_str(raw: Option<&str>) -> bool {
+    let Some(v) = raw else {
+        return false;
+    };
+    let t = v.trim();
+    if t.eq_ignore_ascii_case("1") || t.eq_ignore_ascii_case("true") {
+        true
+    } else {
+        if !t.is_empty() {
+            warn_icaptcha_insecure_non_truthy();
+        }
+        false
+    }
+}
+
+/// Interpret `std::env::var("GITLAWB_ICAPTCHA_INSECURE")` without reading the
+/// environment again — used by production and by unit tests.
+///
+/// `NotPresent` → disabled. `NotUnicode` → disabled + the same fixed warning
+/// as a present non-truthy value (fail closed; do not treat as unset).
+fn icaptcha_insecure_from_var(result: Result<String, std::env::VarError>) -> bool {
+    match result {
+        Ok(v) => icaptcha_insecure_from_str(Some(&v)),
+        Err(std::env::VarError::NotUnicode(_)) => {
+            warn_icaptcha_insecure_non_truthy();
+            false
+        }
+        Err(std::env::VarError::NotPresent) => false,
+    }
+}
+
+/// Whether `GITLAWB_ICAPTCHA_INSECURE` explicitly enables the loopback-HTTP
+/// trust relaxation used by integration tests.
+///
+/// Only an explicit truthy value (`1` / `true`, case-insensitive, after trim)
+/// enables it. Presence alone is not enough: `=0`, `=false`, or an empty
+/// value leave the relaxation off. A non-empty non-truthy value (including a
+/// present non-Unicode value) emits a `tracing::warn!` (visible when the
+/// `icaptcha_client` target is at WARN+, e.g. `RUST_LOG=icaptcha_client=warn`).
+fn icaptcha_insecure_enabled() -> bool {
+    icaptcha_insecure_from_var(std::env::var("GITLAWB_ICAPTCHA_INSECURE"))
+}
+
+/// Whether `u` parses as a trusted URL, given a pre-parsed insecure flag.
+///
+/// Production trusts only `https`. When `insecure` is true the function also
+/// trusts `http://127.0.0.1` and `http://localhost` so the full iCaptcha retry
+/// path can be exercised against a local mockito server.
+fn is_https_with(u: &str, insecure: bool) -> bool {
     let Ok(parsed) = reqwest::Url::parse(u) else {
         return false;
     };
     if parsed.scheme() == "https" {
         return true;
     }
-    if std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some()
+    if insecure
         && parsed.scheme() == "http"
         && matches!(parsed.host_str(), Some("127.0.0.1" | "localhost"))
     {
@@ -139,7 +200,15 @@ fn origin_key(u: &str) -> Option<String> {
 /// explicitly-configured host (never a node-discovered one), so a key is never
 /// exfiltrated to an origin the operator did not choose.
 fn resolve_solver_url(advertised: Option<&str>, operator: Option<&str>) -> (String, bool) {
-    let operator = operator.filter(|u| is_https(u));
+    resolve_solver_url_with(advertised, operator, icaptcha_insecure_enabled())
+}
+
+fn resolve_solver_url_with(
+    advertised: Option<&str>,
+    operator: Option<&str>,
+    insecure: bool,
+) -> (String, bool) {
+    let operator = operator.filter(|u| is_https_with(u, insecure));
     let operator_origin = operator.as_ref().and_then(|u| origin_key(u));
     let default_origin = origin_key(DEFAULT_URL);
 
@@ -157,7 +226,7 @@ fn resolve_solver_url(advertised: Option<&str>, operator: Option<&str>) -> (Stri
     };
 
     let chosen = match advertised {
-        Some(a) if is_https(a) && allowed(&origin_key(a)) => a.to_string(),
+        Some(a) if is_https_with(a, insecure) && allowed(&origin_key(a)) => a.to_string(),
         Some(a) => {
             tracing::warn!(
                 advertised = %a,
@@ -389,27 +458,36 @@ mod tests {
     use std::ffi::OsString;
     use std::sync::{Mutex, MutexGuard};
 
-    /// Serializes tests that touch the process-global
-    /// `GITLAWB_ICAPTCHA_INSECURE` env var so they never race.
+    /// Serializes the real-env reader test so it never races with other
+    /// process-global env mutations (and recovers from a poisoned lock).
     static ICAPTCHA_ENV_LOCK: Mutex<()> = Mutex::new(());
 
-    /// Set `GITLAWB_ICAPTCHA_INSECURE` for the test lifetime, restoring any
-    /// prior value on drop.
-    struct InsecureEnv {
+    struct EnvGuard {
         _lock: MutexGuard<'static, ()>,
         prev: Option<OsString>,
     }
 
-    impl InsecureEnv {
-        fn new() -> Self {
-            let lock = ICAPTCHA_ENV_LOCK.lock().unwrap();
+    impl EnvGuard {
+        fn lock() -> MutexGuard<'static, ()> {
+            ICAPTCHA_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner())
+        }
+
+        fn with_value(val: &str) -> Self {
+            let lock = Self::lock();
             let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE");
-            std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1");
-            InsecureEnv { _lock: lock, prev }
+            std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", val);
+            EnvGuard { _lock: lock, prev }
+        }
+
+        fn unset() -> Self {
+            let lock = Self::lock();
+            let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE");
+            std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE");
+            EnvGuard { _lock: lock, prev }
         }
     }
 
-    impl Drop for InsecureEnv {
+    impl Drop for EnvGuard {
         fn drop(&mut self) {
             match self.prev.take() {
                 Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", v),
@@ -418,6 +496,118 @@ mod tests {
         }
     }
 
+    /// #227 / #246: cover the production env read in `icaptcha_insecure_enabled`.
+    /// Pure-helper tests alone leave mutations of this function green.
+    #[test]
+    fn icaptcha_insecure_enabled_reads_real_env() {
+        {
+            let _g = EnvGuard::unset();
+            assert!(!icaptcha_insecure_enabled(), "unset must disable");
+        }
+        {
+            let _g = EnvGuard::with_value("0");
+            assert!(!icaptcha_insecure_enabled(), "=0 must disable");
+        }
+        {
+            let _g = EnvGuard::with_value("false");
+            assert!(!icaptcha_insecure_enabled(), "=false must disable");
+        }
+        {
+            let _g = EnvGuard::with_value("");
+            assert!(!icaptcha_insecure_enabled(), "empty must disable");
+        }
+        {
+            let _g = EnvGuard::with_value("1");
+            assert!(icaptcha_insecure_enabled(), "=1 must enable");
+        }
+        {
+            let _g = EnvGuard::with_value("true");
+            assert!(icaptcha_insecure_enabled(), "=true must enable");
+        }
+        {
+            let _g = EnvGuard::with_value("TRUE");
+            assert!(icaptcha_insecure_enabled(), "=TRUE must enable");
+        }
+        {
+            let _g = EnvGuard::with_value(" 1 ");
+            assert!(icaptcha_insecure_enabled(), "\" 1 \" (trimmed) must enable");
+        }
+    }
+
+    /// #227: `=0` / `=false` / empty / unset must NOT enable loopback HTTP;
+    /// only explicit truthy `1` / `true` may. Uses pure helpers so tests do
+    /// not mutate process-global environment (avoids races with parallel
+    /// resolver tests that also call `resolve_solver_url`).
+    #[test]
+    fn insecure_env_only_truthy_enables_loopback_http() {
+        assert!(
+            !is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("0"))),
+            "=0 must leave loopback HTTP disabled"
+        );
+        assert!(!icaptcha_insecure_from_str(Some("0")));
+
+        assert!(
+            !is_https_with(
+                "http://127.0.0.1",
+                icaptcha_insecure_from_str(Some("false"))
+            ),
+            "=false must leave loopback HTTP disabled"
+        );
+        assert!(!icaptcha_insecure_from_str(Some("false")));
+
+        assert!(
+            !is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some(""))),
+            "empty value must leave loopback HTTP disabled"
+        );
+        assert!(!icaptcha_insecure_from_str(Some("")));
+
+        assert!(
+            !is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(None)),
+            "unset must leave loopback HTTP disabled"
+        );
+        assert!(!icaptcha_insecure_from_str(None));
+        assert!(!icaptcha_insecure_from_var(Err(
+            std::env::VarError::NotPresent
+        )));
+
+        assert!(
+            is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("1"))),
+            "=1 must enable loopback HTTP"
+        );
+        assert!(is_https_with(
+            "http://localhost",
+            icaptcha_insecure_from_str(Some("1"))
+        ));
+        assert!(icaptcha_insecure_from_str(Some("1")));
+
+        assert!(
+            is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("true"))),
+            "=true must enable loopback HTTP"
+        );
+        assert!(icaptcha_insecure_from_str(Some("true")));
+
+        assert!(
+            is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("TRUE"))),
+            "=TRUE (case-insensitive) must enable loopback HTTP"
+        );
+
+        // Trim is load-bearing: whitespace around a truthy token must still enable.
+        assert!(
+            is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some(" 1 "))),
+            "\" 1 \" (trimmed) must enable loopback HTTP"
+        );
+        assert!(icaptcha_insecure_from_str(Some(" 1 ")));
+
+        // Present non-Unicode: fail closed + same fixed warning path as non-truthy.
+        assert!(!icaptcha_insecure_from_var(Err(
+            std::env::VarError::NotUnicode(OsString::from("x"))
+        )));
+
+        // https remains trusted regardless of the insecure flag.
+        assert!(is_https_with("https://icaptcha.gitlawb.com", false));
+        assert!(is_https_with("https://icaptcha.gitlawb.com", true));
+    }
+
     // ── P1: hostile error bodies must not reach the terminal raw ──────────
 
     #[test]
@@ -543,13 +733,12 @@ mod tests {
 
     #[test]
     fn rejects_insecure_advertised_url_on_different_port() {
-        // With GITLAWB_ICAPTCHA_INSECURE=1, two loopback URLs on different
+        // With insecure mode enabled, two loopback URLs on different
         // ports must be treated as distinct origins so a hostile node cannot
         // redirect the bearer key to a different listener on localhost.
-        let _env = InsecureEnv::new();
-
         let op = "http://localhost:3000";
-        let (url, key_trusted) = resolve_solver_url(Some("http://localhost:9000"), Some(op));
+        let (url, key_trusted) =
+            resolve_solver_url_with(Some("http://localhost:9000"), Some(op), true);
         assert_eq!(
             url, op,
             "must fall back to operator origin, not use different port"