Skip to content

Commit c566455

Browse files
committed
fix(node): pin non-blob denial and map unreadable store to 503
Add regression coverage ensuring non-blob paths deny with 404, cover the storage acquire timeout on the blob route, pin size-mismatch and metadata parse error branches, map unreadable-store faults to 503, and make the 1s delivery-timeout test deadline-safe. Refs #407
1 parent 49aebb2 commit c566455

2 files changed

Lines changed: 261 additions & 6 deletions

File tree

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

Lines changed: 74 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -584,6 +584,11 @@ pub async fn get_blob(
584584
if e.downcast_ref::<smart_http::GitServiceTimeout>().is_some() {
585585
return AppError::Timeout("git service timed out".into());
586586
}
587+
if e.to_string().contains("object store not readable") {
588+
return AppError::Overloaded(
589+
"object store temporarily unavailable, retry shortly".into(),
590+
);
591+
}
587592
AppError::Internal(e)
588593
})?;
589594

@@ -4008,10 +4013,7 @@ mod tests {
40084013
)
40094014
.await;
40104015
assert_eq!(response.status(), StatusCode::OK);
4011-
assert_eq!(
4012-
state.git_blob_semaphore.available_permits(),
4013-
MAX_CONCURRENT_BLOB_READS - 1
4014-
);
4016+
assert!(state.git_blob_semaphore.available_permits() <= MAX_CONCURRENT_BLOB_READS - 1);
40154017
// Keep the body alive without ever polling it; the timer must run anyway.
40164018
tokio::time::timeout(std::time::Duration::from_secs(5), async {
40174019
while state.git_blob_semaphore.available_permits() != MAX_CONCURRENT_BLOB_READS {
@@ -4047,11 +4049,77 @@ mod tests {
40474049
let response =
40484050
blob_route_request_path(state, "z6blobunreadable", "file.txt", "203.0.113.31:5000")
40494051
.await;
4050-
assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);
4052+
assert_eq!(response.status(), StatusCode::SERVICE_UNAVAILABLE);
40514053
use http_body_util::BodyExt;
40524054
let bytes = response.into_body().collect().await.unwrap().to_bytes();
40534055
let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
4054-
assert_eq!(body["message"], crate::error::INTERNAL_ERROR_MESSAGE);
4056+
assert_eq!(body["error"], "overloaded");
4057+
}
4058+
4059+
#[cfg(unix)]
4060+
#[sqlx::test]
4061+
async fn blob_route_returns_not_found_on_non_blob_path(pool: sqlx::PgPool) {
4062+
use axum::http::StatusCode;
4063+
use http_body_util::BodyExt;
4064+
let tmp = tempfile::TempDir::new().unwrap();
4065+
let fake = write_fake_git(
4066+
tmp.path(),
4067+
"#!/bin/sh\nif [ \"$1\" = rev-parse ]; then exit 0; fi\nif [ \"$2\" = --batch-check ]; then echo '1111111111111111111111111111111111111111 tree 1024'; exit 0; fi\nexit 1\n",
4068+
);
4069+
let state = f4_state_with_repo(pool, tmp.path(), &fake, "z6blobtree", "repo", false).await;
4070+
let response =
4071+
blob_route_request_path(state, "z6blobtree", "somedir", "203.0.113.31:5000").await;
4072+
assert_eq!(response.status(), StatusCode::NOT_FOUND);
4073+
let bytes = response.into_body().collect().await.unwrap().to_bytes();
4074+
let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
4075+
assert_eq!(body["error"], "not_found");
4076+
}
4077+
4078+
#[sqlx::test]
4079+
async fn blob_route_acquire_timeout_sheds_503_and_releases_permits(pool: sqlx::PgPool) {
4080+
use axum::http::StatusCode;
4081+
use http_body_util::BodyExt;
4082+
4083+
let tmp = tempfile::TempDir::new().unwrap();
4084+
let repos_dir = tmp.path().to_path_buf();
4085+
let mut state = crate::test_support::test_state(pool.clone()).await;
4086+
let endpoint = crate::test_support::silent_http_endpoint().await;
4087+
let tigris =
4088+
crate::git::tigris::TigrisClient::for_testing_with_endpoint("test-bucket", &endpoint)
4089+
.await;
4090+
state.repo_store = crate::git::repo_store::RepoStore::new(repos_dir, Some(tigris), pool);
4091+
state.push_limiter_trust = crate::rate_limit::TrustedProxy::None;
4092+
let mut cfg = (*state.config).clone();
4093+
cfg.git_acquire_timeout_secs = 1;
4094+
state.config = Arc::new(cfg);
4095+
4096+
// Repo exists in DB but not on disk, so acquire attempts Tigris and times out.
4097+
state
4098+
.db
4099+
.upsert_mirror_repo("z6blobacqtimeout", "repo", "/unused", None, false)
4100+
.await
4101+
.unwrap();
4102+
4103+
let response = blob_route_request_path(
4104+
state.clone(),
4105+
"z6blobacqtimeout",
4106+
"file.txt",
4107+
"203.0.113.31:5000",
4108+
)
4109+
.await;
4110+
assert_eq!(response.status(), StatusCode::SERVICE_UNAVAILABLE);
4111+
let bytes = response.into_body().collect().await.unwrap().to_bytes();
4112+
let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
4113+
assert_eq!(body["error"], "overloaded");
4114+
assert!(state.git_read_semaphore.available_permits() > 0);
4115+
assert_eq!(
4116+
state.git_blob_semaphore.available_permits(),
4117+
MAX_CONCURRENT_BLOB_READS
4118+
);
4119+
assert!(state
4120+
.git_read_per_caller
4121+
.try_acquire("203.0.113.31")
4122+
.is_some());
40554123
}
40564124

40574125
#[cfg(unix)]

‎crates/gitlawb-node/src/git/store.rs‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1302,6 +1302,193 @@ exit 1
13021302
);
13031303
}
13041304

