Skip to content

fix: percent-encode caller-supplied URL path segments - #1025

Merged
vdusek merged 4 commits into
masterfrom
fix/unencoded-url-path-segments
Aug 25, 2026
Merged

vdusek merged 4 commits into
masterfrom
fix/unencoded-url-path-segments

Conversation

@vdusek

@vdusek vdusek commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Caller-supplied values were interpolated into the request path with a plain f-string, so they could restructure the URL. Reproduced against a mock server, reading the raw request line:

key='../../actor-runs/VICTIM/abort'    -> /v2/key-value-stores/actor-runs/VICTIM/abort
request_id='../../../datasets/V/items' -> /v2/datasets/V/items
key='foo?injected=1'                   -> /v2/.../records/foo?injected=1?attachment=true
key='foo#frag'                         -> /v2/.../records/foo   (fragment dropped, wrong record, no error)

Requests carry the caller's token, so traversal acts with full privileges. The # case is a plain correctness bug: any app forwarding user strings as record keys silently reads the wrong one.

Fix

New to_path_segment() in _utils/http.py, applied to the caller-supplied segment at every records/{key} and requests/{request_id} call site, plus resource_id in ResourceClientBase._resource_url. _build_url itself is untouched, so literal paths like requests/batch keep their separator.

The API routes these as a single Express param (records/:recordKey) and decodes it once, so percent-encoding is what it expects — an unencoded / never matched. Signatures stay over the raw key, which is what the server verifies against (key_value_stores.ts:503 HMACs the decoded req.params.recordKey).

Behavior change

'', . and .. now raise ValueError. Encoding cannot protect them — a URL parser resolves dot segments after decoding, so records/%2E%2E collapses exactly like records/... They previously reached the parent endpoint, where the same verb means something else:

  • delete_record('..')DELETE /v2/key-value-stores/{id}, deleting the whole store
  • get_record('..') returned store metadata as record content
  • client.dataset('..')GET /v2/

Also newly rejected: a non-str key, previously coerced by the f-string. A caller who pre-encoded keys as a workaround now gets double encoding — but such keys never resolved anyway, since the platform's key charset forbids both / and %.

✍️ Drafted by Claude Code

Record keys, request IDs and resource IDs were interpolated into the request path with a plain f-string, so a
value carrying `/`, `?` or `#` could reach a different endpoint under the caller's token, inject its own query
parameters, or be silently truncated at a fragment and read the wrong record.

An empty value, `.` and `..` cannot be carried in a path segment at all - a URL parser resolves dot segments
after percent-decoding - so they are now rejected with a `ValueError` instead of landing on the parent or
collection endpoint.
@github-actions github-actions Bot added this to the 147th sprint - Tooling team milestone Aug 21, 2026
@github-actions github-actions Bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Aug 21, 2026
@vdusek vdusek added the adhoc Ad-hoc unplanned task added during the sprint. label Aug 21, 2026
@vdusek
vdusek marked this pull request as draft August 21, 2026 11:36
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.96%. Comparing base (df80da1) to head (6acd295).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1025      +/-   ##
==========================================
+ Coverage   94.95%   94.96%   +0.01%     
==========================================
  Files          58       58              
  Lines        5436     5447      +11     
==========================================
+ Hits         5162     5173      +11     
  Misses        274      274              
Flag Coverage Δ
integration 91.62% <82.35%> (-0.10%) ⬇️
unit 86.39% <100.00%> (+0.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vdusek vdusek changed the title fix(url): percent-encode caller-supplied URL path segments fix: percent-encode caller-supplied URL path segments Aug 21, 2026
@vdusek
vdusek requested a review from Pijukatel August 21, 2026 12:46
@vdusek
vdusek marked this pull request as ready for review August 21, 2026 12:46
@vdusek
vdusek merged commit 3e333a0 into master Aug 25, 2026
30 checks passed
@vdusek
vdusek deleted the fix/unencoded-url-path-segments branch August 25, 2026 07:27
vdusek added a commit that referenced this pull request Aug 25, 2026
…ID (#1032)

Master CI is red on every type-check and unit-test job:
https://github.com/apify/apify-client-python/actions/runs/32821686073

Two PRs that each passed CI on their own conflict semantically once
merged together:

- #1030 made `unique_key` a required field on `RequestBase`.
- #1025 added two tests that build `Request(url='https://example.com')`
to assert `update_request` rejects a request with no ID.

In that order, the `Request` constructor now raises a pydantic
`ValidationError` for the missing `uniqueKey` before `update_request` is
ever reached — so `ty` reported 2 `missing-argument` diagnostics and
both tests failed on the wrong exception.

Supplying the now-required `unique_key` makes the tests exercise what
they were written for (the missing ID), not model validation.

*✍️ Drafted by Claude Code*
barjin added a commit to apify/apify-client-js that referenced this pull request Aug 27, 2026
Swaps string interpolation in URL building for sanitized composition. 

Related to apify/apify-client-python#1025
Closes #1005
vdusek pushed a commit to apify/apify-client-js that referenced this pull request Sep 8, 2026
Swaps string interpolation in URL building for sanitized composition. 

Related to apify/apify-client-python#1025
Closes #1005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants