Merge remote-tracking branch 'origin/main' into fix/227-icaptcha-insecure-parse

* fix(node): canonicalize and 404-on-no-match in remove_label (#344)

add_label stored labels trimmed and lowercased, but remove_label passed the
path segment through as-is, so a label stored as 'bug' did not match
DELETE /repos/{owner}/{repo}/labels/Bug. Worse, remove_label's DB call
discarded the row count and the handler always returned 200 with
{removed: true}, including when the delete matched nothing — the
no-op-rendered-as-success shape the rest of the API rejects.

Move trim+lowercase and charset/length validation into a shared
canonicalize_label helper used by both handlers, and have
db::remove_label return the affected row count so the handler can return
404 when no label was deleted. add_label's behavior is unchanged
(idempotent semantics preserved); the only externally observable change is
that mixed-case and surrounding-whitespace path segments now match the
stored label, and a delete that matches no row returns 404 instead of
{removed: true}.

Tests:
  * remove_label_canonicalizes_and_404s_on_no_match exercises the
    mixed-case path, the trimmed path, the empty-after-trim rejection,
    the disallowed-charset rejection, and the no-op-returns-404 case.
  * remove_label_denies_non_owner re-pins the 403 owner-gated shape
    (matches AGENTS.md: owner-gated mutations return 403 after the repo
    lookup, distinct from the not-found shape).

* test(node): pin len>50 arm in remove_label canonicalize

The helper's length branch had no coverage — 51 chars must 400 and
50 chars must pass validation (404 when absent). This pins the arm
lifted into canonicalize_label and decouples the no-match 404 from the
double-delete above, as requested in review of #390.

---------

Co-authored-by: Ayush7614 <[email protected]>

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/repos.rs

@@ -8289,7 +8289,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/db/mod.rs

@@ -1184,6 +1184,16 @@ const OWNER_KEY_CASE_SQL: &str = "CASE WHEN owner_did LIKE 'did:key:%' AND posit
 /// named `did` (like in agent_profiles) instead of `owner_did`.
 const PROFILE_DID_CASE_SQL: &str = "CASE WHEN did LIKE 'did:key:%' AND position(':' in substr(did, 9)) = 0 THEN substr(did, 9) ELSE did END";
 
+/// SQL CASE expression byte-identical to `normalize_owner_key`, but for columns
+/// named `repo_owner` (bounties table).  The bounties `repo_owner` is stored
+/// verbatim from the URL segment, so it may be either the full `did:key:` form
+/// or the bare key.
+const BOUNTY_OWNER_CASE_SQL: &str = "CASE WHEN repo_owner LIKE 'did:key:%' AND position(':' in substr(repo_owner, 9)) = 0 THEN substr(repo_owner, 9) ELSE repo_owner END";
+
+/// SQL CASE expression byte-identical to `normalize_owner_key`, but for columns
+/// named `claimant_did` (bounties table).
+const BOUNTY_CLAIMANT_CASE_SQL: &str = "CASE WHEN claimant_did LIKE 'did:key:%' AND position(':' in substr(claimant_did, 9)) = 0 THEN substr(claimant_did, 9) ELSE claimant_did END";
+
 #[cfg(test)]
 mod normalize_owner_key_tests {
     use super::normalize_owner_key;
@@ -2251,13 +2261,13 @@ impl Db {
         Ok(result.rows_affected() > 0)
     }
 
-    pub async fn remove_label(&self, repo_id: &str, label: &str) -> Result<()> {
-        sqlx::query("DELETE FROM repo_labels WHERE repo_id = $1 AND label = $2")
+    pub async fn remove_label(&self, repo_id: &str, label: &str) -> Result<bool> {
+        let result = sqlx::query("DELETE FROM repo_labels WHERE repo_id = $1 AND label = $2")
             .bind(repo_id)
             .bind(label)
             .execute(&self.pool)
             .await?;
-        Ok(())
+        Ok(result.rows_affected() > 0)
     }
 
     pub async fn list_labels(&self, repo_id: &str) -> Result<Vec<String>> {
@@ -4569,31 +4579,110 @@ impl Db {
         Ok(())
     }
 
-    pub async fn count_bounties_by_status(&self, status: &str) -> Result<i64> {
-        let row = sqlx::query("SELECT COUNT(*) as c FROM bounties WHERE status = $1")
+    // ── Visibility-filtered bounty aggregates (#477) ─────────────────────
+
+    /// Count bounties by status, restricted to repos whose `(owner_did, name)`
+    /// pairs are in `visible`.  Owner keys are normalized on both sides so
+    /// `did:key:` and bare-key forms match.  An empty set returns 0.
+    pub async fn count_bounties_by_status_visible(
+        &self,
+        status: &str,
+        visible: &[(String, String)],
+    ) -> Result<i64> {
+        if visible.is_empty() {
+            return Ok(0);
+        }
+        let owners: Vec<String> = visible.iter().map(|(o, _)| o.clone()).collect();
+        let names: Vec<String> = visible.iter().map(|(_, n)| n.clone()).collect();
+        let sql = format!(
+            "SELECT COUNT(*) as c FROM bounties b \
+             WHERE b.status = $1 \
+             AND EXISTS ( \
+                 SELECT 1 FROM unnest($2::text[], $3::text[]) AS v(o, n) \
+                 WHERE ({bounty_owner}) = v.o AND b.repo_name = v.n \
+             )",
+            bounty_owner = BOUNTY_OWNER_CASE_SQL,
+        );
+        let row = sqlx::query(&sql)
             .bind(status)
+            .bind(&owners)
+            .bind(&names)
             .fetch_one(&self.pool)
             .await?;
         Ok(row.get::<i64, _>("c"))
     }
 
-    pub async fn agent_bounty_stats(&self, agent_did: &str) -> Result<(i64, i64)> {
-        let row = sqlx::query(
-            "SELECT COUNT(*) as cnt, COALESCE(SUM(amount),0) as total FROM bounties WHERE claimant_did = $1 AND status = 'completed'",
-        )
-        .bind(agent_did)
-        .fetch_one(&self.pool)
-        .await?;
+    /// Per-agent bounty stats restricted to visible repos.  `b.repo_owner` is
+    /// normalized and `b.claimant_did` is matched against both spellings of the
+    /// agent key, so `did:key:` and bare-key forms match.
+    pub async fn agent_bounty_stats_visible(
+        &self,
+        agent_did: &str,
+        visible: &[(String, String)],
+    ) -> Result<(i64, i64)> {
+        if visible.is_empty() {
+            return Ok((0, 0));
+        }
+        // Index-friendly form of `normalize(claimant_did) = N`: a claimant
+        // normalizes to N only when stored as N or as `did:key:N` (the latter
+        // only when N has no ':', else the prefix is not stripped). Matching
+        // the raw column keeps `idx_bounties_claimant` usable.
+        let normalized_agent = normalize_owner_key(agent_did);
+        let mut claimants: Vec<String> = vec![normalized_agent.to_string()];
+        if !normalized_agent.contains(':') {
+            claimants.push(format!("did:key:{normalized_agent}"));
+        }
+        let owners: Vec<String> = visible.iter().map(|(o, _)| o.clone()).collect();
+        let names: Vec<String> = visible.iter().map(|(_, n)| n.clone()).collect();
+        let sql = format!(
+            "SELECT COUNT(*) as cnt, COALESCE(SUM(amount), 0)::BIGINT as total FROM bounties b \
+             WHERE b.claimant_did = ANY($1::text[]) \
+             AND b.status = 'completed' \
+             AND EXISTS ( \
+                 SELECT 1 FROM unnest($2::text[], $3::text[]) AS v(o, n) \
+                 WHERE ({bounty_owner}) = v.o AND b.repo_name = v.n \
+             )",
+            bounty_owner = BOUNTY_OWNER_CASE_SQL,
+        );
+        let row = sqlx::query(&sql)
+            .bind(&claimants)
+            .bind(&owners)
+            .bind(&names)
+            .fetch_one(&self.pool)
+            .await?;
         Ok((row.get::<i64, _>("cnt"), row.get::<i64, _>("total")))
     }
 
-    pub async fn bounty_leaderboard(&self, limit: i64) -> Result<Vec<(String, i64, i64)>> {
-        let rows = sqlx::query(
-            "SELECT claimant_did, COUNT(*) as cnt, COALESCE(SUM(amount),0) as total FROM bounties WHERE status='completed' AND claimant_did IS NOT NULL GROUP BY claimant_did ORDER BY total DESC LIMIT $1",
-        )
-        .bind(limit)
-        .fetch_all(&self.pool)
-        .await?;
+    /// Leaderboard restricted to visible repos.  Owner and claimant keys are
+    /// normalized so `did:key:` and bare-key forms match and group together.
+    pub async fn bounty_leaderboard_visible(
+        &self,
+        limit: i64,
+        visible: &[(String, String)],
+    ) -> Result<Vec<(String, i64, i64)>> {
+        if visible.is_empty() {
+            return Ok(Vec::new());
+        }
+        let owners: Vec<String> = visible.iter().map(|(o, _)| o.clone()).collect();
+        let names: Vec<String> = visible.iter().map(|(_, n)| n.clone()).collect();
+        let sql = format!(
+            "SELECT MIN(claimant_did) as claimant_did, COUNT(*) as cnt, COALESCE(SUM(amount), 0)::BIGINT as total \
+             FROM bounties b \
+             WHERE b.status = 'completed' AND b.claimant_did IS NOT NULL \
+             AND EXISTS ( \
+                 SELECT 1 FROM unnest($1::text[], $2::text[]) AS v(o, n) \
+                 WHERE ({bounty_owner}) = v.o AND b.repo_name = v.n \
+             ) \
+             GROUP BY ({claimant_case}) ORDER BY total DESC LIMIT $3",
+            claimant_case = BOUNTY_CLAIMANT_CASE_SQL,
+            bounty_owner = BOUNTY_OWNER_CASE_SQL,
+        );
+        let rows = sqlx::query(&sql)
+            .bind(&owners)
+            .bind(&names)
+            .bind(limit)
+            .fetch_all(&self.pool)
+            .await?;
         Ok(rows
             .iter()
             .map(|r| {

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/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"