Skip to content

Commit 6c95592

Browse files
authored
fix(node): prefer canonical repo row over mirror row in get_repo (#124) (#141)
* fix(node): prefer canonical repo row over mirror row in get_repo (#124) get_repo matched both mirror rows (bare short-DID, is_public=true, no visibility rules) and canonical rows (full did:key: DID, correct rules), returning one nondeterministically. When the mirror row won, the visibility gate saw is_public=true + empty rules, allowing unauthenticated read of private/withheld content. Add ORDER BY to prefer canonical rows (UUID id, no slash) over mirror rows (slash-form id), matching the ranking logic already used by DEDUP_CTE and dedupe_canonical_repos. Test seeds mirror FIRST so an unordered fetch_optional returns the mirror row — proving the test locks in the fix. * fix(test): date canonical row after mirror to guard CASE term in get_repo fix (#124) * fix(node): tighten get_repo owner matching to did:key only, not any DID method (#124 P2) * fix(node): refactor owner DID normalization to a dedicated function in create_repo * fix(node): align clone URL and slug construction with tightened did:key matching - Make normalize_owner_key pub(crate) and reuse it in api/repos, api/events, api/peers, and visibility modules. - Add read-gate authorize_repo_read and to_response clone_url slug tests. * fix(node): hoist owner-key CASE into shared const + address #124 review feedback - Hoist 7 verbatim SQL CASE copies into OWNER_KEY_CASE_SQL const (dedup_cte(), get_repo, count_repos_deduped), keeping one literal copy in the v7 migration with a cross-reference comment. - Add 9 unit tests for normalize_owner_key over the full boundary set. - Add LIMIT 1 to get_repo query for single-row intent. - Add non_key_owner_bare_short_does_not_match is_owner regression test. - Update is_owner doc to reflect did:key-only rule. - Document non-key clone_url disk-path constraint in to_response. * test(node): add SQL round-trip test for normalize_owner_key vs OWNER_KEY_CASE_SQL Verifies every boundary value produces identical results from the Rust function and the SQL CASE expression, preventing silent drift. --------- Co-authored-by: Gravirei <gravirei@users.noreply.github.com>
1 parent 466a550 commit 6c95592

6 files changed

Lines changed: 469 additions & 58 deletions

File tree

‎Cargo.lock‎

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,7 @@ pub async fn list_repo_events(
6767
let repo_id_str = if let Some(ref record) = repo_record {
6868
format!(
6969
"{}/{}",
70-
record
71-
.owner_did
72-
.split(':')
73-
.next_back()
74-
.unwrap_or(&record.owner_did),
70+
crate::db::normalize_owner_key(&record.owner_did),
7571
repo_name
7672
)
7773
} else {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,8 @@ pub async fn trigger_sync(State(state): State<AppState>) -> Result<Json<serde_js
296296
repo.get("name").and_then(|v| v.as_str()),
297297
) {
298298
(Some(owner), Some(name)) => {
299-
// Use short owner (last colon segment) matching DB convention
300-
let short = owner.split(':').next_back().unwrap_or(owner);
299+
// Use short owner (did:key-only normalization) matching DB convention
300+
let short = crate::db::normalize_owner_key(owner);
301301
format!("{short}/{name}")
302302
}
303303
_ => continue,

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

Lines changed: 63 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -945,11 +945,7 @@ pub async fn git_receive_pack(
945945
.as_deref()
946946
.unwrap_or("http://127.0.0.1:7545")
947947
.trim_end_matches('/');
948-
let owner_short = record
949-
.owner_did
950-
.split(':')
951-
.next_back()
952-
.unwrap_or(&record.owner_did);
948+
let owner_short = crate::db::normalize_owner_key(&record.owner_did);
953949
let clone_url = format!("{}/{}/{}.git", base_url, owner_short, record.name);
954950

955951
for update in &ref_updates {
@@ -1096,7 +1092,7 @@ pub async fn git_receive_pack(
10961092
// this push to Arweave, so the oid->cid index survives total
10971093
// node loss. Best-effort; never fails the push.
10981094
if !delta.is_empty() && !irys_url.is_empty() {
1099-
let owner_short = owner_did.split(':').next_back().unwrap_or(&owner_did);
1095+
let owner_short = crate::db::normalize_owner_key(&owner_did);
11001096
let repo_slug = format!("{owner_short}/{repo_name}");
11011097
let ts = chrono::Utc::now().to_rfc3339();
11021098
let manifest = crate::arweave::EncryptedManifest {
@@ -1141,11 +1137,7 @@ pub async fn git_receive_pack(
11411137
let node_did_str = state.node_did.to_string();
11421138
let repo_slug = format!(
11431139
"{}/{}",
1144-
record
1145-
.owner_did
1146-
.split(':')
1147-
.next_back()
1148-
.unwrap_or(&record.owner_did),
1140+
crate::db::normalize_owner_key(&record.owner_did),
11491141
record.name
11501142
);
11511143
let ref_updates_clone = ref_updates
@@ -1474,7 +1466,7 @@ pub async fn fork_repo(
14741466
}
14751467

14761468
// Check no name conflict under the forker's ownership
1477-
let forker_short = forker_did.split(':').next_back().unwrap_or(&forker_did);
1469+
let forker_short = crate::db::normalize_owner_key(&forker_did);
14781470
if state.db.get_repo(forker_short, &fork_name).await?.is_some() {
14791471
return Err(AppError::BadRequest(format!(
14801472
"you already have a repo named {fork_name}"
@@ -1630,13 +1622,17 @@ fn parse_ref_updates(body: &[u8]) -> Vec<RefUpdate> {
16301622
}
16311623

16321624
// ── Helpers ───────────────────────────────────────────────────────────────
1625+
//
1626+
// For a non-key DID owner, `normalize_owner_key` returns the full DID, so
1627+
// `clone_url` becomes `/did:gitlawb:z6.../repo.git`. That resolves through
1628+
// `get_repo`, but the colon-bearing path segment would break the `sync.rs`
1629+
// disk-path join (`owner_slug/repo`). Not reachable today (auth is
1630+
// did:key-only), so this is a forward constraint to handle before non-key
1631+
// ownership lands: the owner-first disk layout must either reject colons or
1632+
// encode them.
16331633

16341634
fn to_response(record: &crate::db::RepoRecord, state: &AppState, star_count: i64) -> RepoResponse {
1635-
let owner_short = record
1636-
.owner_did
1637-
.split(':')
1638-
.next_back()
1639-
.unwrap_or(&record.owner_did);
1635+
let owner_short = crate::db::normalize_owner_key(&record.owner_did);
16401636

16411637
let base_url = state
16421638
.config
@@ -2340,4 +2336,54 @@ mod tests {
23402336

23412337
_mock.assert_async().await;
23422338
}
2339+
2340+
#[tokio::test]
2341+
async fn to_response_generates_correct_clone_url_slug() {
2342+
let state = crate::test_support::test_state_lazy();
2343+
let now = chrono::Utc::now();
2344+
2345+
// 1. did:key owner (should strip did:key: prefix)
2346+
let repo_key = crate::db::RepoRecord {
2347+
id: "uuid-1".into(),
2348+
name: "my-repo".into(),
2349+
owner_did: "did:key:z6Mkwbud".into(),
2350+
description: None,
2351+
is_public: true,
2352+
default_branch: "main".into(),
2353+
created_at: now,
2354+
updated_at: now,
2355+
disk_path: "/tmp/my-repo".into(),
2356+
forked_from: None,
2357+
machine_id: None,
2358+
};
2359+
let response_key = to_response(&repo_key, &state, 5);
2360+
assert!(
2361+
response_key.clone_url.contains("/z6Mkwbud/my-repo.git"),
2362+
"clone_url should use the bare did:key ID. got: {}",
2363+
response_key.clone_url
2364+
);
2365+
2366+
// 2. did:gitlawb owner (non-key DID method, should NOT strip)
2367+
let repo_non_key = crate::db::RepoRecord {
2368+
id: "uuid-2".into(),
2369+
name: "other-repo".into(),
2370+
owner_did: "did:gitlawb:z6Mkwbud".into(),
2371+
description: None,
2372+
is_public: true,
2373+
default_branch: "main".into(),
2374+
created_at: now,
2375+
updated_at: now,
2376+
disk_path: "/tmp/other-repo".into(),
2377+
forked_from: None,
2378+
machine_id: None,
2379+
};
2380+
let response_non_key = to_response(&repo_non_key, &state, 10);
2381+
assert!(
2382+
response_non_key
2383+
.clone_url
2384+
.contains("/did:gitlawb:z6Mkwbud/other-repo.git"),
2385+
"clone_url should preserve the full non-key owner DID. got: {}",
2386+
response_non_key.clone_url
2387+
);
2388+
}
23432389
}

0 commit comments

Comments
 (0)