Compare commits

...
Author SHA1 Message Date
beardthelion c2afed7b63 Merge remote-tracking branch 'origin/main' into recreate-410-codex/fix-identify-address-bounds 2026-11-07 15:34:35 -06:00
beardthelionandGravirei bac49f150a fix(node): owner-gate create_task repo_id against hosted repos (#541)
* fix(node): owner-gate create_task repo_id against hosted repos (#496)

* fix(node): address review on #496 gate — opaque lookup errors, no-persist asserts, GraphQL arm pins, honest oracle comment

* fix(node): address P3 review on #496 gate — shared denial text, honest oracle wording, db-fault pins

- Give the REST and GraphQL copies of the create_task owner gate one denial
  sentence: add REPO_OWNER_ONLY_MESSAGE, used by require_repo_owner and the
  REST forbidden() envelope, and pin it from both surface tests.
- Carry the corrected one-bit-oracle wording into the GraphQL gate comment and
  the REST test docstring (drop the "oracle-free" claim).
- Fault-inject the gate's repo lookups: drop machine_id (get_repo_by_id) for
  REST and GraphQL, and quarantined (is_repo_quarantined) for REST, asserting
  the fixed opaque db_error envelope and no schema text.

* docs(node): stop claiming caller-scoped task reads in #496 gate comments

The create_task gate is a write gate. Unknown/quarantined repo ids are stored
verbatim, and the REST/GraphQL task-list paths are caller-independent on this
base, so the comments must not imply those ids are read-scoped to the
delegator/assignee. Describe only the enforced ownership check; note the
one-bit oracle split without asserting a read contract.

* fix(node): mirror AppError's 503 split and opaque INSERT in create_task

- db_error now sends pool-class failures to 503 db_unavailable like
  AppError::Db (db_unavailable made pub(crate)); query faults stay the opaque
  500. Unit tests cover the split and context-wrapped sqlx errors.
- Route the create_task INSERT through db_error instead of serializing
  e.to_string(); a REST fault-injection test pins the opaque envelope.
- The deny/allow matrix reads back the unknown and quarantined tasks to prove
  the id is stored verbatim, and drops no-leak asserts on the fixed forbidden
  envelope that could never carry the repo id.

* test(node): pin stored-verbatim repo_id on GraphQL gate arms

The REST deny/allow matrix reads each accepted task back and asserts the
stored repo_id is the supplied id; the GraphQL unknown/quarantined arms only
asserted no errors, so a GraphQL regression that dropped, normalized, or
rebound the id stayed green. Read those tasks back by the mutation's returned
id and assert the stored label matches.

* test(node): pin createTask repo-ownership marker in the GraphQL authz fence

---------

Co-authored-by: Gravirei <[email protected]>
2026-11-07 21:31:50 +00:00
beardthelionandGravirei b4a3122500 fix(core): reject zero threshold, canonicalize did:key, document replay (#509) (#540)
* fix(core): reject zero threshold, canonicalize did:key, document replay (#509)

* test(core): drop error echoes from assert messages flagged by CodeQL

---------

Co-authored-by: Gravirei <[email protected]>
2026-11-07 15:48:05 +00:00
cairn-intern 4d766bd23c fix(node): pin refresh predicate negative direction per beardthelion round 3 2026-11-07 04:20:01 +00:00
cairn-intern 924680bf98 fix(node): pin row-removal and expiry guards in Identify address book
Address beardthelion's round-two P3 findings on #462:

- Add test that a fully-rejected fresh peer at full global budget
  leaves no peers row (pins the empty-row drop in update()).
- Add test that update() past IDENTIFY_ADDRESS_TTL reports the stale
  address in changes.removed (pins expiry reporting).
- Add test that expire() drops rows emptied by expiry (pins the
  sibling drop in expire_peer).
- Distinguish per-peer from cross-peer eviction in the saturation
  test: insert a sentinel peer before the victim and assert its
  address survives, so a global-oldest eviction policy would fail.
2026-11-07 04:20:00 +00:00
cairn-intern bc3e9d12c1 fix(node): address #462 review findings from beardthelion
- remove the write-only insertion_order bookkeeping (field,
  remove_insertion_token, and its call sites) and collapse the repeated
  peers.get/get_mut lookups in IdentifyAddressBook::update into a single
  entry binding
- drop a peer row at the end of update() when every candidate was
  rejected, so zero-address rows no longer linger until the expiry tick
- add a regression test proving per-peer eviction still applies at full
  global budget (fails if the global check is hoisted above eviction)
- extract the explicit-address retention predicate into the tested
  is_explicit_address helper used by both Kademlia removal sites
2026-11-07 04:20:00 +00:00
Marlowe 39dc5233a8 fix(node): address CodeRabbit majors on Identify address bounds
- Reject Identify address admissions once the global address budget
  is full instead of evicting another peer's entry. Cross-peer
  eviction let a flood of new identities push honest peers'
  addresses out of the book and, when the evicted address was their
  last one, out of the Kademlia routing table. Per-peer eviction of
  the reporting peer's own addresses is unchanged.
- Re-add refreshed Identify addresses to Kademlia, not just newly
  added ones. Kademlia may have dropped a retained address after a
  failed dial or never kept it for a full k-bucket, and a TTL
  refresh alone would never re-admit it.
2026-11-07 04:20:00 +00:00
euxarist aa9f94d6ae fix(node): bound Identify report processing and refresh eviction
Avoid retaining peers with empty or rejected address reports, bound canonicalization to a fixed input prefix, and move refreshed addresses behind stale addresses in global eviction order. Add regressions for each bound and below-cap growth.
2026-11-07 04:20:00 +00:00
euxarist 63b48e1952 fix(node): Log rejected known-peer addresses for diagnosis. 2026-11-07 04:19:59 +00:00
euxarist dc1c6d1262 fix(node): preserve refreshed Identify addresses. 2026-11-07 04:19:59 +00:00
euxarist 83ac30a43d fix(node): bound Identify-derived Kademlia addresses. 2026-11-07 04:19:59 +00:00
8 changed files with 1677 additions and 21 deletions

No files matched your search

+157 -4
View File
@@ -5,6 +5,28 @@
//!
//! The schema is frozen at v1. All fields are mandatory for forward compatibility.
//! Nodes that receive a certificate with an unknown version MUST reject it.
//!
//! ## Replay
//!
//! A certificate proves *who* asserted a ref update and *what* they asserted.
//! It does not prove the assertion is fresh. [`RefUpdateBody::seq`] and
//! [`RefUpdateBody::nonce`] are inside the signed bytes (see
//! [`RefUpdateBody::to_signing_bytes`]), so a verifier can trust the values,
//! but deciding whether a nonce is being reused or a `seq` is going backwards
//! needs state.
//!
//! This crate is deliberately stateless — it is embedded in both nodes and
//! clients — so replay is **not** enforced here and cannot be. Ownership of
//! the check sits with the verifier that accepts and persists certificates,
//! which must:
//!
//! - reject a `nonce` it has already accepted for the same `(repo, ref_name)`;
//! - reject a `seq` that does not advance past the last accepted `seq` for
//! that `(repo, ref_name)`.
//!
//! Until such a verifier exists, replay of ref-update certificates is not
//! enforced anywhere in this workspace. This contract exists so the first one
//! inherits the requirement instead of re-deriving it.
use std::collections::HashSet;
@@ -41,11 +63,19 @@ pub struct RefUpdateBody {
pub from: String,
/// The new commit hash (SHA-256 hex).
pub to: String,
/// Monotonically increasing sequence number. Prevents replay.
/// Monotonically increasing sequence number, scoped to `(repo, ref_name)`.
///
/// Signed, but not enforced by this crate: core holds no state. The
/// verifier that persists certificates must reject a `seq` that does not
/// advance for the same repo and ref. See the module docs on replay.
pub seq: u64,
/// RFC 3339 timestamp of this update.
pub timestamp: DateTime<Utc>,
/// Random nonce for deduplication.
/// Random nonce, unique per certificate.
///
/// Signed, but not enforced by this crate: core holds no state. The
/// verifier that persists certificates must reject a nonce it has already
/// accepted for the same repo and ref. See the module docs on replay.
pub nonce: String,
}
@@ -140,7 +170,16 @@ impl RefUpdateCert {
///
/// Counts distinct signer DIDs, not signature entries: a repeated
/// signature from the same maintainer counts once.
///
/// `threshold` must be at least 1. Zero is rejected instead of being
/// treated as "no signatures required": a caller that derives the
/// threshold from configuration and gets 0 would otherwise accept any
/// certificate, whether its signatures verify or not.
pub fn satisfies_threshold(&self, maintainers: &[Did], threshold: usize) -> Result<bool> {
if threshold == 0 {
return Err(Error::RefCert("threshold must be at least 1".to_string()));
}
let valid = self.verify_all()?;
let distinct_signers: HashSet<&Did> =
valid.iter().filter(|d| maintainers.contains(d)).collect();
@@ -378,7 +417,7 @@ mod tests {
}
#[test]
fn threshold_zero_is_always_satisfied() {
fn threshold_zero_is_rejected() {
let kp = Keypair::generate();
let cert = RefUpdateCert::new(
kp.did(),
@@ -389,6 +428,120 @@ mod tests {
&kp,
)
.unwrap();
assert!(cert.satisfies_threshold(&[], 0).unwrap());
// Zero must never read as "no signatures required", even with an empty
// maintainer set.
let err = cert.satisfies_threshold(&[], 0).unwrap_err();
assert!(
err.to_string().contains("threshold must be at least 1"),
"zero must be refused as an invalid threshold"
);
}
/// The threshold is checked before any signature work, so a zero is
/// reported as a threshold error even when the certificate's signatures
/// are also invalid. Otherwise a broken certificate would let a caller
/// mistake the failure for a legitimate verification result.
#[test]
fn threshold_zero_is_rejected_before_signature_verification() {
let kp = Keypair::generate();
let other = Keypair::generate();
let mut cert = RefUpdateCert::new(
kp.did(),
"refs/heads/main".to_string(),
dummy_hash('0'),
dummy_hash('a'),
1,
&kp,
)
.unwrap();
cert.signatures[0].sig = other.sign_b64(b"unrelated bytes");
let err = cert.satisfies_threshold(&[kp.did()], 0).unwrap_err();
assert!(
err.to_string().contains("threshold must be at least 1"),
"the invalid threshold must win over the invalid signature"
);
}
/// `seq` and `nonce` are the verifier's replay inputs, so they have to be
/// inside the signed body; tapping either one must invalidate the
/// certificate rather than leave it verifying with different values.
#[test]
fn replay_fields_are_covered_by_the_signature() {
let kp = Keypair::generate();
let cert = RefUpdateCert::new(
kp.did(),
"refs/heads/main".to_string(),
dummy_hash('0'),
dummy_hash('a'),
7,
&kp,
)
.unwrap();
cert.verify_all().expect("baseline must verify");
let mut bumped = cert.clone();
bumped.body.seq = 8;
assert!(
matches!(bumped.verify_all(), Err(Error::SignatureInvalid)),
"changing seq must invalidate the signature"
);
let mut renamed = cert.clone();
renamed.body.nonce = "different-nonce".to_string();
assert!(
matches!(renamed.verify_all(), Err(Error::SignatureInvalid)),
"changing nonce must invalidate the signature"
);
}
/// A signature minted by a real key must not verify when the signer is
/// spelled in another multibase alphabet. Otherwise the same identity
/// would satisfy a maintainer allowlist under several DID strings.
///
/// Base64url is deliberate: its 47-character method-id fits under the
/// pre-existing length bound, so this test reaches the canonical-prefix
/// rule and fails on the pre-fix code instead of being refused for length.
#[test]
fn verify_all_rejects_a_non_canonical_did_spelling() {
let kp = Keypair::generate();
let mut cert = RefUpdateCert::new(
kp.did(),
"refs/heads/main".to_string(),
dummy_hash('0'),
dummy_hash('a'),
1,
&kp,
)
.unwrap();
let mut prefixed = vec![0xed, 0x01];
prefixed.extend_from_slice(&kp.verifying_key().to_bytes());
cert.signatures[0].signer = format!(
"did:key:{}",
multibase::encode(multibase::Base::Base64Url, &prefixed)
)
.parse()
.expect("method is still `key`");
let err = cert
.verify_all()
.expect_err("a non-canonical did:key spelling must not resolve");
assert!(
err.to_string().contains("base58btc"),
"rejection must come from the canonical-prefix rule, not the length guard"
);
assert!(
!matches!(
cert.satisfies_threshold(&[cert.signatures[0].signer.clone()], 1),
Ok(true)
),
"the alternate spelling must not satisfy a threshold"
);
// The canonical spelling of the same key is unaffected.
cert.signatures[0].signer = kp.did();
assert!(cert.satisfies_threshold(&[kp.did()], 1).unwrap());
}
}
+158 -3
View File
@@ -20,6 +20,19 @@ use crate::{Error, Result};
/// Value: 0xed01 encoded as a varint.
const ED25519_MULTICODEC: &[u8] = &[0xed, 0x01];
/// The only multibase prefix a `did:key` method-specific id may carry.
///
/// The DID:key spec encodes Ed25519 keys as base58btc, which is multibase
/// `z`. `multibase::decode` happily accepts every other alphabet, and each
/// spelling of the same key bytes would produce a distinct DID string while
/// gates, allowlists, and database rows compare DIDs as strings — one
/// identity would read as several. Rejecting the alternatives here keeps the
/// mapping from `did:key` to key single-valued for every caller of
/// [`Did::to_verifying_key`] (HTTP signatures, UCAN, ref certs, peer
/// announces). `gitlawb-attest` enforces the same rule for attestation
/// signers.
const DID_KEY_MULTIBASE_PREFIX: char = 'z';
/// A Decentralized Identifier.
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(transparent)]
@@ -71,8 +84,14 @@ impl Did {
/// Resolve the Ed25519 verifying key from a `did:key`.
///
/// Returns `Err` if the DID is not a `did:key`, the key bytes are malformed,
/// or the key is a small-order (weak) point.
/// Only the canonical base58btc (`z`) spelling is accepted. Other
/// multibase encodings decode to the same key but spell a different DID
/// string, so accepting them would let one identity appear under several
/// identifiers to string-comparing gates and allowlists.
///
/// Returns `Err` if the DID is not a `did:key`, uses a non-canonical
/// multibase prefix, the key bytes are invalid, or the key is a
/// small-order (weak) point.
pub fn to_verifying_key(&self) -> Result<VerifyingKey> {
if !self.is_did_key() {
return Err(Error::InvalidDid(format!(
@@ -94,9 +113,27 @@ impl Did {
));
}
let (_, bytes) =
// Cheap prefix gate before the decode, then the base check on what
// multibase actually parsed. The first keeps a clearly non-canonical id
// off the decoder. The second is belt-and-braces — multibase maps a
// leading `z` only to `Base58Btc` — but it states the requirement
// directly instead of relying on that mapping, and `gitlawb-attest`
// makes the same pair of checks.
if !self.method_id().starts_with(DID_KEY_MULTIBASE_PREFIX) {
return Err(Error::InvalidDid(
"did:key must use base58btc (z-prefix)".to_string(),
));
}
let (base, bytes) =
multibase::decode(self.method_id()).map_err(|e| Error::InvalidDid(e.to_string()))?;
if base != Base::Base58Btc {
return Err(Error::InvalidDid(
"did:key must use base58btc (z-prefix)".to_string(),
));
}
if !bytes.starts_with(ED25519_MULTICODEC) {
return Err(Error::InvalidDid(
"not an ed25519 multicodec key".to_string(),
@@ -389,6 +426,124 @@ mod tests {
did.to_verifying_key().expect("a real did:key must resolve");
}
/// `multibase::decode` accepts every alphabet, so the same Ed25519 key can
/// be spelled as several `did:key` strings. Only base58btc is canonical;
/// the rest must be refused so one identity cannot appear as several to
/// gates and allowlists that compare DID strings.
#[test]
fn to_verifying_key_rejects_non_base58btc_multibase() {
use ed25519_dalek::SigningKey;
let signing = SigningKey::from_bytes(&[9u8; 32]);
let mut prefixed = ED25519_MULTICODEC.to_vec();
prefixed.extend_from_slice(&signing.verifying_key().to_bytes());
let canonical = Did::from_verifying_key(&signing.verifying_key());
assert_eq!(
canonical.to_verifying_key().unwrap().to_bytes(),
signing.verifying_key().to_bytes(),
"base58btc is the accepted spelling"
);
// These spellings fit inside the method-id bound, so they reach the
// canonical-prefix check and must be refused there.
for base in [Base::Base64Url, Base::Base32Lower] {
let alternative = format!("did:key:{}", multibase::encode(base, &prefixed));
let did: Did = alternative.parse().expect("method is still `key`");
assert_ne!(
did.as_str(),
canonical.as_str(),
"the same key must not compare equal under another spelling"
);
let err = did
.to_verifying_key()
.expect_err("non-canonical multibase must be refused");
assert!(
matches!(err, Error::InvalidDid(_)),
"must be an InvalidDid, got {err:?}"
);
assert!(
err.to_string().contains("base58btc"),
"the error must name the canonical encoding: {err}"
);
}
// A base16 spelling of the same key is longer than the method-id
// bound, so it is refused by the length guard before the prefix check.
// Either way it must not resolve.
for base in [Base::Base16Lower, Base::Base16Upper] {
let alternative = format!("did:key:{}", multibase::encode(base, &prefixed));
let did: Did = alternative.parse().expect("method is still `key`");
assert!(
matches!(did.to_verifying_key(), Err(Error::InvalidDid(_))),
"a base16 spelling of a real key must not resolve"
);
}
}
/// base58flickr (`Z`) is the non-bitcoin sibling alphabet. Its spelling of
/// a real key fits under the length bound, so it must be refused for the
/// prefix, not merely for its size.
#[test]
fn to_verifying_key_rejects_a_base58flickr_spelling() {
use ed25519_dalek::SigningKey;
let signing = SigningKey::from_bytes(&[11u8; 32]);
let mut prefixed = ED25519_MULTICODEC.to_vec();
prefixed.extend_from_slice(&signing.verifying_key().to_bytes());
let did: Did = format!(
"did:key:{}",
multibase::encode(Base::Base58Flickr, &prefixed)
)
.parse()
.expect("method is still `key`");
let err = did
.to_verifying_key()
.expect_err("base58flickr must be refused");
assert!(
err.to_string().contains("base58btc"),
"the error must name the canonical encoding: {err}"
);
}
/// An empty method-specific id must be refused by the prefix rule without
/// panicking on a missing first character.
#[test]
fn to_verifying_key_rejects_an_empty_method_id() {
let did: Did = "did:key:".parse().expect("method is still `key`");
let err = did
.to_verifying_key()
.expect_err("an empty method id must be refused");
assert!(
err.to_string().contains("base58btc"),
"the error must name the canonical encoding: {err}"
);
}
/// A canonical base58btc spelling is not enough on its own: the bytes must
/// also carry the ed25519-pub multicodec. This pins the check that stops a
/// key of another type from resolving as an Ed25519 key.
#[test]
fn to_verifying_key_rejects_base58btc_with_a_wrong_multicodec() {
use ed25519_dalek::SigningKey;
let signing = SigningKey::from_bytes(&[12u8; 32]);
let mut buf = vec![0x12, 0x00]; // sha2-256, not ed25519-pub
buf.extend_from_slice(&signing.verifying_key().to_bytes());
let did: Did = format!("did:key:{}", multibase::encode(Base::Base58Btc, &buf))
.parse()
.expect("method is still `key`");
let err = did
.to_verifying_key()
.expect_err("a non-ed25519 multicodec must be refused");
assert!(
err.to_string().contains("ed25519"),
"the error must name the expected multicodec: {err}"
);
}
/// A `did:key` can encode a small-order (weak) Ed25519 point. Such a key
/// satisfies the verification equation for any message and, converted to
/// Montgomery form for X25519, yields the all-zero shared secret, so an
+18 -4
View File
@@ -80,15 +80,20 @@ pub(crate) fn did_matches(a: &str, b: &str) -> bool {
!ka.contains(':') && !kb.contains(':') && ka == kb
}
/// Client-facing denial shared by every repo-owner gate copy. `require_repo_owner`
/// renders it as `AppError::Forbidden`; a handler that emits its own
/// `(StatusCode, Json)` envelope instead of `AppError` (e.g.
/// `api::tasks::create_task`) must use this constant too, so two copies of one
/// gate cannot drift in observable output.
pub(crate) const REPO_OWNER_ONLY_MESSAGE: &str = "only the repo owner can perform this action";
/// 403 unless `caller` is the repo owner. Uses [`did_matches`] so the owner check
/// and the author check (close policy) share one normalization.
pub(crate) fn require_repo_owner(record: &RepoRecord, caller: &str) -> Result<()> {
if did_matches(caller, &record.owner_did) {
Ok(())
} else {
Err(AppError::Forbidden(
"only the repo owner can perform this action".into(),
))
Err(AppError::Forbidden(REPO_OWNER_ONLY_MESSAGE.into()))
}
}
@@ -242,6 +247,11 @@ mod authz_guard {
(events, "list_repo_events", "authorize_repo_read("),
// Bucket C — signer-self: the acting DID is matched/bound to auth.0
(tasks, "create_task", "did_matches("),
// #496: create_task is signer-self AND owner-when-repo-scoped — a
// repo_id naming a hosted repo admits tasks only from its owner.
// Both halves are pinned: the did_matches row guards the signer
// binding, this one the repo-ownership gate.
(tasks, "create_task", "require_repo_owner("),
(tasks, "claim_task", "did_matches("),
(tasks, "complete_task", "did_matches("),
(tasks, "fail_task", "did_matches("),
@@ -341,7 +351,11 @@ mod authz_guard {
(
"create_task",
"createTask",
&["require_signer(ctx)?", "did_matches("],
&[
"require_signer(ctx)?",
"did_matches(",
"require_repo_owner(",
],
),
(
"claim_task",
+92 -6
View File
@@ -30,6 +30,33 @@ fn forbidden(msg: &str) -> (StatusCode, Json<Value>) {
)
}
/// This module's error shape with an opaque body: the real error is logged
/// server-side and never serialized (#250), matching the envelope
/// `AppError::Db` renders. Also mirrors its status split: pool-class failures
/// are a retryable 503 `db_unavailable`, everything else the opaque 500
/// `db_error`, so a transient outage is not reported as permanent.
fn db_error(e: anyhow::Error) -> (StatusCode, Json<Value>) {
tracing::error!(error = %format!("{e:#}"), "task database error");
if let Some(sql) = e.chain().find_map(|c| c.downcast_ref::<sqlx::Error>()) {
if crate::error::db_unavailable(sql) {
return (
StatusCode::SERVICE_UNAVAILABLE,
Json(json!({
"error": crate::error::DB_UNAVAILABLE_CODE,
"message": crate::error::DB_UNAVAILABLE_MESSAGE,
})),
);
}
}
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(json!({
"error": "db_error",
"message": crate::error::DB_ERROR_MESSAGE,
})),
)
}
// ── Request / response types ──────────────────────────────────────────────────
#[derive(Deserialize)]
@@ -101,6 +128,37 @@ pub async fn create_task(
if !crate::api::did_matches(&auth.0, &body.delegator_did) {
return Err(forbidden("delegator_did must be the authenticated signer"));
}
// #496: a caller-supplied repo_id must name a repo the caller owns. Without
// this, any signed caller can plant a task — payload and UCAN included —
// under a foreign repo id, where the named assignee can claim it. Resolve
// the id against hosted repos: a hosted, non-quarantined repo admits tasks
// only from its owner (403 otherwise). An id naming no hosted repo
// (unknown, or quarantined — which is hidden as if it did not exist, so
// 403ing it would confirm a real id) is stored verbatim as an opaque
// label: this is a write gate and does not bind an unresolved id to a
// repo. Task reads are caller-independent on this base (the list endpoints
// are not repo-scoped), before and after this change. The 403-vs-201 split
// between a hosted repo id and an unknown one is itself a one-bit oracle —
// a stranger holding a private repo's id learns it is hosted here — but
// denying foreign repos requires drawing exactly that line, and repo ids
// already surface via the task list on this base. Repo-less tasks are
// unaffected.
if let Some(repo_id) = body.repo_id.as_deref() {
let record = state.db.get_repo_by_id(repo_id).await.map_err(db_error)?;
if let Some(record) = record {
let quarantined = state
.db
.is_repo_quarantined(&record.id)
.await
.map_err(db_error)?;
if !quarantined && crate::api::require_repo_owner(&record, &auth.0).is_err() {
// Same denial text the GraphQL copy relays from
// `require_repo_owner`, so the two copies of this gate do not
// disagree in observable output.
return Err(forbidden(crate::api::REPO_OWNER_ONLY_MESSAGE));
}
}
}
let now = Utc::now().to_rfc3339();
let task = AgentTask {
id: Uuid::new_v4().to_string(),
@@ -117,12 +175,7 @@ pub async fn create_task(
updated_at: now,
deadline: body.deadline,
};
state.db.create_task(&task).await.map_err(|e| {
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(json!({ "error": e.to_string() })),
)
})?;
state.db.create_task(&task).await.map_err(db_error)?;
Ok((StatusCode::CREATED, Json(task_to_json(&task))))
}
@@ -297,3 +350,36 @@ pub async fn fail_task(
});
Ok(Json(task_to_json(&task)))
}
#[cfg(test)]
mod tests {
use super::*;
/// The gate's DB helper must mirror `AppError::Db`: a pool-level failure is
/// the retryable 503 `db_unavailable`, a query fault the opaque 500.
#[test]
fn db_error_matches_app_error_status_split() {
let unavailable = db_error(anyhow::Error::from(sqlx::Error::PoolTimedOut));
assert_eq!(unavailable.0, StatusCode::SERVICE_UNAVAILABLE);
assert_eq!(unavailable.1 .0["error"], crate::error::DB_UNAVAILABLE_CODE);
assert_eq!(
unavailable.1 .0["message"],
crate::error::DB_UNAVAILABLE_MESSAGE
);
let query = db_error(anyhow::Error::from(sqlx::Error::Protocol(
"column \"machine_id\" does not exist".into(),
)));
assert_eq!(query.0, StatusCode::INTERNAL_SERVER_ERROR);
assert_eq!(query.1 .0["error"], "db_error");
assert_eq!(query.1 .0["message"], crate::error::DB_ERROR_MESSAGE);
assert!(!query.1 .0.to_string().contains("machine_id"));
}
/// `.context()` layers must not hide the sqlx error from the status split.
#[test]
fn db_error_sees_sqlx_through_context() {
let wrapped = anyhow::Error::from(sqlx::Error::PoolClosed).context("repo lookup failed");
assert_eq!(db_error(wrapped).0, StatusCode::SERVICE_UNAVAILABLE);
}
}
+1 -1
View File
@@ -101,7 +101,7 @@ pub const DB_ERROR_MESSAGE: &str = "a database error occurred";
/// Connection-level sqlx failures that mean the database is unreachable right
/// now (retryable, 503), as opposed to server-reported query errors.
fn db_unavailable(e: &sqlx::Error) -> bool {
pub(crate) fn db_unavailable(e: &sqlx::Error) -> bool {
matches!(
e,
sqlx::Error::PoolTimedOut
+220
View File
@@ -36,6 +36,33 @@ impl MutationRoot {
}
let delegator_did = caller.to_string();
let db = ctx.data_unchecked::<Arc<Db>>();
// #496: same write gate as the REST handler — a repo_id naming a
// hosted, non-quarantined repo admits tasks only from its owner, so a
// stranger cannot file under a foreign repo id. An id naming no hosted
// repo (unknown, or quarantined — hidden as if it did not exist) is
// stored verbatim as an opaque label; the gate does not bind an
// unresolved id to a repo. Task reads are caller-independent on this
// base (the tasks resolver is not repo-scoped), before and after this
// change. As in tasks.rs, the success-vs-denial split between a hosted
// repo id and an unknown one is itself a one-bit oracle — a stranger
// holding a private repo's id learns it is hosted here — but denying
// foreign repos requires drawing exactly that line.
if let Some(repo_id) = input.repo_id.as_deref() {
let record = db
.get_repo_by_id(repo_id)
.await
.map_err(crate::graphql::graphql_db_err)?;
if let Some(record) = record {
let quarantined = db
.is_repo_quarantined(&record.id)
.await
.map_err(crate::graphql::graphql_db_err)?;
if !quarantined {
crate::api::require_repo_owner(&record, caller)
.map_err(crate::graphql::graphql_app_err)?;
}
}
}
let now = Utc::now().to_rfc3339();
let task = AgentTask {
id: Uuid::new_v4().to_string(),
@@ -186,6 +213,22 @@ mod tests {
.join("; ")
}
/// The `id` scalar returned by a successful `createTask` mutation.
fn created_id(resp: &Response) -> String {
assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors);
let async_graphql::Value::Object(obj) = &resp.data else {
panic!("data not an object: {:?}", resp.data);
};
let async_graphql::Value::Object(task) = obj.get("createTask").expect("createTask key")
else {
panic!("createTask not an object");
};
let async_graphql::Value::String(id) = task.get("id").expect("id key") else {
panic!("id not a string");
};
id.clone()
}
/// N2: every GraphQL mutation requires a verified signer and binds the acting
/// DID to it. Unsigned is rejected; a signer other than the claimed actor is
/// rejected; the matching signer passes the auth gate.
@@ -353,4 +396,181 @@ mod tests {
errors(&resp)
);
}
/// #496 (GraphQL): createTask applies the same repo-ownership gate as the
/// REST handler — a stranger naming a hosted repo is rejected without
/// persisting anything, while the owner files. The quarantined and
/// unknown-id arms are pinned too, including that each accepted write
/// stores the supplied id verbatim, so this copy of the gate cannot regress
/// silently.
#[sqlx::test]
async fn create_task_rejects_foreign_repo_id(pool: PgPool) {
let state = crate::test_support::test_state(pool).await;
let owner = "did:key:zGQLTASKOWNERAAAAAAAAAAAAAAAAAAAAAAAAAA";
let stranger = "did:key:zGQLTASKSTRANGERBBBBBBBBBBBBBBBBBBBBBB";
let now = chrono::Utc::now();
let repo = crate::db::RepoRecord {
id: uuid::Uuid::new_v4().to_string(),
name: "gql-task-gate-repo".to_string(),
owner_did: owner.to_string(),
description: None,
is_public: true,
default_branch: "main".to_string(),
created_at: now,
updated_at: now,
disk_path: "/tmp/gql-task-gate-repo".to_string(),
forked_from: None,
machine_id: None,
};
state.db.create_repo(&repo).await.expect("seed repo");
let schema = state.graphql_schema.as_ref();
let q = |actor: &str| {
format!(
r#"mutation {{ createTask(delegatorDid: "{actor}", input: {{ kind: "build", capability: "repo:write", repoId: "{}" }}) {{ id }} }}"#,
repo.id
)
};
// Stranger signs as themselves (so the signer binding passes) but
// names the victim's repo → rejected by the ownership gate, leaking
// nothing about the repo.
let resp = schema
.execute(Request::new(q(stranger)).data(AuthenticatedDid(stranger.into())))
.await;
let errs = errors(&resp);
assert!(
errs.contains(crate::api::REPO_OWNER_ONLY_MESSAGE),
"a non-owner must not file under a foreign repo id (shared denial text): {errs}"
);
// The denial must not persist: no task row may carry the victim's
// repo id afterwards.
let stored = state
.db
.list_tasks(None, None, 50)
.await
.expect("list tasks");
assert!(
stored
.iter()
.all(|t| t.repo_id.as_deref() != Some(repo.id.as_str())),
"a denied write must not leave a task under the foreign repo id"
);
// The owner files under their own repo id.
let resp = schema
.execute(Request::new(q(owner)).data(AuthenticatedDid(owner.into())))
.await;
assert!(
errors(&resp).is_empty(),
"the owner should file against their repo: {}",
errors(&resp)
);
// Unknown repo ids stay accepted as opaque labels.
let q_unknown = format!(
r#"mutation {{ createTask(delegatorDid: "{stranger}", input: {{ kind: "build", capability: "repo:write", repoId: "no-such-repo-id" }}) {{ id }} }}"#
);
let resp = schema
.execute(Request::new(q_unknown).data(AuthenticatedDid(stranger.into())))
.await;
assert!(
errors(&resp).is_empty(),
"an id naming no hosted repo stays an opaque label: {}",
errors(&resp)
);
// Read back the row: the unresolved id must be stored verbatim, not
// dropped or rewritten to a repo it never named.
let stored = state
.db
.get_task(&created_id(&resp))
.await
.expect("get task")
.expect("task must be stored");
assert_eq!(
stored.repo_id.as_deref(),
Some("no-such-repo-id"),
"an unresolved id must be stored verbatim as a label"
);
// A quarantined repo id is treated as unknown (accepted, never a 403
// that would confirm the id names a real repo). This pins the
// `!quarantined` arm on this copy of the gate.
state
.db
.set_repo_quarantine(&repo.id, true)
.await
.expect("quarantine");
let resp = schema
.execute(Request::new(q(stranger)).data(AuthenticatedDid(stranger.into())))
.await;
assert!(
errors(&resp).is_empty(),
"a quarantined repo id must not 403 (existence oracle): {}",
errors(&resp)
);
// Read back the row: the quarantined id is stored verbatim as a label
// too, so the accepted write is not silently dropped.
let stored = state
.db
.get_task(&created_id(&resp))
.await
.expect("get task")
.expect("task must be stored");
assert_eq!(
stored.repo_id.as_deref(),
Some(repo.id.as_str()),
"a quarantined id must be stored verbatim as a label"
);
}
/// #496 (GraphQL): a repo-lookup fault in createTask's gate must stay
/// opaque. Dropping a column `get_repo_by_id` selects faults that lookup;
/// the resolver must return the fixed DB message and leak no schema text
/// (#250), as `create_task_db_error_message_is_opaque` pins for the
/// agent_tasks fault.
#[sqlx::test]
async fn create_task_gate_repo_lookup_fault_is_opaque(pool: PgPool) {
let state = crate::test_support::test_state(pool.clone()).await;
let owner = "did:key:zGQLTASKFAULTOWNERAAAAAAAAAAAAAAAAAAAAAAAA";
let now = chrono::Utc::now();
let repo = crate::db::RepoRecord {
id: uuid::Uuid::new_v4().to_string(),
name: "gql-task-gate-fault-repo".to_string(),
owner_did: owner.to_string(),
description: None,
is_public: true,
default_branch: "main".to_string(),
created_at: now,
updated_at: now,
disk_path: "/tmp/gql-task-gate-fault-repo".to_string(),
forked_from: None,
machine_id: None,
};
state.db.create_repo(&repo).await.expect("seed repo");
sqlx::query("ALTER TABLE repos DROP COLUMN machine_id")
.execute(&pool)
.await
.expect("drop machine_id");
let q = format!(
r#"mutation {{ createTask(delegatorDid: "{owner}", input: {{ kind: "build", capability: "repo:write", repoId: "{}" }}) {{ id }} }}"#,
repo.id
);
let resp = state
.graphql_schema
.execute(Request::new(&q).data(AuthenticatedDid(owner.into())))
.await;
let errs = errors(&resp);
assert!(
errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE),
"gate sqlx fault must be opaque: {errs}"
);
assert!(
!errs.contains("machine_id") && !errs.contains("column"),
"schema text leaked through the gate: {errs}"
);
}
}
+692 -3
View File
@@ -7,11 +7,11 @@
//! The node's PeerId comes from an Ed25519 keypair loaded from a persistent
//! key file, so the PeerId is stable across restarts.
use std::collections::{hash_map::DefaultHasher, HashMap};
use std::collections::{hash_map::DefaultHasher, HashMap, HashSet, VecDeque};
use std::hash::{Hash, Hasher};
use std::path::{Component, Path, PathBuf};
use std::sync::Arc;
use std::time::Duration;
use std::time::{Duration, Instant};
use anyhow::{Context, Result};
use chrono::{DateTime, Utc};
@@ -315,6 +315,17 @@ fn is_zero(v: &u32) -> bool {
*v == 0
}
// Identify is an untrusted address source. Keep enough entries for normal
// multi-homed peers while bounding both one peer and the whole routing table.
const IDENTIFY_ADDRESS_LIMIT: usize = 8;
// Bound canonicalization and sorting too, including rejected/duplicate entries.
const IDENTIFY_REPORT_ADDRESS_LIMIT: usize = 64;
const IDENTIFY_NEW_ADDRESS_LIMIT: usize = 8;
const IDENTIFY_GLOBAL_ADDRESS_LIMIT: usize = 1024;
const IDENTIFY_ADDRESS_WINDOW: Duration = Duration::from_secs(60);
const IDENTIFY_ADDRESS_TTL: Duration = Duration::from_secs(30 * 60);
const IDENTIFY_ADDRESS_CLEANUP_INTERVAL: Duration = Duration::from_secs(60);
/// A ref-update event published to Gossipsub when a push lands.
///
/// The signing bytes are this struct serialized with `sig` set to None (see
@@ -1631,6 +1642,200 @@ fn did_to_kad_key(did: &str) -> kad::RecordKey {
kad::RecordKey::new(&format!("/gitlawb/did/{did}").as_bytes())
}
/// Returns true when `address` was explicitly configured for `peer` through
/// AddKnownPeer. Explicit addresses are retained in Kademlia even when an
/// overlapping Identify lease expires or is evicted.
fn is_explicit_address(
explicit_addresses: &HashMap<PeerId, HashSet<Multiaddr>>,
peer: &PeerId,
address: &Multiaddr,
) -> bool {
explicit_addresses
.get(peer)
.is_some_and(|addresses| addresses.contains(address))
}
#[derive(Debug)]
struct IdentifyAddress {
address: Multiaddr,
expires_at: Instant,
}
#[derive(Debug)]
struct IdentifyPeerAddresses {
addresses: VecDeque<IdentifyAddress>,
window_started: Instant,
new_addresses: usize,
}
#[derive(Debug, Default, PartialEq, Eq)]
struct IdentifyAddressChanges {
added: Vec<Multiaddr>,
refreshed: Vec<Multiaddr>,
removed: Vec<(PeerId, Multiaddr)>,
}
#[derive(Debug, Default)]
struct IdentifyAddressBook {
peers: HashMap<PeerId, IdentifyPeerAddresses>,
address_count: usize,
}
impl IdentifyAddressBook {
fn update(
&mut self,
peer_id: PeerId,
now: Instant,
addresses: &[Multiaddr],
) -> IdentifyAddressChanges {
let mut changes = IdentifyAddressChanges::default();
if self.peers.contains_key(&peer_id) {
changes.removed.extend(self.expire_peer(peer_id, now));
}
let reported = addresses
.iter()
.take(IDENTIFY_REPORT_ADDRESS_LIMIT)
.filter_map(|address| address.clone().with_p2p(peer_id).ok())
.collect::<HashSet<_>>();
if reported.is_empty() && !self.peers.contains_key(&peer_id) {
return changes;
}
let state = self
.peers
.entry(peer_id)
.or_insert_with(|| IdentifyPeerAddresses {
addresses: VecDeque::new(),
window_started: now,
new_addresses: 0,
});
if now.saturating_duration_since(state.window_started) >= IDENTIFY_ADDRESS_WINDOW {
state.window_started = now;
state.new_addresses = 0;
}
// Refresh every retained address in the bounded report before admission.
// Refreshed addresses are reported so the Identify handler can re-admit
// them to Kademlia. Kademlia may have dropped a retained address
// after a failed dial or never kept it for a full k-bucket, and a
// TTL refresh alone would never re-add it.
for existing in &mut state.addresses {
if reported.contains(&existing.address) {
existing.expires_at = now + IDENTIFY_ADDRESS_TTL;
changes.refreshed.push(existing.address.clone());
}
}
let mut new_addresses: Vec<Multiaddr> = reported
.iter()
.filter(|address| {
!state
.addresses
.iter()
.any(|existing| &existing.address == *address)
})
.cloned()
.collect();
// HashSet iteration is deliberately unordered. Sort new candidates so
// an oversized Identify report has deterministic admission behavior.
new_addresses.sort();
for address in new_addresses {
if state.new_addresses >= IDENTIFY_NEW_ADDRESS_LIMIT {
break;
}
let evicted = if state.addresses.len() < IDENTIFY_ADDRESS_LIMIT {
None
} else {
state
.addresses
.iter()
.position(|existing| !reported.contains(&existing.address))
.and_then(|index| state.addresses.remove(index))
};
if let Some(evicted) = evicted {
self.address_count -= 1;
changes.removed.push((peer_id, evicted.address));
} else if state.addresses.len() >= IDENTIFY_ADDRESS_LIMIT {
// Every retained address was refreshed in this report. Keep
// that stable subset and drop surplus new candidates.
break;
}
if self.address_count >= IDENTIFY_GLOBAL_ADDRESS_LIMIT {
// Reject the admission instead of evicting another peer's
// entry: cross-peer eviction lets a flood of new identities
// push honest peers' addresses out of the book and, when the
// evicted address is their last one, out of the Kademlia
// routing table. Per-peer eviction above still applies to the
// reporting peer's own addresses.
break;
}
state.new_addresses += 1;
state.addresses.push_back(IdentifyAddress {
address: address.clone(),
expires_at: now + IDENTIFY_ADDRESS_TTL,
});
self.address_count += 1;
changes.added.push(address);
}
// Do not keep a row when every candidate was rejected: a zero-address
// row is invisible to address_count and would otherwise linger until
// the expiry tick. This matches expire_peer, which drops empty rows.
if state.addresses.is_empty() {
self.peers.remove(&peer_id);
}
changes
}
fn expire(&mut self, now: Instant) -> Vec<(PeerId, Multiaddr)> {
let mut removed = Vec::new();
let peers = self.peers.keys().copied().collect::<Vec<_>>();
for peer_id in peers {
removed.extend(self.expire_peer(peer_id, now));
}
removed
}
fn expire_peer(&mut self, peer_id: PeerId, now: Instant) -> Vec<(PeerId, Multiaddr)> {
let expired = self
.peers
.get_mut(&peer_id)
.map(|state| Self::expire_state(state, now))
.unwrap_or_default();
self.address_count -= expired.len();
if self
.peers
.get(&peer_id)
.is_some_and(|state| state.addresses.is_empty())
{
self.peers.remove(&peer_id);
}
expired
.into_iter()
.map(|address| (peer_id, address))
.collect()
}
fn expire_state(state: &mut IdentifyPeerAddresses, now: Instant) -> Vec<Multiaddr> {
let mut expired = Vec::new();
let mut retained = VecDeque::with_capacity(state.addresses.len());
while let Some(address) = state.addresses.pop_front() {
if address.expires_at <= now {
expired.push(address.address);
} else {
retained.push_back(address);
}
}
state.addresses = retained;
expired
}
}
/// Combined libp2p behaviour.
#[derive(NetworkBehaviour)]
#[behaviour(prelude = "libp2p_swarm::derive_prelude")]
@@ -3960,6 +4165,14 @@ pub async fn start(
// Track in-flight GetRecord queries → reply channels
let mut pending_get_did: HashMap<kad::QueryId, oneshot::Sender<Option<DidRecord>>> =
HashMap::new();
// Keep Identify-derived entries across disconnects so a bootstrap address
// learned through Identify remains usable for redial. The TTL cleanup below
// removes entries that are no longer refreshed without touching explicit
// AddKnownPeer addresses.
let mut identify_addresses = IdentifyAddressBook::default();
let mut explicit_addresses: HashMap<PeerId, HashSet<Multiaddr>> = HashMap::new();
let mut identify_cleanup = tokio::time::interval(IDENTIFY_ADDRESS_CLEANUP_INTERVAL);
identify_cleanup.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);
// Start the event loop as a background task
tokio::spawn(async move {
@@ -3979,6 +4192,13 @@ pub async fn start(
_ = ingest_sweep.tick() => {
run_ingest_sweep(&ingest_limiters, &replay_guard, ingest_now()).await;
}
_ = identify_cleanup.tick() => {
for (peer_id, address) in identify_addresses.expire(Instant::now()) {
if !is_explicit_address(&explicit_addresses, &peer_id, &address) {
swarm.behaviour_mut().kademlia.remove_address(&peer_id, &address);
}
}
}
// Graceful shutdown: exit the swarm loop when the
// process-wide signal flips. This drops the Swarm
// which closes all libp2p connections cleanly.
@@ -4113,7 +4333,21 @@ pub async fn start(
identify::Event::Received { peer_id, info, .. }
)) => {
debug!(peer = %peer_id, "identify received");
for addr in info.listen_addrs {
let changes = identify_addresses.update(
peer_id,
Instant::now(),
&info.listen_addrs,
);
for (removed_peer, addr) in changes.removed {
if !is_explicit_address(&explicit_addresses, &removed_peer, &addr)
{
swarm
.behaviour_mut()
.kademlia
.remove_address(&removed_peer, &addr);
}
}
for addr in changes.refreshed.into_iter().chain(changes.added) {
swarm.behaviour_mut().kademlia.add_address(&peer_id, addr);
}
}
@@ -4150,6 +4384,14 @@ pub async fn start(
}
}
P2pCommand::AddKnownPeer { peer_id, addr } => {
let Ok(addr) = addr.clone().with_p2p(peer_id) else {
warn!(%peer_id, %addr, "dropping known-peer address with a foreign peer suffix");
continue;
};
explicit_addresses
.entry(peer_id)
.or_default()
.insert(addr.clone());
swarm.behaviour_mut().kademlia.add_address(&peer_id, addr);
}
P2pCommand::Dial(addr) => {
@@ -4201,6 +4443,453 @@ pub async fn start(
mod tests {
use super::*;
fn identify_address(peer_id: PeerId, octet: u8, port: u16) -> Multiaddr {
format!("/ip4/192.0.2.{octet}/udp/{port}/quic-v1")
.parse::<Multiaddr>()
.unwrap()
.with_p2p(peer_id)
.unwrap()
}
fn identify_address_without_peer(octet: u8, port: u16) -> Multiaddr {
format!("/ip4/192.0.2.{octet}/udp/{port}/quic-v1")
.parse::<Multiaddr>()
.unwrap()
}
#[test]
fn identify_empty_reports_do_not_retain_peer_entries() {
let now = Instant::now();
let mut book = IdentifyAddressBook::default();
let foreign = identify_address(PeerId::random(), 1, 10_000);
for _ in 0..IDENTIFY_GLOBAL_ADDRESS_LIMIT + 1 {
let peer = PeerId::random();
assert_eq!(
book.update(peer, now, &[]),
IdentifyAddressChanges::default()
);
assert_eq!(
book.update(peer, now, std::slice::from_ref(&foreign)),
IdentifyAddressChanges::default()
);
}
assert!(book.peers.is_empty());
assert_eq!(book.address_count, 0);
}
#[test]
fn identify_fully_rejected_peer_leaves_no_row_at_full_budget() {
let now = Instant::now();
let mut book = IdentifyAddressBook::default();
// Fill the global budget with one-address peers.
for index in 0..IDENTIFY_GLOBAL_ADDRESS_LIMIT {
let peer = PeerId::random();
let address = identify_address(peer, 1, 10_000 + index as u16);
book.update(peer, now, std::slice::from_ref(&address));
}
assert_eq!(book.address_count, IDENTIFY_GLOBAL_ADDRESS_LIMIT);
// A fresh peer reporting at saturation has every candidate rejected
// by the global-budget check. The row created by or_insert_with must
// be dropped: a zero-address row is invisible to address_count and
// would otherwise linger until the expiry tick.
let fresh = PeerId::random();
let fresh_address = identify_address(fresh, 2, 30_000);
let changes = book.update(fresh, now, std::slice::from_ref(&fresh_address));
assert!(changes.added.is_empty());
assert!(!book.peers.contains_key(&fresh));
assert_eq!(book.address_count, IDENTIFY_GLOBAL_ADDRESS_LIMIT);
}
#[test]
fn identify_update_reports_expired_addresses_as_removed() {
let peer = PeerId::random();
let now = Instant::now();
let address = identify_address(peer, 1, 10_000);
let mut book = IdentifyAddressBook::default();
book.update(peer, now, std::slice::from_ref(&address));
// An update past the TTL expires the stale address through
// expire_peer and reports it in changes.removed so the Identify
// handler can drop it from Kademlia.
let changes = book.update(
peer,
now + IDENTIFY_ADDRESS_TTL + Duration::from_secs(1),
std::slice::from_ref(&address),
);
assert_eq!(changes.removed, vec![(peer, address.clone())]);
assert_eq!(changes.added, vec![address]);
}
#[test]
fn identify_expire_drops_rows_emptied_by_expiry() {
let peer = PeerId::random();
let now = Instant::now();
let address = identify_address(peer, 1, 10_000);
let mut book = IdentifyAddressBook::default();
book.update(peer, now, std::slice::from_ref(&address));
assert!(book.peers.contains_key(&peer));
// Once every address in the row has expired, expire() must drop the
// row itself, matching the empty-row drop in update().
let removed = book.expire(now + IDENTIFY_ADDRESS_TTL + Duration::from_secs(1));
assert_eq!(removed, vec![(peer, address)]);
assert!(!book.peers.contains_key(&peer));
assert_eq!(book.address_count, 0);
}
#[test]
fn identify_report_processing_stops_at_the_input_limit() {
let peer = PeerId::random();
let now = Instant::now();
let foreign = identify_address(PeerId::random(), 1, 10_000);
let accepted = identify_address(peer, 2, 10_001);
let ignored = identify_address(peer, 3, 10_002);
let mut report = vec![foreign; IDENTIFY_REPORT_ADDRESS_LIMIT - 1];
report.push(accepted.clone());
report.push(ignored);
let mut book = IdentifyAddressBook::default();
let changes = book.update(peer, now, &report);
assert_eq!(changes.added, vec![accepted]);
assert!(changes.removed.is_empty());
assert_eq!(book.address_count, 1);
}
#[test]
fn identify_addresses_grow_without_eviction_below_the_peer_cap() {
let peer = PeerId::random();
let now = Instant::now();
let first = identify_address(peer, 1, 10_000);
let second = identify_address(peer, 2, 10_001);
let mut book = IdentifyAddressBook::default();
book.update(peer, now, std::slice::from_ref(&first));
let changes = book.update(
peer,
now + IDENTIFY_ADDRESS_WINDOW,
std::slice::from_ref(&second),
);
assert_eq!(changes.added, vec![second]);
assert!(changes.removed.is_empty());
assert_eq!(book.peers[&peer].addresses.len(), 2);
assert_eq!(book.address_count, 2);
}
#[test]
fn identify_global_budget_rejects_new_admissions() {
let now = Instant::now();
let mut book = IdentifyAddressBook::default();
let peers = (0..IDENTIFY_GLOBAL_ADDRESS_LIMIT)
.map(|index| {
let peer = PeerId::random();
let address = identify_address(peer, 1, 10_000 + index as u16);
book.update(peer, now, std::slice::from_ref(&address));
(peer, address)
})
.collect::<Vec<_>>();
let (peer, refreshed) = &peers[0];
let new_address = identify_address(*peer, 2, 30_000);
let changes = book.update(
*peer,
now + IDENTIFY_ADDRESS_WINDOW,
&[new_address.clone(), refreshed.clone()],
);
// The budget is full: the new address is rejected rather than
// evicting another peer's entry, and the refreshed address keeps its
// slot and is reported for re-admission to Kademlia.
assert!(changes.added.is_empty());
assert!(changes.removed.is_empty());
assert_eq!(changes.refreshed, vec![(*refreshed).clone()]);
assert_eq!(book.peers.len(), IDENTIFY_GLOBAL_ADDRESS_LIMIT);
assert_eq!(book.address_count, IDENTIFY_GLOBAL_ADDRESS_LIMIT);
assert_eq!(
book.peers[peer]
.addresses
.iter()
.map(|a| &a.address)
.collect::<Vec<_>>(),
vec![refreshed]
);
}
#[test]
fn identify_per_peer_eviction_applies_at_full_global_budget() {
let now = Instant::now();
let mut book = IdentifyAddressBook::default();
// One filler peer goes in before the victim so the evicted address
// is not also the globally-oldest insertion. If saturation evicted
// the globally-oldest entry instead of the victim's own oldest,
// the sentinel's address would be the one removed.
let sentinel = PeerId::random();
let sentinel_address = identify_address(sentinel, 200, 19_999);
book.update(sentinel, now, std::slice::from_ref(&sentinel_address));
// The victim peer holds a full per-peer row while the rest of the
// global budget is filled by one-address peers.
let victim = PeerId::random();
let victim_addresses: Vec<Multiaddr> = (0..IDENTIFY_ADDRESS_LIMIT)
.map(|index| identify_address(victim, index as u8 + 1, 10_000 + index as u16))
.collect();
book.update(victim, now, &victim_addresses);
for index in 0..IDENTIFY_GLOBAL_ADDRESS_LIMIT - IDENTIFY_ADDRESS_LIMIT - 1 {
let peer = PeerId::random();
let address = identify_address(peer, 200, 20_000 + index as u16);
book.update(peer, now, &[address]);
}
assert_eq!(book.address_count, IDENTIFY_GLOBAL_ADDRESS_LIMIT);
// The victim reports a ninth address after the rate window rolls over.
// Per-peer eviction must still apply at global saturation: the oldest
// non-reported address is evicted to make room, and the global count
// is unchanged. This ordering only holds because eviction runs before
// the global-budget check; hoisting that check above the eviction
// block makes this test fail.
let ninth = identify_address(victim, 100, 30_000);
let changes = book.update(
victim,
now + IDENTIFY_ADDRESS_WINDOW,
std::slice::from_ref(&ninth),
);
assert_eq!(changes.added, vec![ninth]);
assert_eq!(changes.removed, vec![(victim, victim_addresses[0].clone())]);
assert_eq!(book.address_count, IDENTIFY_GLOBAL_ADDRESS_LIMIT);
assert_eq!(book.peers[&victim].addresses.len(), IDENTIFY_ADDRESS_LIMIT);
// The sentinel was inserted first; per-peer eviction must leave it
// alone. A global-oldest eviction policy would have removed it.
assert_eq!(
book.peers[&sentinel]
.addresses
.iter()
.map(|a| &a.address)
.collect::<Vec<_>>(),
vec![&sentinel_address]
);
}
#[test]
fn explicit_address_helper_detects_retained_addresses() {
let peer = PeerId::random();
let other = PeerId::random();
let address = identify_address(peer, 1, 10_000);
let mut explicit: HashMap<PeerId, HashSet<Multiaddr>> = HashMap::new();
// Unknown peer: nothing is explicit.
assert!(!is_explicit_address(&explicit, &peer, &address));
explicit.entry(peer).or_default().insert(address.clone());
assert!(is_explicit_address(&explicit, &peer, &address));
// The same address under a different peer is not retained.
assert!(!is_explicit_address(&explicit, &other, &address));
// A different address for the same peer is not retained.
assert!(!is_explicit_address(
&explicit,
&peer,
&identify_address(peer, 2, 10_001)
));
}
#[test]
fn identify_addresses_are_capped_with_fifo_eviction() {
let peer_id = PeerId::random();
let now = Instant::now();
let initial = (0..IDENTIFY_ADDRESS_LIMIT)
.map(|index| identify_address(peer_id, index as u8 + 1, 10_000 + index as u16))
.collect::<Vec<_>>();
let replacement = identify_address(peer_id, 200, 20_000);
let mut book = IdentifyAddressBook::default();
let first = book.update(peer_id, now, &initial);
assert_eq!(first.added, initial);
assert!(first.removed.is_empty());
let second = book.update(
peer_id,
now + IDENTIFY_ADDRESS_WINDOW,
std::slice::from_ref(&replacement),
);
assert_eq!(second.added, vec![replacement]);
assert_eq!(second.removed, vec![(peer_id, initial[0].clone())]);
}
#[test]
fn identify_oversized_reports_keep_the_same_refreshed_subset() {
let peer_id = PeerId::random();
let now = Instant::now();
let mut report = (0..IDENTIFY_ADDRESS_LIMIT + 2)
.map(|index| identify_address(peer_id, index as u8 + 1, 10_000 + index as u16))
.collect::<Vec<_>>();
let mut book = IdentifyAddressBook::default();
let first = book.update(peer_id, now, &report);
assert_eq!(first.added.len(), IDENTIFY_ADDRESS_LIMIT);
assert_eq!(first.removed.len(), 0);
let retained = first.added.clone();
report.reverse();
let second = book.update(peer_id, now + IDENTIFY_ADDRESS_WINDOW, &report);
assert!(second.added.is_empty());
assert!(second.removed.is_empty());
let current = book
.peers
.get(&peer_id)
.unwrap()
.addresses
.iter()
.map(|entry| entry.address.clone())
.collect::<Vec<_>>();
assert_eq!(current, retained);
}
#[test]
fn identify_addresses_rate_limit_new_entries_across_updates() {
let peer_id = PeerId::random();
let now = Instant::now();
let initial = (0..IDENTIFY_NEW_ADDRESS_LIMIT)
.map(|index| identify_address(peer_id, index as u8 + 1, 10_000 + index as u16))
.collect::<Vec<_>>();
let replacement = identify_address(peer_id, 200, 20_000);
let mut book = IdentifyAddressBook::default();
assert_eq!(book.update(peer_id, now, &initial).added, initial);
assert!(book
.update(peer_id, now, std::slice::from_ref(&replacement))
.added
.is_empty());
assert_eq!(
book.update(
peer_id,
now + IDENTIFY_ADDRESS_WINDOW,
std::slice::from_ref(&replacement),
)
.added,
vec![replacement]
);
}
#[test]
fn identify_addresses_have_a_global_budget_across_peers() {
let now = Instant::now();
let mut book = IdentifyAddressBook::default();
let mut first = None;
for index in 0..IDENTIFY_GLOBAL_ADDRESS_LIMIT {
let peer_id = PeerId::random();
let address = identify_address(peer_id, (index % 254) as u8 + 1, 10_000 + index as u16);
if first.is_none() {
first = Some((peer_id, address.clone()));
}
assert!(book.update(peer_id, now, &[address]).removed.is_empty());
}
let (peer_id, first_address) = first.unwrap();
let address = identify_address(peer_id, 250, 30_000);
let changes = book.update(
peer_id,
now + IDENTIFY_ADDRESS_WINDOW,
std::slice::from_ref(&address),
);
// The global budget is full, so the admission is rejected and the
// peer's existing entry is left alone.
assert!(changes.added.is_empty());
assert!(changes.removed.is_empty());
assert_eq!(
book.peers[&peer_id]
.addresses
.iter()
.map(|a| &a.address)
.collect::<Vec<_>>(),
vec![&first_address]
);
assert_eq!(book.address_count, IDENTIFY_GLOBAL_ADDRESS_LIMIT);
}
#[test]
fn identify_addresses_refresh_and_expire() {
let peer_id = PeerId::random();
let now = Instant::now();
let address = identify_address(peer_id, 1, 10_000);
let mut book = IdentifyAddressBook::default();
assert_eq!(
book.update(peer_id, now, std::slice::from_ref(&address))
.added,
vec![address.clone()]
);
let refreshed = now + IDENTIFY_ADDRESS_TTL - Duration::from_secs(1);
let refresh = book.update(peer_id, refreshed, std::slice::from_ref(&address));
assert!(refresh.added.is_empty());
assert!(refresh.removed.is_empty());
// Refreshed addresses are reported so the Identify handler re-adds
// them to Kademlia even though they were not newly admitted.
assert_eq!(refresh.refreshed, vec![address.clone()]);
assert!(book
.expire(now + IDENTIFY_ADDRESS_TTL + Duration::from_secs(1))
.is_empty());
assert_eq!(
book.expire(refreshed + IDENTIFY_ADDRESS_TTL + Duration::from_secs(1)),
vec![(peer_id, address)]
);
}
#[test]
fn identify_refresh_reports_only_addresses_in_the_report() {
let peer_id = PeerId::random();
let now = Instant::now();
let reported = identify_address(peer_id, 1, 10_000);
let unreported = identify_address(peer_id, 2, 20_000);
let mut book = IdentifyAddressBook::default();
// Admit both addresses up front so the peer holds two.
assert_eq!(
book.update(peer_id, now, &[reported.clone(), unreported.clone()])
.added
.len(),
2
);
// A later report carrying only one of the two addresses must refresh
// just that one: pins the negative direction of the refresh
// predicate, so an address the peer stopped advertising keeps its
// original TTL instead of being refreshed back onto the Kademlia
// re-admit path on every report.
let report_at = now + IDENTIFY_ADDRESS_TTL - Duration::from_secs(1);
let changes = book.update(peer_id, report_at, std::slice::from_ref(&reported));
assert!(changes.added.is_empty());
assert!(changes.removed.is_empty());
assert_eq!(changes.refreshed, vec![reported.clone()]);
// The unreported address still expires at its original TTL, while
// the refreshed address survives until its refreshed TTL.
assert_eq!(
book.expire(now + IDENTIFY_ADDRESS_TTL + Duration::from_secs(1)),
vec![(peer_id, unreported)]
);
assert!(book.expire(report_at + Duration::from_secs(1)).is_empty());
assert_eq!(
book.expire(report_at + IDENTIFY_ADDRESS_TTL + Duration::from_secs(1)),
vec![(peer_id, reported)]
);
}
#[test]
fn identify_addresses_reject_foreign_peer_suffixes() {
let peer_id = PeerId::random();
let other_peer_id = PeerId::random();
let address = identify_address(other_peer_id, 1, 10_000);
let mut book = IdentifyAddressBook::default();
assert!(book
.update(peer_id, Instant::now(), &[address])
.added
.is_empty());
}
#[test]
fn identify_addresses_canonicalize_the_peer_suffix_once() {
let peer_id = PeerId::random();
let raw = identify_address_without_peer(1, 10_000);
let canonical = identify_address(peer_id, 1, 10_000);
let mut book = IdentifyAddressBook::default();
assert_eq!(
book.update(peer_id, Instant::now(), &[raw]).added,
vec![canonical]
);
}
#[test]
fn dir_mode_replace_authority_is_the_write_bits() {
assert!(!dir_mode_allows_untrusted_replace(0o755));
+339
View File
@@ -1036,6 +1036,345 @@ mod tests {
);
}
/// #496: create_task must reject a caller-supplied repo_id that names a
/// hosted repo the caller does not own. A stranger naming the victim's repo
/// gets 403, leaking nothing about the repo and persisting nothing; the
/// owner files (201). Repo-less tasks stay allowed; an id naming no hosted
/// repo (unknown, or quarantined, hidden as if it did not exist) is stored
/// verbatim as an opaque label, and the 403-vs-201 split between a hosted
/// id and an unknown one is itself a one-bit oracle. This is a write gate:
/// task reads are caller-independent and not repo-scoped on this base.
#[sqlx::test]
async fn create_task_rejects_foreign_repo_id(pool: PgPool) {
let owner = "did:key:zTASKREPOOWNERAAAAAAAAAAAAAAAAAAAAAAAAAA";
let stranger = "did:key:zTASKREPOSTRANGERBBBBBBBBBBBBBBBBBBBBBB";
let state = test_state(pool).await;
let repo = seed_repo(owner, "task-gate-repo");
state.db.create_repo(&repo).await.expect("seed repo");
let router = || {
Router::new()
.route(
"/api/v1/tasks",
axum::routing::post(crate::api::tasks::create_task),
)
.with_state(state.clone())
};
let post_as = |signer: &str, body: String| {
router().oneshot(signed_request_as(
signer,
Method::POST,
"/api/v1/tasks",
Body::from(body),
))
};
let body_with = |signer: &str, repo_id: Option<&str>| {
let repo_field = repo_id
.map(|id| format!(r#","repo_id":"{id}""#))
.unwrap_or_default();
format!(
r#"{{"kind":"build","capability":"repo:write","delegator_did":"{signer}"{repo_field}}}"#
)
};
let created_id = |bytes: &[u8]| -> String {
let v: serde_json::Value = serde_json::from_slice(bytes).expect("task json");
v["id"].as_str().expect("task id").to_string()
};
// Stranger filing under the victim's repo id → exact 403.
let resp = post_as(stranger, body_with(stranger, Some(&repo.id)))
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::FORBIDDEN,
"a non-owner must not file tasks against a foreign repo id"
);
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
.await
.unwrap();
let text = String::from_utf8_lossy(&bytes);
assert!(
text.contains(r#""error":"forbidden""#),
"denial must keep the forbidden envelope: {text}"
);
assert!(
text.contains(crate::api::REPO_OWNER_ONLY_MESSAGE),
"denial must carry the shared owner-denial text (same as GraphQL): {text}"
);
// The denial must not persist: no task row may carry the victim's
// repo id afterwards — a 403 that still writes is the injection.
let stored = state
.db
.list_tasks(None, None, 50)
.await
.expect("list tasks");
assert!(
stored
.iter()
.all(|t| t.repo_id.as_deref() != Some(repo.id.as_str())),
"a denied write must not leave a task under the foreign repo id"
);
// Owner filing under their own repo id → 201.
let resp = post_as(owner, body_with(owner, Some(&repo.id)))
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::CREATED,
"the owner must be able to file tasks against their repo"
);
// Repo-less task → 201 (unaffected).
let resp = post_as(stranger, body_with(stranger, None)).await.unwrap();
assert_eq!(
resp.status(),
StatusCode::CREATED,
"repo-less task creation must keep working"
);
// Unknown repo id → 201 as an opaque label.
let resp = post_as(stranger, body_with(stranger, Some("no-such-repo-id")))
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::CREATED,
"an id naming no hosted repo stays an opaque label"
);
// Read back the row: the unresolved id must be stored verbatim, not
// dropped or rewritten to a repo it never named.
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
.await
.unwrap();
let task = state
.db
.get_task(&created_id(&bytes))
.await
.expect("get task")
.expect("task must be stored");
assert_eq!(
task.repo_id.as_deref(),
Some("no-such-repo-id"),
"an unresolved id must be stored verbatim as a label"
);
// Quarantined repo id → treated as unknown (201), never a 403 that
// would confirm the id names a real repo.
state
.db
.set_repo_quarantine(&repo.id, true)
.await
.expect("quarantine");
let resp = post_as(stranger, body_with(stranger, Some(&repo.id)))
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::CREATED,
"a quarantined repo id must not 403 (existence oracle)"
);
// Read back the row: the quarantined id is stored verbatim as a label
// too, so the accepted write is not silently dropped.
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
.await
.unwrap();
let task = state
.db
.get_task(&created_id(&bytes))
.await
.expect("get task")
.expect("task must be stored");
assert_eq!(
task.repo_id.as_deref(),
Some(repo.id.as_str()),
"a quarantined id must be stored verbatim as a label"
);
}
/// #496 (REST): a repo-lookup fault in the create_task gate must render the
/// module's opaque `db_error` envelope, not the raw sqlx text. Dropping a
/// column `get_repo_by_id` selects faults that first lookup; the 500 body
/// stays fixed and names no column (#250/#226).
#[sqlx::test]
async fn create_task_gate_repo_lookup_fault_is_opaque(pool: PgPool) {
let owner = "did:key:zTASKFAULTOWNERAAAAAAAAAAAAAAAAAAAAAAAAA";
let state = test_state(pool.clone()).await;
let repo = seed_repo(owner, "task-gate-fault-repo");
state.db.create_repo(&repo).await.expect("seed repo");
let router = || {
Router::new()
.route(
"/api/v1/tasks",
axum::routing::post(crate::api::tasks::create_task),
)
.with_state(state.clone())
};
let body = format!(
r#"{{"kind":"build","capability":"repo:write","delegator_did":"{owner}","repo_id":"{}"}}"#,
repo.id
);
// Drop a column the lookup's SELECT names so get_repo_by_id faults.
sqlx::query("ALTER TABLE repos DROP COLUMN machine_id")
.execute(&pool)
.await
.expect("drop machine_id");
let resp = router()
.oneshot(signed_request_as(
owner,
Method::POST,
"/api/v1/tasks",
Body::from(body),
))
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::INTERNAL_SERVER_ERROR,
"a faulted repo lookup must be a 500"
);
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
.await
.unwrap();
let text = String::from_utf8_lossy(&bytes);
assert!(
text.contains(r#""error":"db_error""#),
"gate DB fault must keep the opaque envelope: {text}"
);
assert!(
text.contains(crate::error::DB_ERROR_MESSAGE),
"gate DB fault must carry the fixed message: {text}"
);
assert!(
!text.contains("machine_id") && !text.contains("column"),
"schema text leaked through the gate: {text}"
);
}
/// #496 (REST): the gate's second lookup (`is_repo_quarantined`) runs through
/// the same opaque helper. Dropping the column only it selects leaves
/// `get_repo_by_id` succeeding, so this pins that second call site too.
#[sqlx::test]
async fn create_task_gate_quarantine_lookup_fault_is_opaque(pool: PgPool) {
let owner = "did:key:zTASKFAULT2OWNERAAAAAAAAAAAAAAAAAAAAAAAA";
let state = test_state(pool.clone()).await;
let repo = seed_repo(owner, "task-gate-fault2-repo");
state.db.create_repo(&repo).await.expect("seed repo");
let router = || {
Router::new()
.route(
"/api/v1/tasks",
axum::routing::post(crate::api::tasks::create_task),
)
.with_state(state.clone())
};
let body = format!(
r#"{{"kind":"build","capability":"repo:write","delegator_did":"{owner}","repo_id":"{}"}}"#,
repo.id
);
// `quarantined` is not in get_repo_by_id's SELECT, so that lookup still
// succeeds and the fault lands on is_repo_quarantined.
sqlx::query("ALTER TABLE repos DROP COLUMN quarantined")
.execute(&pool)
.await
.expect("drop quarantined");
let resp = router()
.oneshot(signed_request_as(
owner,
Method::POST,
"/api/v1/tasks",
Body::from(body),
))
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::INTERNAL_SERVER_ERROR,
"a faulted quarantine lookup must be a 500"
);
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
.await
.unwrap();
let text = String::from_utf8_lossy(&bytes);
assert!(
text.contains(r#""error":"db_error""#),
"quarantine DB fault must keep the opaque envelope: {text}"
);
assert!(
text.contains(crate::error::DB_ERROR_MESSAGE),
"quarantine DB fault must carry the fixed message: {text}"
);
assert!(
!text.contains("quarantined") && !text.contains("column"),
"schema text leaked through the gate: {text}"
);
}
/// #496 (REST): the create_task INSERT goes through the same opaque helper,
/// so a write fault must not serialize sqlx text either. A repo-less task
/// reaches the INSERT without a gate lookup; dropping a column the INSERT
/// names faults it.
#[sqlx::test]
async fn create_task_insert_fault_is_opaque(pool: PgPool) {
let signer = "did:key:zTASKINSERTFAULTAAAAAAAAAAAAAAAAAAAAAAAA";
let state = test_state(pool.clone()).await;
let router = || {
Router::new()
.route(
"/api/v1/tasks",
axum::routing::post(crate::api::tasks::create_task),
)
.with_state(state.clone())
};
let body =
format!(r#"{{"kind":"build","capability":"repo:write","delegator_did":"{signer}"}}"#);
sqlx::query("ALTER TABLE agent_tasks DROP COLUMN status")
.execute(&pool)
.await
.expect("drop status");
let resp = router()
.oneshot(signed_request_as(
signer,
Method::POST,
"/api/v1/tasks",
Body::from(body),
))
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::INTERNAL_SERVER_ERROR,
"a faulted INSERT must be a 500"
);
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
.await
.unwrap();
let text = String::from_utf8_lossy(&bytes);
assert!(
text.contains(r#""error":"db_error""#),
"INSERT fault must keep the opaque envelope: {text}"
);
assert!(
text.contains(crate::error::DB_ERROR_MESSAGE),
"INSERT fault must carry the fixed message: {text}"
);
assert!(
!text.contains("status") && !text.contains("column"),
"schema text leaked through the INSERT: {text}"
);
}
/// N3: get_tree gates on the REQUESTED subtree, not the repo root. A caller
/// denied a withheld subtree is rejected there (404) but passes the gate on a
/// non-withheld path (so the rejection is path-scoped, not repo-wide).