Skip to content

Commit 86341d1

Browse files
authored
test(node): guard that every repo-scoped handler is visibility-gated (#122)
* test(node): guard that every repo-scoped handler is visibility-gated Mirrors the existing every_in_scope_mutation_has_its_gate source-string CI test on the egress side: scrapes every repo-scoped handler (any pub/pub(crate) async fn taking Path<(String,String..)>) and asserts it carries an authz marker, either a read gate (authorize_repo_read / visibility_check) or a write/owner/self gate (require_repo_owner / require_owner / did_matches / &auth.0), or is listed in KNOWN_UNGATED with its tracking issue. Verb-agnostic on purpose: a repo-scoped read named outside list_/get_ (git_info_refs, replicate_encrypted_blobs, withheld_paths) is still checked, so a new ungated egress of any name fails CI instead of shipping as the next leak. A completeness scan over src/api/ asserts every module declaring a repo-scoped handler is wired into the guard, so a whole new file cannot add an ungated handler the scrape never looks at. KNOWN_UNGATED is the live gap list (certs/issues/labels/ bounties/stars per #120, plus events/webhooks/replicas/protected pending PR #113); the staleness assert forces each entry out the moment its gate lands. A count floor trips if the scrape silently breaks, and unit tests pin handler_names and is_repo_scoped. Verified by execution: removing an allowlist entry trips the missing-gate assert; listing a gated handler trips the staleness assert; dropping a source file trips the completeness scan; breaking the parser trips the count floor; and a non-list/get handler (git_info_refs) is confirmed scraped and gate-checked. * test(node): don't let a service-conditional gate satisfy the egress guard (#122) The guard counted a handler as gated on bare substring presence of a gate marker, so git_info_refs false-passed on visibility_check( even though that check only runs under `if service == "git-upload-pack"` and leaves the git-receive-pack advertisement ungated. Add gate_runs_unconditionally, which ignores markers that sit only inside an `if service ==` block, and track git_info_refs in KNOWN_UNGATED until #119 makes the gate unconditional. When that lands, the staleness assert forces the entry's removal. * fix(review): harden gate_runs_unconditionally against unclosed blocks (#122) Code review of the egress-guard hardening: - Clamp the span-advance index so an unclosed `if service ==` block can't slice past the body end and panic (latent: real source is balanced, but the scraper should not crash on a pathological future body). Keep span-to-EOF as the fail-safe direction. - Document that only `if service ==` is detected, not `match service`, so a future match-arm gate isn't silently treated as full. - Add unit cases: gate inside both service blocks (conditional), gate inside and outside (full), and the unclosed-block no-panic regression.
1 parent 8b9ceec commit 86341d1

1 file changed

Lines changed: 318 additions & 0 deletions

File tree

  • crates/gitlawb-node/src/api

‎crates/gitlawb-node/src/api/mod.rs‎

Lines changed: 318 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,4 +250,322 @@ mod authz_guard {
250250
"a marker present only in a comment must not count as an enforced gate"
251251
);
252252
}
253+
254+
/// All `pub async fn` / `pub(crate) async fn` handler names declared in `src`.
255+
/// Verb-agnostic on purpose: a repo-scoped read of any name is in scope, so a
256+
/// handler named `fetch_*` / `replicate_*` / `info_refs` cannot escape the gate
257+
/// check by not being called `list_*` / `get_*`.
258+
fn handler_names(src: &str) -> Vec<String> {
259+
["pub async fn ", "pub(crate) async fn "]
260+
.iter()
261+
.flat_map(|decl| {
262+
src.match_indices(decl).map(move |(i, _)| {
263+
src[i + decl.len()..]
264+
.chars()
265+
.take_while(|c| c.is_alphanumeric() || *c == '_')
266+
.collect::<String>()
267+
})
268+
})
269+
.collect()
270+
}
271+
272+
/// True when the handler's signature takes an owner+repo path
273+
/// (`Path<(String, String...)>`), i.e. it is a repo-scoped read rather than a
274+
/// node-wide aggregate (`/stats`, `/ipfs/pins`, the global feeds).
275+
fn is_repo_scoped(body: &str) -> bool {
276+
let header = &body[..body.find('{').unwrap_or(body.len())];
277+
header.contains("Path<(String, String")
278+
}
279+
280+
/// True when at least one gate marker runs for EVERY service — i.e. some
281+
/// marker sits outside any `if service == ...` discriminator block. A gate
282+
/// that appears ONLY inside such a block (as the info/refs advertisement gate
283+
/// did before #119: `visibility_check` ran under `if service ==
284+
/// "git-upload-pack"`, leaving `git-receive-pack` ungated) covers a subset of
285+
/// services and must NOT count as a full gate. Other handlers carry no
286+
/// `service ==` discriminator, so for them this matches the plain
287+
/// substring check. NOTE: only `if service ==` is detected — a
288+
/// `match service { .. }` discriminator is NOT tracked and a gate inside one
289+
/// arm would pass as full; avoid that shape, or extend the span loop below.
290+
fn gate_runs_unconditionally(body: &str, markers: &[&str]) -> bool {
291+
// Brace-matched spans of each `if service == ...` block.
292+
let mut cond_spans: Vec<(usize, usize)> = Vec::new();
293+
let mut search = 0;
294+
while let Some(rel) = body[search..].find("if service ==") {
295+
let cond_start = search + rel;
296+
let Some(brace_rel) = body[cond_start..].find('{') else {
297+
break;
298+
};
299+
let open = cond_start + brace_rel;
300+
let mut depth = 0i32;
301+
let mut end = body.len();
302+
for (i, c) in body[open..].char_indices() {
303+
match c {
304+
'{' => depth += 1,
305+
'}' => {
306+
depth -= 1;
307+
if depth == 0 {
308+
end = open + i;
309+
break;
310+
}
311+
}
312+
_ => {}
313+
}
314+
}
315+
cond_spans.push((open, end));
316+
// On an unclosed block `end` stays at body.len() (the fail-safe
317+
// direction: treat the rest as conditional rather than mask a gate);
318+
// clamp so the next slice can't index past the end and panic.
319+
search = (end + 1).min(body.len());
320+
}
321+
markers.iter().any(|m| {
322+
body.match_indices(m)
323+
.any(|(pos, _)| !cond_spans.iter().any(|(s, e)| pos >= *s && pos <= *e))
324+
})
325+
}
326+
327+
/// Egress gate guard: every repo-scoped handler (`Path<(String, String..)>`)
328+
/// must carry an authz marker — a read gate (`authorize_repo_read` /
329+
/// `visibility_check`), or a write gate (`require_repo_owner` / `require_owner`
330+
/// / `did_matches` / a `&auth.0` self-binding) — or be listed in KNOWN_UNGATED
331+
/// (the tracked, ungated reads). A new ungated handler of ANY verb trips this,
332+
/// and a completeness scan over `src/api/` trips it for a whole new module that
333+
/// adds a repo-scoped handler without being wired into `sources`. Mutations are
334+
/// also checked precisely, per handler, by the mutation guard above; here they
335+
/// only need *some* binding so the net stays verb-agnostic.
336+
///
337+
/// Scope and limits (this is a source scrape; the runtime route guard covers
338+
/// behaviour):
339+
/// - It proves a gate is CALLED, not that it runs on the requested path: a
340+
/// content handler gating `"/"` instead of the subtree still passes here.
341+
/// - It sees handlers taking the owner+repo tuple `Path<(String, String..)>`; a
342+
/// repo identified by a struct/custom extractor would be missed (the helper
343+
/// unit tests pin these boundaries).
344+
/// - KNOWN_UNGATED entries need a real open issue and review; the staleness
345+
/// assert removes one only once its gate lands.
346+
#[test]
347+
fn every_repo_scoped_handler_is_gated() {
348+
let sources: &[(&str, &str)] = &[
349+
(include_str!("bounties.rs"), "bounties.rs"),
350+
(include_str!("certs.rs"), "certs.rs"),
351+
(include_str!("changelog.rs"), "changelog.rs"),
352+
(include_str!("encrypted.rs"), "encrypted.rs"),
353+
(include_str!("events.rs"), "events.rs"),
354+
(include_str!("issues.rs"), "issues.rs"),
355+
(include_str!("labels.rs"), "labels.rs"),
356+
(include_str!("protect.rs"), "protect.rs"),
357+
(include_str!("pulls.rs"), "pulls.rs"),
358+
(include_str!("replicas.rs"), "replicas.rs"),
359+
(include_str!("repos.rs"), "repos.rs"),
360+
(include_str!("stars.rs"), "stars.rs"),
361+
(include_str!("visibility.rs"), "visibility.rs"),
362+
(include_str!("webhooks.rs"), "webhooks.rs"),
363+
];
364+
let listed: std::collections::HashSet<&str> = sources.iter().map(|(_, f)| *f).collect();
365+
assert!(!listed.is_empty(), "read-guard `sources` is empty");
366+
367+
// Completeness: every api/*.rs NOT already in `sources` must declare no
368+
// repo-scoped handler, so a brand-new module cannot add an ungated handler
369+
// the scrape never looks at. Reads the directory at test time (so the list
370+
// cannot silently drift from the filesystem) and only inspects unlisted
371+
// files — listed files are covered by the per-handler loop below.
372+
let api_dir = concat!(env!("CARGO_MANIFEST_DIR"), "/src/api");
373+
for (_, f) in sources {
374+
assert!(
375+
std::path::Path::new(api_dir).join(f).exists(),
376+
"read-guard `sources` lists {f} but the file does not exist"
377+
);
378+
}
379+
for entry in std::fs::read_dir(api_dir).expect("read src/api") {
380+
let path = entry.expect("dir entry").path();
381+
let fname = path.file_name().unwrap().to_string_lossy().into_owned();
382+
if !fname.ends_with(".rs") || fname == "mod.rs" || listed.contains(fname.as_str()) {
383+
continue;
384+
}
385+
let src = std::fs::read_to_string(&path).expect("read api file");
386+
let has_repo_handler = handler_names(&src)
387+
.iter()
388+
.any(|n| is_repo_scoped(&fn_body(&src, n)));
389+
assert!(
390+
!has_repo_handler,
391+
"api/{fname} declares a repo-scoped handler but is not in the egress \
392+
guard `sources` list — add it so its handlers are gate-checked"
393+
);
394+
}
395+
396+
// Repo-scoped reads known to be ungated today, each tracked by an issue.
397+
// Remove an entry the moment its gate lands (the staleness assert enforces it).
398+
let known_ungated: &[(&str, &str)] = &[
399+
// info/refs gates only git-upload-pack today; git-receive-pack
400+
// advertisement is ungated until #119 makes the gate unconditional.
401+
("git_info_refs", "#119"),
402+
("list_certs", "#120"),
403+
("get_cert", "#120"),
404+
("list_issues", "#120"),
405+
("get_issue", "#120"),
406+
("list_issue_comments", "#120"),
407+
("list_labels", "#120"),
408+
("list_repo_bounties", "#120"),
409+
("get_star_status", "#120"),
410+
("list_repo_events", "#94 (PR #113)"),
411+
("list_webhooks", "#94 (PR #113)"),
412+
("list_replicas", "PR #113"),
413+
("list_protected_branches", "PR #113"),
414+
];
415+
let is_known = |n: &str| known_ungated.iter().any(|(k, _)| *k == n);
416+
// Any one of these = the handler binds the caller to an authz decision: the
417+
// first two are read gates, the rest are the write/owner/self-binding forms.
418+
// A repo-scoped mutation passes here on its write gate; the mutation guard
419+
// above additionally verifies the exact gate type per handler. NOTE: a local
420+
// rename of `caller`/`replica_did` must be paired with a marker update here.
421+
let markers = [
422+
"authorize_repo_read(",
423+
"visibility_check(",
424+
"require_repo_owner(",
425+
"require_owner(",
426+
"did_matches(",
427+
"caller = &auth.0",
428+
"replica_did = &auth.0",
429+
];
430+
431+
// Every KNOWN_UNGATED name must be a real handler (catch typos / renames).
432+
let all: std::collections::HashSet<String> =
433+
sources.iter().flat_map(|(s, _)| handler_names(s)).collect();
434+
for (n, _) in known_ungated {
435+
assert!(
436+
all.contains(*n),
437+
"KNOWN_UNGATED lists `{n}`, which is not a real handler (renamed or removed?)"
438+
);
439+
}
440+
441+
let mut checked = 0usize;
442+
for (src, file) in sources {
443+
for name in handler_names(src) {
444+
let body = fn_body(src, &name);
445+
if !is_repo_scoped(&body) {
446+
continue; // node-wide aggregate, not a repo-scoped surface
447+
}
448+
checked += 1;
449+
let gated = gate_runs_unconditionally(&body, &markers);
450+
assert!(
451+
gated || is_known(&name),
452+
"repo-scoped handler `{name}` ({file}) has no authz gate and is \
453+
not in KNOWN_UNGATED — add the visibility/owner gate with the \
454+
caller, or track it there"
455+
);
456+
assert!(
457+
!(gated && is_known(&name)),
458+
"handler `{name}` ({file}) is now gated — remove it from \
459+
KNOWN_UNGATED so the allowlist stays an accurate gap list"
460+
);
461+
}
462+
}
463+
// Tripwire: if the scrape silently stopped finding handlers (e.g. a parser
464+
// regression in handler_names/is_repo_scoped), this floor fails loudly
465+
// instead of the guard passing vacuously. Current count is ~54 repo-scoped
466+
// handlers; 20 is a deliberate floor that trips only on a gross regression.
467+
assert!(
468+
checked >= 20,
469+
"egress guard only checked {checked} repo-scoped handlers — the scrape likely broke"
470+
);
471+
}
472+
473+
/// Pins the `handler_names` boundary: it collects every `pub`/`pub(crate)
474+
/// async fn` regardless of verb (so a `fetch_*` read cannot escape), and skips
475+
/// private `async fn` and sync `fn` helpers.
476+
#[test]
477+
fn handler_names_finds_all_pub_async_and_skips_others() {
478+
let src = "pub async fn list_things() {}\n\
479+
pub async fn fetch_thing() {}\n\
480+
pub(crate) async fn get_crate_thing() {}\n\
481+
async fn private_helper() {}\n\
482+
fn sync_helper() {}\n";
483+
let names = handler_names(src);
484+
// Verb-agnostic: a non-list/get read verb is still seen.
485+
assert!(names.contains(&"list_things".to_string()));
486+
assert!(names.contains(&"fetch_thing".to_string()));
487+
// pub(crate) routed handlers are in scope too.
488+
assert!(names.contains(&"get_crate_thing".to_string()));
489+
// Private/sync helpers are not routed handlers and are skipped.
490+
assert!(!names
491+
.iter()
492+
.any(|n| n == "private_helper" || n == "sync_helper"));
493+
}
494+
495+
/// Pins the `is_repo_scoped` boundary: an owner+repo tuple Path is repo-scoped;
496+
/// a single-segment or absent Path is a node-wide aggregate.
497+
#[test]
498+
fn is_repo_scoped_requires_owner_repo_path() {
499+
let repo =
500+
"fn get_x(s: State, Path((owner, name)): Path<(String, String)>) {\n body();\n}";
501+
let three = "fn get_y(Path((o, n, id)): Path<(String, String, String)>) {\n body();\n}";
502+
let node_wide = "fn list_z(s: State<AppState>) {\n body();\n}";
503+
let single = "fn get_w(Path(cid): Path<String>) {\n body();\n}";
504+
assert!(is_repo_scoped(repo), "owner+repo tuple is repo-scoped");
505+
assert!(is_repo_scoped(three), "owner+repo+id tuple is repo-scoped");
506+
assert!(!is_repo_scoped(node_wide), "no Path is node-wide");
507+
assert!(
508+
!is_repo_scoped(single),
509+
"single-segment Path is not repo-scoped"
510+
);
511+
}
512+
513+
/// Pins `gate_runs_unconditionally`: a gate nested only inside an
514+
/// `if service == ...` block is conditional (does NOT count), while the same
515+
/// gate at the top level — or an additional unconditional one — does.
516+
#[test]
517+
fn conditional_service_gate_is_not_a_full_gate() {
518+
let markers = ["visibility_check("];
519+
// Gate runs only for one service: not a full gate.
520+
let conditional = "fn f() {\n \
521+
if service == \"git-upload-pack\" {\n \
522+
visibility_check(rules, caller);\n \
523+
}\n \
524+
if service == \"git-receive-pack\" { acquire_fresh(); }\n}";
525+
assert!(
526+
!gate_runs_unconditionally(conditional, &markers),
527+
"a gate only inside `if service ==` covers a subset of services"
528+
);
529+
// Same gate at top level: full gate.
530+
let unconditional = "fn f() {\n \
531+
visibility_check(rules, caller);\n \
532+
if service == \"git-receive-pack\" { acquire_fresh(); }\n}";
533+
assert!(
534+
gate_runs_unconditionally(unconditional, &markers),
535+
"an unconditional gate runs for every service"
536+
);
537+
// A gate inside EACH of two service blocks, none outside: still a
538+
// subset (no service clears it unconditionally), so not a full gate.
539+
let both_conditional = "fn f() {\n \
540+
if service == \"git-upload-pack\" { visibility_check(a); }\n \
541+
if service == \"git-receive-pack\" { visibility_check(b); }\n}";
542+
assert!(
543+
!gate_runs_unconditionally(both_conditional, &markers),
544+
"a gate inside every service block is still conditional"
545+
);
546+
// A marker inside one block AND again unconditionally: the
547+
// unconditional occurrence makes it a full gate (exercises the
548+
// match_indices scan past the in-block hit).
549+
let inside_and_outside = "fn f() {\n \
550+
if service == \"git-upload-pack\" { visibility_check(a); }\n \
551+
visibility_check(b);\n}";
552+
assert!(
553+
gate_runs_unconditionally(inside_and_outside, &markers),
554+
"an unconditional occurrence counts even when another is conditional"
555+
);
556+
// No marker at all: not gated.
557+
assert!(!gate_runs_unconditionally(
558+
"fn f() { do_thing(); }",
559+
&markers
560+
));
561+
// An unclosed `if service ==` block (e.g. phantom brace from a string
562+
// literal) must not panic on the slice advance; the span runs to EOF, so
563+
// the in-block marker reads as conditional. Real Rust source is balanced,
564+
// so this only guards the scraper against a future pathological body.
565+
let unclosed = "fn f() { if service == \"x\" { visibility_check(a);";
566+
assert!(
567+
!gate_runs_unconditionally(unclosed, &markers),
568+
"an unclosed service block must not panic and stays conditional"
569+
);
570+
}
253571
}

0 commit comments

Comments
 (0)