fix: keep the API key on the origin it was configured for

[grokspice]

Summary

The async query client attaches the API key as an X-API-Key header. On a redirect that leaves the origin, reqwest strips only the standard credential headers, so a custom header rides along to whatever the Location names — and a 307/308 preserves the method and the body, so the entire credentialed POST is replayed to the new origin.

Verified against the pinned major rather than assumed. reqwest v0.12.24, src/redirect.rs:

let cross_host = next.host_str() != previous.host_str()
    || next.port_or_known_default() != previous.port_or_known_default();
if cross_host {
    headers.remove(AUTHORIZATION);
    headers.remove(COOKIE);
    headers.remove("cookie2");
    headers.remove(PROXY_AUTHORIZATION);
    headers.remove(WWW_AUTHENTICATE);
}

X-API-Key is not in that set, so nothing stripped it. This is the SDK half of spiceai/spiceai#12502: the CLI set a same-origin policy on its own client for spiceai/spiceai#12495, but spice query and spice nsql analyze build this crate's client instead, and the policy cannot be set from the CLI — the SDK pins reqwest 0.12 while the workspace is on 0.13, so the two Client types are not interchangeable and ClientBuilder exposes no redirect setter. The crate that attaches the credential is the one that has to own the policy.

Changes

• New src/redirect.rs — credentialed_client_builder(), a reqwest::ClientBuilder carrying a same-origin redirect policy.
• Applied at both construction sites — SpiceClientBuilder::build (the production path) and QueryHttpClient::new. Both now come from the one helper, so the policy cannot be set on one path and missed on another.

Design notes worth a reviewer's eye

The hop limit is enforced by hand, on purpose. Policy::custom does not inherit the default limit — reqwest's own docs say "The default Policy handles a maximum loop chain, but the custom variant does not do that for you automatically." Dropping it would have turned a same-origin redirect loop into an infinite one, so the policy applies the bound itself, compared the way Policy::limited compares it (previous() includes the originating URL, so the bound is exclusive).

Origin means scheme, host and effective port — deliberately stricter than the cross_host check quoted above, which never looks at scheme. An https://host:8443 → http://host:8443 downgrade keeps the host and port identical, so reqwest would not call it cross-host, but it is exactly where a credential must not go. There is a test for that specific case.

It stops rather than errors. The callers in query.rs already report an unexpected status with its code, so a refused redirect surfaces as Failed to submit query (HTTP 307) instead of an opaque transport error.

Secure by default, with no opt-out. This is a behaviour change: a deployment that today relies on the SDK following an off-origin redirect will now get a 307 back instead. That configuration is precisely the one leaking the key, so it fails closed rather than continuing to disclose the credential — and it matches the choice already made for the CLI client in spiceai/spiceai#12495.

Test plan

cargo fmt --all --check, cargo clippy --all-features and cargo test (what CI runs).

• tests/redirect_test.rs::test_cross_origin_redirect_does_not_carry_the_api_key — the regression test, driven through the public production path (ClientBuilder::build). Two mock servers: the runtime answers the credentialed POST with a 307 pointing at a second origin, which is mounted to accept it. Asserts the second origin received zero requests, and that the refused hop surfaces as HTTP 307. On the old code the redirect is followed and the off-origin server receives the POST with X-API-Key, so this fails without the fix.
• tests/redirect_test.rs::test_same_origin_redirect_is_still_followed — guards against over-correcting: a 307 to a different path on the same origin is still followed, the body is replayed, and the submit succeeds.
• src/redirect.rs unit tests — origin equality across implicit/explicit default ports, differing host, differing port, scheme downgrade on an identical port, and path/query/fragment being irrelevant.

Notes

I could not build or run these locally (no Rust toolchain on the machine that wrote the patch), so CI is the first real execution — every external API used here was instead checked against the pinned sources: reqwest v0.12.24 for Attempt/Policy/Action::Stop semantics, and wiremock v0.6.5 for insert_header, set_body_json and received_requests. Flagging it rather than implying a green local run. The adversarial review pass I would normally run on the diff could not execute either (the review engine's workspace is out of credits).

Follow-up, not in this PR: once this lands and the spiceai rev is bumped in spiceai/spiceai, spice query and spice nsql analyze inherit the policy and spiceai/spiceai#12502 can close. Whether the CLI should build SDK clients per command at all — versus routing both through the shared context client — is the broader question that issue also raises, and is left open.

Refs: spiceai/spiceai#12502

[Copilot]

Pull request overview

This PR hardens the Rust SDK’s async query HTTP client against credential leakage by enforcing a same-origin redirect policy, preventing X-API-Key from being replayed to a different origin on 307/308 redirects.

Changes:

• Added a shared reqwest::ClientBuilder helper configured with a same-origin redirect policy.
• Applied that helper to both HTTP-client construction sites to ensure consistent redirect behavior.
• Added regression/invariant tests covering cross-origin and same-origin redirects, plus origin comparison edge cases (scheme/port).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/redirect_test.rs	New integration regression tests verifying API key is not sent off-origin and same-origin redirects still work.
src/redirect.rs	New redirect policy + shared client builder enforcing same-origin redirects with a hop limit.
src/query.rs	Updates QueryHttpClient::new to build from the shared credentialed client builder.
src/lib.rs	Wires in the new redirect module.
src/client.rs	Uses the shared credentialed client builder in SpiceClientBuilder::build HTTP client construction.

[grokspice]

Filed #86 for the convention-vs-CI gap this review surfaced: the four clippy lints .github/copilot-instructions.md documents as "Enforced in CI" are not enforced by the workflow, by Cargo.toml, or by any crate attribute — only by a developer's local rust-analyzer settings. Out of scope here; this PR just complies with the convention.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.