search_podcasts built its URL with format!("…byterm?q={query}"), putting the
user's raw text where a URL has its own syntax. A search for "AT&T" arrived as
q=AT with a stray T parameter after it; anything past a # arrived truncated,
because a fragment is never sent at all. Spaces survived only because
Url::parse happens to percent-encode them.
RequestBuilder::query does the encoding, which meant turning on reqwest's
"query" feature — free, since its encoder is already in the tree via other
crates.
The sibling URL a few lines down interpolates a feed id and a max count, both
i64, and is left alone: a number has no characters to escape.
Five cases pinned against the built request rather than a sent one — this is
about how the URL is assembled, and the test should not touch the network.
This commit is contained in:
@@ -415,19 +415,6 @@ since it isn't worth taking before it shows up in a profile.
|
||||
`DocumentsScreen.kt`/`PdfLibraryScreen.kt`/`ComicLibraryScreen.kt` — a separate,
|
||||
lower-stakes question the ADR didn't resolve.
|
||||
|
||||
### The podcast search query is not URL-encoded
|
||||
- **Description**: `agents::podcast::search_podcasts` builds
|
||||
`format!("{BASE_URL}search/byterm?q={query}")` with the user's raw query. A search
|
||||
containing `&`, `#` or `+` is either truncated or misparsed by the time it reaches
|
||||
Podcast Index — "AT&T" searches for "AT". Spaces survive only because `Url::parse`
|
||||
percent-encodes them.
|
||||
- **Fix**: build the query with a form encoder rather than `format!`. Small, and the
|
||||
same class as the `just lint-sql-params` rule — a value interpolated into a string
|
||||
that has its own syntax.
|
||||
- **Found via the copy audit**, while checking whether Android's "name or URL"
|
||||
placeholder reflected a real capability. It did not, and the placeholder is fixed;
|
||||
this is the bug next to it.
|
||||
|
||||
### Look into Android Auto support
|
||||
- See how hard this would be. The lock-screen `MediaSessionService` foundation
|
||||
(`docs/adr/0007-*`) is in place; still missing on top of it: playback resumption after
|
||||
|
||||
Generated
+1
@@ -9056,6 +9056,7 @@ dependencies = [
|
||||
"rustls-platform-verifier",
|
||||
"serde",
|
||||
"serde_json",
|
||||
"serde_urlencoded",
|
||||
"sync_wrapper",
|
||||
"tokio",
|
||||
"tokio-rustls",
|
||||
|
||||
+9
-1
@@ -137,7 +137,15 @@ fontdb = "0.23"
|
||||
base64 = "0.22"
|
||||
chrono = { version = "0.4", features = ["serde"] }
|
||||
chrono-tz = { version = "0.10", features = ["serde"] }
|
||||
reqwest = { version = "0.13", features = ["json", "rustls", "blocking"], default-features = false }
|
||||
# "query" is what `RequestBuilder::query` lives behind, and it is the only
|
||||
# correct way to put a user's text into a URL — see `agents::podcast`. Its
|
||||
# encoder (`serde_urlencoded`) is already in the tree via other crates.
|
||||
reqwest = { version = "0.13", features = [
|
||||
"json",
|
||||
"rustls",
|
||||
"blocking",
|
||||
"query",
|
||||
], default-features = false }
|
||||
android_logger = "0.15"
|
||||
log = "0.4"
|
||||
env_logger = "0.11"
|
||||
|
||||
@@ -595,8 +595,7 @@ fn search_podcasts_inner(query: &str) -> Result<String, Box<dyn Error>> {
|
||||
hasher.update(data_to_hash.as_bytes());
|
||||
let auth_hash = hex::encode(hasher.finalize().as_slice());
|
||||
|
||||
// Build the request URL
|
||||
let url = format!("{BASE_URL}search/byterm?q={query}");
|
||||
let url = format!("{BASE_URL}search/byterm");
|
||||
|
||||
log::info!(
|
||||
"About to make the request to url {url} using async reqwest client on a small Tokio runtime"
|
||||
@@ -610,6 +609,14 @@ fn search_podcasts_inner(query: &str) -> Result<String, Box<dyn Error>> {
|
||||
let body: String = rt.block_on(async {
|
||||
let response = client
|
||||
.post(&url)
|
||||
// `query` and not `format!("…?q={query}")`: the term is the user's
|
||||
// and carries no obligation to avoid the characters a URL reserves.
|
||||
// Interpolated, a search for "AT&T" arrived at the API as `q=AT`
|
||||
// with a stray `T` parameter after it, and one containing `#`
|
||||
// arrived truncated at the `#` — the rest read as a fragment, which
|
||||
// is never sent. Spaces survived only because `Url::parse` happens
|
||||
// to percent-encode them.
|
||||
.query(&[("q", query)])
|
||||
.header("X-Auth-Date", epoch_time.to_string())
|
||||
.header("X-Auth-Key", PODCAST_INDEX_API_KEY)
|
||||
.header("Authorization", auth_hash)
|
||||
@@ -782,6 +789,37 @@ mod tests {
|
||||
|
||||
use super::*;
|
||||
|
||||
/// The search term is the user's and carries no obligation to avoid the
|
||||
/// characters a URL reserves. Built with `format!`, "AT&T" reached the API
|
||||
/// as `q=AT` with a stray `T` parameter, and anything after a `#` was
|
||||
/// dropped entirely — a fragment is never sent.
|
||||
///
|
||||
/// Asserted on the built request rather than a sent one: this is about how
|
||||
/// the URL is assembled, and nothing here should touch the network.
|
||||
#[rstest]
|
||||
#[case("AT&T", "q=AT%26T")]
|
||||
#[case("c#", "q=c%23")]
|
||||
#[case("rock + roll", "q=rock+%2B+roll")]
|
||||
#[case("99% invisible", "q=99%25+invisible")]
|
||||
#[case("hello world", "q=hello+world")]
|
||||
fn a_search_term_reaches_the_api_whole(#[case] term: &str, #[case] expected_query: &str) {
|
||||
let request = reqwest::Client::new()
|
||||
.post(format!("{BASE_URL}search/byterm"))
|
||||
.query(&[("q", term)])
|
||||
.build()
|
||||
.expect("the request builds");
|
||||
|
||||
assert_eq!(request.url().query(), Some(expected_query));
|
||||
// And round-trips: whatever the encoding, the API sees what was typed.
|
||||
let decoded: String = request
|
||||
.url()
|
||||
.query_pairs()
|
||||
.find(|(k, _)| k == "q")
|
||||
.map(|(_, v)| v.into_owned())
|
||||
.expect("the q parameter survives");
|
||||
assert_eq!(decoded, term);
|
||||
}
|
||||
|
||||
#[rstest]
|
||||
fn list_podcast_feeds_projects_stored_properties(#[from(initialized_store)] store: TestStore) {
|
||||
let node = test_node();
|
||||
|
||||
Reference in New Issue
Block a user