fix(git): address review blockers — --channel on create, Broken denies before owner short-circuit

Blocker 1: ReposCmd::Create gains --channel; build_create_announcement is extracted pure so the emitted kind:30617 event is unit-testable. Exactly one shape-validated buzz-channel tag is appended; existence/membership stays the relay's authority (same TOCTOU posture as repos bind). Blocker 2: hook_policy_check no longer collapses Broken|NotBound. Broken now denies 403 "invalid channel binding" for everyone — including the announcement owner — before owner resolution, matching the read gate's fail-closed posture. The remediation token stays NotBound-only. New Postgres-gated test push_gate_denies_owner_through_broken_binding pins owner + malformed-first/valid-second → 403 generic body, with a never-bound control staying 200. Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
2026-08-18 06:50:31 +02:00 · 2026-07-29 18:14:24 -04:00
parent ede1065227
commit f914c70669
3 changed files with 290 additions and 13 deletions
@@ -214,21 +214,30 @@ async fn submit_repo_update(client: &BuzzClient, builder: EventBuilder) -> Resul
    Ok(())
 }

-pub async fn cmd_create_repo(
-    client: &BuzzClient,
+/// Build the kind:30617 announcement for `repos create`, including the
+/// `buzz-channel` binding when requested.
+///
+/// Pure (no I/O) so the emitted tags are unit-testable. Exactly one
+/// validated `buzz-channel` tag is appended — the tag is the git ACL
+/// (issue #3527: without it the relay 404s every clone/fetch/push), so the
+/// UUID is shape-validated here and its existence/membership is the relay's
+/// authority at git-access time, same posture as `repos bind`.
+#[allow(clippy::too_many_arguments)]
+fn build_create_announcement(
    repo_id: &str,
    name: Option<&str>,
    description: Option<&str>,
    clone_urls: &[String],
    web_url: Option<&str>,
    relays: &[String],
-) -> Result<(), CliError> {
+    channel: Option<&str>,
+) -> Result<EventBuilder, CliError> {
    validate_repo_id(repo_id)?;

    let clone_refs: Vec<&str> = clone_urls.iter().map(|s| s.as_str()).collect();
    let relay_refs: Vec<&str> = relays.iter().map(|s| s.as_str()).collect();

-    let builder = buzz_sdk::build_repo_announcement(
+    let mut builder = buzz_sdk::build_repo_announcement(
        repo_id,
        name,
        description,
@@ -238,6 +247,33 @@ pub async fn cmd_create_repo(
    )
    .map_err(|e| CliError::Other(format!("build_repo_announcement failed: {e}")))?;

+    if let Some(channel) = channel {
+        crate::validate::validate_uuid(channel)?;
+        builder = builder.tag(Tag::parse(["buzz-channel", channel]).map_err(tag_error)?);
+    }
+    Ok(builder)
+}
+
+#[allow(clippy::too_many_arguments)]
+pub async fn cmd_create_repo(
+    client: &BuzzClient,
+    repo_id: &str,
+    name: Option<&str>,
+    description: Option<&str>,
+    clone_urls: &[String],
+    web_url: Option<&str>,
+    relays: &[String],
+    channel: Option<&str>,
+) -> Result<(), CliError> {
+    let builder = build_create_announcement(
+        repo_id,
+        name,
+        description,
+        clone_urls,
+        web_url,
+        relays,
+        channel,
+    )?;
    let event = client.sign_event(builder)?;
    let resp = client.submit_event(event).await?;
    println!("{resp}");
@@ -391,6 +427,7 @@ pub async fn dispatch(cmd: crate::ReposCmd, client: &BuzzClient) -> Result<(), C
            clone_urls,
            web,
            relays,
+            channel,
        } => {
            cmd_create_repo(
                client,
@@ -400,6 +437,7 @@ pub async fn dispatch(cmd: crate::ReposCmd, client: &BuzzClient) -> Result<(), C
                &clone_urls,
                web.as_deref(),
                &relays,
+                channel.as_deref(),
            )
            .await
        }
@@ -439,8 +477,8 @@ mod tests {
    use nostr::{EventBuilder, Keys, Kind, Tag, Timestamp};

    use super::{
-        build_protection_tag, build_updated_repo_announcement, protection_rules_json,
-        validate_write_response, RepoChange,
+        build_create_announcement, build_protection_tag, build_updated_repo_announcement,
+        protection_rules_json, validate_write_response, RepoChange,
    };

    fn signed_repo(tags: Vec<Tag>, content: &str, created_at: u64) -> nostr::Event {
@@ -732,6 +770,64 @@ mod tests {
        assert!(matches!(error, crate::error::CliError::Usage(_)));
    }

+    /// Issue #3527: `repos create --channel` must emit exactly one
+    /// `buzz-channel` tag so the primary create command stops producing
+    /// repos the relay 404s forever.
+    #[test]
+    fn create_with_channel_emits_exactly_one_binding_tag() {
+        let channel = uuid::Uuid::new_v4().to_string();
+        let event = build_create_announcement(
+            "demo",
+            Some("Demo"),
+            None,
+            &["https://relay.example/git/owner/demo".to_string()],
+            None,
+            &[],
+            Some(&channel),
+        )
+        .expect("build create announcement")
+        .sign_with_keys(&Keys::generate())
+        .expect("sign create announcement");
+
+        assert_eq!(event.kind, Kind::Custom(30617));
+        let bindings: Vec<_> = event
+            .tags
+            .iter()
+            .filter(|tag| tag.as_slice().first().map(String::as_str) == Some("buzz-channel"))
+            .collect();
+        assert_eq!(bindings.len(), 1, "exactly one buzz-channel tag");
+        assert_eq!(bindings[0].as_slice(), ["buzz-channel", channel.as_str()]);
+        // The standard metadata still rides along.
+        assert!(event.tags.iter().any(|tag| tag.as_slice() == ["d", "demo"]));
+        assert!(event
+            .tags
+            .iter()
+            .any(|tag| tag.as_slice() == ["name", "Demo"]));
+    }
+
+    #[test]
+    fn create_without_channel_emits_no_binding_tag() {
+        let event = build_create_announcement("demo", None, None, &[], None, &[], None)
+            .expect("build create announcement")
+            .sign_with_keys(&Keys::generate())
+            .expect("sign create announcement");
+
+        assert!(
+            !event
+                .tags
+                .iter()
+                .any(|tag| tag.as_slice().first().map(String::as_str) == Some("buzz-channel")),
+            "no --channel means no binding tag (vanilla NIP-34 stays possible)"
+        );
+    }
+
+    #[test]
+    fn create_rejects_malformed_channel_uuid() {
+        let error = build_create_announcement("demo", None, None, &[], None, &[], Some("nope"))
+            .expect_err("malformed channel id must not build an announcement");
+        assert!(matches!(error, crate::error::CliError::Usage(_)));
+    }
+
    #[test]
    fn duplicate_write_response_is_a_conflict() {
        let error = validate_write_response(
@@ -1129,6 +1129,11 @@ pub enum ReposCmd {
        /// Preferred Nostr relay(s) for repo discovery — can be specified multiple times
        #[arg(long = "nostr-relay")]
        relays: Vec<String>,
+        /// Channel UUID to bind the repo to. The `buzz-channel` tag is the
+        /// git ACL: without it the relay 404s every clone/fetch/push until
+        /// the author runs `buzz repos bind` (issue #3527).
+        #[arg(long)]
+        channel: Option<String>,
    },
    /// Get a repository announcement
    Get {
@@ -302,15 +302,26 @@ pub async fn hook_policy_check(

    // 6. Resolve channel binding via the shared resolver (same first-tag,
    // fail-closed semantics as the read gate) and check archived state
-    // (applies to ALL pushers including owner). On the push side, NotBound
-    // and Broken collapse into the same bucket — both historically produced
-    // the "no channel binding" denial for non-owners, and a pusher is past
-    // NIP-98 auth so the body leaks nothing new. Only the read gate
-    // distinguishes them (author remediation is NotBound-only).
+    // (applies to ALL pushers including owner).
+    //
+    // `Broken` denies HERE, before owner resolution: a malformed or
+    // ambiguous first binding fails closed for *everyone*, exactly like the
+    // read gate. Letting it fall through as "unbound" would hand the owner
+    // short-circuit below a push path through a binding the read gate
+    // refuses to honor — the tri-state exists precisely so Broken and
+    // NotBound cannot collapse. Only genuinely-NotBound repos proceed, and
+    // only they may earn the remediation-token denial.
    let channel_id = match crate::api::git::binding::resolve_repo_binding(&repo_event.event) {
        crate::api::git::binding::RepoBinding::Bound(id) => Some(id),
-        crate::api::git::binding::RepoBinding::NotBound
-        | crate::api::git::binding::RepoBinding::Broken => None,
+        crate::api::git::binding::RepoBinding::NotBound => None,
+        crate::api::git::binding::RepoBinding::Broken => {
+            warn!(repo = %req.repo_id, "hook callback: broken buzz-channel binding");
+            // Deliberately NOT the no_channel_binding token body: the
+            // remediation contract is NotBound-only. A broken binding is
+            // ambiguity, and ambiguity gets a generic denial (matching the
+            // read gate's posture for the same announcement).
+            return (StatusCode::FORBIDDEN, "invalid channel binding").into_response();
+        }
    };

    if let Some(ch_id) = channel_id {
@@ -808,4 +819,169 @@ printf '%s' "$HMAC_INPUT" | openssl dgst -sha256 -hmac "{secret}" -hex 2>/dev/nu
            "Single-ref HMAC mismatch!\n  Rust: {rust_sig}\n  Bash: {bash_sig}"
        );
    }
+
+    // ── hook_policy_check binding gate (requires Postgres) ──────────────
+
+    const TEST_DB_URL: &str = "postgres://buzz:buzz_dev@localhost:5432/buzz"; // sadscan:disable np.postgres.1
+
+    async fn policy_test_state() -> Arc<AppState> {
+        let mut config = crate::config::Config::from_env().expect("default config loads");
+        config.require_relay_membership = false;
+        config.redis_url = "redis://127.0.0.1:1".to_string();
+        config.database_url = std::env::var("BUZZ_TEST_DATABASE_URL")
+            .or_else(|_| std::env::var("DATABASE_URL"))
+            .unwrap_or_else(|_| TEST_DB_URL.to_string());
+        let pool = sqlx::PgPool::connect(&config.database_url)
+            .await
+            .expect("connect test DB");
+        let db = buzz_db::Db::from_pool(pool.clone());
+        let redis_pool = deadpool_redis::Config::from_url(&config.redis_url)
+            .create_pool(Some(deadpool_redis::Runtime::Tokio1))
+            .expect("redis pool");
+        let pubsub = Arc::new(
+            buzz_pubsub::PubSubManager::new(&config.redis_url, redis_pool.clone())
+                .await
+                .expect("pubsub manager"),
+        );
+        let audit = buzz_audit::AuditService::new(pool.clone());
+        let auth = buzz_auth::AuthService::new(config.auth.clone());
+        let search = buzz_search::SearchService::new(pool.clone());
+        let workflow_engine = Arc::new(buzz_workflow::WorkflowEngine::new(
+            db.clone(),
+            buzz_workflow::WorkflowConfig::default(),
+        ));
+        let media_storage = buzz_media::MediaStorage::new(&config.media).expect("media storage");
+        let (state, _audit_shutdown) = AppState::new(
+            config,
+            db,
+            redis_pool,
+            audit,
+            pubsub,
+            auth,
+            search,
+            workflow_engine,
+            nostr::Keys::generate(),
+            media_storage,
+        );
+        Arc::new(state)
+    }
+
+    /// Announce `repo_id` with the given tags, then push to it as its own
+    /// announcement author and return the response.
+    async fn owner_push_response(
+        state: &Arc<AppState>,
+        community: buzz_core::CommunityId,
+        keys: &nostr::Keys,
+        repo_id: &str,
+        binding_tags: Vec<nostr::Tag>,
+    ) -> axum::response::Response {
+        use nostr::{EventBuilder, Kind, Tag};
+
+        let mut tags = vec![Tag::parse(["d", repo_id]).unwrap()];
+        tags.extend(binding_tags);
+        let event = EventBuilder::new(Kind::Custom(30617), "")
+            .tags(tags)
+            .sign_with_keys(keys)
+            .expect("sign 30617");
+        state
+            .db
+            .insert_event(community, &event, None)
+            .await
+            .expect("insert 30617");
+
+        let owner_hex = keys.public_key().to_hex();
+        let mut req = HookCallbackRequest {
+            repo_id: repo_id.to_string(),
+            repo_owner: owner_hex.clone(),
+            community_id: community.as_uuid().to_string(),
+            pusher_pubkey: owner_hex,
+            ref_updates: vec![HookRefUpdate {
+                old_oid: "0".repeat(40),
+                new_oid: "2".repeat(40),
+                ref_name: "refs/heads/main".to_string(),
+                is_ancestor: false,
+            }],
+            timestamp: SystemTime::now()
+                .duration_since(UNIX_EPOCH)
+                .unwrap()
+                .as_secs(),
+            signature: String::new(),
+        };
+        let secret = state.config.git_hook_hmac_secret.clone();
+        sign_request(&mut req, secret.as_bytes());
+        hook_policy_check(State(Arc::clone(state)), Json(req)).await
+    }
+
+    async fn body_string(response: axum::response::Response) -> (StatusCode, String) {
+        let status = response.status();
+        let bytes = axum::body::to_bytes(response.into_body(), usize::MAX)
+            .await
+            .expect("read body");
+        (status, String::from_utf8(bytes.to_vec()).expect("utf-8"))
+    }
+
+    /// The tri-state trap the resolver exists to prevent: a broken (malformed
+    /// or ambiguous-first) binding must fail closed for EVERYONE on push —
+    /// including the announcement author — *before* the owner short-circuit
+    /// grants `MemberRole::Owner`. Collapsing `Broken` into "unbound" hands
+    /// the owner a push path through a binding the read gate refuses to
+    /// honor. The remediation token stays reserved for genuinely NotBound.
+    #[tokio::test]
+    #[ignore = "requires Postgres"]
+    async fn push_gate_denies_owner_through_broken_binding() {
+        use nostr::{Keys, Tag};
+
+        let state = policy_test_state().await;
+        let host = format!("policy-{}.example", uuid::Uuid::new_v4().simple());
+        let community = state
+            .db
+            .ensure_configured_community(&host)
+            .await
+            .expect("community")
+            .id;
+        let keys = Keys::generate();
+
+        // Malformed first + valid-looking second: the ambiguity must deny,
+        // and the parseable duplicate must not rescue the push.
+        let response = owner_push_response(
+            &state,
+            community,
+            &keys,
+            &format!("repo-{}", uuid::Uuid::new_v4().simple()),
+            vec![
+                Tag::parse(["buzz-channel", "not-a-uuid"]).unwrap(),
+                Tag::parse(["buzz-channel", &uuid::Uuid::new_v4().to_string()]).unwrap(),
+            ],
+        )
+        .await;
+        let (status, body) = body_string(response).await;
+        assert_eq!(status, StatusCode::FORBIDDEN);
+        assert_eq!(
+            body, "invalid channel binding",
+            "owner pushing through a broken binding must be denied generically"
+        );
+        assert!(
+            !body.contains(buzz_core::git_perms::GIT_NO_CHANNEL_BINDING_TOKEN),
+            "remediation token is NotBound-only; Broken must never earn it"
+        );
+
+        // Control: the same owner pushing a genuinely NEVER-BOUND repo is
+        // allowed (owner authority over an unbound announcement is the
+        // long-standing push semantics). This pins the denial above to
+        // Broken specifically, not to some broader regression.
+        let response = owner_push_response(
+            &state,
+            community,
+            &keys,
+            &format!("repo-{}", uuid::Uuid::new_v4().simple()),
+            vec![],
+        )
+        .await;
+        let (status, body) = body_string(response).await;
+        assert_eq!(
+            status,
+            StatusCode::OK,
+            "owner push to a never-bound repo must remain allowed (got body: {body})"
+        );
+    }
 }