fix(node): bound the invalid Hash echoed in the add error

Merge remote-tracking branch 'origin/main' into rehome-453
test(node): return real CIDs from the add-response test mocks
2026-11-08 17:55:53 -10:00 · 2026-11-05 21:18:07 -06:00 · 2026-11-05 19:35:39 -06:00 · 2026-09-14 16:47:01 -05:00 · 2026-09-14 16:29:47 -05:00 · 2026-09-14 16:01:46 -05:00
24 changed files with 163 additions and 2827 deletions
@@ -504,23 +504,6 @@ 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();
@@ -150,48 +150,23 @@ 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>> {
-        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);
+        let mut verified = Vec::with_capacity(attestations.len());
+        for a in attestations {
+            verified.push(self.verify(a, expected_cert_hash)?);
        }
-
-        // 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()));
+        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()));
+                }
            }
        }
        Ok(verified)
@@ -349,38 +324,6 @@ 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
@@ -398,65 +341,6 @@ 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();
@@ -285,22 +285,11 @@ pub async fn submit_bounty(
    Path(id): Path<String>,
    Json(req): Json<SubmitBountyRequest>,
 ) -> Result<Json<BountyRecord>> {
-    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());
-    }
+    let bounty = state
+        .db
+        .get_bounty(&id)
+        .await?
+        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;

    if bounty.status != "claimed" {
        return Err(AppError::BadRequest(format!(
@@ -336,22 +325,11 @@ pub async fn approve_bounty(
    Path(id): Path<String>,
    Json(req): Json<ApproveBountyRequest>,
 ) -> Result<Json<BountyRecord>> {
-    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());
-    }
+    let bounty = state
+        .db
+        .get_bounty(&id)
+        .await?
+        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;

    if bounty.status != "submitted" {
        return Err(AppError::BadRequest(format!(
@@ -394,22 +372,11 @@ pub async fn cancel_bounty(
    Extension(auth): Extension<AuthenticatedDid>,
    Path(id): Path<String>,
 ) -> Result<Json<BountyRecord>> {
-    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());
-    }
+    let bounty = state
+        .db
+        .get_bounty(&id)
+        .await?
+        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;

    if bounty.status != "open" {
        return Err(AppError::BadRequest(format!(
@@ -440,22 +407,11 @@ pub async fn dispute_bounty(
    Extension(auth): Extension<AuthenticatedDid>,
    Path(id): Path<String>,
 ) -> Result<Json<BountyRecord>> {
-    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());
-    }
+    let bounty = state
+        .db
+        .get_bounty(&id)
+        .await?
+        .ok_or_else(|| AppError::NotFound(format!("bounty {id} not found")))?;

    if bounty.status != "claimed" && bounty.status != "submitted" {
        return Err(AppError::BadRequest(format!(
@@ -500,68 +456,21 @@ 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 visible = visible_repo_pairs(&state).await?;
-
-    let open = state
-        .db
-        .count_bounties_by_status_visible("open", &visible)
-        .await?;
+    let open = state.db.count_bounties_by_status("open").await.unwrap_or(0);
    let claimed = state
        .db
-        .count_bounties_by_status_visible("claimed", &visible)
-        .await?;
+        .count_bounties_by_status("claimed")
+        .await
+        .unwrap_or(0);
    let completed = state
        .db
-        .count_bounties_by_status_visible("completed", &visible)
-        .await?;
+        .count_bounties_by_status("completed")
+        .await
+        .unwrap_or(0);

-    let leaders = state.db.bounty_leaderboard_visible(10, &visible).await?;
+    let leaders = state.db.bounty_leaderboard(10).await.unwrap_or_default();
    let leaderboard = leaders
        .into_iter()
        .map(|(did, cnt, total)| AgentBountyEntry {
@@ -580,14 +489,11 @@ 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 visible = visible_repo_pairs(&state).await?;
-    let (count, total) = state.db.agent_bounty_stats_visible(&did, &visible).await?;
+    let (count, total) = state.db.agent_bounty_stats(&did).await.unwrap_or((0, 0));
    Ok(Json(serde_json::json!({
        "did": did,
        "completed_bounties": count,
@@ -14,13 +14,14 @@ pub struct LabelRequest {
    pub label: String,
 }

-/// 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();
+/// 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();
    if label.is_empty() || label.len() > 50 {
        return Err(AppError::BadRequest("label must be 1–50 characters".into()));
    }
@@ -32,17 +33,6 @@ fn canonicalize_label(raw: &str) -> std::result::Result<String, AppError> {
            "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
@@ -69,8 +59,6 @@ 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)
@@ -78,10 +66,7 @@ pub async fn remove_label(
        .ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
    crate::api::require_repo_owner(&record, &auth.0)?;

-    let removed = state.db.remove_label(&record.id, &label).await?;
-    if !removed {
-        return Err(AppError::NotFound(format!("label '{label}'")));
-    }
+    state.db.remove_label(&record.id, &label).await?;
    Ok(Json(serde_json::json!({ "label": label, "removed": true })))
 }

@@ -70,8 +70,6 @@ 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
@@ -106,8 +104,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` / `.localhost` / `.local` / `.internal` hostnames. Used at
-/// announce time and by the boot-time prune of already-poisoned rows.
+/// and `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,
@@ -127,7 +125,6 @@ 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")
    {
@@ -263,9 +260,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(format!(
-            "http_url {PUBLIC_HTTP_URL_REQUIREMENT}"
-        )));
+        return Err(AppError::BadRequest(
+            "http_url must be a public http(s) URL (no loopback, private, or .internal/.local hosts)".into(),
+        ));
    }

    // Reject self-announcements: a peer row whose http_url is our own public
@@ -544,7 +541,6 @@ 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",
@@ -624,14 +620,11 @@ 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 now
-        // exercises the `.localhost` suffix rejection together with the extra
-        // trailing dot.
-        for bad in [
-            "http://localhost../",
-            "http://localhost...:7545",
-            "http://node.localhost../",
-        ] {
+        // 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"] {
            assert!(!is_public_http_url(bad), "{bad:?} must be rejected");
        }
    }
@@ -1589,31 +1582,6 @@ 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
@@ -46,10 +46,9 @@ 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(format!(
-            "webhook URL {}",
-            crate::api::peers::PUBLIC_HTTP_URL_REQUIREMENT
-        )));
+        return Err(AppError::BadRequest(
+            "webhook URL must be a public http(s) URL (no loopback, private, or .internal/.local hosts)".into(),
+        ));
    }

    let events = req.events.unwrap_or_else(|| vec!["*".into()]);
@@ -142,19 +141,7 @@ pub async fn delete_webhook(

 #[cfg(test)]
 mod tests {
-    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;
+    use crate::api::peers::is_public_http_url;

    // 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
@@ -165,7 +152,6 @@ 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).
@@ -183,53 +169,4 @@ 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"
-        );
-    }
 }
@@ -141,10 +141,19 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result<Option<S
        return Ok(None);
    }

-    // 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.
+    // Try exact match first — fast path for callers passing the full UUID.
    let exact_ref = format!("refs/gitlawb/issues/{id_or_prefix}");
-    let prefix_glob = format!("{exact_ref}*");
+    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 list = Command::new("git")
        .args(["for-each-ref", "--format=%(refname)", &prefix_glob])
        .current_dir(repo_path)
@@ -156,44 +165,25 @@ 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()
-        .map(str::trim)
-        .filter(|l| !l.is_empty())
-        .collect();
+    let matches: Vec<&str> = output.lines().filter(|l| !l.trim().is_empty()).collect();

-    // 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)
+    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))
        }
-        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))
+        _ => anyhow::bail!(
+            "ambiguous issue prefix '{}': matches {} issues",
+            id_or_prefix,
+            matches.len()
+        ),
+    }
 }

 /// Close an issue by updating its status to "closed" in the git ref.
@@ -277,43 +267,6 @@ 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();
@@ -314,9 +314,7 @@ pub fn record_sync_processed(status: &str) {
 }

 /// Record one webhook delivery attempt outcome. `result` ∈
-/// {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.
+/// {ok, http_error, network_error, skipped}.
 pub fn record_webhook_delivery(result: &str) {
    if let Some(c) = WEBHOOK_DELIVERIES.get() {
        c.with_label_values(&[result]).inc();
@@ -79,19 +79,6 @@ 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
@@ -88,7 +88,7 @@ async fn cmd_list(node: String, capability: Option<String>) -> Result<()> {
        let short = did
            .split(':')
            .next_back()
-            .map(|s| crate::text::truncate(s, 16))
+            .map(|s| &s[..s.len().min(16)])
            .unwrap_or("?");
        let trust = agent["trust_score"].as_f64().unwrap_or(0.0);
        let caps = agent["capabilities"]
@@ -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 = crate::text::truncate(id, 8);
+                let short_id = &id[..8.min(id.len())];
                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 = crate::text::truncate_suffix(did, 8);
+                let short = &did[did.len().saturating_sub(8)..];
                println!(
                    "  {}. ...{short}  {count} bounties  {earned} $GITLAWB",
                    i + 1
@@ -117,10 +117,7 @@ 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| crate::text::truncate(s, 19))
-            .unwrap_or("?");
+        let issued_at = cert["issued_at"].as_str().map(|s| &s[..19]).unwrap_or("?");
        println!("  {id:.8}  {issued_at}  {ref_name}  {new_sha:.12}");
    }
    Ok(())
@@ -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 = crate::text::truncate(ts, 10);
+        let date = &ts[..ts.len().min(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 = crate::text::truncate(sha, 8);
+                let short_sha = &sha[..sha.len().min(8)];
                println!("  {date}  commit      {short_sha}  {first_line}");
            }
            "pr_merged" => {
@@ -131,7 +131,11 @@ 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 = crate::text::truncate(pinned_at, 19);
+        let ts = if pinned_at.len() >= 19 {
+            &pinned_at[..19]
+        } else {
+            pinned_at
+        };
        println!("  {cid}");
        println!("    sha256: {sha}");
        println!("    pinned: {ts}");
@@ -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| crate::text::truncate(s, 10))
+            .map(|s| &s[..10])
            .unwrap_or("?");
        let icon = match status {
            "open" => "○",
@@ -365,13 +365,10 @@ async fn cmd_issue_comments(
        let author_short = author
            .split(':')
            .next_back()
-            .map(|s| crate::text::truncate(s, 8))
+            .map(|s| &s[..s.len().min(8)])
            .unwrap_or("?");
        let cbody = c["body"].as_str().unwrap_or("");
-        let created = c["created_at"]
-            .as_str()
-            .map(|s| crate::text::truncate(s, 10))
-            .unwrap_or("?");
+        let created = c["created_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
        println!("  · {author_short} ({created})");
        println!("    {cbody}");
        println!();
@@ -442,34 +439,6 @@ 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();
@@ -30,7 +30,6 @@ mod star;
 mod status;
 mod sync;
 mod task;
-mod text;
 mod ucan_cmd;
 mod visibility;
 mod webhook;
@@ -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| crate::text::truncate(s, 10))
+                        .map(|s| &s[..10.min(s.len())])
                        .unwrap_or("?");
                    println!("  {ts}  {repo}  {ref_name}");
                }
@@ -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| crate::text::truncate(s, 10))
+            .map(|s| &s[..10])
            .unwrap_or("never");
        let status = if reachable { "✓" } else { "✗" };
        println!("  {status} {url}");
@@ -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| crate::text::truncate(s, 8))
+            .map(|s| &s[..s.len().min(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| crate::text::truncate(s, 8))
+                .map(|s| &s[..s.len().min(8)])
                .unwrap_or("?");
            let rstatus = r["status"].as_str().unwrap_or("?");
            let rbody = r["body"].as_str().unwrap_or("");
@@ -414,13 +414,10 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option<PathBuf>)
            let author_short = author
                .split(':')
                .next_back()
-                .map(|s| crate::text::truncate(s, 8))
+                .map(|s| &s[..s.len().min(8)])
                .unwrap_or("?");
            let cbody = c["body"].as_str().unwrap_or("");
-            let created = c["created_at"]
-                .as_str()
-                .map(|s| crate::text::truncate(s, 10))
-                .unwrap_or("?");
+            let created = c["created_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
            println!("  · {author_short} ({created})");
            println!("    {cbody}");
        }
@@ -467,7 +464,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: {}", crate::text::truncate(sha, 12));
+    println!("  Merge commit: {}", &sha[..sha.len().min(12)]);
    Ok(())
 }

@@ -558,13 +555,10 @@ async fn cmd_comments(repo: String, number: u64, node: String, dir: Option<PathB
        let author_short = author
            .split(':')
            .next_back()
-            .map(|s| crate::text::truncate(s, 8))
+            .map(|s| &s[..s.len().min(8)])
            .unwrap_or("?");
        let cbody = c["body"].as_str().unwrap_or("");
-        let created = c["created_at"]
-            .as_str()
-            .map(|s| crate::text::truncate(s, 10))
-            .unwrap_or("?");
+        let created = c["created_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
        println!("  · {author_short} ({created})");
        println!("    {cbody}");
        println!();
@@ -337,10 +337,7 @@ 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| crate::text::truncate(s, 10))
-            .unwrap_or("?");
+        let updated = r["updated_at"].as_str().map(|s| &s[..10]).unwrap_or("?");
        let vis = if public { "public" } else { "private" };
        println!("  {name:<24}  {vis:<8}  {updated}  {desc}");
    }
@@ -517,7 +514,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| crate::text::truncate(s, 10))
+                .map(|s| &s[..10.min(s.len())])
                .unwrap_or("?");
            println!("  {registered}  {did}  →  {url}");
        }
@@ -559,7 +556,7 @@ pub(crate) async fn cmd_commits(
            .or_else(|| c["sha"].as_str())
            .or_else(|| c["oid"].as_str())
            .unwrap_or("?");
-        let short_sha = crate::text::truncate(sha, 10);
+        let short_sha = &sha[..sha.len().min(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"]
@@ -569,7 +566,7 @@ pub(crate) async fn cmd_commits(
        let date = c["date"]
            .as_str()
            .or_else(|| c["committer_date"].as_str())
-            .map(|s| crate::text::truncate(s, 10))
+            .map(|s| &s[..10.min(s.len())])
            .unwrap_or("?");
        println!("  {short_sha}  {date}  {first_line}  ({author})");
    }
@@ -1,63 +0,0 @@
-//! 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), "");
-    }
-}
@@ -88,81 +88,20 @@ fn sanitize_excerpt(s: &str) -> String {
    out
 }

-/// 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.
+/// Whether `u` parses as a trusted URL.
 ///
-/// 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 {
+/// 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 {
    let Ok(parsed) = reqwest::Url::parse(u) else {
        return false;
    };
    if parsed.scheme() == "https" {
        return true;
    }
-    if insecure
+    if std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some()
        && parsed.scheme() == "http"
        && matches!(parsed.host_str(), Some("127.0.0.1" | "localhost"))
    {
@@ -200,15 +139,7 @@ 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) {
-    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 = operator.filter(|u| is_https(u));
    let operator_origin = operator.as_ref().and_then(|u| origin_key(u));
    let default_origin = origin_key(DEFAULT_URL);

@@ -226,7 +157,7 @@ fn resolve_solver_url_with(
    };

    let chosen = match advertised {
-        Some(a) if is_https_with(a, insecure) && allowed(&origin_key(a)) => a.to_string(),
+        Some(a) if is_https(a) && allowed(&origin_key(a)) => a.to_string(),
        Some(a) => {
            tracing::warn!(
                advertised = %a,
@@ -458,36 +389,27 @@ mod tests {
    use std::ffi::OsString;
    use std::sync::{Mutex, MutexGuard};

-    /// Serializes the real-env reader test so it never races with other
-    /// process-global env mutations (and recovers from a poisoned lock).
+    /// Serializes tests that touch the process-global
+    /// `GITLAWB_ICAPTCHA_INSECURE` env var so they never race.
    static ICAPTCHA_ENV_LOCK: Mutex<()> = Mutex::new(());

-    struct EnvGuard {
+    /// Set `GITLAWB_ICAPTCHA_INSECURE` for the test lifetime, restoring any
+    /// prior value on drop.
+    struct InsecureEnv {
        _lock: MutexGuard<'static, ()>,
        prev: Option<OsString>,
    }

-    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();
+    impl InsecureEnv {
+        fn new() -> Self {
+            let lock = ICAPTCHA_ENV_LOCK.lock().unwrap();
            let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE");
-            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 }
+            std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1");
+            InsecureEnv { _lock: lock, prev }
        }
    }

-    impl Drop for EnvGuard {
+    impl Drop for InsecureEnv {
        fn drop(&mut self) {
            match self.prev.take() {
                Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", v),
@@ -496,118 +418,6 @@ 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]
@@ -733,12 +543,13 @@ mod tests {

    #[test]
    fn rejects_insecure_advertised_url_on_different_port() {
-        // With insecure mode enabled, two loopback URLs on different
+        // With GITLAWB_ICAPTCHA_INSECURE=1, 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_with(Some("http://localhost:9000"), Some(op), true);
+        let (url, key_trusted) = resolve_solver_url(Some("http://localhost:9000"), Some(op));
        assert_eq!(
            url, op,
            "must fall back to operator origin, not use different port"
Author	SHA1	Message	Date
beardthelion	ef597a1936	fix(node): bound the invalid Hash echoed in the add error	2026-11-05 21:18:07 -06:00
beardthelion	99a9c661c7	Merge remote-tracking branch 'origin/main' into rehome-453	2026-11-05 19:35:39 -06:00
beardthelion	dc3ed80360	test(node): return real CIDs from the add-response test mocks The pin_git_object CID validation rejects non-CID Hash strings, and a dozen test_support fixtures answered 200 with placeholder values like "bafyprovider" or "x". Replace them with a valid CID so the fixtures model a healthy backend; no test asserts on the literal value.	2026-09-14 16:47:01 -05:00
beardthelion	8dc6c26190	fix(node): reject a non-CID Hash in the add response Address PR review feedback (#453): a string-valued Hash that does not parse as a CID would be recorded into encrypted_blobs.cid and fail on every later cat, the same failure class as a missing Hash. Validate the returned Hash before accepting it and cover the malformed case with a regression test.	2026-09-14 16:29:47 -05:00
beardthelion	63e6e3f34b	fix(node): fail pin_git_object when the add response carries no Hash A 200 from /api/v0/add without a parseable Hash fell back to the caller-computed raw CID, recording an address the backend never confirmed stored. For payloads above the backend's chunk threshold (262144 bytes on Kubo) that raw CID is not even a stored block, so every later /api/v0/cat on it fails permanently: get_encrypted_blob errors on every fetch and mirrors retry the envelope on every sync. A Hash-less 200 is now an add error carrying a truncated body excerpt. The delaying_endpoint test fixture, which relied on the fallback by returning an empty 200 body, now answers with a conformant NDJSON line.	2026-09-14 16:01:46 -05:00