fix(node): stop a peer-supplied repo slug from escaping repos_dir (#272) - #274
Conversation
The sync queue carries a peer-supplied `repo` string that the worker turns into a filesystem path, and `PathBuf::join` does not normalize, so an absolute second component replaces the accumulated path. `a//tmp/x` resolved to `/tmp/x.git`, outside repos_dir entirely. Validate the slug once, in the module that already owns the owner and name rules, and return the two halves so callers cannot re-split and drift. The only rule added here is the leading `.`/`-` check on the owner half, which `validate_owner_did` lacks because it also serves full DIDs. Each guard is covered by a case that isolates it: the earlier rules shadow the obvious candidates, so `../hello` does not actually exercise the owner delegation and `a//tmp/x` does not exercise the separator count.
…272) process_batch split item.repo on the first '/' and joined both halves onto repos_dir. Since PathBuf::join does not normalize and an absolute second component replaces the path, a queued `a//tmp/x` cloned an attacker's origin to /tmp/x.git with repos_dir left empty. The worker is the real choke point rather than defence in depth: three writers reach sync_queue (the notify handler, trigger_sync, and the gossipsub ref-update handler at p2p/mod.rs), and rows queued before this change are already in the table. The tests serve the composed remote URL for every hostile slug, not just the escaping one. With an unreachable remote those rows end failed anyway because the clone fails, so the scenarios would have passed before the guard existed and proven nothing.
/api/v1/sync/notify accepts unsigned callers by default, and its only gate was that node_did appear in the peer table, which one unsigned announce of the caller's own DID satisfies. req.repo then reached enqueue_sync verbatim. Validate after the peer check so an unknown peer still gets the unknown-peer message, and before enqueue_sync so nothing is written. The 400 names the field and carries no path: the validator's text never interpolates the slug, so a hostile string is not echoed back either. NOTIFY_BODY used "demo", a slug with no separator, so the three brake tests would have started asserting against the new 400 instead of the brake.
…it (#272) The character rules on the slug cannot see a symlink. Two cases get through them: a symlinked owner directory, and a symlinked <name>.git leaf. The leaf is the one a parent-only check misses, since the parent canonicalizes clean, exists() follows the link, and the fetch branch then runs git -C through it with the mirror refspec, force-overwriting refs outside repos_dir. So the predicate canonicalizes the candidate when symlink_metadata says it exists, and its parent only when it does not. Canonicalizing the candidate unconditionally would reject every first clone, which is total loss of mirroring, and the must-not-break test covers that direction. Three-valued rather than a bool because the worker has to tell a hostile path from a filesystem that could not answer. Outside is terminal; an I/O error leaves the row pending, since dequeue only selects pending and a transient EACCES would otherwise retire a legitimate repo permanently. The predicate is pure. create_dir_all lives at the call site so the admin purge path can ask a containment question without mutating anything.
The two throwaway probes from the investigation, in attack form, asserting the closed outcome. The router-level one drives the full router with no auth extension, because the point it pins is reachability: the attack needs no signature and no operator config change, only an unsigned announce followed by an unsigned notify. The worker-level one seeds the peer row directly, since process_batch resolves the peer URL before it looks at the slug and an unseeded row dies at the no-peer arm, which would leave the test green with the guard gone. It asserts on the parent directory too, because git clone removes the destination on failure but leaves the parent. Note for reviewers: reverting the worker slug check alone does not recreate the absolute-path escape, since containment catches it and leaves the row pending. That check is independently load-bearing for ./hello, which canonicalizes back inside repos_dir and so passes containment.
Code review caught an availability regression introduced by the guard added earlier on this branch. validate_owner_did bails only above 256 chars, but the owner half becomes a single path component and Linux NAME_MAX is 255, so a 256-char owner passed validation and then failed create_dir_all with ENAMETOOLONG on every attempt. The worker left such a row pending, and dequeue_pending_syncs is oldest-first with a fixed batch of ten, so ten unsigned notify calls held the entire window and starved every healthy repo behind them. Before this branch the same row reached git clone, failed, and was retired. Two changes. Reject the length in the shared validator, so an undeliverable slug never enters the queue from either boundary. Then split the create_dir_all arm by whether the error can ever clear: a transient one still leaves the row pending to be retried, a permanently uncreatable path is marked failed rather than re-picked forever. The earlier test missed this by asserting 257, one past the bound. It now pins both sides: 256 rejected, 255 still accepted. Also make the two permission-bit tests fail loudly instead of returning early. One probed with symlink_metadata on a path that was never created, which is Err for every user, so its root guard could never fire; the other returned green after an eprintln that cargo swallows on a passing test, so a root runner would have reported ok while exercising nothing.
Two follow-ups from code review. The pending-on-IoError arm had no operator-visible signal: the only record_sync_processed calls were the done and failed arms, so a queue stalled on an unmounted repos_dir looked identical to an idle one. Both deferring arms now record "deferred" and the guard rejections record "rejected", which also distinguishes a retryable stall from a terminal refusal. There is still no attempt cap; bounding retries needs an attempts column on sync_queue, which is a migration and belongs in its own change. The two escape tests were asserting that repos_dir stays empty, but the owner directory is created before containment has a parent to canonicalize, so that assertion was detecting the create_dir_all side effect rather than an escape. They now assert no mirror exists anywhere under repos_dir. Their comments were also wrong about which layer holds. Mutation testing says the slug rule and containment are each independently sufficient for that input: removing either alone leaves the tests green, and only removing both turns them red, with the mirror observed written outside repos_dir. The comments now say that, and point at the per-layer witnesses instead: ./hello isolates the slug rule because it resolves back inside repos_dir and containment approves it, and the two symlink tests isolate containment because no character rule can see a symlink.
📝 WalkthroughWalkthroughRepo slug validation and canonical containment checks protect notification and sync-worker filesystem paths. Sync dequeue now records attempt timestamps and rotates previously attempted rows. Unit, database, notification, and end-to-end tests cover these behaviors. ChangesRepo path safety
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NotifyRequest
participant notify_sync
participant SyncQueue
participant process_batch
participant repos_dir
NotifyRequest->>notify_sync: submit repo and node_did
notify_sync->>notify_sync: validate_repo_slug
notify_sync->>SyncQueue: enqueue valid sync item
process_batch->>SyncQueue: dequeue and stamp attempted_at
process_batch->>process_batch: validate_repo_slug
process_batch->>repos_dir: verify canonical containment
process_batch->>repos_dir: create mirror within root
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/sync.rs (1)
292-305: 🩺 Stability & Availability | 🔵 TrivialDeferred rows can head-of-line block the batch.
dequeue_pending_syncsis oldest-first with a fixed limit of 10, so ten rows stuck on a persistentIoError(e.g. an attacker-planted dangling symlink underrepos_dir, or a stale EACCES directory) occupy the whole window on every poll and starve healthy repos indefinitely. The attempt cap is explicitly deferred, so consider an alert onsync_processed{status="deferred"}sustained above zero, plus a backlog-age gauge, so the stall is caught operationally until theattemptscolumn lands.🤖 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/gitlawb-node/src/sync.rs` around lines 292 - 305, Add operational monitoring for deferred sync rows so persistent IoErrors cannot silently starve the fixed oldest-first batch. Extend the existing sync metrics around record_sync_processed("deferred") with an alert for sustained nonzero deferred processing and expose a gauge for the age of the oldest pending sync row. Keep the current retry and attempt-cap behavior unchanged.
🤖 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.
Nitpick comments:
In `@crates/gitlawb-node/src/sync.rs`:
- Around line 292-305: Add operational monitoring for deferred sync rows so
persistent IoErrors cannot silently starve the fixed oldest-first batch. Extend
the existing sync metrics around record_sync_processed("deferred") with an alert
for sustained nonzero deferred processing and expose a gauge for the age of the
oldest pending sync row. Keep the current retry and attempt-cap behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8270845-202b-43f6-a607-95f173e2779e
📒 Files selected for processing (4)
crates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/metrics.rscrates/gitlawb-node/src/sync.rs
beardthelion
added
crate:node
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] Do not let deterministic directory failures pin the whole sync queue
crates/gitlawb-node/src/sync.rs:255
A dangling owner symlink, symlink loop, or regular file atrepos_dir/<owner>makescreate_dir_allfail with errors such asAlreadyExists/FilesystemLoop, but only three error kinds are treated as permanent. The row remainspending; becausedequeue_pending_syncs(10)is oldest-first, ten such rows are retried every poll and permanently starve every healthy item behind them. This is a regression from the prior clone/fetch path, which terminally failed these rows. Classify deterministic filesystem states as terminal (after a non-following containment check) or add retry scheduling/queue progress so deferred rows cannot monopolize the batch.
…owner path create_dir_all reports AlreadyExists when a regular file, dangling symlink, or link loop occupies repos_dir/<owner>, and none of those clear on a retry. Classifying them as transient left the row pending, and since dequeue_pending_syncs is oldest-first over a fixed batch of 10, a handful of them held the window and starved every healthy repo behind them. This restores the pre-#272 behavior, where the same state failed the clone and hit the terminal Err arm. A concurrent mkdir resolves to Ok rather than AlreadyExists, because create_dir_all falls back to is_dir() on EEXIST, so the race is not caught by this classification.
A row the worker cannot make progress on stays pending, and while its ordering key never moved it stayed among the oldest rows forever, holding the fixed-size window against every healthy repo behind it. Classifying more error kinds as terminal does not close this: an owner directory that exists but denies traversal lets create_dir_all succeed and defers at path_within_root instead, per-repo rather than as a whole-repos_dir outage. dequeue_pending_syncs now selects and stamps in one statement, ordering by COALESCE(attempted_at, enqueued_at), so the key means least recently handed out. Doing it in the dequeue rather than at each deferral branch makes it hold by construction: no call site can forget it, a batch that dies mid-loop still leaves its rows stamped, and a failed stamp is a failed dequeue the caller already handles. enqueued_at keeps meaning enqueue time so backlog age stays measurable. The column lands as migration v12, not a v1 append: v1 never re-runs on an existing install, so appending there would leave every deployed node dequeueing against a column that does not exist. The upgrade-path test simulates a v11 node and is the only test that catches that placement, since every #[sqlx::test] otherwise starts from a fresh database. N stuck rows still cost N slots per rotation, delaying a healthy row by up to ceil(N/10) polls, so the attempts column and a retry cap remain worth doing; this removes the starvation, not the residual cost.
…emise Three review fixes on the two commits above. The docstring claimed more than the code delivers. UPDATE ... RETURNING does not order its output, so the batch is the right set in no particular order (nothing in process_batch depends on the order); and nothing is claimed, since the rows stay pending with no lock held past the statement, so two workers against one database can still be handed the same batch. Single-worker is the existing assumption and FOR UPDATE SKIP LOCKED is what would change it. It also implied the caller handles a failed stamp robustly; the caller logs and skips the poll, which is worth stating plainly now that this writes and can fail for reasons a plain SELECT could not. The outer UPDATE now repeats status = 'pending'. Between the subquery and the update a concurrent worker can settle a row, and without the predicate the statement would stamp and return a row that had already left the pending set. Both starvation tests now assert the healthy row is still pending before the poll that frees it. Without that they would keep passing if the batch size ever grew past the stuck set, having silently stopped exercising head-of-line yield, and the larger test now also asserts the stuck rows rotated rather than settled.
…branches v12 was main's current_max + 1, which INV-7a says is necessary and not sufficient: the runner keys the applied set on the integer alone, so a version another in-flight branch also claims is skipped in full on whichever side merges second. No error, no warning, schema_migrations still reading healthy, and the column simply absent, after which dequeue_pending_syncs fails on every poll and sync ingestion stalls. Two open branches already claim into that range. #135/#173 holds through 14 (15 once it rebases past main's v11) and #253 took 16. 17 clears both, and the reservation is recorded in a comment so the next author can see it. Gaps are harmless; the runner iterates the array and never requires contiguity. Not doing the other half here. INV-7a's load-bearing rule is the name check that makes the silent skip impossible rather than merely unlikely, and that lands with #253 on test/replay-guards-253; adding it on this branch would collide with that work.
|
Confirmed, with one correction to the mechanism and one addition. All three states you listed come back as Classifying error kinds does not close the class, though. An owner directory that exists but denies traversal makes So both halves:
Each new test was observed failing first: a regular file and a dangling symlink at One thing I would rather state than have found later: this removes the starvation, not the residual cost. N permanently deferring rows still consume N dequeue slots per rotation and delay a healthy row by up to Two smaller notes on the diff. The migration is version 17 rather than 12 because two open branches already claim into that range and the runner keys the applied set on the version integer alone; the reservation comment above it records which branches and why. And |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/gitlawb-node/src/db/mod.rs (2)
1658-1697: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a supporting index for the dequeue
ORDER BY.
dequeue_pending_syncsnow sorts onCOALESCE(attempted_at, enqueued_at)every poll, but the only index touchingsync_queueis the plainstatusindex from v1. The PR's own test suite exercises scenarios where pending rows accumulate indefinitely (deliberately no attempt cap), so this sort will run over a growing pending set without index support. A partial expression index would let Postgres avoid the in-memory sort as the backlog grows.♻️ Proposed index addition to migration v17
Migration { version: 17, name: "sync_queue_attempted_at", stmts: &[ // Scheduling key for dequeue_pending_syncs: when the row was last // handed to a worker. Null until first dequeued, which is why the // ordering coalesces onto enqueued_at. "ALTER TABLE sync_queue ADD COLUMN IF NOT EXISTS attempted_at TEXT", + // Supports the dequeue ORDER BY without an in-memory sort as the + // pending backlog grows. + "CREATE INDEX IF NOT EXISTS idx_sync_queue_pending_order \ + ON sync_queue (COALESCE(attempted_at, enqueued_at)) \ + WHERE status = 'pending'", ], },🤖 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/gitlawb-node/src/db/mod.rs` around lines 1658 - 1697, Add a migration index supporting dequeue_pending_syncs: create a partial expression index on sync_queue for pending rows, keyed by COALESCE(attempted_at, enqueued_at), so the ORDER BY can remain efficient as the backlog grows. Register it in migration v17 alongside the existing sync_queue indexes.
886-903: 📐 Maintainability & Code Quality | 🔵 TrivialVersion-17 reservation is a manual, out-of-band convention — works here, worth a heads-up for future collisions.
The gap-tolerant strictly-increasing check plus the upgrade-path test give confidence this specific jump is safe, but the underlying risk the comment itself describes (two branches independently claiming the same version number, silently dropping one migration on whichever side merges second) is inherent to hand-picking version numbers by convention rather than deriving them automatically (e.g., from a counter file or CI-assigned sequence). Not a blocker for this PR, but worth considering as a longer-term fix for the migration process.
🤖 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/gitlawb-node/src/db/mod.rs` around lines 886 - 903, The migration version is manually reserved, creating a future collision risk when branches choose the same number. No immediate code change is required for this migration; record this as a follow-up to replace hand-picked versions in the migration registry with an automatic or CI-managed sequencing mechanism, while preserving the strictly increasing and gap-tolerant upgrade behavior.
🤖 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.
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 1658-1697: Add a migration index supporting dequeue_pending_syncs:
create a partial expression index on sync_queue for pending rows, keyed by
COALESCE(attempted_at, enqueued_at), so the ORDER BY can remain efficient as the
backlog grows. Register it in migration v17 alongside the existing sync_queue
indexes.
- Around line 886-903: The migration version is manually reserved, creating a
future collision risk when branches choose the same number. No immediate code
change is required for this migration; record this as a follow-up to replace
hand-picked versions in the migration registry with an automatic or CI-managed
sequencing mechanism, while preserving the strictly increasing and gap-tolerant
upgrade behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a595f9a-2eeb-4559-9a78-dae850814995
📒 Files selected for processing (2)
crates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/sync.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/src/sync.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
My prior queue-starvation finding is fixed on this head: AlreadyExists at the owner path is terminal again, and attempted_at stamping rotates deferred rows so a full batch of stuck items no longer starves healthy repos behind them.
Thanks for the contribution. I do not see any additional actionable issues from my review.
Prior feedback
- [P2] Deterministic owner-path failures pinning the sync queue — resolved on
fbc5868.create_dir_allAlreadyExistsatrepos_dir/<owner>is terminal again (crates/gitlawb-node/src/sync.rs:274-287), anddequeue_pending_syncsstampsattempted_atso deferred rows rotate (crates/gitlawb-node/src/db/mod.rs:1689-1694). The starvation tests pin that premise before asserting yield.
Reviewed and rejected
I looked at these again on the current head and am recording them here so they do not get re-opened as new blockers.
path_within_root/repos_extraprefix trap — not a bug.Path::starts_withis component-wise;/var/repos_extra/...does not match/var/repos. Canonical containment still blocks the symlink cases this PR tests.- Gossip /
trigger_synclack ingress slug validation — real asymmetry, but an acknowledged residual. The worker re-validates before any path join or git call; this PR's notify boundary is additive, not incomplete coverage of the stated threat model. list_peerserror marks the dequeued batchfailed— pre-existing on base; not introduced by this diff. Worth a follow-up if you want transient DB blips to defer rather than retire, but not a regression from #272.- Empty
http_urlreturnsSome("")fromresolve_origin_url— pre-existing inconsistency withtrigger_sync's empty-URL skip; not introduced here. upsert_mirror_repoerror ignored beforemark_sync_done— pre-existing; unchanged by this PR.- Migration v17 manual reservation / missing dequeue index — operational and performance nits, not correctness defects on this head. The upgrade-path test covers the happy path; the reservation comment documents the collision risk.
|
@jatmn thanks for the re-review. Your latest review on Could you re-submit as an approval? Nothing has changed on the head since your pass, and the two CodeRabbit nits you already triaged (v17 version reservation, the partial index for the dequeue |
3 participants
Closes #272.
An unauthenticated caller could plant a git mirror at an arbitrary path on disk.
The bug
POST /api/v1/sync/notifyaccepts unsigned callers by default:require_signed_peer_writesis false, soserver.rsmounts the peer-write routes underoptional_signaturerather than the auth layers. Its only gate is thatnode_didappear in the peers table, and one unsigned announce of your own DID satisfies that.req.repothen reachedenqueue_syncverbatim, andprocess_batchsplit it on the first/and joined both halves ontorepos_dir.PathBuf::joindoes not normalize, and an absolute second component replaces the accumulated path, soa//tmp/xresolved to/tmp/x.git.Reproduced end to end before any of this was written: unsigned announce 200, unsigned notify 200, then the worker cloned an attacker-controlled origin to
/tmp/gitlawb-escape-probe.gitwithrepos_dirleft empty and the peer'slast_ping_okFALSE. The sync path never reads that flag, so nothing about peer reachability was in the way.The fix
Four layers, each catching something the others cannot:
repo_store::validate_repo_slug./hello, which canonicalizes back insiderepos_dirand so passes containmentapi/peers.rssync_queueand out ofreceived_ref_updatessync.rsprocess_batchrepo_store::path_within_root.gitleaf, which no character rule can seeThe validator reuses
validate_owner_didandvalidate_repo_namerather than adding a second character rule to the crate. It adds two rules of its own: an owner half may not begin with.or-(those validators have no leading-character rule, and an owner of.puts a peer-controlled mirror at therepos_dirroot), and the owner half is bounded by NAME_MAX at 255 rather than the DID column's 256.Containmentis three-valued rather than a bool, and that distinction is load-bearing.Outsideis a deterministic verdict, so the row is retired.IoErrormeans the filesystem could not answer, so the row stays pending and is retried, because marking it failed is terminal and a transient EACCES would otherwise permanently retire a legitimate repo. The predicate is pure;create_dir_alllives at the call site so the admin purge path in #196 can ask a containment question without mutating anything.Verification
549 tests, clippy clean under
-D warnings,cargo fmt --checkclean.Every guard was mutation-tested: removed, observed red on a named case, restored. Worth stating plainly because five tests here were green before the guard they named existed, and were rewritten once that was found:
../hellodoes not exercise the owner delegation; the leading-character rule catches it first.a..b/hellodoes.z6Mkfoo/hello/extradoes.failedregardless when the remote was unreachable, because the clone simply failed. The fixture now serves every composed URL for real.symlink_metadataon a path that was never created, which isErrfor every user, so its guard could never fire.The two escape tests assert the end-to-end property rather than isolating a layer: the slug rule and containment are each independently sufficient for that input, so removing either alone leaves them green and only removing both turns them red, with the mirror observed written outside
repos_dir. Per-layer isolation lives in the./hellocase and the two symlink tests. The comments say so.An independent adversarial pass through a different model family returned no findings against the final state.
Deliberately not in this PR
node_didto be the repo's recorded origin. That needs an answer to what happens on the first sync of a repo this node has never seen, which depends on the DID key-binding decision in Announce does not bind a DID to its first-seen key, so any caller can rewrite any peer's http_url #273.RepoStore::local_path, thesync.rsjoin,repo_disk_path). It relocates every existing mirror and orphans thedisk_pathalready recorded on mirror rows, so it needs its own change with a migration.attemptscolumn onsync_queue, which is a migration. The newdeferredandrejectedmetric labels are what make a stalled queue visible in the meantime.Known residuals
A well-formed slug naming a repo this node already mirrors still lets a known peer drive a fetch into it, repointing
remote.originand force-overwriting refs through the mirror refspec. That survives every guard here because the slug is well formed and the path stays insiderepos_dir. It closes with the origin-ownership check above.The window between the containment check and the git invocation is not atomic. Exploiting it needs local write access inside
repos_dir, so it sits outside the unauthenticated threat model this addresses.The gossip and trigger writers still reach
sync_queuewithout an ingress check. The worker gate covers the path-write consequence; the ref-update feed remains poisonable with arbitrary strings, which is pre-existing.Note for whoever rebases:
repo_store.rsis also touched by #196, additively and in the same area. Whichever lands second resolves a small textual conflict there. Nothing else in this branch overlaps it.Summary by CodeRabbit
rejectedanddeferredstatuses.