Skip to content

fix(node): close quarantine bypass on encrypted blobs, visibility routes, and sibling mutations #566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
beardthelion merged 6 commits into from
Nov 10, 2026
Merged

fix(node): close quarantine bypass on encrypted blobs, visibility routes, and sibling mutations #566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
30e6856
fix(node): close quarantine bypass on encrypted blobs and visibility …
Ayush7614 Aug 10, 2026
7bdf9be
Merge remote-tracking branch 'origin/main' into pr-276
beardthelion Nov 9, 2026
55242a8
fix(node): gate sibling mutation endpoints on quarantine, reconcile 4…
beardthelion Nov 10, 2026
92f75c9
fix(node): release quarantined mirror fixtures, bound 404 body read i…
beardthelion Nov 10, 2026
b56b733
test(node): pin read-before-owner order on every dual-gated handler (…
beardthelion Nov 10, 2026
344b194
test(node): pin list_webhooks in the order loop, close-owner second a…
beardthelion Nov 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 10 additions & 31 deletions crates/gitlawb-node/src/api/encrypted.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,24 @@ use axum::Json;
use crate::auth::AuthenticatedDid;
use crate::error::{AppError, Result};
use crate::state::AppState;
use crate::visibility::{visibility_check, Decision};

/// GET /api/v1/repos/{owner}/{repo}/encrypted-blobs
/// Returns [{oid, cid}] for every encrypted blob in the repo, to any caller who
/// can read the repo. Not recipient-scoped: recipient identities are not stored,
/// so access control here is repo readability and decryption is gated by the
/// envelope crypto (only a real recipient can open an envelope).
///
/// Quarantined repos are opaque 404 via [`crate::api::authorize_repo_read`] —
/// same as issues/changelogs — so a public-but-quarantined mirror cannot leak
/// its encrypted blob index.
pub async fn list_encrypted_blobs(
State(state): State<AppState>,
auth: Option<Extension<AuthenticatedDid>>,
Path((owner, repo)): Path<(String, String)>,
) -> Result<Json<serde_json::Value>> {
let record = state
.db
.get_repo(&owner, &repo)
.await?
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?;
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
let rules = state.db.list_visibility_rules(&record.id).await?;
if visibility_check(&rules, record.is_public, &record.owner_did, caller, "/") == Decision::Deny
{
return Err(AppError::RepoNotFound(format!("{owner}/{repo}")));
}
let (record, _rules) =
crate::api::authorize_repo_read(&state, &owner, &repo, caller, "/").await?;
let rows = state.db.list_all_encrypted_blobs(&record.id).await?;
let blobs: Vec<_> = rows
.into_iter()
Expand All @@ -45,17 +40,9 @@ pub async fn get_encrypted_blob(
auth: Option<Extension<AuthenticatedDid>>,
Path((owner, repo, oid)): Path<(String, String, String)>,
) -> Result<Vec<u8>> {
let record = state
.db
.get_repo(&owner, &repo)
.await?
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?;
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
let rules = state.db.list_visibility_rules(&record.id).await?;
if visibility_check(&rules, record.is_public, &record.owner_did, caller, "/") == Decision::Deny
{
return Err(AppError::RepoNotFound(format!("{owner}/{repo}/{oid}")));
}
let (record, _rules) =
crate::api::authorize_repo_read(&state, &owner, &repo, caller, "/").await?;
let cid = state
.db
.encrypted_blob_cid(&record.id, &oid)
Expand All @@ -81,17 +68,9 @@ pub async fn replicate_encrypted_blobs(
auth: Option<Extension<AuthenticatedDid>>,
Path((owner, repo)): Path<(String, String)>,
) -> Result<Json<serde_json::Value>> {
let record = state
.db
.get_repo(&owner, &repo)
.await?
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?;
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
let rules = state.db.list_visibility_rules(&record.id).await?;
if visibility_check(&rules, record.is_public, &record.owner_did, caller, "/") == Decision::Deny
{
return Err(AppError::RepoNotFound(format!("{owner}/{repo}")));
}
let (record, _rules) =
crate::api::authorize_repo_read(&state, &owner, &repo, caller, "/").await?;
let rows = state.db.list_all_encrypted_blobs(&record.id).await?;
let blobs: Vec<_> = rows
.into_iter()
Expand Down
115 changes: 75 additions & 40 deletions crates/gitlawb-node/src/api/issues.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,49 +238,32 @@ pub async fn close_issue(
headers: axum::http::HeaderMap,
crate::rate_limit::PeerAddr(peer): crate::rate_limit::PeerAddr,
) -> Result<Json<serde_json::Value>> {
let record = state
.db
.get_repo(&owner, &repo)
.await?
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?;

// READ-GATE before any snapshot work or rate charging. The author fallback below
// needs the issue blob, which needs the repo tree, so authorship cannot be established
// without a download; but a caller who cannot even READ the repo must be
// stopped here, cheaply, before any Tigris transfer or extraction happens.
// Without this, any signed non-owner could issue parallel close requests for
// arbitrary issue ids and drive unbounded downloads and blocking extraction
// (a disposable-identity DoS), because the route has no other pre-authorization.
// READ-GATE before any snapshot work or rate charging, through the shared
// authorize_repo_read gate so quarantined repos are opaque here too. The
// author fallback below needs the issue blob, which needs the repo tree, so
// authorship cannot be established without a download; but a caller who
// cannot even READ the repo must be stopped here, cheaply, before any
// Tigris transfer or extraction happens. Without this, any signed non-owner
// could issue parallel close requests for arbitrary issue ids and drive
// unbounded downloads and blocking extraction (a disposable-identity DoS),
// because the route has no other pre-authorization.
//
// Rate limiting runs AFTER this gate so a 429 cannot distinguish a hidden repo
// from a missing one (INV-12 read-denial status contract).
// Rate limiting runs AFTER this gate so a 429 cannot distinguish a hidden
// repo from a missing one (INV-12 read-denial status contract).
//
// mirror-rows-handled: a repo row synced from a peer is stored public and
// carries none of the owner's visibility rules, so for such a row this check
// can only return allow. That is deliberate rather than overlooked, and it is
// the same verdict every other read gate in this API reaches for one. Refusing
// instead would deny the repo's real owner and the issue's real author on any
// node whose only copy of the repo is a synced one, which is an ordinary state
// here, and it would not buy the protection it appears to, because a synced
// row's recorded owner comes from the peer that sent it. The expensive work
// this check guards is bounded ahead of it by the per-IP limiter and the read
// pool slot taken below, and the authoritative owner-or-author decision still
// runs below and again under the write lock.
{
// mirror-rows-handled: synced mirror rows carry no owner rules; read gate only.
let rules = state.db.list_visibility_rules(&record.id).await?;
let caller = auth.0.as_str();
if crate::visibility::visibility_check(
&rules,
record.is_public,
&record.owner_did,
Some(caller),
"/",
) == crate::visibility::Decision::Deny
{
return Err(AppError::RepoNotFound(format!("{owner}/{repo}")));
}
}
// carries none of the owner's visibility rules, so for such a row this gate
// can only return allow. That is deliberate rather than overlooked, and it
// is the same verdict every other read gate in this API reaches for one.
// Refusing instead would deny the repo's real owner and the issue's real
// author on any node whose only copy of the repo is a synced one, which is
// an ordinary state here, and it would not buy the protection it appears
// to, because a synced row's recorded owner comes from the peer that sent
// it. The expensive work this check guards is bounded ahead of it by the
// per-IP limiter and the read pool slot taken below, and the authoritative
// owner-or-author decision still runs below and again under the write lock.
let (record, _rules) =
crate::api::authorize_repo_read(&state, &owner, &repo, Some(auth.0.as_str()), "/").await?;

// Per-IP flood brake, layered on the same shared limiter and trusted-proxy
// policy as the push advertisement. The pre-lock snapshot downloads the
Expand Down Expand Up @@ -502,6 +485,15 @@ mod tests {
.await
.expect("get_repo")
.expect("repo exists");
// upsert_mirror_repo seeds the row quarantined, and the read gate now
// hides quarantined rows as opaque 404 before the owner-or-author check
// this test names. Release it so the stranger is refused by the 403, not
// by the quarantine short-circuit.
state
.db
.set_repo_quarantine(&record.id, false)
.await
.expect("release seed quarantine");

// An independent session holds the repo's write lock for the whole call.
let key = crate::git::repo_store::advisory_lock_key_for_test(
Expand Down Expand Up @@ -703,6 +695,15 @@ mod tests {
.await
.expect("get_repo")
.expect("seeded repo exists");
// upsert_mirror_repo seeds the admission flag as quarantined, but the
// gate now hides quarantined rows from every caller class. Release it
// so these tests exercise what they name: the owner-or-author check
// after the read gate, not the quarantine short-circuit.
state
.db
.set_repo_quarantine(&record.id, false)
.await
.expect("release seed quarantine");
let _ = owner_did;
let path = state
.repo_store
Expand All @@ -728,6 +729,40 @@ mod tests {
path
}

/// The read gate blocks the owner (and any caller) before the
/// owner-or-author checks: even the owner gets the opaque 404 on a
/// quarantined repo, and the seeded row's quarantined flag is left set
/// deliberately. The released-mirror variants above exercise the
/// owner-or-author path.
#[sqlx::test]
async fn quarantined_repo_close_is_opaque_404(pool: PgPool) {
let state = crate::test_support::test_state(pool.clone()).await;
let owner_did = "did:key:z6MkT1Owner";
state
.db
.upsert_mirror_repo("z6MkT1Owner", "tquar", "/unused", None, true)
.await
.expect("seed repo row");

let res = close_issue(
axum::extract::State(state.clone()),
axum::Extension(crate::auth::AuthenticatedDid(owner_did.to_string())),
axum::extract::Path((
"z6MkT1Owner".to_string(),
"tquar".to_string(),
"1".to_string(),
)),
axum::http::HeaderMap::new(),
crate::rate_limit::PeerAddr(Some("203.0.113.70:5000".parse().unwrap())),
)
.await;
assert!(
matches!(res, Err(crate::error::AppError::RepoNotFound(_))),
"a quarantined repo must 404 close_issue for every caller: {:?}",
res.ok().map(|_| "unexpected success")
);
}

/// INV-21(c) positive twin 1: the OWNER can still close. The reorder moved the
/// owner check above the lock, so this is the arm most likely to have broken,
/// and the deny test alone could not see it.
Expand Down
14 changes: 4 additions & 10 deletions crates/gitlawb-node/src/api/labels.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,8 @@ pub async fn add_label(
) -> Result<(StatusCode, Json<serde_json::Value>)> {
let label = canonicalize_label(&req.label)?;

let record = state
.db
.get_repo(&owner, &name)
.await?
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
let (record, _rules) =
crate::api::authorize_repo_read(&state, &owner, &name, Some(auth.0.as_str()), "/").await?;
crate::api::require_repo_owner(&record, &auth.0)?;

let added = state.db.add_label(&record.id, &label).await?;
Expand All @@ -71,11 +68,8 @@ pub async fn remove_label(
) -> Result<Json<serde_json::Value>> {
let label = canonicalize_label(&label)?;

let record = state
.db
.get_repo(&owner, &name)
.await?
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
let (record, _rules) =
crate::api::authorize_repo_read(&state, &owner, &name, Some(auth.0.as_str()), "/").await?;
crate::api::require_repo_owner(&record, &auth.0)?;

let removed = state.db.remove_label(&record.id, &label).await?;
Expand Down
78 changes: 71 additions & 7 deletions crates/gitlawb-node/src/api/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,22 @@ mod authz_guard {
"\nasync fn ",
"\npub fn ",
"\nfn ",
"#[cfg(test)]",
]
.iter()
.filter_map(|p| rest[1..].find(p).map(|i| i + 1))
.min()
.unwrap_or(rest.len());
rest[..end]
.lines()
// Strip block comments too: a marker surviving only inside /* */ would
// otherwise satisfy a row with the real gate deleted.
let mut body = rest[..end].to_string();
while let Some(open) = body.find("/*") {
match body[open..].find("*/").map(|i| open + i + 2) {
Some(close) => body.replace_range(open..close, ""),
None => break,
}
}
body.lines()
.filter(|l| !l.trim_start().starts_with("//"))
.collect::<Vec<_>>()
.join("\n")
Expand All @@ -209,9 +218,14 @@ mod authz_guard {

// (source, handler, expected gate marker)
let rows: &[(&str, &str, &str)] = &[
// Bucket A — owner-gate (require_repo_owner -> 403)
// Bucket A — owner-gate (require_repo_owner -> 403), now behind
// the shared read gate first so a hidden repo 404s for every
// caller class before the owner check can answer 403.
(pulls, "merge_pr", "authorize_repo_read("),
(pulls, "merge_pr", "require_repo_owner("),
(webhooks, "create_webhook", "authorize_repo_read("),
(webhooks, "create_webhook", "require_repo_owner("),
(webhooks, "delete_webhook", "authorize_repo_read("),
(webhooks, "delete_webhook", "require_repo_owner("),
// Bucket A/B hybrid — list_webhooks is read-visibility THEN owner:
// authorize_repo_read 404s a non-reader of a private repo, then
Expand All @@ -220,10 +234,15 @@ mod authz_guard {
// (existence hiding) and require_repo_owner guards the owner half.
(webhooks, "list_webhooks", "authorize_repo_read("),
(webhooks, "list_webhooks", "require_repo_owner("),
(labels, "add_label", "authorize_repo_read("),
(labels, "add_label", "require_repo_owner("),
(labels, "remove_label", "authorize_repo_read("),
(labels, "remove_label", "require_repo_owner("),
// Bucket A' — owner OR author (did_matches against the author)
// Bucket A' — owner OR author (did_matches against the author),
// also behind the shared read gate first.
(pulls, "close_pr", "authorize_repo_read("),
(pulls, "close_pr", "did_matches("),
(issues, "close_issue", "authorize_repo_read("),
(issues, "close_issue", "did_matches("),
// Bucket B — read-gate (authorize_repo_read)
(pulls, "create_review", "authorize_repo_read("),
Expand Down Expand Up @@ -269,21 +288,66 @@ mod authz_guard {
(bounties, "dispute_bounty", "did_matches("),
(replicas, "register_replica", "did_matches("),
(replicas, "unregister_replica", "replica_did = &auth.0"),
// PRE-GATED — already owner-gated, in-scope group; guard the gate itself
// PRE-GATED — owner-gated behind the shared read gate; guard both
(protect, "protect_branch", "authorize_repo_read("),
(protect, "protect_branch", "did_matches("),
(protect, "unprotect_branch", "authorize_repo_read("),
(protect, "unprotect_branch", "did_matches("),
(visibility, "set_visibility", "authorize_repo_read("),
(visibility, "set_visibility", "require_owner("),
(visibility, "remove_visibility", "authorize_repo_read("),
(visibility, "remove_visibility", "require_owner("),
(visibility, "list_visibility", "authorize_repo_read("),
(visibility, "list_visibility", "require_owner("),
];

// The visibility rows prove require_owner is CALLED; this proves the helper
// itself does DID-safe matching, not a raw/trailing-segment compare.
// Rows above prove each visibility handler calls authorize_repo_read and
// require_owner; this proves the helper itself does DID-safe matching.
assert!(
fn_body(visibility, "require_owner").contains("did_matches("),
"visibility::require_owner must use did_matches for DID-safe owner matching"
);

// Order matters as much as presence: hoisting the owner/author check
// above the read gate would 403 a non-owner on a hidden repo, an
// existence oracle. Pin read-gate-then-owner on every handler that
// takes both; the owner marker differs per bucket (require_repo_owner
// for owner-only, require_owner for visibility, did_matches for the
// owner-or-author and protect paths).
for (src, func, owner_marker) in [
(visibility, "set_visibility", "require_owner("),
(visibility, "remove_visibility", "require_owner("),
(visibility, "list_visibility", "require_owner("),
(pulls, "merge_pr", "require_repo_owner("),
(webhooks, "list_webhooks", "require_repo_owner("),
(webhooks, "create_webhook", "require_repo_owner("),
(webhooks, "delete_webhook", "require_repo_owner("),
(labels, "add_label", "require_repo_owner("),
(labels, "remove_label", "require_repo_owner("),
(pulls, "close_pr", "did_matches("),
(issues, "close_issue", "did_matches("),
(protect, "protect_branch", "did_matches("),
(protect, "unprotect_branch", "did_matches("),
] {
let body = fn_body(src, func);
let gate = body.find("authorize_repo_read(").expect("gate marker");
let owner = body.find(owner_marker).expect("owner marker");
assert!(
gate < owner,
"{func} must run authorize_repo_read before {owner_marker}"
);
// The owner-or-author and protect handlers also carry
// require_repo_owner: a hoist of that call above the read gate is
// the same 403-vs-404 oracle, so pin it too where it exists.
if func == "close_pr" || func == "close_issue" {
let owner_gate = body.find("require_repo_owner(").expect("owner marker");
assert!(
gate < owner_gate,
"{func} must run authorize_repo_read before require_repo_owner("
);
}
}

// The CID read surface (#173) enforces its gate inside the shared
// `gate_and_serve`, which BOTH the provenance path and the legacy scan call, so
// the markers must live there (the get_by_cid row above only proves delegation).
Expand Down
Loading
Loading