feat(gl): sanctioned iCaptcha client flow + secure git lifecycle#138
Conversation
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds a new ChangesiCaptcha proof flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant gl as gl NodeClient
participant Node
participant IcaptchaClient
gl->>Node: signed write request (send_once)
Node-->>gl: 403 Forbidden + x-icaptcha-url/level
gl->>IcaptchaClient: obtain_proof(cfg)
IcaptchaClient->>Node: request_challenge
Node-->>IcaptchaClient: Challenge
IcaptchaClient->>IcaptchaClient: solvers::solve / interactive_prompt
IcaptchaClient->>Node: submit_answer
Node-->>IcaptchaClient: Passed (proof token)
IcaptchaClient-->>gl: proof token
gl->>Node: retry signed request with x-icaptcha-proof
Node-->>gl: 200 OK
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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.
| // ── 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`.
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.
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).
Rebased onto main (resolving the git-remote-gitlawb/main.rs conflict from #119) and addressed the review findings: - [beardthelion P2 / jatmn P2 / CodeRabbit] Removed the dead iCaptcha retry loop from the push path. git_receive_pack never calls verify_request (only create/fork/register are iCaptcha-gated), so a push 403 never carries x-icaptcha-* headers and the loop could never solve. Taking main's push path (via #119) deletes it, which also resolves the pack-clone (retries no longer copy the whole pack) and the interactive-prompt-over-git-stream threads, and restores the sanitized info/refs error path. git-remote-gitlawb no longer depends on icaptcha-client. - [beardthelion P1] Sanitize and length-bound iCaptcha error bodies before they reach the terminal. The origin is only as trusted as the node that advertised it, so its non-2xx bodies (and Failed{reason}) are attacker-influenceable; they went raw into bail! (CWE-150, the #137 class). Now bounded-read + C0/C1-stripped + capped, same shape as #137. - [beardthelion P2] Do not trust or hand credentials to a node-chosen iCaptcha URL. x-icaptcha-url was used as the base with no scheme/host check and the API key was sent to it on the first hop (SSRF + key exfiltration). resolve_solver_url now honors an advertised URL only when https + host-allowlisted (public default or the operator's GITLAWB_ICAPTCHA_URL), else falls back to the trusted origin; the API key is attached only to the operator's own configured origin. - [jatmn P2 / CodeRabbit] gl repo clone with a bare name now derives the short owner key via resolve_owner_did, matching the other commands' gitlawb://z.../name URL shape instead of the colon-bearing full DID. Skipped (with reason): jatmn P3 (doctor GITLAWB_ICAPTCHA_URL) is resolved by the P2 change — that env var is now read by the write path, so the doctor's probe is accurate. CodeRabbit User-Agent-version nit lived in the push code reverted to main. README push wording kept ("signed-only, no per-push challenge") — now accurate since the retry path is gone; the API-key bullet was updated for the new trust model. Tests: added sanitize + resolve_solver_url unit tests; full workspace green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kevincodex1
force-pushed
the
feat/icaptcha-client
branch
from
4aba938 to
83f3c74
Compare
|
@coderabbitai please do a full review. Approve if its all good |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 138 --repo Gitlawb/node --json title,body,additions,deletions,changedFiles,files -q '.'
echo "---DIFF---"
gh pr diff 138 --repo Gitlawb/node > /tmp/pr138.diff
wc -l /tmp/pr138.diffLength of output: 7406 🏁 Script executed: #!/bin/bash
sed -n '1,1278p' /tmp/pr138.diffLength of output: 49843 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,60p' crates/gitlawb-node/src/error.rsLength of output: 1847 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '195,225p' crates/gl/src/repo.rsLength of output: 1452 Review summaryI went through the full diff (all 12 changed files) rather than relying solely on the AI summary. Overall this is a solid, well-tested implementation: Strengths
One minor nit (non-blocking) In /// Derive the short DID key segment from a keypair, or fall back to the node's DID.
/// Resolve the owner short-DID for a bare repo name from the LOCAL identity.
/// Never falls back to the node's own DID (that produced bogus "owned by the
/// node" results for repos that don't exist) — if there's no local identity the
/// caller must pass an explicit `owner/name`.
async fn resolve_owner_did(_node: &str, dir: Option<&std::path::Path>) -> Result<String> {The first line should be deleted since it describes the old (now-removed) fallback behavior that the new lines explicitly say no longer happens. Nothing else stood out as a functional or security issue. This is a nit, not a blocker, so I'm approving. [approve] ✏️ Learnings added
|
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