Skip to content

fix(node): fail pin_git_object when the add response carries no Hash #453

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

Open
beardthelion wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(node): fail pin_git_object when the add response carries no Hash #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63e6e3f
fix(node): fail pin_git_object when the add response carries no Hash
beardthelion Sep 14, 2026
8dc6c26
fix(node): reject a non-CID Hash in the add response
beardthelion Sep 14, 2026
dc3ed80
test(node): return real CIDs from the add-response test mocks
beardthelion Sep 14, 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
2 changes: 1 addition & 1 deletion crates/gitlawb-node/src/api/repos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6984,7 +6984,7 @@ mod tests {
let add = server
.mock("POST", mockito::Matcher::Any)
.with_status(200)
.with_body(r#"{"Hash":"QmShouldNotHappen"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down
86 changes: 70 additions & 16 deletions crates/gitlawb-node/src/ipfs_pin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1485,9 +1485,6 @@ pub async fn pin_git_object(
return Ok(String::new());
}

// Compute the expected CIDv1 from the content bytes
let expected_cid = Cid::from_git_object_bytes(data).to_string();

let url = format!(
"{}/api/v0/add?cid-version=1&raw-leaves=true&pin=true",
ipfs_api.trim_end_matches('/')
Expand Down Expand Up @@ -1534,6 +1531,10 @@ pub async fn pin_git_object(
.text()
.await
.map_err(|e| anyhow::anyhow!("IPFS add response body read failed: {e}"))?;
// A 200 without a parseable Hash is not a successful pin. Fabricating the
// caller-computed CID would record an address the backend never confirmed;
// above its chunk threshold that raw CID is not even a stored block, so
// every later cat fails permanently.
let cid = body
.lines()
.filter(|l| !l.trim().is_empty())
Expand All @@ -1542,7 +1543,17 @@ pub async fn pin_git_object(
v["Hash"].as_str().map(|s| s.to_string())
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})
Comment thread
beardthelion marked this conversation as resolved.
.next_back()
.unwrap_or(expected_cid.clone());
.ok_or_else(|| {
anyhow::anyhow!(
"IPFS /api/v0/add returned 200 without a usable Hash: {:.200}",
body
)
})?;
// The string still has to be a CID. A garbage Hash would be recorded and
// later cat-ed the same way a missing one was, so reject it here.
Cid::from_str(&cid).map_err(|e| {
anyhow::anyhow!("IPFS /api/v0/add returned a Hash that is not a CID ({cid}): {e}")
})?;

tracing::debug!(sha256 = %sha256_hex, %cid, "pinned git object to IPFS");
Ok(cid)
Expand Down Expand Up @@ -2200,16 +2211,16 @@ mod tests {
endpoint
}

/// A sleeping-but-live endpoint. Answers `200` with an empty body after
/// `delays[i]` for the i-th request it accepts (the last entry repeats), so
/// a test can make one add slow and the next fast. Drains the full request,
/// headers plus the declared `Content-Length` body, before sleeping: exactly
/// as in `rejecting_endpoint`, answering early and closing would surface as
/// a write failure on the client and turn a slow-but-healthy add into a
/// different failure shape.
/// A sleeping-but-live endpoint. Answers `200` with a conformant NDJSON add
/// response after `delays[i]` for the i-th request it accepts (the last
/// entry repeats), so a test can make one add slow and the next fast.
/// Drains the full request, headers plus the declared `Content-Length`
/// body, before sleeping: exactly as in `rejecting_endpoint`, answering
/// early and closing would surface as a write failure on the client and
/// turn a slow-but-healthy add into a different failure shape.
///
/// An empty body is a successful pin: `pin_git_object` falls back to the CID
/// it computed from the bytes when the response carries no `Hash`.
/// The Hash is a fixed, real CID: the timing tests assert on pin counts,
/// not on the recorded address.
async fn delaying_endpoint(delays: Vec<Duration>) -> String {
use tokio::io::{AsyncReadExt, AsyncWriteExt};
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
Expand Down Expand Up @@ -2246,9 +2257,14 @@ mod tests {
}
}
tokio::time::sleep(delay).await;
let _ = sock
.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n")
.await;
let body = br#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4","Size":"12"}
"#;
let resp = format!(
"HTTP/1.1 200 OK\r\nContent-Type: application/x-ndjson\r\nContent-Length: {}\r\n\r\n",
body.len()
);
let _ = sock.write_all(resp.as_bytes()).await;
let _ = sock.write_all(body).await;
let _ = sock.flush().await;
});
}
Expand Down Expand Up @@ -2350,6 +2366,44 @@ mod tests {
);
}

/// A 200 add response with no parseable Hash must surface as an add
/// failure. Falling back to the caller-computed CID records an
/// unverifiable address: above the backend's chunk threshold that raw
/// CID is not a stored block, so every later cat 500s.
#[tokio::test]
async fn pin_git_object_rejects_a_200_without_a_hash() {
let mut server = mockito::Server::new_async().await;
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Name":"object","Size":"19"}"#)
.create_async()
.await;
let result = pin_git_object(&server.url(), "deadbeef", b"some object bytes\n", None).await;
assert!(
result.is_err(),
"a 200 without a Hash must be an add failure, not a fabricated CID: {result:?}"
);
}

/// A Hash that is a string but not a CID must also fail. It would be
/// recorded into `encrypted_blobs.cid` and every later cat would miss.
#[tokio::test]
async fn pin_git_object_rejects_a_malformed_hash() {
let mut server = mockito::Server::new_async().await;
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"not-a-cid","Size":"19"}"#)
.create_async()
.await;
let result = pin_git_object(&server.url(), "deadbeef", b"some object bytes\n", None).await;
assert!(
result.is_err(),
"a Hash that is not a CID must be an add failure: {result:?}"
);
}

/// The permit-hold bound. `pin_new_objects` runs under a deferring
/// `pin_semaphore`, so without a batch deadline the hold is O(N) with N
/// chosen by the pusher. Five objects against an endpoint that takes 2s
Expand Down
62 changes: 42 additions & 20 deletions crates/gitlawb-node/src/test_support.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3754,7 +3754,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyshouldnothappen"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down Expand Up @@ -3880,7 +3880,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyshouldnothappen"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down Expand Up @@ -4070,7 +4070,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyshouldnothappen"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down Expand Up @@ -5890,7 +5890,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovtest"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -5950,7 +5950,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyproviderhash"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(2)
.create_async()
.await;
Expand Down Expand Up @@ -6209,7 +6209,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyshouldnothappen"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down Expand Up @@ -6332,7 +6332,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyshouldnothappen"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down Expand Up @@ -6434,7 +6434,7 @@ mod tests {
let m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"x"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down Expand Up @@ -6498,7 +6498,7 @@ mod tests {
server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"x"}"#)
.with_body(r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#)
.expect(0)
.create_async()
.await;
Expand Down Expand Up @@ -15689,7 +15689,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -15757,7 +15759,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -15825,7 +15829,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -15895,7 +15901,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -16018,7 +16026,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -16080,7 +16090,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -16217,7 +16229,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -16296,7 +16310,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -16357,7 +16373,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -16456,7 +16474,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down Expand Up @@ -16542,7 +16562,9 @@ mod tests {
let _m = server
.mock("POST", mockito::Matcher::Regex(r"^/api/v0/add".to_string()))
.with_status(200)
.with_body(r#"{"Hash":"bafyprovider"}"#)
.with_body(
r#"{"Hash":"bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4"}"#,
)
.expect_at_least(1)
.create_async()
.await;
Expand Down
Loading