Repository navigation
Conversation
🔐 Codex Security Review
|
salman1993
left a comment
There was a problem hiding this comment.
🤖 Major — query-bearing relay URLs still produce requests to the wrong endpoint (crates/buzz-cli/src/client.rs:1371)
The new normalizer now returns bases such as https://relay.example?token=abc, but every HTTP operation still appends its route with string formatting (for example, line 795 builds format!("{}/query", self.relay_url)). That yields https://relay.example?token=abc/query: /query is part of the query value and the request path remains /. A non-root input such as wss://relay.example/nostr/?token=abc similarly produces a request to /nostr/ with token=abc/query, not /nostr/query?token=abc. The ACP RestClient has the same composition pattern (relay.rs:292,460).
As a result, the exact query-bearing configurations this PR intends to support still cannot use the REST bridge: CLI commands and ACP context fetches hit the wrong route (typically 404 or the NIP-11 root) despite preserving the query text. Please compose endpoint paths structurally with Url while retaining the original query, and add a production-seam request test that asserts both the received path and query. The current normalization-only tests pass while missing this failure.
Validation at 8a5e8c1d21de0971a4598dd1542ea8a099f5c0ce: full cargo test -p buzz-core (256 unit + 2 doc), cargo test -p buzz-cli (464 unit; 1 ignored doc), and cargo test -p buzz-acp (916 unit + 9 integration) passed. FastValidator independently ran the three focused package/seam suites at the same SHA and confirmed query bytes survive normalization; that lane did not exercise request routing. No live relay run was performed.
@salman1993 Thanks. Modified. Please review it again. |
Normalize relay URL components without modifying query values. Compose CLI and ACP HTTP endpoint paths with Url, preserving base paths and raw relay queries while appending endpoint query parameters. Exercise production HTTP clients against local servers and assert received paths, raw queries, and NIP-98 signed URLs. Cover bridge reads and writes, metadata, uploads and their legacy fallback, and media downloads. Signed-off-by: cuishuang <imcusg@gmail.com>
|
Thanks for catching this, @salman1993. I’ve updated the PR and rebased it onto main. The CLI and ACP REST clients now append endpoint paths using Url while preserving the original relay query. The new regression tests send requests to a local HTTP server and verify that it receives /query or /nostr/query, depending on the configured base path, with the original query intact. Could you take another look at 2740197 when you have a chance? The PR workflows are also awaiting maintainer approval. Could someone with write access approve the runs and authorize the security review by posting @buzz-security-review 2740197 as a separate comment? |
Summary
Relay URL normalization could trim query values ending in
/or rewrite nested URL schemes. Appending HTTP routes as strings also put routes inside query values:https://relay.example?token=abc/queryrequested/instead of/query?token=abc.url::Urland preserve query strings during normalization and scheme conversion.Rebased onto upstream
mainat96af488d968a1ae9d961c2132bea27822735f8da; the PR remains one signed-off commit.Validation
cargo test -p buzz-core -p buzz-cli -p buzz-acp— 2087 passed, 4 ignored, no failures (unit, integration, and doc tests).cargo clippy -p buzz-core -p buzz-cli -p buzz-acp --all-targets -- -D warnings— passed.cargo fmt --all -- --check— passed.CHECK_FILE_SIZES_BASE=96af488d968a1ae9d961c2132bea27822735f8da just file-size-check— passed.buzz channels listagainst a local HTTP fixture with root and/nostr/relay bases. Verified the server received/query?...and/nostr/query?..., retained the raw query (escaping, duplicate keys,+, nested URL, trailing slash), and received matching signed URLs.The repository-wide
just ciattempt passed formatting but was stopped before workspace compilation after dependency downloads reduced free disk space below 4 GiB. The three affected packages were tested and linted separately as listed above. No live external relay was used.