Repository navigation
fix(python-sdk): reject non-finite vectors and bad limits before signing (WALM-854) - #1264
harrymove-ctrl wants to merge 1 commit into
Conversation
remember_manual/recall_manual passed vectors straight to json.dumps, which writes bare NaN/Infinity tokens that are not JSON, so the relayer failed with a parse error. recall, recall_manual, restore and list_namespaces also sent limit=NaN, -5 or ?limit=nan. Raise ValueError naming the argument for a vector that is not a non-empty list of finite numbers and for a limit that is not a positive integer, and sign bodies with allow_nan=False as a backstop. Fixes #1218 WALM-854
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| async def _signed_request( | ||
| self, method, path, body, accepted_statuses=(200,), include_seal_session=True | ||
| ): |
|
@duccn could you review this one when you get a chance? Thanks! |
|
@ducnmm when you have time, could you review this? Tracked in WALM-854, CI is green. |
ducnmm
left a comment
ducnmm
left a comment
There was a problem hiding this comment.
Summary
Non-finite manual vectors and non-positive limits are rejected on recall, recall_manual, and restore before those bodies are signed, and _signed_request refuses NaN / Infinity. list_namespaces still signs POST /api/stats before the new limit check, so a bad limit is not rejected before every signature.
Issue counts by severity
- bugs: 1
- suggestions: 1
- nits: 0
| if cursor is not None: | ||
| params["updated_after"] = cursor | ||
| if limit is not None: | ||
| _check_limit(limit) |
There was a problem hiding this comment.
[bug] _check_limit runs only after await self._resolve_owner() (line 1374). On a fresh client that lookup is POST /api/stats through _signed_request. list_namespaces(limit=float("nan")) (or 0, -1, True) therefore signs and sends a request before ValueError. If /version or /api/stats fails, the caller gets MemWalError or a transport error instead of ValueError, and the bad limit is never reported. CaptureMemWal._resolve_owner returns without signing, so the new test's captured == [] does not cover the real method.
Suggestion: Call _check_limit(limit) before _resolve_owner() when limit is not None. Add a test on MemWal, not the capture stub, that a bad limit performs no /api/stats POST and raises ValueError.
|
|
||
| def _check_limit(limit: Any) -> None: | ||
| """Reject a limit that is not a positive integer before it is signed (#1218).""" | ||
| if isinstance(limit, bool) or not isinstance(limit, int) or limit <= 0: |
There was a problem hiding this comment.
[suggestion] This rejects 0 and True, matching the TypeScript SDK. MemWalMock.recall still uses limit < 0, so limit=0 returns an empty page and limit=True is treated as 1. MemWalMock.list_namespaces still slices with a negative limit. Offline tests can pass calls the real client now rejects.
Suggestion: Use the same positive-integer check in MemWalMock.recall, MemWalMock.list_namespaces, and restore.
This branch has not been deployed
2 participants
WALM-854
Fixes #1218
remember_manualandrecall_manualnow raiseValueErrorfor a vector that isn't a non-empty list of finite numbers.recall,recall_manual,restoreandlist_namespacesraiseValueErrorfor a limit that isn't a positive integer. Both checks run before anything is signed._signed_requestalso serializes bodies withallow_nan=Falseas a backstop, so a bare NaN or Infinity token can never be sent.Behaviour change:
recall(limit=0)used to be sent and now raises. This matches the TypeScript SDK.Tests: tests/test_nonfinite_validation.py, 60 cases (9 bad vectors x 2 methods, 8 bad limits x 5 call paths, 1 valid round-trip, 1 backstop). Before the fix 59 failed; after, all 60 pass. Full suite without the network integration tests: 314 passed.
ruff checkclean.May conflict with #1254 (
recallbody validation). Rebase whichever merges second.SP: 2