Skip to content

Commit 3e333a0

Browse files
vdusekvdusek
authored
fix: percent-encode caller-supplied URL path segments (#1025)
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*
1 parent 697f8fe commit 3e333a0

6 files changed

Lines changed: 527 additions & 30 deletions

File tree

‎src/apify_client/_resource_clients/_resource_client.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
from apify_client._docs import docs_group
1212
from apify_client._logging import WithLogDetailsClient
1313
from apify_client._utils.errors import catch_not_found_for_resource_or_throw, catch_not_found_or_throw
14-
from apify_client._utils.http import response_to_dict, to_safe_id
14+
from apify_client._utils.http import response_to_dict, to_path_segment, to_safe_id
1515
from apify_client._utils.time import to_seconds
1616
from apify_client.errors import ApifyApiError
1717

@@ -75,7 +75,7 @@ def _resource_url(self) -> str:
7575
"""Build the full resource URL from base URL, path, and optional ID."""
7676
url = f'{self._base_url}/{self._resource_path}'
7777
if self._resource_id is not None:
78-
url = f'{url}/{to_safe_id(self._resource_id)}'
78+
url = f'{url}/{to_path_segment(to_safe_id(self._resource_id))}'
7979
return url
8080

8181
@cached_property

‎src/apify_client/_resource_clients/key_value_store.py‎

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
from apify_client._utils.crypto import create_hmac_signature, create_storage_content_signature
1919
from apify_client._utils.encoding import encode_key_value_store_record_value
2020
from apify_client._utils.errors import catch_not_found_or_throw
21-
from apify_client._utils.http import response_to_dict
21+
from apify_client._utils.http import response_to_dict, to_path_segment
2222
from apify_client.errors import ApifyApiError, InvalidResponseBodyError
2323

2424
if TYPE_CHECKING:
@@ -239,7 +239,7 @@ def get_record(self, key: str, *, signature: str | None = None, timeout: Timeout
239239
"""
240240
try:
241241
response = self._http_client.call(
242-
url=self._build_url(f'records/{key}'),
242+
url=self._build_url(f'records/{to_path_segment(key)}'),
243243
method='GET',
244244
params=self._build_params(signature=signature, attachment=True),
245245
timeout=timeout,
@@ -270,7 +270,7 @@ def record_exists(self, key: str, *, timeout: Timeout = 'long') -> bool:
270270
"""
271271
try:
272272
response = self._http_client.call(
273-
url=self._build_url(f'records/{key}'),
273+
url=self._build_url(f'records/{to_path_segment(key)}'),
274274
method='HEAD',
275275
params=self._build_params(),
276276
timeout=timeout,
@@ -298,7 +298,7 @@ def get_record_as_bytes(self, key: str, *, signature: str | None = None, timeout
298298
"""
299299
try:
300300
response = self._http_client.call(
301-
url=self._build_url(f'records/{key}'),
301+
url=self._build_url(f'records/{to_path_segment(key)}'),
302302
method='GET',
303303
params=self._build_params(signature=signature, attachment=True),
304304
timeout=timeout,
@@ -334,7 +334,7 @@ def stream_record(
334334
response = None
335335
try:
336336
response = self._http_client.call(
337-
url=self._build_url(f'records/{key}'),
337+
url=self._build_url(f'records/{to_path_segment(key)}'),
338338
method='GET',
339339
params=self._build_params(signature=signature, attachment=True),
340340
stream=True,
@@ -390,7 +390,7 @@ def set_record(
390390
headers['content-encoding'] = content_encoding
391391

392392
self._http_client.call(
393-
url=self._build_url(f'records/{key}'),
393+
url=self._build_url(f'records/{to_path_segment(key)}'),
394394
method='PUT',
395395
params=self._build_params(),
396396
data=value,
@@ -408,7 +408,7 @@ def delete_record(self, key: str, *, timeout: Timeout = 'short') -> None:
408408
timeout: Timeout for the API HTTP request.
409409
"""
410410
self._http_client.call(
411-
url=self._build_url(f'records/{key}'),
411+
url=self._build_url(f'records/{to_path_segment(key)}'),
412412
method='DELETE',
413413
params=self._build_params(),
414414
timeout=timeout,
@@ -430,14 +430,18 @@ def get_record_public_url(self, key: str, *, timeout: Timeout = 'long') -> str:
430430
if self._resource_id is None:
431431
raise ValueError('resource_id cannot be None when generating a public URL')
432432

433+
# Encode the key first, so a key that cannot address a record is refused before spending a request.
434+
record_path = f'records/{to_path_segment(key)}'
435+
433436
metadata = self.get(timeout=timeout)
434437

435438
request_params = self._build_params()
436439

440+
# The signature covers the raw key, which is what the API sees once it decodes the path segment.
437441
if metadata and metadata.url_signing_secret_key:
438442
request_params['signature'] = create_hmac_signature(metadata.url_signing_secret_key, key)
439443

440-
return self._build_public_url(f'records/{key}', request_params)
444+
return self._build_public_url(record_path, request_params)
441445

442446
def create_keys_public_url(
443447
self,
@@ -666,7 +670,7 @@ async def get_record(self, key: str, *, signature: str | None = None, timeout: T
666670
"""
667671
try:
668672
response = await self._http_client.call(
669-
url=self._build_url(f'records/{key}'),
673+
url=self._build_url(f'records/{to_path_segment(key)}'),
670674
method='GET',
671675
params=self._build_params(signature=signature, attachment=True),
672676
timeout=timeout,
@@ -697,7 +701,7 @@ async def record_exists(self, key: str, *, timeout: Timeout = 'long') -> bool:
697701
"""
698702
try:
699703
response = await self._http_client.call(
700-
url=self._build_url(f'records/{key}'),
704+
url=self._build_url(f'records/{to_path_segment(key)}'),
701705
method='HEAD',
702706
params=self._build_params(),
703707
timeout=timeout,
@@ -727,7 +731,7 @@ async def get_record_as_bytes(
727731
"""
728732
try:
729733
response = await self._http_client.call(
730-
url=self._build_url(f'records/{key}'),
734+
url=self._build_url(f'records/{to_path_segment(key)}'),
731735
method='GET',
732736
params=self._build_params(signature=signature, attachment=True),
733737
timeout=timeout,
@@ -763,7 +767,7 @@ async def stream_record(
763767
response = None
764768
try:
765769
response = await self._http_client.call(
766-
url=self._build_url(f'records/{key}'),
770+
url=self._build_url(f'records/{to_path_segment(key)}'),
767771
method='GET',
768772
params=self._build_params(signature=signature, attachment=True),
769773
stream=True,
@@ -819,7 +823,7 @@ async def set_record(
819823
headers['content-encoding'] = content_encoding
820824

821825
await self._http_client.call(
822-
url=self._build_url(f'records/{key}'),
826+
url=self._build_url(f'records/{to_path_segment(key)}'),
823827
method='PUT',
824828
params=self._build_params(),
825829
data=value,
@@ -837,7 +841,7 @@ async def delete_record(self, key: str, *, timeout: Timeout = 'short') -> None:
837841
timeout: Timeout for the API HTTP request.
838842
"""
839843
await self._http_client.call(
840-
url=self._build_url(f'records/{key}'),
844+
url=self._build_url(f'records/{to_path_segment(key)}'),
841845
method='DELETE',
842846
params=self._build_params(),
843847
timeout=timeout,
@@ -859,14 +863,18 @@ async def get_record_public_url(self, key: str, *, timeout: Timeout = 'long') ->
859863
if self._resource_id is None:
860864
raise ValueError('resource_id cannot be None when generating a public URL')
861865

866+
# Encode the key first, so a key that cannot address a record is refused before spending a request.
867+
record_path = f'records/{to_path_segment(key)}'
868+
862869
metadata = await self.get(timeout=timeout)
863870

864871
request_params = self._build_params()
865872

873+
# The signature covers the raw key, which is what the API sees once it decodes the path segment.
866874
if metadata and metadata.url_signing_secret_key:
867875
request_params['signature'] = create_hmac_signature(metadata.url_signing_secret_key, key)
868876

869-
return self._build_public_url(f'records/{key}', request_params)
877+
return self._build_public_url(record_path, request_params)
870878

871879
async def create_keys_public_url(
872880
self,

‎src/apify_client/_resource_clients/request_queue.py‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
from apify_client._pagination import DEFAULT_CHUNK_SIZE, get_cursor_iterator, get_cursor_iterator_async
3939
from apify_client._resource_clients._resource_client import ResourceClient, ResourceClientAsync
4040
from apify_client._utils.errors import catch_not_found_or_throw
41-
from apify_client._utils.http import response_to_dict
41+
from apify_client._utils.http import response_to_dict, to_path_segment
4242
from apify_client._utils.time import to_seconds
4343
from apify_client.errors import ApifyApiError
4444

@@ -275,7 +275,7 @@ def get_request(self, request_id: str, *, timeout: Timeout = 'short') -> Request
275275
"""
276276
try:
277277
response = self._http_client.call(
278-
url=self._build_url(f'requests/{request_id}'),
278+
url=self._build_url(f'requests/{to_path_segment(request_id)}'),
279279
method='GET',
280280
params=self._build_params(),
281281
timeout=timeout,
@@ -300,20 +300,26 @@ def update_request(
300300
https://docs.apify.com/api/v2#/reference/request-queues/request/update-request
301301
302302
Args:
303-
request: The updated request.
303+
request: The updated request. It must carry the ID of the request to update.
304304
forefront: Whether to put the updated request in the beginning or the end of the queue.
305305
timeout: Timeout for the API HTTP request.
306306
307307
Returns:
308308
The updated request.
309+
310+
Raises:
311+
ValueError: If the request carries no ID.
309312
"""
310313
if not isinstance(request, Request):
311314
request = Request.model_validate(request)
312315

316+
if request.id is None:
317+
raise ValueError('The request to update must have an ID.')
318+
313319
request_params = self._build_params(forefront=forefront, clientKey=self.client_key)
314320

315321
response = self._http_client.call(
316-
url=self._build_url(f'requests/{request.id}'),
322+
url=self._build_url(f'requests/{to_path_segment(request.id)}'),
317323
method='PUT',
318324
json=request.model_dump(by_alias=True, exclude_none=True),
319325
params=request_params,
@@ -337,7 +343,7 @@ def delete_request(self, request_id: str, *, timeout: Timeout = 'short') -> None
337343
)
338344

339345
self._http_client.call(
340-
url=self._build_url(f'requests/{request_id}'),
346+
url=self._build_url(f'requests/{to_path_segment(request_id)}'),
341347
method='DELETE',
342348
params=request_params,
343349
timeout=timeout,
@@ -368,7 +374,7 @@ def prolong_request_lock(
368374
)
369375

370376
response = self._http_client.call(
371-
url=self._build_url(f'requests/{request_id}/lock'),
377+
url=self._build_url(f'requests/{to_path_segment(request_id)}/lock'),
372378
method='PUT',
373379
params=request_params,
374380
timeout=timeout,
@@ -396,7 +402,7 @@ def delete_request_lock(
396402
request_params = self._build_params(clientKey=self.client_key, forefront=forefront)
397403

398404
self._http_client.call(
399-
url=self._build_url(f'requests/{request_id}/lock'),
405+
url=self._build_url(f'requests/{to_path_segment(request_id)}/lock'),
400406
method='DELETE',
401407
params=request_params,
402408
timeout=timeout,
@@ -802,7 +808,7 @@ async def get_request(self, request_id: str, *, timeout: Timeout = 'short') -> R
802808
"""
803809
try:
804810
response = await self._http_client.call(
805-
url=self._build_url(f'requests/{request_id}'),
811+
url=self._build_url(f'requests/{to_path_segment(request_id)}'),
806812
method='GET',
807813
params=self._build_params(),
808814
timeout=timeout,
@@ -825,20 +831,26 @@ async def update_request(
825831
https://docs.apify.com/api/v2#/reference/request-queues/request/update-request
826832
827833
Args:
828-
request: The updated request.
834+
request: The updated request. It must carry the ID of the request to update.
829835
forefront: Whether to put the updated request in the beginning or the end of the queue.
830836
timeout: Timeout for the API HTTP request.
831837
832838
Returns:
833839
The updated request.
840+
841+
Raises:
842+
ValueError: If the request carries no ID.
834843
"""
835844
if not isinstance(request, Request):
836845
request = Request.model_validate(request)
837846

847+
if request.id is None:
848+
raise ValueError('The request to update must have an ID.')
849+
838850
request_params = self._build_params(forefront=forefront, clientKey=self.client_key)
839851

840852
response = await self._http_client.call(
841-
url=self._build_url(f'requests/{request.id}'),
853+
url=self._build_url(f'requests/{to_path_segment(request.id)}'),
842854
method='PUT',
843855
json=request.model_dump(by_alias=True, exclude_none=True),
844856
params=request_params,
@@ -860,7 +872,7 @@ async def delete_request(self, request_id: str, *, timeout: Timeout = 'short') -
860872
request_params = self._build_params(clientKey=self.client_key)
861873

862874
await self._http_client.call(
863-
url=self._build_url(f'requests/{request_id}'),
875+
url=self._build_url(f'requests/{to_path_segment(request_id)}'),
864876
method='DELETE',
865877
params=request_params,
866878
timeout=timeout,
@@ -891,7 +903,7 @@ async def prolong_request_lock(
891903
)
892904

893905
response = await self._http_client.call(
894-
url=self._build_url(f'requests/{request_id}/lock'),
906+
url=self._build_url(f'requests/{to_path_segment(request_id)}/lock'),
895907
method='PUT',
896908
params=request_params,
897909
timeout=timeout,
@@ -919,7 +931,7 @@ async def delete_request_lock(
919931
request_params = self._build_params(clientKey=self.client_key, forefront=forefront)
920932

921933
await self._http_client.call(
922-
url=self._build_url(f'requests/{request_id}/lock'),
934+
url=self._build_url(f'requests/{to_path_segment(request_id)}/lock'),
923935
method='DELETE',
924936
params=request_params,
925937
timeout=timeout,

‎src/apify_client/_utils/http.py‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import warnings
44
from typing import TYPE_CHECKING
5+
from urllib.parse import quote
56

67
from apify_client._consts import (
78
ALREADY_COMPRESSED_MEDIA_TYPE_PREFIXES,
@@ -27,6 +28,30 @@ def to_safe_id(id: str) -> str:
2728
return id.replace('/', '~')
2829

2930

31+
def to_path_segment(value: str) -> str:
32+
"""Percent-encode a caller-supplied value so it stays a single URL path segment.
33+
34+
Without this, a value carrying `/`, `?` or `#` would restructure the URL it is interpolated into: it could
35+
reach a different endpoint, append its own query parameters, or be silently truncated at a fragment.
36+
37+
Args:
38+
value: The value to place in a single path segment, for example a key-value store record key.
39+
40+
Returns:
41+
The percent-encoded value, which a URL parser can only read as one path segment.
42+
43+
Raises:
44+
ValueError: If the value is empty, `.` or `..`, none of which can be carried in a path segment at all.
45+
"""
46+
# Encoding cannot save these three: an empty value leaves nothing but the separator, and a URL parser
47+
# resolves a dot segment after percent-decoding, so `%2E%2E` collapses just like `..`. Either way the
48+
# request lands on a parent endpoint, where `..` makes the same verb act on the whole resource.
49+
if value in {'', '.', '..'}:
50+
raise ValueError(f'"{value}" cannot be used as a URL path segment.')
51+
52+
return quote(value, safe='')
53+
54+
3055
def is_compressible_content_type(content_type: str | None) -> bool:
3156
"""Decide whether a request body with the given content type is worth compressing.
3257

0 commit comments

Comments
 (0)