Repository navigation
SEC-774: fix(utils): percent-encode path parameters so an ID can't change which API route is called - #1230
Open
alexb-openrouter wants to merge 2 commits into
Open
SEC-774: fix(utils): percent-encode path parameters so an ID can't change which API route is called#1230alexb-openrouter wants to merge 2 commits into
alexb-openrouter wants to merge 2 commits into
Conversation
…quest route Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…n with a patch file Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
This branch has not been deployed
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.
The risk in plain terms
client.files.download(file_id).../before sending, so an ID like../keys?x=turnedGET /api/v1/files/{file_id}/contentintoGET /api/v1/keys.files.delete("../keys/<hash>")becameDELETE /api/v1/keys/<hash>./api/v1route that key can use. It doesn't cross into other accounts, but it does let an outside party pick which route the app calls.How this PR fixes it
generate_url()now percent-encodes every path parameter value withurllib.parse.quote(value, safe=""), the same encoding as the TypeScript SDK'sencodeSimplewithcharEncoding: "percent"./,?,#,%, spaces and non-ASCII characters are encoded, so a value always stays inside its own path segment."."and".."have no characters to encode and would still be treated as dot segments, so they now raiseValueErrorbefore any request is sent. No real ID has either value.Keeping the fix when the SDK is regenerated
src/openrouter/utils/url.pyis a Speakeasy-generated file. The fix is kept as a Speakeasy patch file at.speakeasy/patches/src/openrouter/utils/url.py.patch, which Speakeasy re-applies after every generation. If a future template change conflicts with it, generation fails with conflict markers instead of silently dropping the fix.Options I checked and why I didn't use them:
gen.yamloption: the Python generator has none. The pinned CLI (1.803.2) only supportsx-speakeasy-path-encoding-overridefor other languages; for Python it reports "not currently supported for this language"._hooks/): abefore_requesthook gets thehttpx.Requestafter httpx has already removed the../, so the original value is gone by then.gen.yamlhaspersistentEdits: {}, which isn't enabled. A local regeneration with only the edit, and no patch file, reverted the file to the unencoded version..genignore: this would freeze the whole file and drop future generator updates to it.What changed
src/openrouter/utils/url.py: adds_encode_path_param()and uses it ingenerate_url()..speakeasy/patches/src/openrouter/utils/url.py.patch: the same change as a patch file, so regeneration keeps it.tests/test_path_param_encoding.py: regression tests that use a local127.0.0.1mock server and a fake key.Testing
tests/test_path_param_encoding.pychecks the exact request line the mock server receives:test_traversal_in_file_id_stays_on_files_download_route:files.download("../keys?x=")sendsGET /api/v1/files/..%2Fkeys%3Fx%3D/content.test_traversal_in_file_id_stays_on_files_delete_route:files.delete("../keys/abc")sendsDELETE /api/v1/files/..%2Fkeys%2Fabc.test_normal_file_id_is_unchanged,test_spaces_unicode_and_fragment_are_percent_encoded,test_dot_segment_ids_are_rejected_before_sending,test_generate_url_percent_encodes_path_params(also covers%2e%2ebecoming%252e%252e).main, 8 of the 10 tests fail. For example, the download test receives('GET', '/api/v1/keys'). The two that pass onmainare the normal-ID test and the space/unicode case in thegenerate_urltest. With this PR, all 10 pass.mypy src,pyright src,pylint src --rcfile pylintrc(10.00/10) andpytest(15 passed), the same steps as PR Validation.speakeasy run --skip-versioning --skip-upload-spec --skip-compile, registry upload removed from the local workflow copy) starting from the unpatchedurl.py. It loggedApplying patch files..., and the generatedurl.pywas byte-identical to this PR's version. The only other changes were the usualgen.lockandworkflow.lockupdates, which are not committed here.Existing-PR search: searched open PRs and PRs from the last 90 days for path param, encode,
url.py, percent,generate_url, traversal and SEC-774. None make this change. #1127 and #1118 touch publish workflows and overlays, noturl.pyor generation config, so they don't conflict with this PR.Fixes SEC-774
Link to Devin session: https://openrouter.devinenterprise.com/sessions/7437b67a506642b38fc4a5ffd44c4f6c
Open in Devin Desktop: https://openrouter.devinenterprise.com/desktop/session/7437b67a506642b38fc4a5ffd44c4f6c?variant=devin
Requested by: @alexb-openrouter