Repository navigation
fix(ads-client): panic on a malformed OHTTP preflight response - #7578
Merged
Merged
Conversation
Almaju
marked this pull request as ready for review
September 3, 2026 03:19
Almaju
force-pushed
the
ads-client-ordering-pass
branch
from
September 21, 2026 23:33
b9d71e2 to
e9dfadc
Compare
Almaju
changed the title
…esponse
`From<PreflightResponse> for Headers` used `.expect("valid header")` on
`geo_location` and `normalized_ua`, both echoed verbatim out of the MARS
`/v1/ads-preflight` response body. `Headers::insert` rejects any value that
is not printable ASCII, so a preflight response carrying a non-ASCII geo
location — or CRLF — panicked inside the caller's process instead of
failing the request. Every OHTTP ad request and OHTTP click/impression/report
callback goes through this conversion.
It is now `TryFrom<PreflightResponse> for Headers` with
`Error = viaduct::ViaductError`. Both call sites in `MARSClient` already
return an error type that converts from `ViaductError` (`FetchAdsError` and
`CallbackRequestError`), so the failure propagates with `?` and surfaces to
the caller as a request error. Four unit tests cover the happy path, the
omitted-empty-UA path, non-ASCII, and CRLF injection.
The remaining `unwrap`/`expect`/`panic!` sites in this component were
audited at the same time. Three are `#[cfg(test)]`-gated and one — the
`path_segments_mut()` call in `Environment::into_url` — cannot fail because
every `base_url()` arm is an `https` URL, which `url` guarantees is
hierarchical. Only that last one is non-obvious from the code, so only it
gets a comment; no other code changes.
Almaju
force-pushed
the
ads-client-ordering-pass
branch
from
September 21, 2026 23:38
e9dfadc to
be6a4df
Compare
thesuzerain
approved these changes
Sep 21, 2026
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From<PreflightResponse> for Headerscalled.expect()ongeo_locationandnormalized_ua, both echoed from the MARS/v1/ads-preflightresponse body. Header values must be printable ASCII, so a non-ASCII geo location or a CRLF panicked in the caller's process instead of failing the request — on every OHTTP ad request and click/impression/report callback.Pull Request checklist
From→TryFromis on a privatemars::preflighttype; no binding API change.fmtandclippy --all-targetsclean, 105 unit tests pass.mars::preflight— happy path, empty UA, non-ASCII geo, CRLF injection.### Ads-Clientin v158.0.