mirror of
https://relay.ngit.dev/npub15qydau2hjma6ngxkl2cyar74wzyjshvl65za5k5rl69264ar2exs5cyejr/ngit-grasp.git
synced 2026-10-05 15:08:24 +00:00
Production traffic containing unparseable client messages tore down the entire WebSocket connection. A single bad message - most visibly requests carrying invalid event IDs, failing deserialization with `Invalid input length 64` - propagated out of the relay's read loop and closed the session, so every affected client had to reconnect and re-subscribe. One malformed frame cost a client all of its live subscriptions, and clients that retried the same payload produced sustained connection churn. The defect was upstream in rust-nostr's local relay, not in ngit-grasp, and was reported as nostr:nevent1qqsqmmfv6fxk995yr5858eev7z6zmchchgmk90d4rffuuwe6ewcvx0cpz3mhxue69uhhyetvv9ujumn8d96zuer9wckedd82 and fixed by nostr:nevent1qqs24l0rv6qw3mdqdaj8nj4wlds2gy8a9wrqs3gj5kcmz27c8gm7kagpz3mhxue69uhhyetvv9ujumn8d96zuer9wc7gp6cp Deserialization failures are now contained inside the WebSocket loop and answered with a `NOTICE`, leaving the session and its subscriptions intact. Upgrade the NostrDevKit crates from 0.45.0-alpha.3 to 0.45.0-alpha.8, the first published release containing the fix. Inclusion was verified three ways rather than by release notes: upstream commit 7c0f3aa2cf2fbeb9f0067cb2349579754919eabd is an ancestor of the `Bump to v0.45.0-alpha.8` commit on upstream master; the alpha.8 crate downloaded from crates.io contains the repaired match arm in `src/local_relay/local/inner.rs`; and it also ships the upstream regression test `test_malformed_client_message_does_not_close_connection`, which drives a real WebSocket with a short-author REQ and asserts a subsequent valid REQ still reaches EOSE on the same socket. That upstream test covers the behaviour directly, so no equivalent test is duplicated here. All four rust-nostr crates move together to keep the tree off a mixture of alpha versions. `nostr-relay-builder` is gone: upstream merged it into `nostr-sdk` at alpha.4, so its imports become `nostr_sdk::local_relay` and `nostr_sdk::prelude`, and `nostr-sdk` now enables the `local-relay` feature. Three further alpha-to-alpha API removals are absorbed: `nostr::hashes` is no longer re-exported, so the `Sec-WebSocket-Accept` derivation depends on `bitcoin_hashes` directly - the same crate `nostr` still uses internally, so no second hash implementation enters the tree; `BoxedFuture` became crate-private, so `WritePolicy::admit_event` spells out its `Pin<Box<dyn Future<...>>>` return type; and `EventBuilder::text_note` was removed in favour of `EventBuilder::new(Kind::TextNote, ..)`, which affects test code only. These requirements are pinned exactly as `=0.45.0-alpha.n` rather than left as caret requirements. Cargo reads `"0.45.0-alpha.3"` as `^0.45.0-alpha.3`, which admits *any* later prerelease of 0.45.0 even though prereleases promise no compatibility - exactly the surprise reported against ngit in nostr:nevent1qqs0yvj4z302cjsnqx9jpp78jrfujhse0w47adjncwkxr922rjltk8spz3mhxue69uhhyetvv9ujumn8d96zuer9wcr42j8n where a declared alpha.2 resolved to alpha.7. This upgrade is direct evidence that the risk is real: alpha.4 deleted a whole crate this project depended on, which a caret requirement would have accepted silently. Pinning is safe because it only narrows resolution, and the committed `Cargo.lock` already selects these versions; what it adds is protection for builds that do not honour the lockfile - `cargo install`, and downstream consumers of the `ngit_grasp` library. The pins should be relaxed to caret requirements once 0.45.0 is released. Validated in the project Nix development shell with `CARGO_BUILD_JOBS=2`: `cargo fmt --all --check` clean, `cargo clippy --workspace --all-targets -D warnings` clean, and `cargo test --workspace --no-fail-fast` at 1937 passed / 32 failed. The 32 failures were confirmed pre-existing by running the identical suite on unmodified master in this environment: the same 1937/32 split and a byte-identical set of failing test names, so this upgrade introduces no regressions. All dependencies remain crates.io sources, so no `flake.nix` or `nix/module.nix` hashes required updating.
501 lines
16 KiB
Rust
501 lines
16 KiB
Rust
//! Integration coverage for Git Smart HTTP response streaming.
|
|
|
|
use std::collections::HashSet;
|
|
use std::path::Path;
|
|
use std::sync::Arc;
|
|
use std::time::Duration;
|
|
|
|
use async_trait::async_trait;
|
|
use clap::Parser;
|
|
use http_body_util::BodyExt;
|
|
use hyper::body::{Bytes, Frame};
|
|
use ngit_grasp::config::Config;
|
|
use ngit_grasp::git::handlers::handle_receive_pack;
|
|
use ngit_grasp::git::sync::PurgatoryPromotionHooks;
|
|
use ngit_grasp::grasp06::endpoint::PrsUrl;
|
|
use ngit_grasp::grasp06::paths::prs_repo_path;
|
|
use ngit_grasp::grasp06::receive::{handle_prs_receive_pack, new_repo_init_locks};
|
|
use ngit_grasp::nostr::builder::Nip34WritePolicy;
|
|
use ngit_grasp::nostr::lifecycle::{
|
|
HoldingStore, ReplaceableHistoryStore, RepositoryLifecycle, Tombstones,
|
|
};
|
|
use ngit_grasp::nostr::SharedDatabase;
|
|
use ngit_grasp::purgatory::Purgatory;
|
|
use ngit_grasp::sync::rejected_index::RejectedEventsIndex;
|
|
use nostr_sdk::prelude::LocalRelayBuilder;
|
|
use nostr_sdk::prelude::*;
|
|
use tokio::sync::Semaphore;
|
|
use tokio::time::timeout;
|
|
|
|
static PATH_ENV_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());
|
|
|
|
struct PathOverride {
|
|
original: Option<std::ffi::OsString>,
|
|
}
|
|
|
|
impl PathOverride {
|
|
fn prepend(dir: &Path) -> Self {
|
|
let original = std::env::var_os("PATH");
|
|
let mut paths = vec![dir.to_path_buf()];
|
|
if let Some(existing) = original.as_ref() {
|
|
paths.extend(std::env::split_paths(existing));
|
|
}
|
|
let joined = std::env::join_paths(paths).expect("join PATH entries");
|
|
std::env::set_var("PATH", joined);
|
|
Self { original }
|
|
}
|
|
}
|
|
|
|
impl Drop for PathOverride {
|
|
fn drop(&mut self) {
|
|
if let Some(original) = self.original.take() {
|
|
std::env::set_var("PATH", original);
|
|
} else {
|
|
std::env::remove_var("PATH");
|
|
}
|
|
}
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn receive_pack_response_streams_stdout_before_subprocess_exit() {
|
|
let _env_lock = PATH_ENV_LOCK.lock().await;
|
|
|
|
let fake_bin = tempfile::tempdir().expect("fake git bin tempdir");
|
|
write_fake_git(fake_bin.path());
|
|
let _path = PathOverride::prepend(fake_bin.path());
|
|
|
|
let repo = tempfile::tempdir().expect("repo tempdir");
|
|
let git_data = tempfile::tempdir().expect("git data tempdir");
|
|
let database: SharedDatabase = Arc::new(nostr_memory::MemoryDatabase::unbounded());
|
|
let relay = LocalRelayBuilder::default().build();
|
|
let purgatory = Arc::new(Purgatory::new(git_data.path().to_path_buf()));
|
|
let owner_pubkey = "0".repeat(64);
|
|
let request_body = receive_pack_request_body();
|
|
|
|
let response = handle_receive_pack(
|
|
repo.path().to_path_buf(),
|
|
request_body,
|
|
database,
|
|
relay,
|
|
"streaming-test",
|
|
&owner_pubkey,
|
|
purgatory,
|
|
git_data.path().to_str().expect("utf-8 temp path"),
|
|
None,
|
|
None,
|
|
None,
|
|
None,
|
|
)
|
|
.await
|
|
.expect("receive-pack handler should start fake subprocess");
|
|
|
|
let mut body = response.into_body();
|
|
|
|
let first = timeout(Duration::from_secs(1), body.frame())
|
|
.await
|
|
.expect("first stdout chunk should arrive before fake git exits")
|
|
.expect("body should still be open")
|
|
.expect("first frame should not be an HTTP body error");
|
|
let first = frame_data(first);
|
|
assert!(
|
|
!first.is_empty(),
|
|
"first progress frame should contain data"
|
|
);
|
|
|
|
let no_second_yet = timeout(Duration::from_millis(250), body.frame()).await;
|
|
assert!(
|
|
no_second_yet.is_err(),
|
|
"body produced another frame while fake git was still sleeping; \
|
|
this test needs the first frame to be observed before subprocess EOF"
|
|
);
|
|
|
|
let mut streamed = first.to_vec();
|
|
loop {
|
|
let frame = timeout(Duration::from_secs(3), body.frame())
|
|
.await
|
|
.expect("remaining stdout should arrive after fake git wakes");
|
|
let Some(frame) = frame else {
|
|
break;
|
|
};
|
|
streamed.extend_from_slice(
|
|
&frame
|
|
.expect("remaining frame should not be an HTTP body error")
|
|
.into_data()
|
|
.expect("remaining frame should contain data"),
|
|
);
|
|
}
|
|
assert_eq!(streamed, b"first-progress\nsecond-progress\n");
|
|
}
|
|
|
|
#[derive(Clone)]
|
|
struct BlockingAnnouncementPromotion {
|
|
entered: Arc<Semaphore>,
|
|
release: Arc<Semaphore>,
|
|
}
|
|
|
|
#[async_trait]
|
|
impl PurgatoryPromotionHooks for BlockingAnnouncementPromotion {
|
|
async fn before_announcement_promote(&self, _event: &Event, _identifier: &str) {
|
|
self.entered.add_permits(1);
|
|
self.release
|
|
.acquire()
|
|
.await
|
|
.expect("promotion release semaphore should remain open")
|
|
.forget();
|
|
}
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn receive_pack_terminal_flush_waits_for_purgatory_promotion() {
|
|
let _env_lock = PATH_ENV_LOCK.lock().await;
|
|
|
|
let fake_bin = tempfile::tempdir().expect("fake git bin tempdir");
|
|
write_fake_git_with_terminal_flush(fake_bin.path());
|
|
let _path = PathOverride::prepend(fake_bin.path());
|
|
|
|
let keys = Keys::generate();
|
|
let owner_npub = keys.public_key().to_bech32().expect("encode owner npub");
|
|
let identifier = "push-readiness";
|
|
let git_data = tempfile::tempdir().expect("git data tempdir");
|
|
let repo_path = git_data
|
|
.path()
|
|
.join(&owner_npub)
|
|
.join(format!("{identifier}.git"));
|
|
std::fs::create_dir_all(&repo_path).expect("create fake bare repo path");
|
|
|
|
let database: SharedDatabase = Arc::new(nostr_memory::MemoryDatabase::unbounded());
|
|
let relay = LocalRelayBuilder::default().build();
|
|
let purgatory = Arc::new(Purgatory::new(git_data.path().to_path_buf()));
|
|
let announcement = EventBuilder::new(Kind::GitRepoAnnouncement, "")
|
|
.tags(vec![Tag::identifier(identifier)])
|
|
.finalize(&keys)
|
|
.expect("build announcement");
|
|
purgatory.add_announcement(
|
|
announcement.clone(),
|
|
identifier.to_string(),
|
|
keys.public_key(),
|
|
repo_path.clone(),
|
|
HashSet::new(),
|
|
);
|
|
|
|
let entered = Arc::new(Semaphore::new(0));
|
|
let release = Arc::new(Semaphore::new(0));
|
|
let hooks = BlockingAnnouncementPromotion {
|
|
entered: entered.clone(),
|
|
release: release.clone(),
|
|
};
|
|
|
|
let response = handle_receive_pack(
|
|
repo_path,
|
|
receive_pack_request_body(),
|
|
database.clone(),
|
|
relay,
|
|
identifier,
|
|
&keys.public_key().to_hex(),
|
|
purgatory,
|
|
git_data.path().to_str().expect("utf-8 temp path"),
|
|
None,
|
|
None,
|
|
Some(Arc::new(hooks)),
|
|
None,
|
|
)
|
|
.await
|
|
.expect("receive-pack handler should start fake subprocess");
|
|
|
|
let mut body = response.into_body();
|
|
let first = timeout(Duration::from_secs(1), body.frame())
|
|
.await
|
|
.expect("receive-pack progress should stream before promotion")
|
|
.expect("body should contain progress")
|
|
.expect("progress frame should not be an HTTP body error");
|
|
let mut streamed = frame_data(first).to_vec();
|
|
|
|
timeout(Duration::from_secs(3), entered.acquire())
|
|
.await
|
|
.expect("post-push promotion hook should run")
|
|
.expect("promotion semaphore should remain open")
|
|
.forget();
|
|
|
|
let before_save = database
|
|
.query(Filter::new().id(announcement.id))
|
|
.await
|
|
.expect("query announcement before promotion");
|
|
assert!(
|
|
before_save.is_empty(),
|
|
"announcement must not be queryable while promotion is blocked"
|
|
);
|
|
|
|
while let Ok(Some(frame)) = timeout(Duration::from_millis(25), body.frame()).await {
|
|
streamed.extend_from_slice(
|
|
&frame
|
|
.expect("progress frame should not be an HTTP body error")
|
|
.into_data()
|
|
.expect("progress frame should contain data"),
|
|
);
|
|
}
|
|
assert!(
|
|
!streamed.ends_with(b"0000"),
|
|
"receive-pack terminal flush must remain hidden while promotion is blocked"
|
|
);
|
|
|
|
let keepalive = timeout(Duration::from_secs(6), body.frame())
|
|
.await
|
|
.expect("sideband keepalive should arrive during blocked promotion")
|
|
.expect("body should remain open while promotion is blocked")
|
|
.expect("keepalive should not be an HTTP body error");
|
|
streamed.extend_from_slice(
|
|
&keepalive
|
|
.into_data()
|
|
.expect("keepalive frame should contain data"),
|
|
);
|
|
assert!(
|
|
streamed
|
|
.windows(b"GRASP is finalizing the push\n".len())
|
|
.any(|window| window == b"GRASP is finalizing the push\n"),
|
|
"blocked post-push processing should emit sideband progress"
|
|
);
|
|
assert!(
|
|
!streamed.ends_with(b"0000"),
|
|
"keepalive must not expose the terminal flush"
|
|
);
|
|
|
|
release.add_permits(1);
|
|
|
|
loop {
|
|
let frame = timeout(Duration::from_secs(1), body.frame())
|
|
.await
|
|
.expect("response should finish after promotion is released");
|
|
let Some(frame) = frame else {
|
|
break;
|
|
};
|
|
streamed.extend_from_slice(
|
|
&frame
|
|
.expect("terminal frame should not be an HTTP body error")
|
|
.into_data()
|
|
.expect("terminal frame should contain data"),
|
|
);
|
|
}
|
|
|
|
assert!(
|
|
streamed.starts_with(b"first-progress\nsecond-progress\n"),
|
|
"git progress should remain at the start of the response"
|
|
);
|
|
assert!(
|
|
streamed.ends_with(b"0000"),
|
|
"terminal flush should remain the final client-visible bytes"
|
|
);
|
|
let after_save = database
|
|
.query(Filter::new().id(announcement.id))
|
|
.await
|
|
.expect("query announcement after promotion");
|
|
assert_eq!(
|
|
after_save.len(),
|
|
1,
|
|
"announcement must be queryable before push success is exposed"
|
|
);
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn prs_receive_pack_streams_stdout_before_cleanup_removes_empty_repo() {
|
|
let _env_lock = PATH_ENV_LOCK.lock().await;
|
|
|
|
let fake_bin = tempfile::tempdir().expect("fake git bin tempdir");
|
|
write_fake_git(fake_bin.path());
|
|
let _path = PathOverride::prepend(fake_bin.path());
|
|
|
|
let keys = Keys::generate();
|
|
let git_data = tempfile::tempdir().expect("git data tempdir");
|
|
let database: SharedDatabase = Arc::new(nostr_memory::MemoryDatabase::unbounded());
|
|
let relay = LocalRelayBuilder::default().build();
|
|
let purgatory = Arc::new(Purgatory::new(git_data.path().to_path_buf()));
|
|
let repo_init_locks = new_repo_init_locks();
|
|
let write_policy = Arc::new(test_write_policy(
|
|
database.clone(),
|
|
purgatory.clone(),
|
|
repo_init_locks.clone(),
|
|
git_data.path(),
|
|
));
|
|
let rejected_events_index = Arc::new(RejectedEventsIndex::new(
|
|
Duration::from_secs(120),
|
|
Duration::from_secs(7 * 24 * 60 * 60),
|
|
));
|
|
let prs = PrsUrl {
|
|
submitter: keys.public_key(),
|
|
identifier: "prs-streaming-cleanup".to_string(),
|
|
subpath: "git-receive-pack".to_string(),
|
|
};
|
|
let repo_path = prs_repo_path(git_data.path(), &prs.submitter.to_hex(), &prs.identifier);
|
|
|
|
let response = handle_prs_receive_pack(
|
|
&prs,
|
|
receive_pack_request_body(),
|
|
database,
|
|
relay,
|
|
purgatory,
|
|
write_policy,
|
|
rejected_events_index,
|
|
git_data.path().to_str().expect("utf-8 temp path"),
|
|
None,
|
|
repo_init_locks,
|
|
"streaming-test.example",
|
|
None,
|
|
)
|
|
.await
|
|
.expect("/prs/ receive-pack handler should start fake subprocess");
|
|
|
|
assert!(
|
|
repo_path.exists(),
|
|
"/prs/ repo should exist while fake receive-pack is in flight"
|
|
);
|
|
|
|
let mut body = response.into_body();
|
|
|
|
let first = timeout(Duration::from_secs(1), body.frame())
|
|
.await
|
|
.expect("first /prs/ stdout chunk should arrive before fake git exits")
|
|
.expect("/prs/ body should still be open")
|
|
.expect("first /prs/ frame should not be an HTTP body error");
|
|
assert_eq!(frame_data(first), Bytes::from_static(b"first-progress\n"));
|
|
|
|
assert!(
|
|
repo_path.exists(),
|
|
"/prs/ cleanup must not remove the repo before receive-pack exits"
|
|
);
|
|
|
|
let no_second_yet = timeout(Duration::from_millis(250), body.frame()).await;
|
|
assert!(
|
|
no_second_yet.is_err(),
|
|
"/prs/ body produced another frame while fake git was still sleeping; \
|
|
this test needs the first frame to be observed before subprocess EOF"
|
|
);
|
|
|
|
let second = timeout(Duration::from_secs(3), body.frame())
|
|
.await
|
|
.expect("second /prs/ stdout chunk should arrive after fake git wakes")
|
|
.expect("/prs/ body should still be open for second chunk")
|
|
.expect("second /prs/ frame should not be an HTTP body error");
|
|
assert_eq!(frame_data(second), Bytes::from_static(b"second-progress\n"));
|
|
|
|
let eof = timeout(Duration::from_secs(1), body.frame())
|
|
.await
|
|
.expect("/prs/ body should close after cleanup");
|
|
assert!(
|
|
eof.is_none(),
|
|
"/prs/ body should be closed after fake git exits"
|
|
);
|
|
|
|
assert!(
|
|
!repo_path.exists(),
|
|
"/prs/ cleanup should remove zero-ref repo after receive-pack exits"
|
|
);
|
|
}
|
|
|
|
fn frame_data(frame: Frame<Bytes>) -> Bytes {
|
|
frame.into_data().expect("frame should contain data")
|
|
}
|
|
|
|
fn receive_pack_request_body() -> Bytes {
|
|
let old_oid = "0".repeat(40);
|
|
let new_oid = "1".repeat(40);
|
|
let event_id = "a".repeat(64);
|
|
let mut payload = Vec::new();
|
|
payload.extend_from_slice(format!("{old_oid} {new_oid} refs/nostr/{event_id}").as_bytes());
|
|
payload.push(0);
|
|
payload.extend_from_slice(b"report-status side-band-64k\n");
|
|
|
|
let mut request = Vec::new();
|
|
request.extend_from_slice(format!("{:04x}", payload.len() + 4).as_bytes());
|
|
request.extend_from_slice(&payload);
|
|
request.extend_from_slice(b"0000");
|
|
Bytes::from(request)
|
|
}
|
|
|
|
fn test_write_policy(
|
|
database: SharedDatabase,
|
|
purgatory: Arc<Purgatory>,
|
|
repo_init_locks: ngit_grasp::grasp06::receive::RepoInitLocks,
|
|
git_data_path: &Path,
|
|
) -> Nip34WritePolicy {
|
|
let config = Config::parse_from([
|
|
"ngit-grasp-test",
|
|
"--domain",
|
|
"streaming-test.example",
|
|
"--grasp06-enable",
|
|
]);
|
|
|
|
Nip34WritePolicy::new(
|
|
database,
|
|
Tombstones::in_memory(),
|
|
HoldingStore::in_memory(),
|
|
RepositoryLifecycle::in_memory(),
|
|
ReplaceableHistoryStore::in_memory(),
|
|
git_data_path.to_path_buf(),
|
|
purgatory,
|
|
config,
|
|
repo_init_locks,
|
|
)
|
|
}
|
|
|
|
fn write_fake_git(bin_dir: &Path) {
|
|
write_fake_git_script(bin_dir, false);
|
|
}
|
|
|
|
fn write_fake_git_with_terminal_flush(bin_dir: &Path) {
|
|
write_fake_git_script(bin_dir, true);
|
|
}
|
|
|
|
fn write_fake_git_script(bin_dir: &Path, terminal_flush: bool) {
|
|
let git_path = bin_dir.join("git");
|
|
let terminal_flush = if terminal_flush {
|
|
"printf '0000'\n"
|
|
} else {
|
|
""
|
|
};
|
|
std::fs::write(
|
|
&git_path,
|
|
r#"#!/usr/bin/env bash
|
|
set -euo pipefail
|
|
|
|
is_receive_pack=0
|
|
for arg in "$@"; do
|
|
if [ "$arg" = "receive-pack" ]; then
|
|
is_receive_pack=1
|
|
fi
|
|
done
|
|
|
|
if [ "$is_receive_pack" = "1" ]; then
|
|
cat >/dev/null
|
|
printf 'first-progress\n'
|
|
sleep 2
|
|
printf 'second-progress\n'
|
|
__TERMINAL_FLUSH__exit 0
|
|
fi
|
|
|
|
if [ "$1" = "init" ]; then
|
|
repo="${@: -1}"
|
|
mkdir -p "$repo"
|
|
exit 0
|
|
fi
|
|
|
|
if [ "$1" = "for-each-ref" ]; then
|
|
exit 0
|
|
fi
|
|
|
|
echo "fake git only supports init, for-each-ref, and receive-pack" >&2
|
|
exit 1
|
|
"#
|
|
.replace("__TERMINAL_FLUSH__", terminal_flush),
|
|
)
|
|
.expect("write fake git executable");
|
|
|
|
#[cfg(unix)]
|
|
{
|
|
use std::os::unix::fs::PermissionsExt;
|
|
let mut perms = std::fs::metadata(&git_path)
|
|
.expect("fake git metadata")
|
|
.permissions();
|
|
perms.set_mode(0o755);
|
|
std::fs::set_permissions(&git_path, perms).expect("chmod fake git");
|
|
}
|
|
}
|