Sanitize discovery-document paths to prevent request-host hijacking#12
Merged
Conversation
Method and media-upload paths are taken verbatim from an (untrusted) discovery
document and become the generated client's request URL, built as
`@base_url |> URI.merge(path)`. RFC 3986 reference resolution lets a path that
carries a scheme (`https:...`) or an authority (a leading `//`) REPLACE the base
host. So a crafted path -- e.g. `//attacker.example/x` -- silently redirects the
request off the API host and ships the caller's `Authorization: Bearer` token
(and, for uploads, the payload) to an attacker-chosen host, even when `rootUrl`
is a legitimate googleapis.com URL. Since generated clients are published to Hex,
that is supply-chain credential exfiltration downstream.
The prior sanitization pass (names, rootUrl) closed *code injection* by routing
untrusted values through `inspect/1` -- but inspect only makes a string safe as
*code*, not as a *URL*: an inert `"//attacker/x"` literal is still a
host-overriding reference at `URI.merge` time.
Reduce every derived path to its host-relative component via
`Endpoint.host_relative_path/1` (keep only `URI.parse/1`'s `:path`, force a
single leading slash) at the point the endpoint is built -- covering basic,
`_media`, and `_multipart` variants. Real Google paths are already host-relative
(`URI.parse` preserves `{+name}`/`:verb` templates), so regenerating all three
clients produces zero drift and they still compile with --warnings-as-errors.
Not addressed here: a runtime value expanded into a root-level `{+reserved}`
segment could still resolve off-host, which generation cannot see. That requires
an unusual no-prefix API shape plus an app feeding untrusted input into a
resource-name argument -- treated as the consuming app's responsibility.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
What & why
Method and media-upload paths are taken verbatim from an (untrusted) discovery document and become the generated client's request URL, built as
@base_url |> URI.merge(path). Under RFC 3986 reference resolution, a path that carries a scheme (https:...) or an authority (a leading//) replaces the base host.So a crafted path — e.g.
//attacker.example/exfil/{id}— silently redirects the request off the API host and ships the caller'sAuthorization: Bearertoken (and, for uploads, the payload) to an attacker-chosen host, even whenrootUrlis a legitimate googleapis.com URL. Generated clients are published to Hex, so this is supply-chain credential exfiltration downstream (CWE-918 / CWE-522), the same threat model as the earlier name/rootUrl injection fixes.The prior sanitization pass closed code injection by routing untrusted values through
inspect/1— butinspectonly makes a string safe as code, not as a URL. An inert"//attacker/x"literal is still a host-overriding reference atURI.mergetime; that blind spot is what this PR closes.The fix
Reduce every derived path to its host-relative component in
Endpoint.host_relative_path/1(keep onlyURI.parse/1's:path, force a single leading slash), applied where the endpoint is built — so it covers the basic,_media, and_multipartvariants in one place.URI.parsepreserves{+name}/:verbtemplates, so real Google paths are unchanged — regenerating all three clients produces zero drift.Out of scope (intentional)
A runtime value expanded into a root-level
{+reserved}segment (e.g. a/{+name}template with no version prefix) could still resolve off-host, which generation cannot see. That requires an unusual no-prefix API shape plus an app feeding untrusted input into a resource-name argument — treated as the consuming app's responsibility rather than shipping a host-pin check into every published client's request path.Testing
test/googly/generator/url_safety_test.exs: protocol-relative path, absolute-URL path, and media-upload path all stay on the API host after merge; plus a regression guard that a legitimate relative path is preserved.--warnings-as-errors.