Repository navigation
fix(url): reject "." and ".." path parameters so URL resolution can't change the route - #1390
Closed
alexb-openrouter wants to merge 1 commit into
Closed
alexb-openrouter wants to merge 1 commit into
alexb-openrouter wants to merge 1 commit into
Conversation
… change the route
Contributor
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
mattapperson
added a commit
that referenced
this pull request
Nov 11, 2026
…hange the route Generated operations percent-encode path parameters, which leaves "." and ".." unchanged, and ClientSDK._createRequest resolves the path with new URL(path, baseURL), which collapses dot segments. A path parameter of exactly ".." or "." sent the request, with the client's API key, to a different route. pathToFunc now throws before any request is built. src/lib/url.ts is added to .genignore and hand-maintained, because persistent edits are off and neither a hook, an overlay nor gen.yaml can express the check. gen.lock drops the file's tracking entry, as the generator does for ignored files. Supersedes #1390.
Contributor
|
Closing in favour of #1372, which carries this fix. #1372 turns Speakeasy persistent edits off, so a hand edit to the generated |
alexb-openrouter
added a commit
that referenced
this pull request
Nov 13, 2026
… edits (#1372) ## Summary Two things, both patch-level: 1. Turns off Speakeasy persistent edits, and moves the hand edits that depended on it out of generated code. 2. **Security fix:** a path parameter of exactly `..` or `.` no longer sends the request to a different route. Supersedes #1390, whose fix was a hand edit to a generated file and would be overwritten once persistent edits are off. No version change and no API removal: this is intended to ship as an ordinary patch release. The branch is up to date with `main` (`8d929a22`, SDK 1.5.102). The merge takes `main`'s generated `src/lib/env.ts` (the `OPENROUTER_DEBUG` parser fix) without the `OPENROUTER_BASE_URL` hand edit, keeps `src/lib/url.ts` out of `gen.lock` `trackedFiles`, and strips `@generated-id` headers on files added since. See "Dropped from the original PR" at the bottom. ## Security fix: dot-segment path parameters Generated operations percent-encode path parameters, which leaves `.` and `..` unchanged. `ClientSDK._createRequest` (`src/lib/sdks.ts`) then builds the URL with `new URL(path, baseURL)`, which resolves them as dot segments. Code that passes an untrusted value as a path parameter could therefore have the request, with the client's API key, sent to another route: ```ts client.containers.downloadContainerFileContent({ containerId: "..", fileId: "file-abc" }) // before: GET /api/v1/files/file-abc/content // after: rejects with Error "Parameter 'container_id' cannot be the dot segment '..'"; nothing is sent ``` **Behaviour.** `pathToFunc` (`src/lib/url.ts`), which every operation uses to fill its path template, now throws when a parameter that forms a whole path segment is exactly `.` or `..`. There is no whole-path check, so a base URL that itself contains a dot segment (`serverURL` or `OPENROUTER_BASE_URL`) normalises as on `main`. `...`, `.env`, `a..b` are unaffected, `%2e%2e` is still sent double-encoded as `%252e%252e`, and the existing "is required" / "must be a string or number" errors are unchanged. The only inputs that behave differently are ones that were already being misrouted. **Error type.** A plain `Error`, as a rejected promise, the same as the existing "Parameter 'x' is required" error. It is thrown before `_createRequest`, so it is not wrapped in the SDK's `Result` / `InvalidRequestError`, including for callers of the standalone `funcs/*` functions. **Mechanism: `src/lib/url.ts` is added to `.genignore` and is now hand-maintained.** The alternatives were tried first: | Option | Result | |---|---| | OpenAPI `pattern` on path-parameter schemas via an overlay | Tested with the pinned generator: an overlay put the pattern on all 111 path-parameter schemas in `out.openapi.yaml`, and regeneration produced no change under `src/`. The generator does not emit `pattern` (or `minLength`: the spec has both today and the generated zod schemas have neither). | | `gen.yaml` option | None exists for path-parameter encoding or validation. | | SDK hook | Not sound. `beforeCreateRequest` runs at the end of `_createRequest`, after `new URL(path, baseURL)`, and receives only the resolved URL; `HookContext` has no path template or raw parameters. A misrouted `/files/file-abc/content` is indistinguishable from a real call to that route. | Regenerating with the entry in place leaves `src/lib/url.ts` byte-identical, and the generator removes the file's entry from `gen.lock` `trackedFiles`, which is committed here. **Cost and mitigation.** Generator updates to this one file no longer arrive. `tests/unit/path-param-dot-segments.test.ts` has a "generator contract" block that fails if generated operations start using the file differently (the exported signature, what they import from it, the call shape). **Removing it.** The bug has been reported to Speakeasy. When the generator rejects or safely encodes dot segments: delete the `src/lib/url.ts` line from `.genignore`, regenerate, keep the behaviour tests. The file's header comment says the same. ## Consumer-visible behaviour changes Please read these before deciding this is fine as a patch. 1. **`serverURLFromOptions()` called directly no longer reads `OPENROUTER_BASE_URL`.** With the env var set, `serverURLFromOptions({})` now returns the production URL. `new OpenRouter()` and every sub-SDK (`chat`, etc.) still honour the env var with the same precedence, through a hook. This one is not avoidable while persistent edits are off: the helper lives in generated `src/lib/config.ts`, and Speakeasy's TypeScript target has no setting that makes the server URL read an env var (`envVarPrefix` covers security and global parameters only). Workaround for direct callers: `serverURLFromOptions({ serverURL: process.env.OPENROUTER_BASE_URL })`. 2. **`Env` and `envSchema` in `src/lib/env.ts` no longer have an `OPENROUTER_BASE_URL` field.** Only code that deep-imports `@openrouter/sdk/lib/env.js` and reads `env().OPENROUTER_BASE_URL` is affected. Same cause as (1). 3. Minor, strictly more permissive: the hook reads `OPENROUTER_BASE_URL` each time a client is constructed. Before, the value was memoised with the rest of `env()` on first use. `client._options.serverURL` is now populated with the env value when it is applied. Not changed: - Precedence is still `serverURL` > `server` > `OPENROUTER_BASE_URL` > production. - An invalid `OPENROUTER_BASE_URL` still throws a `ZodError` (issue path `["OPENROUTER_BASE_URL"]`) at client construction, including when `serverURL` is passed explicitly. The original version of this PR changed it to a `TypeError`; that is gone. - The `// @generated-id` comment line disappears from generated sources. Comment only, no runtime or type effect. ## Audit of hand edits Every non-deleted file under `trackedFiles` in `.speakeasy/gen.lock` (3,955 files) was hashed and compared with its `pristine_git_object`. Re-run in this round against `main` at `a709c011`: the same five differ, nothing new. | File | Hand edit | Where it went | |---|---|---| | `src/lib/config.ts` | `OPENROUTER_BASE_URL` fallback in `serverURLFromOptions` (#832) | `SDKInitHook` in `src/hooks/base-url-env.ts`, registered in `src/hooks/registration.ts`. Direct helper calls lose it (change 1 above). | | `src/lib/env.ts` | `OPENROUTER_BASE_URL` in `Env` / `envSchema` (#832) | Same hook, which validates with the same zod rule (change 2 above). | | `RUNTIMES.md` | "v18, v20, and v22" | Added to `.genignore` | | `.devcontainer/devcontainer.json` | Node 24 bookworm image (new since the original audit) | Added to `.genignore` | | `tsconfig.json` | one whitespace-only line removed | Back to generated form | `src/sdk/sdk.ts` no longer differs from pristine: the `callModel` regions were removed on `main` in #1356. `enableCustomCodeRegions` is a separate feature and stays on. Existing hooks are unaffected: `src/hooks/signal-hooks.ts` and `registration.ts` are not generator-owned, and the regen did not touch them. The new hook is registered before `SignalTimeoutHook`; the two do not interact. ## Commits 1. `chore: disable Speakeasy persistent edits`: `persistentEdits.enabled: never`, `.genignore`, the hook and its registration, tests, `CLAUDE.md`, and the regenerated `config.ts` / `env.ts` / `tsconfig.json`. This is the one to review. 2. `chore: regenerate SDK without @generated-id headers`: generator output only. 1,392 files lose the one header line, `gen.lock` drops the matching tracking data, and Speakeasy removes one blank line from `.gitignore`. Nothing else. 3. `test: remove unused schema-test-helpers`: a test helper left behind by #1356 that nothing imports. 4. `fix(url): reject "." and ".." path segments so URL resolution can't change the route`: `src/lib/url.ts`, its `.genignore` entry, the `gen.lock` entry removal, tests, `CLAUDE.md`. ## Version Left at `1.5.85` in `gen.yaml`, `package.json`, `jsr.json` and `SDK_METADATA`, exactly as on `main`. Versioning is `versioningStrategy: automatic` and the Generate workflows run `sdk-generation-action` in `mode: pr`, which bumps the patch when it regenerates. `gen.lock` here keeps the old `docChecksum` and has a new `configChecksum`, so the next release regen sees a change and bumps to the next patch. ## How it was regenerated Pinned Speakeasy 1.803.2, local only: ``` speakeasy run -t openrouter --skip-upload-spec --skip-versioning -o console -y ``` with the two `registry:` blocks removed from an uncommitted copy of `workflow.yaml` (code samples written to a local file), so nothing was uploaded or tagged. `workflow.yaml` and `workflow.lock` are unchanged from `main`. `main`'s `in.openapi.yaml` is one spec update (#1434) ahead of its generated code. To keep this diff to the persistent-edits change, generation used the spec revision the current generated code was built from. The pending spec changes are left for the normal Generate run. A run on the final tree (after commit 4) produced no diff apart from key ordering inside `package.json`, which the generator shuffles on every run and which is not committed. `src/lib/url.ts` had the same hash before and after. ## Testing - `pnpm run lint`, `pnpm run typecheck`, `pnpm run build`: pass. - `pnpm test`: 122/122 in 10 files, no type errors. `tests/unit/path-param-dot-segments.test.ts` (28 tests) runs three operations (`containers.downloadContainerFileContent`, `batch.getBatches`, `presets.restore`) against a local mock server and checks the route hit, that `.` / `..` reject with the parameter named and send nothing, the unaffected values, numeric parameters, and the generator contract. 11 of them fail with `main`'s `url.ts`. `tests/unit/server-url-env.test.ts` (10 tests) goes through `new OpenRouter()` and covers: unset, env applied, env applied to `chat`, `serverURL` over env, `server` over env, `serverURL` over `server`, invalid value is a `ZodError` with the right path, invalid value still throws with explicit `serverURL`, empty value throws, and the direct-helper behaviour. - End to end through built `esm/` against a local HTTP stub: with `OPENROUTER_BASE_URL` set, `chat.send` hit the stub at the env path; an explicit `serverURL` won; `server: "production"` won; unset fell back to production. - End to end through `esm/` built from the regenerated tree: `containerId` `..` and `.`, `batch.getBatches({ id: ".." })` and `files.download({ fileId: "." })` threw and the stub received nothing; a normal id and `%2e%2e` hit the container route. - Not run: the e2e suite against the live API. ## Dropped from the original PR - **The v2.0.0 release**: no version bump, no `feat!` / `chore!`, no "Migrating from v1" README section. The README on `main` already says `callModel` moved to `@openrouter/agent`. - **The callModel removal commit**: already on `main` via #1356. Of the 37 files that commit deleted, two are still on `main`: `tests/utils/schema-test-helpers.ts` (unreferenced, removed here) and `src/models/claude-message.ts` (kept: it is listed as hand-written in `CLAUDE.md` and is reachable as a public deep import, so deleting it would be a consumer-visible removal). - **The `TypeError` for an invalid `OPENROUTER_BASE_URL`**: reverted to the existing `ZodError`. ## Notes - Once this merges, any hand edit to a generated file is overwritten by the next regen without warning. `CLAUDE.md` is updated to say so. `src/lib/url.ts` is the exception, because it is in `.genignore`. - The next Generate PR will conflict with this one on the header lines, whichever lands first; rerunning generation resolves it. --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Alex Bujduveanu <alex.bujduveanu@openrouter.ai>
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.
Summary
Risk: code that passes an untrusted value as a path parameter (for example
containerIdfrom an end user or a tool call) could set it to exactly..or..encodeURIComponentleaves those values unchanged, andnew URL(path, baseURL)inClientSDK._createRequestthen resolves them as dot segments, so the request goes to a different/api/v1route with the app's API key attached. Example:The request still carries only the app's own key and can reach only what that key can already reach. The problem is that it bypasses any container-only restriction the app relies on.
Fix:
pathToFunc(src/lib/url.ts), which every operation uses to fill path templates, now throws when an encoded parameter is exactly.or... No request is sent. All other values are left alone....,.envanda..bare not dot segments and still pass.%2e%2ewas already double-encoded to%252e%252e. Errors for missing or non-string parameters already work this way.src/lib/url.tsis a Speakeasy-generated file.persistentEditsis enabled in.speakeasy/gen.yaml, so this edit should survive regeneration. Worth checking on the next generate run.Tests:
tests/unit/path-param-dot-segments.test.tsruns the SDK against a mock server on127.0.0.1. It checks that a normal container download hits the container route, and thatcontainerId../.andfileId..throw without sending any request. It also checks thepathToFuncedge cases directly. These tests fail onmainand pass with this change.pnpm run test,pnpm run lintandpnpm run typecheckpass.