Skip to content

Commit e37ea7f

Browse files
beardthelionbeardthelion
authored
fix(node): enforce path-scoped visibility on the REST read API (#52)
* fix(node): enforce path-scoped visibility on the REST read API The repo-scoped REST read endpoints (blob, tree, commits, refs, changelog, pulls, pr diff, repo metadata) read straight from disk after a bare repo lookup, with no visibility check, so private repositories and private subtrees of public repositories were served to unauthenticated callers, bypassing the withholding the git and replication paths enforce. Add a shared authorize_repo_read gate (resolve repo, load rules, visibility_check, 404 on deny) and call it from every repo-scoped read handler. get_blob gates at the requested file path; get_pr_diff withholds the whole diff when it touches a path the caller cannot read. Attach optional_signature to the read routes so authorized readers authenticate. Listings stay gated at the repo root, consistent with mode B exposing tree and commit SHAs over git. Closes #51 * fix(node): gate PR reviews/comments and fork on repo read visibility The REST visibility gate added in this PR missed three read surfaces: list_reviews and list_comments served a private repo's PR reviews and comments to unauthenticated callers, and fork_repo cloned a full mirror of the source without checking the caller could read it. Route all three through authorize_repo_read at the repo root, matching the git-protocol gate. Also make branch_diff_names fail closed: it ignored git's exit status and returned an empty path list on error, which would let get_pr_diff serve a diff it should have withheld. Fix changelog.rs import order to satisfy cargo fmt. * fix(node): NUL-delimit branch_diff_names to keep path-gate enforcement exact git diff --name-only quotes and escapes paths containing newlines, so they no longer match the visibility globs in get_pr_diff. On a public repo where / is allowed but a subtree is denied, a touched restricted path could be missed by the deny check and the full PR diff disclosed. Use -z and split on NUL to preserve exact path bytes.
1 parent 197c4e1 commit e37ea7f

6 files changed

Lines changed: 193 additions & 67 deletions

File tree

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
//! Changelog endpoint — unified timeline of commits, merged PRs, and closed issues.
22
3-
use axum::extract::{Path, Query, State};
3+
use axum::extract::{Extension, Path, Query, State};
44
use axum::Json;
55
use serde::Deserialize;
66

7+
use crate::auth::AuthenticatedDid;
78
use crate::error::{AppError, Result};
89
use crate::git::store;
910
use crate::state::AppState;
@@ -27,12 +28,11 @@ pub async fn get_changelog(
2728
State(state): State<AppState>,
2829
Path((owner, repo)): Path<(String, String)>,
2930
Query(query): Query<ChangelogQuery>,
31+
auth: Option<Extension<AuthenticatedDid>>,
3032
) -> Result<Json<serde_json::Value>> {
31-
let record = state
32-
.db
33-
.get_repo(&owner, &repo)
34-
.await?
35-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?;
33+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
34+
let (record, _rules) =
35+
crate::api::authorize_repo_read(&state, &owner, &repo, caller, "/").await?;
3636

3737
let limit = query.limit.min(100);
3838

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
use crate::db::{RepoRecord, VisibilityRule};
2+
use crate::error::{AppError, Result};
3+
use crate::state::AppState;
4+
use crate::visibility::{visibility_check, Decision};
5+
16
pub mod agents;
27
pub mod arweave;
38
pub mod bounties;
@@ -20,3 +25,32 @@ pub mod stars;
2025
pub mod tasks;
2126
pub mod visibility;
2227
pub mod webhooks;
28+
29+
/// Resolve a repo for a read request and enforce path-scoped visibility.
30+
///
31+
/// Returns 404 (`RepoNotFound`) if the repo does not exist or the caller may not
32+
/// read `path`, using the same opaque response the git serve path returns so
33+
/// existence is not confirmed. Returns the record and its visibility rules so a
34+
/// content handler can apply an extra per-path check without a second DB query.
35+
///
36+
/// Callers pass `"/"` for repo-level reads (listings); content endpoints pass the
37+
/// specific path so a withheld subtree is denied even on an otherwise-public repo.
38+
pub(crate) async fn authorize_repo_read(
39+
state: &AppState,
40+
owner: &str,
41+
name: &str,
42+
caller: Option<&str>,
43+
path: &str,
44+
) -> Result<(RepoRecord, Vec<VisibilityRule>)> {
45+
let record = state
46+
.db
47+
.get_repo(owner, name)
48+
.await?
49+
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
50+
let rules = state.db.list_visibility_rules(&record.id).await?;
51+
if visibility_check(&rules, record.is_public, &record.owner_did, caller, path) == Decision::Deny
52+
{
53+
return Err(AppError::RepoNotFound(format!("{owner}/{name}")));
54+
}
55+
Ok((record, rules))
56+
}

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

Lines changed: 39 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,11 @@ pub async fn create_pr(
101101
pub async fn list_prs(
102102
State(state): State<AppState>,
103103
Path((owner, name)): Path<(String, String)>,
104+
auth: Option<Extension<AuthenticatedDid>>,
104105
) -> Result<Json<serde_json::Value>> {
105-
let record = state
106-
.db
107-
.get_repo(&owner, &name)
108-
.await?
109-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
106+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
107+
let (record, _rules) =
108+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
110109

111110
let prs = state.db.list_prs(&record.id).await?;
112111
Ok(Json(
@@ -118,12 +117,11 @@ pub async fn list_prs(
118117
pub async fn get_pr(
119118
State(state): State<AppState>,
120119
Path((owner, name, number)): Path<(String, String, i64)>,
120+
auth: Option<Extension<AuthenticatedDid>>,
121121
) -> Result<Json<PullRequest>> {
122-
let record = state
123-
.db
124-
.get_repo(&owner, &name)
125-
.await?
126-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
122+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
123+
let (record, _rules) =
124+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
127125

128126
let pr = state
129127
.db
@@ -138,12 +136,11 @@ pub async fn get_pr(
138136
pub async fn get_pr_diff(
139137
State(state): State<AppState>,
140138
Path((owner, name, number)): Path<(String, String, i64)>,
139+
auth: Option<Extension<AuthenticatedDid>>,
141140
) -> Result<Json<serde_json::Value>> {
142-
let record = state
143-
.db
144-
.get_repo(&owner, &name)
145-
.await?
146-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
141+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
142+
let (record, rules) =
143+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
147144

148145
let pr = state
149146
.db
@@ -156,6 +153,25 @@ pub async fn get_pr_diff(
156153
.acquire(&record.owner_did, &record.name)
157154
.await
158155
.map_err(|e| AppError::Git(e.to_string()))?;
156+
157+
// Withhold the entire diff if it touches a path the caller cannot read, so a
158+
// PR diff cannot leak private-subtree content of an otherwise-public repo.
159+
let touched = store::branch_diff_names(&disk_path, &pr.target_branch, &pr.source_branch)
160+
.map_err(|e| AppError::Git(e.to_string()))?;
161+
for p in &touched {
162+
let gate = format!("/{}", p.trim_start_matches('/'));
163+
if crate::visibility::visibility_check(
164+
&rules,
165+
record.is_public,
166+
&record.owner_did,
167+
caller,
168+
&gate,
169+
) == crate::visibility::Decision::Deny
170+
{
171+
return Err(AppError::NotFound(format!("PR #{number} not found")));
172+
}
173+
}
174+
159175
let diff = store::branch_diff(&disk_path, &pr.target_branch, &pr.source_branch)
160176
.map_err(|e| AppError::Git(e.to_string()))?;
161177

@@ -324,12 +340,11 @@ pub async fn create_review(
324340
pub async fn list_reviews(
325341
State(state): State<AppState>,
326342
Path((owner, name, number)): Path<(String, String, i64)>,
343+
auth: Option<Extension<AuthenticatedDid>>,
327344
) -> Result<Json<serde_json::Value>> {
328-
let record = state
329-
.db
330-
.get_repo(&owner, &name)
331-
.await?
332-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
345+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
346+
let (record, _rules) =
347+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
333348

334349
let pr = state
335350
.db
@@ -383,12 +398,11 @@ pub async fn create_comment(
383398
pub async fn list_comments(
384399
State(state): State<AppState>,
385400
Path((owner, name, number)): Path<(String, String, i64)>,
401+
auth: Option<Extension<AuthenticatedDid>>,
386402
) -> Result<Json<serde_json::Value>> {
387-
let record = state
388-
.db
389-
.get_repo(&owner, &name)
390-
.await?
391-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
403+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
404+
let (record, _rules) =
405+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
392406

393407
let pr = state
394408
.db

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

Lines changed: 29 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -186,12 +186,11 @@ pub async fn list_repos(
186186
pub async fn get_repo(
187187
State(state): State<AppState>,
188188
Path((owner, name)): Path<(String, String)>,
189+
auth: Option<Extension<AuthenticatedDid>>,
189190
) -> Result<Json<RepoResponse>> {
190-
let record = state
191-
.db
192-
.get_repo(&owner, &name)
193-
.await?
194-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
191+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
192+
let (record, _rules) =
193+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
195194
let count = state.db.count_stars(&record.id).await.unwrap_or(0);
196195
Ok(Json(to_response(&record, &state, count)))
197196
}
@@ -200,12 +199,11 @@ pub async fn get_repo(
200199
pub async fn list_commits(
201200
State(state): State<AppState>,
202201
Path((owner, name)): Path<(String, String)>,
202+
auth: Option<Extension<AuthenticatedDid>>,
203203
) -> Result<Json<serde_json::Value>> {
204-
let record = state
205-
.db
206-
.get_repo(&owner, &name)
207-
.await?
208-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
204+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
205+
let (record, _rules) =
206+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
209207

210208
let disk_path = state
211209
.repo_store
@@ -222,6 +220,7 @@ pub async fn list_commits(
222220
pub async fn get_blob(
223221
State(state): State<AppState>,
224222
Path((owner, name, file_path)): Path<(String, String, String)>,
223+
auth: Option<Extension<AuthenticatedDid>>,
225224
) -> Result<Response> {
226225
use axum::http::header;
227226
use axum::response::IntoResponse;
@@ -238,11 +237,10 @@ pub async fn get_blob(
238237
return Err(AppError::BadRequest("invalid file path".into()));
239238
}
240239

241-
let record = state
242-
.db
243-
.get_repo(&owner, &name)
244-
.await?
245-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
240+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
241+
let gate_path = format!("/{file_path}");
242+
let (record, _rules) =
243+
crate::api::authorize_repo_read(&state, &owner, &name, caller, &gate_path).await?;
246244

247245
let disk_path = state
248246
.repo_store
@@ -283,12 +281,11 @@ pub async fn get_blob(
283281
pub async fn get_tree_root(
284282
State(state): State<AppState>,
285283
Path((owner, name)): Path<(String, String)>,
284+
auth: Option<Extension<AuthenticatedDid>>,
286285
) -> Result<Json<serde_json::Value>> {
287-
let record = state
288-
.db
289-
.get_repo(&owner, &name)
290-
.await?
291-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
286+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
287+
let (record, _rules) =
288+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
292289

293290
let disk_path = state
294291
.repo_store
@@ -305,12 +302,11 @@ pub async fn get_tree_root(
305302
pub async fn get_tree(
306303
State(state): State<AppState>,
307304
Path((owner, name, tree_path)): Path<(String, String, String)>,
305+
auth: Option<Extension<AuthenticatedDid>>,
308306
) -> Result<Json<serde_json::Value>> {
309-
let record = state
310-
.db
311-
.get_repo(&owner, &name)
312-
.await?
313-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
307+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
308+
let (record, _rules) =
309+
crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?;
314310

315311
let disk_path = state
316312
.repo_store
@@ -982,12 +978,11 @@ pub async fn git_receive_pack(
982978
pub async fn list_refs(
983979
State(state): State<AppState>,
984980
Path((owner, repo)): Path<(String, String)>,
981+
auth: Option<Extension<AuthenticatedDid>>,
985982
) -> Result<Json<serde_json::Value>> {
986-
let _record = state
987-
.db
988-
.get_repo(&owner, &repo)
989-
.await?
990-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?;
983+
let caller = auth.as_ref().map(|e| e.0 .0.as_str());
984+
let (_record, _rules) =
985+
crate::api::authorize_repo_read(&state, &owner, &repo, caller, "/").await?;
991986

992987
let repo_slug = format!("{owner}/{repo}");
993988
let refs = state.db.list_branch_cids(&repo_slug).await?;
@@ -1090,11 +1085,10 @@ pub async fn fork_repo(
10901085
Path((owner, name)): Path<(String, String)>,
10911086
Json(req): Json<ForkRepoRequest>,
10921087
) -> Result<(StatusCode, Json<RepoResponse>)> {
1093-
let source = state
1094-
.db
1095-
.get_repo(&owner, &name)
1096-
.await?
1097-
.ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{name}")))?;
1088+
// Enforce read visibility on the source before cloning: an unauthorized
1089+
// caller must not be able to fork (full mirror) a repo they cannot read.
1090+
let (source, _rules) =
1091+
crate::api::authorize_repo_read(&state, &owner, &name, Some(auth.0.as_str()), "/").await?;
10981092

10991093
let fork_name = req.name.unwrap_or_else(|| source.name.clone());
11001094
let forker_did = auth.0;

0 commit comments

Comments
 (0)