feat(gl): sanctioned iCaptcha client flow + secure git lifecycle#138
feat(gl): sanctioned iCaptcha client flow + secure git lifecycle#138kevincodex1 wants to merge 1 commit into
Conversation
Builds the client side of the iCaptcha gate so a legitimate agent passes the challenge transparently, with no manual headers and no GITLAWB_ICAPTCHA_PROOF env var. Server enforcement is unchanged. New crate `icaptcha-client` (blocking, shared by gl + the git helper): - deterministic solvers for the computational challenge types (arithmetic, algebra, sequence) matching the iCaptcha generators; anagram/logic/LLM fall back to a solver hook / interactive prompt. - obtain_proof(): POST /v1/challenge (requesterId = caller DID), solve, POST /v1/answer, handle escalation, return the signed proof. Optional bearer auth via GITLAWB_ICAPTCHA_API_KEY. gl (crates/gl): - http.rs: unify post/put/delete through one signed sender that, on a 403 iCaptcha challenge (detected via x-icaptcha-url / x-icaptcha-level headers), solves it and retries the same signed request with the x-icaptcha-proof header (bounded retry absorbs the ~5 min proof TTL). Removes the env hack. Emits an actionable hint on 401 not_an_agent (old-CLI / unregistered). - repo.rs: `repo info` checks status and returns a real 404 instead of a stub card with `?` fields; `repo info`/`clone` resolve the owner from the local identity or an explicit owner/name — never the node's own DID. - doctor.rs: warn on gl-vs-node version drift; add an iCaptcha reachability check. git-remote-gitlawb: same shared client — push stays signed (RFC 9421); on a 403 iCaptcha (safety net; push is signed-only, not gated) it solves and retries. Clone/fetch unsigned; missing repo keeps its clear 404. gitlawb-node: the 403 icaptcha_proof_required response now advertises the service url + required level as JSON fields and x-icaptcha-url / x-icaptcha-level headers (mirroring the human_detected pattern) so clients discover them instead of scraping the message. The sub == authenticated-DID and expiry/level checks are unchanged — enforcement is not weakened. README: documents the full lifecycle (identity -> register -> repo create with auto-iCaptcha -> push -> clone), the requesterId == DID rule, and proof TTL. Tested: solver unit tests; full workspace green against Postgres (node still rejects missing/expired/wrong-subject proofs).
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
📝 WalkthroughWalkthroughThis PR adds a new ChangesiCaptcha proof flow
Sequence Diagram(s)sequenceDiagram
participant Client as gl/git-remote-gitlawb
participant Node as gitlawb-node
participant IcaptchaClient as icaptcha-client
participant IcaptchaService
Client->>Node: signed write request
Node-->>Client: 403 (x-icaptcha-url, x-icaptcha-level)
Client->>IcaptchaClient: obtain_proof(cfg)
IcaptchaClient->>IcaptchaService: request challenge
IcaptchaService-->>IcaptchaClient: challenge
IcaptchaClient->>IcaptchaClient: solve (deterministic/interactive)
IcaptchaClient->>IcaptchaService: submit answer
IcaptchaService-->>IcaptchaClient: proof token
IcaptchaClient-->>Client: proof token
Client->>Node: retry signed request + x-icaptcha-proof
Node-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:git-remote
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/git-remote-gitlawb/src/main.rs (1)
309-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hard-coding the helper version in the User-Agent.
Line 309 now advertises
0.4.0, while the refs request in this function still uses another hard-coded value. Useenv!("CARGO_PKG_VERSION")via a shared constant so version-drift checks don’t go stale.♻️ Proposed refactor
+const USER_AGENT: &str = concat!("git/2.0 git-remote-gitlawb/", env!("CARGO_PKG_VERSION")); + ... - .header("User-Agent", "git/2.0 git-remote-gitlawb/0.4.0"); + .header("User-Agent", USER_AGENT);Also reuse
USER_AGENTfor the/info/refsrequest in the same function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/git-remote-gitlawb/src/main.rs` at line 309, The User-Agent string is hard-coded with a stale helper version, and the same version value is duplicated for the /info/refs request. Update the `main` function to use a shared `USER_AGENT` constant based on `env!("CARGO_PKG_VERSION")`, and reuse that constant for both the Git HTTP request and the refs request so the version stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/git-remote-gitlawb/src/main.rs`:
- Around line 340-342: The remote helper path in main should not call
icaptcha_client::obtain_proof with a fallback that can reach interactive_prompt,
since this code runs over the Git protocol stream. Update the proof acquisition
flow in the icaptcha challenge handling block to use a non-interactive proof
source only, and if no proof can be obtained, return an actionable error instead
of prompting on stdin.
- Around line 329-333: The push request in main currently clones the entire
request_body before sending, which adds unnecessary peak-memory overhead; update
the send path to use a shared body representation instead of duplicating the
pack. Refactor the request_body handling in main around req.body(...).send() to
use a cloneable shared type such as bytes::Bytes so retries still work without
copying the full payload.
In `@crates/gl/src/doctor.rs`:
- Around line 207-231: The iCaptcha doctor check is using a different endpoint
source than the signed-write retry flow, so the warning points users at the
wrong fix. Update the iCaptcha probe in doctor.rs (the
`NodeClient::new(...).get("/v1/pubkey")` check) to align with the retry path
that uses the `x-icaptcha-url` header and `IcaptchaCfg::new`, and adjust the
warning text to reference that header-driven URL instead of
`GITLAWB_ICAPTCHA_URL`.
In `@crates/gl/src/repo.rs`:
- Around line 304-329: The bare-name branch in cmd_clone is building the repo
URL from kp.did().to_string(), which produces a full DID instead of the short
owner segment used elsewhere. Update cmd_clone to reuse resolve_owner_did (or
the same keypair-to-owner logic used by cmd_info and resolve_owner_repo_pair) so
the constructed gitlawb:// URL uses the short form consistently. Keep the
existing explicit owner/name path unchanged, and ensure bare-name cloning
matches the format expected by git-remote-gitlawb and the related tests.
In `@README.md`:
- Around line 217-219: Clarify the `git push` wording in README.md so it does
not imply an absolute “no per-push challenge” guarantee. Update the sentence in
the access-policy section near the `create / fork / register` and `git push`
description to state that pushes are signed-only by default while the helper may
still retry on 403 iCaptcha responses, using the same policy terms already used
elsewhere in the document.
---
Nitpick comments:
In `@crates/git-remote-gitlawb/src/main.rs`:
- Line 309: The User-Agent string is hard-coded with a stale helper version, and
the same version value is duplicated for the /info/refs request. Update the
`main` function to use a shared `USER_AGENT` constant based on
`env!("CARGO_PKG_VERSION")`, and reuse that constant for both the Git HTTP
request and the refs request so the version stays consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ee1649b-ba1c-4095-a96e-671cf41600d0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlREADME.mdcrates/git-remote-gitlawb/Cargo.tomlcrates/git-remote-gitlawb/src/main.rscrates/gitlawb-node/src/error.rscrates/gitlawb-node/src/icaptcha.rscrates/gl/Cargo.tomlcrates/gl/src/doctor.rscrates/gl/src/http.rscrates/gl/src/repo.rscrates/icaptcha-client/Cargo.tomlcrates/icaptcha-client/src/lib.rscrates/icaptcha-client/src/solvers.rs
| // Clone the pack so the body survives a possible iCaptcha retry. (Push | ||
| // is signed-only today, so the retry path is essentially never taken.) | ||
| let resp = req | ||
| .body(request_body.clone()) | ||
| .send() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'reqwest\s*=|bytes\s*=' Cargo.toml crates -g 'Cargo.toml'Repository: Gitlawb/node
Length of output: 586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo.toml excerpts =="
sed -n '1,120p' Cargo.toml
echo
echo "== git-remote-gitlawb main.rs around request body =="
sed -n '280,380p' crates/git-remote-gitlawb/src/main.rs
echo
echo "== sign_request definition =="
rg -n 'fn sign_request|sign_request\(' crates/git-remote-gitlawb/src/main.rsRepository: Gitlawb/node
Length of output: 6122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== reqwest body API docs from local cargo registry if available =="
python3 - <<'PY'
import os, glob, textwrap, pathlib, sys
home = pathlib.Path.home()
candidates = list(home.glob('.cargo/registry/src/*/reqwest-0.12.*'))
print("\n".join(str(p) for p in candidates[:20]))
PY
echo
echo "== search local registry for Bytes body support =="
rg -n 'impl From<Bytes>|From<Bytes>|body<B: Into<Body>>|struct Body' ~/.cargo/registry/src -g '*.rs' | head -n 80Repository: Gitlawb/node
Length of output: 345
Avoid cloning the full pack body here.
request_body.clone() duplicates the entire pack before every send, so large pushes pay the extra peak-memory cost even on the first attempt. A shared body type like bytes::Bytes would avoid the copy and still allow retries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/git-remote-gitlawb/src/main.rs` around lines 329 - 333, The push
request in main currently clones the entire request_body before sending, which
adds unnecessary peak-memory overhead; update the send path to use a shared body
representation instead of duplicating the pack. Refactor the request_body
handling in main around req.body(...).send() to use a cloneable shared type such
as bytes::Bytes so retries still work without copying the full payload.
| if let Some(cfg) = icaptcha_cfg_from_headers(resp.headers(), keypair) { | ||
| attempts += 1; | ||
| proof = Some(icaptcha_client::obtain_proof(&cfg, None)?); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 6 'fn interactive_prompt|interactive_prompt|obtain_proof' crates/icaptcha-client/src crates/git-remote-gitlawb/srcRepository: Gitlawb/node
Length of output: 4208
🏁 Script executed:
#!/bin/bash
sed -n '1,240p' crates/git-remote-gitlawb/src/main.rs
printf '\n--- icaptcha-client ---\n'
sed -n '1,240p' crates/icaptcha-client/src/lib.rsRepository: Gitlawb/node
Length of output: 16017
🏁 Script executed:
#!/bin/bash
rg -n 'obtain_proof_noninteractive|noninteractive|interactive_prompt|obtain_proof\(' cratesRepository: Gitlawb/node
Length of output: 865
Disable interactive iCaptcha prompting in the remote helper. icaptcha_client::obtain_proof(&cfg, None) still falls back to interactive_prompt, which reads from stdin(); here that stdin is the Git remote-helper protocol stream, so a challenge prompt can hang or corrupt the exchange. Use a non-interactive proof path here and fail with an actionable error instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/git-remote-gitlawb/src/main.rs` around lines 340 - 342, The remote
helper path in main should not call icaptcha_client::obtain_proof with a
fallback that can reach interactive_prompt, since this code runs over the Git
protocol stream. Update the proof acquisition flow in the icaptcha challenge
handling block to use a non-interactive proof source only, and if no proof can
be obtained, return an actionable error instead of prompting on stdin.
| // ── 4b. iCaptcha capability ─────────────────────────────────────────── | ||
| // Gated writes (repo create / register / fork) auto-solve a challenge at the | ||
| // iCaptcha service; check it's reachable so the failure mode is obvious. | ||
| let icaptcha_url = std::env::var("GITLAWB_ICAPTCHA_URL") | ||
| .unwrap_or_else(|_| icaptcha_client::DEFAULT_URL.to_string()); | ||
| match NodeClient::new(&icaptcha_url, None).get("/v1/pubkey").await { | ||
| Ok(resp) if resp.status().is_success() => { | ||
| checks.push(Check::pass("iCaptcha", format!("{icaptcha_url} reachable"))); | ||
| } | ||
| Ok(resp) => { | ||
| checks.push(Check::warn( | ||
| "iCaptcha", | ||
| format!("{icaptcha_url} returned HTTP {}", resp.status()), | ||
| "gated writes (repo create / register) may fail until iCaptcha is reachable", | ||
| )); | ||
| } | ||
| Err(e) => { | ||
| checks.push(Check::warn( | ||
| "iCaptcha", | ||
| format!("{icaptcha_url} unreachable: {e}"), | ||
| "set GITLAWB_ICAPTCHA_URL or check connectivity — repo create / register solve a challenge there", | ||
| )); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether GITLAWB_ICAPTCHA_URL is read anywhere besides doctor.rs
rg -n 'GITLAWB_ICAPTCHA_URL' --type=rustRepository: Gitlawb/node
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files | rg '(^crates/gl/src/(doctor|http)\.rs$|icaptcha|git-remote-gitlawb|remote-gitlawb|x-icaptcha-url|IcaptchaCfg)'
printf '\n== icaptcha symbols ==\n'
rg -n 'IcaptchaCfg|x-icaptcha-url|ICAPTCHA|GITLAWB_ICAPTCHA_API_KEY|GITLAWB_ICAPTCHA_URL' crates gl . --type=rustRepository: Gitlawb/node
Length of output: 9784
Use the same iCaptcha endpoint as the retry path. The doctor check reads GITLAWB_ICAPTCHA_URL, but the signed-write retry flow derives iCaptcha from the x-icaptcha-url response header and IcaptchaCfg::new only reads GITLAWB_ICAPTCHA_API_KEY. Telling users to set this env var won’t fix gated-write failures; point the warning at the header-driven URL instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gl/src/doctor.rs` around lines 207 - 231, The iCaptcha doctor check is
using a different endpoint source than the signed-write retry flow, so the
warning points users at the wrong fix. Update the iCaptcha probe in doctor.rs
(the `NodeClient::new(...).get("/v1/pubkey")` check) to align with the retry
path that uses the `x-icaptcha-url` header and `IcaptchaCfg::new`, and adjust
the warning text to reference that header-driven URL instead of
`GITLAWB_ICAPTCHA_URL`.
| async fn cmd_clone(name: String, node: String, dir: Option<PathBuf>) -> Result<()> { | ||
| let did = if let Ok(kp) = load_keypair_from_dir(dir.as_deref()) { | ||
| kp.did().to_string() | ||
| // Owner is taken from an explicit `owner/name`, else the LOCAL identity — | ||
| // never the node's own DID. A missing repo then surfaces as the helper's | ||
| // clear 404 rather than a clone under an invented owner. | ||
| let (did, repo_name) = if let Some((owner, rest)) = name.split_once('/') { | ||
| (owner.to_string(), rest.to_string()) | ||
| } else { | ||
| let client = NodeClient::new(&node, None); | ||
| let info: Value = client.get("/").await?.json().await?; | ||
| info["did"] | ||
| .as_str() | ||
| .context("node missing DID")? | ||
| .to_string() | ||
| let kp = load_keypair_from_dir(dir.as_deref()).context( | ||
| "no local identity to resolve the repo owner — pass `owner/name`, or run `gl identity new`", | ||
| )?; | ||
| (kp.did().to_string(), name) | ||
| }; | ||
| let url = format!("gitlawb://{did}/{name}"); | ||
| let url = format!("gitlawb://{did}/{repo_name}"); | ||
| println!(" cloning {url}"); | ||
| let status = std::process::Command::new("git") | ||
| .arg("clone") | ||
| .arg(&url) | ||
| // Point the remote helper at the same node the user selected. | ||
| .env("GITLAWB_NODE", &node) | ||
| .status() | ||
| .context("failed to run git clone — is git installed?")?; | ||
| if !status.success() { | ||
| anyhow::bail!("git clone failed"); | ||
| anyhow::bail!("git clone failed — does the repo exist on this node?"); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
DID format inconsistency breaks bare-name gl repo clone.
When no explicit owner/name is given, this branch uses kp.did().to_string() (the full DID, e.g. did:key:z6Mk...) directly, instead of the short key segment that resolve_owner_did produces (and that the rest of the codebase — cmd_info, resolve_owner_repo_pair, and the test_resolve_owner_did_uses_keypair test — consistently relies on). The resulting gitlawb://did:key:z6Mk.../repo URL contains colons in the authority position, which is inconsistent with the short-form gitlawb://z6Mk.../repo URLs expected elsewhere (e.g. the /z6Mk.../my-repo/git-receive-pack path shape used by git-remote-gitlawb), and is likely to break git clone for bare repo names.
🐛 Proposed fix — reuse resolve_owner_did for consistency
let (did, repo_name) = if let Some((owner, rest)) = name.split_once('/') {
(owner.to_string(), rest.to_string())
} else {
- let kp = load_keypair_from_dir(dir.as_deref()).context(
- "no local identity to resolve the repo owner — pass `owner/name`, or run `gl identity new`",
- )?;
- (kp.did().to_string(), name)
+ let did = resolve_owner_did(&node, dir.as_deref()).await?;
+ (did, name)
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn cmd_clone(name: String, node: String, dir: Option<PathBuf>) -> Result<()> { | |
| let did = if let Ok(kp) = load_keypair_from_dir(dir.as_deref()) { | |
| kp.did().to_string() | |
| // Owner is taken from an explicit `owner/name`, else the LOCAL identity — | |
| // never the node's own DID. A missing repo then surfaces as the helper's | |
| // clear 404 rather than a clone under an invented owner. | |
| let (did, repo_name) = if let Some((owner, rest)) = name.split_once('/') { | |
| (owner.to_string(), rest.to_string()) | |
| } else { | |
| let client = NodeClient::new(&node, None); | |
| let info: Value = client.get("/").await?.json().await?; | |
| info["did"] | |
| .as_str() | |
| .context("node missing DID")? | |
| .to_string() | |
| let kp = load_keypair_from_dir(dir.as_deref()).context( | |
| "no local identity to resolve the repo owner — pass `owner/name`, or run `gl identity new`", | |
| )?; | |
| (kp.did().to_string(), name) | |
| }; | |
| let url = format!("gitlawb://{did}/{name}"); | |
| let url = format!("gitlawb://{did}/{repo_name}"); | |
| println!(" cloning {url}"); | |
| let status = std::process::Command::new("git") | |
| .arg("clone") | |
| .arg(&url) | |
| // Point the remote helper at the same node the user selected. | |
| .env("GITLAWB_NODE", &node) | |
| .status() | |
| .context("failed to run git clone — is git installed?")?; | |
| if !status.success() { | |
| anyhow::bail!("git clone failed"); | |
| anyhow::bail!("git clone failed — does the repo exist on this node?"); | |
| } | |
| Ok(()) | |
| } | |
| async fn cmd_clone(name: String, node: String, dir: Option<PathBuf>) -> Result<()> { | |
| // Owner is taken from an explicit `owner/name`, else the LOCAL identity — | |
| // never the node's own DID. A missing repo then surfaces as the helper's | |
| // clear 404 rather than a clone under an invented owner. | |
| let (did, repo_name) = if let Some((owner, rest)) = name.split_once('/') { | |
| (owner.to_string(), rest.to_string()) | |
| } else { | |
| let did = resolve_owner_did(&node, dir.as_deref()).await?; | |
| (did, name) | |
| }; | |
| let url = format!("gitlawb://{did}/{repo_name}"); | |
| println!(" cloning {url}"); | |
| let status = std::process::Command::new("git") | |
| .arg("clone") | |
| .arg(&url) | |
| // Point the remote helper at the same node the user selected. | |
| .env("GITLAWB_NODE", &node) | |
| .status() | |
| .context("failed to run git clone — is git installed?")?; | |
| if !status.success() { | |
| anyhow::bail!("git clone failed — does the repo exist on this node?"); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gl/src/repo.rs` around lines 304 - 329, The bare-name branch in
cmd_clone is building the repo URL from kp.did().to_string(), which produces a
full DID instead of the short owner segment used elsewhere. Update cmd_clone to
reuse resolve_owner_did (or the same keypair-to-owner logic used by cmd_info and
resolve_owner_repo_pair) so the constructed gitlawb:// URL uses the short form
consistently. Keep the existing explicit owner/name path unchanged, and ensure
bare-name cloning matches the format expected by git-remote-gitlawb and the
related tests.
| - **What needs what:** create / fork / register are signed **and** iCaptcha-gated; | ||
| `git push` is **signed-only** (owner signature is the gate — no per-push challenge); | ||
| reads (clone / fetch / `repo info`) need no proof. A non-existent repo returns a |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the push behavior.
git push is signed, but the helper still has a 403 iCaptcha retry path. Saying there is “no per-push challenge” reads as an absolute guarantee that the implementation doesn’t make.
📝 Suggested wording
- git push is **signed-only** (owner signature is the gate — no per-push challenge);
+ git push is **signed** and will transparently retry if the node returns an iCaptcha challenge;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **What needs what:** create / fork / register are signed **and** iCaptcha-gated; | |
| `git push` is **signed-only** (owner signature is the gate — no per-push challenge); | |
| reads (clone / fetch / `repo info`) need no proof. A non-existent repo returns a | |
| - **What needs what:** create / fork / register are signed **and** iCaptcha-gated; | |
| `git push` is **signed** and will transparently retry if the node returns an iCaptcha challenge; | |
| reads (clone / fetch / `repo info`) need no proof. A non-existent repo returns a |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 217 - 219, Clarify the `git push` wording in
README.md so it does not imply an absolute “no per-push challenge” guarantee.
Update the sentence in the access-policy section near the `create / fork /
register` and `git push` description to state that pushes are signed-only by
default while the helper may still retry on 403 iCaptcha responses, using the
same policy terms already used elsewhere in the document.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Complete CodeRabbit's request to keep the remote helper non-interactive
crates/git-remote-gitlawb/src/main.rs:342
CodeRabbit's thread is still unresolved, and the current push retry path callsicaptcha_client::obtain_proof(&cfg, None). PassingNonelets the client fall back tointeractive_prompt, which reads from process stdin. In the remote helper, stdin is the Git remote-helper protocol stream, so an unexpected challenge type can block waiting for input or consume protocol bytes instead of returning a clean Git error. Please complete that review request by using a non-interactive proof path ingit-remote-gitlawband surfacing an actionable failure when the proof cannot be solved automatically. -
[P2] Complete CodeRabbit's request to preserve the short owner form for bare clone names
crates/gl/src/repo.rs:311
CodeRabbit's bare-name clone comment is still valid.cmd_infoandresolve_owner_repo_pairresolve owner-less repo names throughresolve_owner_did, which returns the short key segment, butcmd_clonenow useskp.did().to_string()directly. That makesgl repo clone myrepobuildgitlawb://did:key:z.../myrepowhile the adjacent commands and helper path shape usegitlawb://z.../myrepo. Please complete the review request by reusingresolve_owner_didor the same short-owner derivation for the bare-name branch. -
[P2] Complete CodeRabbit's request to avoid copying the full push pack
crates/git-remote-gitlawb/src/main.rs:332
The retry loop clonesrequest_bodybefore every send, so even the normal first push attempt duplicates the entire pack in memory. Large pushes are exactly where this helper needs predictable memory use, and the previous code avoided that copy by moving the body once. Please complete CodeRabbit's request by using a shared cloneable body representation, such asbytes::Bytes, so retries can reuse the body without duplicating the pack bytes. -
[P3] Complete CodeRabbit's request to align
gl doctorwith header-driven iCaptcha discovery
crates/gl/src/doctor.rs:210
The write retry path derives the iCaptcha endpoint from the node'sx-icaptcha-urlheader andIcaptchaCfg::new; the only env var that path reads isGITLAWB_ICAPTCHA_API_KEY.gl doctorinstead probesGITLAWB_ICAPTCHA_URLor the default URL and tells users to set that variable, which will not fix a gated-write failure against a node advertising a different iCaptcha service. Please complete the review request by checking or describing the same header-driven endpoint the retry path actually uses. -
[P3] Complete CodeRabbit's request to fix the push/iCaptcha documentation drift
README.md:217
The README saysgit pushhas "no per-push challenge", but this PR adds agit-remote-gitlawb403 challenge/retry path for push. Even if push is signed-only by policy today, the absolute wording contradicts the implementation and will mislead users debugging a node that does return a challenge. Please complete the review request by documenting that pushes are signed by default while the helper can retry if the node returns an iCaptcha challenge. -
[P3] Link the protocol discussion required by the contribution guide
PR description
The PR body marks DID/signing/wire-format impact and notes newx-icaptcha-*response headers plus clientx-icaptcha-proofbehavior, but the issue link is stillCloses #and the protocol-change checklist item for prior discussion is unchecked.CONTRIBUTING.mdasks contributors to open an issue first for protocol-level changes. Please link the issue or discussion that covers this wire change before this lands, or split/adjust the scope if the protocol direction has not been agreed yet.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The core flow is right: structured iCaptcha discovery mirroring human_detected, sub-bound proofs, node gate untouched, CI green. Two security issues in the new icaptcha-client crate that the earlier reviews did not reach, plus a consolidation of the open push-path threads.
Findings
-
[P1] Sanitize and length-bound the iCaptcha response before it reaches the terminal
crates/icaptcha-client/src/lib.rs:133
On a non-2xx from the iCaptcha service,resp.text().unwrap_or_default()(andAnswerResult::Failed { reason }at :113) goes raw intobail!, which prints throughmain() -> Resultin bothglandgit-remote-gitlawb. The service URL is the node'sx-icaptcha-url, and nodes are untrusted here (a MITM on the default http transport qualifies), so these are attacker-influenceable bytes reaching the TTY with control chars intact. That is the CWE-150 terminal-escape class #137 fixed inhttp_error_message, reintroduced in the new crate. Driving a hostile endpoint throughobtain_proofconfirmed it: raw ESC bytes land in the error and a 50KB body is echoed unbounded. Strip C0/C1 controls and cap the excerpt (same shape as #137), and bound the read. -
[P2] Do not hand credentials or trust to a node-chosen iCaptcha URL
crates/icaptcha-client/src/lib.rs:126
x-icaptcha-urlbecomes the base URL with no scheme or host check, thenrequest_challenge/submit_answerPOST the caller DID and, whenGITLAWB_ICAPTCHA_API_KEYis set, that key as a bearer token to it on the first hop. In the repro a hostile node capturedAuthorization: Bearer <key>directly, no redirect involved (reqwest already stripsAuthorizationon cross-host redirects, so that path is closed). It is also a client-side SSRF primitive. Require https and pin or allowlist the host, and do not attach the API key to a node-discovered origin. With a key configured this is effectively P1 exfiltration. -
[P2] Remove the dead iCaptcha retry loop from the push path
crates/git-remote-gitlawb/src/main.rs:319
This is the root cause under the two open main.rs threads (clone-the-pack and interactive-prompt).git_receive_pack(crates/gitlawb-node/src/api/repos.rs:789) never callsverify_request; only create/fork/register do. So a push 403 is always a plainForbiddenwith nox-icaptcha-*headers, and the loop bails on the first pass without ever solving (icaptcha_cfg_from_headersreturnsNoneon a headerless 403). The loop is dead, yet it reintroducesrequest_body.clone()on every push and carries the latentinteractive_promptstdin read. Deleting it and restoring the moved body resolves both threads at once and keeps the helper non-interactive, rather than adding abytes::Bytesbody plus a non-interactive solver for a path that cannot fire. -
[P3] Concur on the two open doctor/README threads
crates/gl/src/doctor.rs:207,README.md:219
gl doctorprobesGITLAWB_ICAPTCHA_URL, which nothing in the solve path reads (IcaptchaCfg::newreads onlyGITLAWB_ICAPTCHA_API_KEY), and it hits/v1/pubkeywhile the client uses/v1/challengeand/v1/answer. The README "no per-push challenge" line becomes accurate again once the dead loop above is removed. Both already raised by CodeRabbit and jatmn; the only add is that removing the loop moots the README wording.
On the DID-format thread (CodeRabbit repo.rs:329, seconded as jatmn's J2): I ran this end to end and it is not a break. git clone gitlawb://did:key:z6Mk.../repo (full DID) and the short form both dispatch to the helper and produce a byte-identical node request (GET /z6Mk.../repo/info/refs... in both), because parse_gitlawb_url reduces either authority to the same short owner before any node contact. Full-DID is also what gl repo create (repo.rs:261) and gl repo info (repo.rs:362) emit and what the README documents, so bare-name clone matches them; the short form is used only for REST API paths, never a gitlawb:// URL. Reusing resolve_owner_did here is harmless but would make the emitted clone URL differ from what create prints, so I would leave it. jatmn's protocol-discussion-link ask (J6) still stands on its own.
3 participants
Summary
Builds the client side of the iCaptcha gate so a legitimate agent completes the full git lifecycle (register → repo create → push → clone) transparently — no manual headers, no
GITLAWB_ICAPTCHA_PROOFenv hack. Server enforcement is unchanged.Motivation & context
The node enforces iCaptcha on writes (create/fork/register), but
gl/git-remote-gitlawbhad no native support — an agent testing the lifecycle hit a chain of failures (silent version drift →401, then403 iCaptcha proof required, thensubmismatch, plus misleadingrepo infostubs). This implements the sanctioned flow: solve the challenge and retry automatically.Closes #
Kind of change
Summary
Builds the client side of the iCaptcha gate so a legitimate agent completes the full git lifecycle (register → repo create → push → clone) transparently — no manual headers, no
GITLAWB_ICAPTCHA_PROOFenv hack. Server enforcement is unchanged.Motivation & context
The node enforces iCaptcha on writes (create/fork/register), but
gl/git-remote-gitlawbhad no native support — an agent testing the lifecycle hit a chain of failures (silent version drift →401, then403 iCaptcha proof required, thensubmismatch, plus misleadingrepo infostubs). This implements the sanctioned flow: solve the challenge and retry automatically.Closes #
Kind of change
What changed
icaptcha-client— deterministic solvers (arithmetic/algebra/sequence) +obtain_proof()challenge→solve→answer loop;requesterId= caller DID (so proofsubmatches the signer).gl(crates/gl) —http.rs: unified signed writes that, on a403iCaptcha challenge (detected via newx-icaptcha-url/x-icaptcha-levelheaders), solve and retry withx-icaptcha-proof(bounded retry for the ~5 min TTL); removes the env hack; actionable hint on401 not_an_agent.repo.rs:repo info/clonereturn a real 404 instead of a stub with a placeholder owner DID.doctor.rs: gl-vs-node version drift + iCaptcha reachability checks.git-remote-gitlawb— same shared client; push stays RFC-9421 signed, with a 403-iCaptcha solve/retry safety net; clone/fetch unsigned; missing-repo 404 kept.gitlawb-node— the403 icaptcha_proof_requiredresponse now advertises the service URL + required level as JSON fields andx-icaptcha-url/x-icaptcha-levelheaders (mirrors the existinghuman_detectedpattern). Thesub == authenticated-DID, expiry, and level checks are unchanged.requesterId == DIDrule, proof TTL).How a reviewer can verify