1305+
#[cfg(unix)]
1306+
#[test]
1307+
fn bounded_file_read_guards_size_mismatch_and_metadata_parse() {
1308+
use std::os::unix::fs::PermissionsExt;
1309+
1310+
let td = tempfile::TempDir::new().unwrap();
1311+
let script = td.path().join("fakegit");
1312+
let write_fake = |body: &str| {
1313+
std::fs::write(&script, body).unwrap();
1314+
let mut permissions = std::fs::metadata(&script).unwrap().permissions();
1315+
permissions.set_mode(0o755);
1316+
std::fs::set_permissions(&script, permissions).unwrap();
1317+
};
1318+
1319+
let oid = "c".repeat(40);
1320+
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10);
1321+
1322+
// 1. Size mismatch (emitted bytes fewer than declared size):
1323+
// Declares 20 bytes, emits 5 bytes ("hello"). max_bytes is 64 (so exceeded is false).
1324+
write_fake(&format!(
1325+
"#!/bin/sh\n\
1326+
if [ \"$1\" = \"rev-parse\" ]; then echo {oid}; exit 0; fi\n\
1327+
if [ \"$1\" = \"cat-file\" ] && [ \"$2\" = \"--batch-check\" ]; then read spec; echo \"{oid} blob 20\"; exit 0; fi\n\
1328+
if [ \"$1\" = \"cat-file\" ] && [ \"$2\" = \"blob\" ]; then printf hello; exit 0; fi\n\
1329+
exit 1\n"
1330+
));
1331+
let err = super::read_file_bounded(
1332+
script.to_str().unwrap(),
1333+
td.path(),
1334+
"main",
1335+
"mismatch.txt",
1336+
64,
1337+
deadline,
1338+
)
1339+
.unwrap_err();
1340+
assert!(
1341+
err.to_string()
1342+
.contains("size changed between metadata and content reads"),
1343+
"expected size mismatch error, got: {err:#}"
1344+
);
1345+
1346+
// 2. Metadata parse: missing fields (only 2 fields, no size)
1347+
write_fake(&format!(
1348+
"#!/bin/sh\n\
1349+
if [ \"$1\" = \"rev-parse\" ]; then echo {oid}; exit 0; fi\n\
1350+
if [ \"$1\" = \"cat-file\" ] && [ \"$2\" = \"--batch-check\" ]; then read spec; echo \"{oid} blob\"; exit 0; fi\n\
1351+
exit 1\n"
1352+
));
1353+
let err = super::read_file_bounded(
1354+
script.to_str().unwrap(),
1355+
td.path(),
1356+
"main",
1357+
"bad.txt",
1358+
64,
1359+
deadline,
1360+
)
1361+
.unwrap_err();
1362+
assert!(
1363+
err.to_string().contains("omitted the object size"),
1364+
"expected missing fields error, got: {err:#}"
1365+
);
1366+
1367+
// 3. Metadata parse: non-hex OID
1368+
write_fake(
1369+
"#!/bin/sh\n\
1370+
if [ \"$1\" = \"rev-parse\" ]; then echo nothex; exit 0; fi\n\
1371+
if [ \"$1\" = \"cat-file\" ] && [ \"$2\" = \"--batch-check\" ]; then read spec; echo \"not-a-valid-hex-oid-1234567890123456789012 blob 10\"; exit 0; fi\n\
1372+
exit 1\n"
1373+
);
1374+
let err = super::read_file_bounded(
1375+
script.to_str().unwrap(),
1376+
td.path(),
1377+
"main",
1378+
"bad.txt",
1379+
64,
1380+
deadline,
1381+
)
1382+
.unwrap_err();
1383+
assert!(
1384+
err.to_string().contains("invalid object metadata"),
1385+
"expected non-hex oid error, got: {err:#}"
1386+
);
1387+
1388+
// 4. Metadata parse: trailing fields
1389+
write_fake(&format!(
1390+
"#!/bin/sh\n\
1391+
if [ \"$1\" = \"rev-parse\" ]; then echo {oid}; exit 0; fi\n\
1392+
if [ \"$1\" = \"cat-file\" ] && [ \"$2\" = \"--batch-check\" ]; then read spec; echo \"{oid} blob 10 trailing-junk\"; exit 0; fi\n\
1393+
exit 1\n"
1394+
));
1395+
let err = super::read_file_bounded(
1396+
script.to_str().unwrap(),
1397+
td.path(),
1398+
"main",
1399+
"bad.txt",
1400+
64,
1401+
deadline,
1402+
)
1403+
.unwrap_err();
1404+
assert!(
1405+
err.to_string().contains("invalid object metadata"),
1406+
"expected trailing fields error, got: {err:#}"
1407+
);
1408+
1409+
// 5. Metadata parse: multi-record output
1410+
write_fake(&format!(
1411+
"#!/bin/sh\n\
1412+
if [ \"$1\" = \"rev-parse\" ]; then echo {oid}; exit 0; fi\n\
1413+
if [ \"$1\" = \"cat-file\" ] && [ \"$2\" = \"--batch-check\" ]; then read spec; echo \"{oid} blob 10\n{oid} blob 10\"; exit 0; fi\n\
1414+
exit 1\n"
1415+
));
1416+
let err = super::read_file_bounded(
1417+
script.to_str().unwrap(),
1418+
td.path(),
1419+
"main",
1420+
"bad.txt",
1421+
64,
1422+
deadline,
1423+
)
1424+
.unwrap_err();
1425+
assert!(
1426+
err.to_string().contains("multiple object metadata records"),
1427+
"expected multi-record error, got: {err:#}"
1428+
);
1429+
}
1430+
1431+
#[cfg(unix)]
1432+
#[test]
1433+
fn blob_metadata_bounded_reprobe_budget_exhaustion_returns_timeout() {
1434+
use std::os::unix::fs::PermissionsExt;
1435+
1436+
let td = tempfile::TempDir::new().unwrap();
1437+
let script = td.path().join("fakegit");
1438+
std::fs::write(
1439+
&script,
1440+
"#!/bin/sh\n\
1441+
if [ \"$1\" = \"cat-file\" ] && [ \"$2\" = \"--batch-check\" ]; then sleep 0.05; read spec; echo \"$spec missing\"; exit 0; fi\n\
1442+
exit 1\n",
1443+
)
1444+
.unwrap();
1445+
let mut permissions = std::fs::metadata(&script).unwrap().permissions();
1446+
permissions.set_mode(0o755);
1447+
std::fs::set_permissions(&script, permissions).unwrap();
1448+
1449+
let deadline = std::time::Instant::now() + std::time::Duration::from_millis(30);
1450+
let err = super::blob_metadata_bounded(
1451+
script.to_str().unwrap(),
1452+
td.path(),
1453+
"spec",
1454+
b"spec\n",
1455+
deadline,
1456+
)
1457+
.unwrap_err();
1458+
assert!(
1459+
err.downcast_ref::<crate::git::smart_http::GitServiceTimeout>()
1460+
.is_some(),
1461+
"exhausted reprobe budget must return GitServiceTimeout, got: {err:#}"
1462+
);
1463+
}
1464+
1465+
#[test]
1466+
fn read_file_bounded_denies_non_blob_paths() {
1467+
let td = tempfile::TempDir::new().unwrap();
1468+
let work = td.path();
1469+
let run = |args: &[&str]| {
1470+
let status = std::process::Command::new("git")
1471+
.args(args)
1472+
.current_dir(work)
1473+
.status()
1474+
.unwrap();
1475+
assert!(status.success(), "git {args:?} failed");
1476+
};
1477+
run(&["init", "-q"]);
1478+
run(&["config", "user.email", "t@t"]);
1479+
run(&["config", "user.name", "t"]);
1480+
std::fs::create_dir(work.join("subfolder")).unwrap();
1481+
std::fs::write(work.join("subfolder/file.txt"), b"contents").unwrap();
1482+
run(&["add", "subfolder"]);
1483+
run(&["commit", "-qm", "add directory"]);
1484+
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30);
1485+
1486+
// "subfolder" resolves to a tree object in git, not a blob.
1487+
let read =
1488+
super::read_file_bounded("git", work, "main", "subfolder", 1024, deadline).unwrap();
1489+
assert_eq!(read, super::BoundedFileRead::Missing);
1490+
}
1491+
13051492
#[test]
13061493
fn resolve_head_bounded_covers_all_fallback_arms() {
13071494
let td = tempfile::TempDir::new().unwrap();

0 commit comments

Comments
 (0)