Skip to content

Commit 1964f88

Browse files
authored
Merge pull request #280 from Gitlawb/fix/270-reset-peer-reachability-on-url-change
fix(node): reset a peer's reachability gate when an announce changes its http_url (#270)
2 parents 9418e31 + 811ad50 commit 1964f88

1 file changed

Lines changed: 204 additions & 1 deletion

File tree

  • crates/gitlawb-node/src/db

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

Lines changed: 204 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2140,10 +2140,46 @@ impl Db {
21402140
anyhow::bail!("refusing to register non-public peer http_url: {http_url}");
21412141
}
21422142
let now = Utc::now().to_rfc3339();
2143+
// A changed URL drops the reachability gate, so a repointed peer does
2144+
// not inherit a probe the previous host earned. In the DO UPDATE branch
2145+
// `peers.http_url` is the existing pre-update row (the proposed value is
2146+
// $2), and the comparison runs under the conflict row lock, so
2147+
// concurrent announces for one DID serialize instead of racing a
2148+
// read-then-write. That orders announces against each other and nothing
2149+
// more: mark_peer_ping writes by DID with no http_url predicate, so a
2150+
// probe of the previous URL that lands after a reset can still re-grant
2151+
// the flag until the next round.
2152+
//
2153+
// Comparison is exact. http_url is stored as announced, so a cosmetic
2154+
// difference such as a trailing slash also clears the flag; the peer
2155+
// re-earns it on a later gossip round, provided no further announce
2156+
// lands first. Normalizing instead would mean canonicalizing the stored
2157+
// value, this comparison, and the existing trim_end_matches call sites.
2158+
//
2159+
// last_ping_ok is NOT a trust signal. An unauthenticated caller can
2160+
// clear it by announcing a different URL, and until #248 lands can also
2161+
// set it through the unauthenticated GET /api/v1/peers/{did}/ping, which
2162+
// writes mark_peer_ping from the stored URL's own probe response. Do not
2163+
// build a new consumer on this flag as if it were attacker-resistant.
2164+
//
2165+
// Only the federated repo fan-out in api/repos.rs gates on this flag.
2166+
// Four consumers act on a repointed http_url regardless of it (sync.rs's
2167+
// origin resolve, the post-receive notify fan-out, trigger_sync, and the
2168+
// public resolve route), and two read surfaces republish it as
2169+
// `reachable` (api/resolve.rs, api/peers.rs), which is where a reset
2170+
// becomes externally visible. So this bounds the automatic inheritance
2171+
// rather than closing the rewrite. Binding a DID to its first-seen
2172+
// announcing key is what closes it: #273.
21432173
sqlx::query(
21442174
"INSERT INTO peers (did, http_url, last_seen, last_ping_ok, announced_at)
21452175
VALUES ($1, $2, $3, FALSE, $3)
2146-
ON CONFLICT(did) DO UPDATE SET http_url = $2, last_seen = $3",
2176+
ON CONFLICT(did) DO UPDATE SET
2177+
http_url = $2,
2178+
last_seen = $3,
2179+
last_ping_ok = CASE
2180+
WHEN peers.http_url IS DISTINCT FROM $2 THEN FALSE
2181+
ELSE peers.last_ping_ok
2182+
END",
21472183
)
21482184
.bind(did)
21492185
.bind(http_url)
@@ -5912,3 +5948,170 @@ mod ref_update_db_tests {
59125948
assert_eq!(all[0].new_sha, "gggg");
59135949
}
59145950
}
5951+
5952+
#[cfg(test)]
5953+
mod peer_reachability_tests {
5954+
use super::Db;
5955+
use sqlx::PgPool;
5956+
5957+
const VICTIM_DID: &str = "did:key:z6MkvictimPeerFixture";
5958+
const HONEST_URL: &str = "https://honest-peer.example.com";
5959+
const ATTACKER_URL: &str = "https://attacker.example.com";
5960+
5961+
async fn db(pool: PgPool) -> Db {
5962+
let db = Db::for_testing(pool);
5963+
db.run_migrations().await.unwrap();
5964+
db
5965+
}
5966+
5967+
/// Read the row back through `list_peers`, the same surface the federated
5968+
/// fan-out filters on, rather than issuing raw SQL from the test.
5969+
async fn peer(db: &Db, did: &str) -> (String, bool) {
5970+
let row = db
5971+
.list_peers()
5972+
.await
5973+
.unwrap()
5974+
.into_iter()
5975+
.find(|p| p.did == did)
5976+
.expect("seeded peer row is missing");
5977+
(row.http_url, row.last_ping_ok)
5978+
}
5979+
5980+
/// Parsed rather than string-compared, so the ordering assertion does not
5981+
/// depend on the stored timestamp's textual precision.
5982+
async fn last_seen(db: &Db, did: &str) -> chrono::DateTime<chrono::Utc> {
5983+
let row = db
5984+
.list_peers()
5985+
.await
5986+
.unwrap()
5987+
.into_iter()
5988+
.find(|p| p.did == did)
5989+
.expect("seeded peer row is missing");
5990+
chrono::DateTime::parse_from_rfc3339(&row.last_seen.expect("last_seen is set by upsert"))
5991+
.expect("last_seen is rfc3339")
5992+
.with_timezone(&chrono::Utc)
5993+
}
5994+
5995+
/// Seed a peer that has earned reachability, asserting the seed took so a
5996+
/// later case cannot pass vacuously on a row that was never written.
5997+
async fn seed_reachable(db: &Db) {
5998+
db.upsert_peer(VICTIM_DID, HONEST_URL).await.unwrap();
5999+
db.mark_peer_ping(VICTIM_DID, true).await.unwrap();
6000+
assert_eq!(
6001+
peer(db, VICTIM_DID).await,
6002+
(HONEST_URL.to_string(), true),
6003+
"seed did not take"
6004+
);
6005+
}
6006+
6007+
/// Repointing an existing peer's URL must drop the reachability gate: the
6008+
/// new host has not been probed, so it cannot inherit the old host's
6009+
/// earned `last_ping_ok`.
6010+
#[sqlx::test]
6011+
async fn url_change_clears_reachability(pool: PgPool) {
6012+
let db = db(pool).await;
6013+
seed_reachable(&db).await;
6014+
6015+
db.upsert_peer(VICTIM_DID, ATTACKER_URL).await.unwrap();
6016+
6017+
let (url, reachable) = peer(&db, VICTIM_DID).await;
6018+
assert_eq!(url, ATTACKER_URL, "the URL should still be rewritten");
6019+
assert!(
6020+
!reachable,
6021+
"a repointed peer must re-earn reachability, not inherit it"
6022+
);
6023+
}
6024+
6025+
/// A plain liveness re-announce carries the same URL and must not cost an
6026+
/// honest peer its place in the federated fan-out. Guards against a fix
6027+
/// that clears the flag on every conflict instead of only on a change.
6028+
#[sqlx::test]
6029+
async fn same_url_reannounce_keeps_reachability(pool: PgPool) {
6030+
let db = db(pool).await;
6031+
seed_reachable(&db).await;
6032+
6033+
db.upsert_peer(VICTIM_DID, HONEST_URL).await.unwrap();
6034+
6035+
let (url, reachable) = peer(&db, VICTIM_DID).await;
6036+
assert_eq!(url, HONEST_URL);
6037+
assert!(
6038+
reachable,
6039+
"an unchanged-URL re-announce must not drop the gate"
6040+
);
6041+
}
6042+
6043+
/// The must-not-grant direction. An unchanged URL preserves the flag as it
6044+
/// stands, which means FALSE stays FALSE: reachability is earned by a probe,
6045+
/// never by announcing. This is the only case that fails if the conditional
6046+
/// is flattened to `last_ping_ok = (peers.http_url IS NOT DISTINCT FROM $2)`,
6047+
/// which would let any unsigned same-URL re-announce set the flag TRUE.
6048+
#[sqlx::test]
6049+
async fn same_url_reannounce_does_not_grant_reachability(pool: PgPool) {
6050+
let db = db(pool).await;
6051+
db.upsert_peer(VICTIM_DID, HONEST_URL).await.unwrap();
6052+
assert_eq!(peer(&db, VICTIM_DID).await, (HONEST_URL.to_string(), false));
6053+
6054+
db.upsert_peer(VICTIM_DID, HONEST_URL).await.unwrap();
6055+
6056+
let (_, reachable) = peer(&db, VICTIM_DID).await;
6057+
assert!(
6058+
!reachable,
6059+
"announcing must never grant reachability without a probe"
6060+
);
6061+
}
6062+
6063+
/// A first insert stays out of the fan-out until a probe confirms it. Guards
6064+
/// against the conditional leaking into the INSERT branch.
6065+
#[sqlx::test]
6066+
async fn fresh_peer_inserts_unreachable(pool: PgPool) {
6067+
let db = db(pool).await;
6068+
6069+
db.upsert_peer("did:key:z6MkfreshPeerFixture", HONEST_URL)
6070+
.await
6071+
.unwrap();
6072+
6073+
let (_, reachable) = peer(&db, "did:key:z6MkfreshPeerFixture").await;
6074+
assert!(!reachable, "a never-probed peer must insert unreachable");
6075+
}
6076+
6077+
/// Comparison is exact, by decision: http_url is stored as announced and
6078+
/// nothing normalizes it, so a trailing slash is a different remote as far
6079+
/// as this row is concerned and clears the gate. Pins that decision against
6080+
/// a future normalizing comparison, which every other case here would pass
6081+
/// because they only ever compare identical or wholly different hosts.
6082+
#[sqlx::test]
6083+
async fn cosmetic_url_difference_counts_as_a_change(pool: PgPool) {
6084+
let db = db(pool).await;
6085+
seed_reachable(&db).await;
6086+
6087+
let with_slash = format!("{HONEST_URL}/");
6088+
db.upsert_peer(VICTIM_DID, &with_slash).await.unwrap();
6089+
6090+
let (url, reachable) = peer(&db, VICTIM_DID).await;
6091+
assert_eq!(url, with_slash);
6092+
assert!(
6093+
!reachable,
6094+
"comparison is exact, so a cosmetic difference clears the gate too"
6095+
);
6096+
}
6097+
6098+
/// The reset must ride the existing UPDATE, not gate it. Hoisting the
6099+
/// condition to a statement-level WHERE would leave every case above green
6100+
/// while silently skipping the whole update on a same-URL re-announce, so
6101+
/// liveness would stop advancing and the peer would age out on last_seen.
6102+
#[sqlx::test]
6103+
async fn same_url_reannounce_still_advances_last_seen(pool: PgPool) {
6104+
let db = db(pool).await;
6105+
db.upsert_peer(VICTIM_DID, HONEST_URL).await.unwrap();
6106+
let first = last_seen(&db, VICTIM_DID).await;
6107+
6108+
db.upsert_peer(VICTIM_DID, HONEST_URL).await.unwrap();
6109+
6110+
let second = last_seen(&db, VICTIM_DID).await;
6111+
assert!(
6112+
second > first,
6113+
"a same-URL re-announce is a liveness signal and must still \
6114+
advance last_seen: {first} then {second}"
6115+
);
6116+
}
6117+
}

0 commit comments

Comments
 (0)