fix(sdk-python): decode multipart download path labels#68
Conversation
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
|
All contributors have signed the CLA. ✅ Thank you! |
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
There was a problem hiding this comment.
1 issue found and verified against the latest diff
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk-python/src/daytona/common/file_transfer.py">
<violation number="1" location="sdk-python/src/daytona/common/file_transfer.py:93">
P3: Malformed `filename*` responses are now rejected in several new branches, but the added sync/async tests only generate valid UTF-8 percent encoding or omit `filename*` entirely. Adding cases for an invalid escape (for example `%GG`), unsupported charset, and invalid UTF-8 bytes would protect this protocol-error behavior and verify both clients surface `DaytonaError` as intended.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| return name, parameters.get("filename") | ||
|
|
||
| extended_parts = extended_filename.split("'", 2) | ||
| if len(extended_parts) != 3 or extended_parts[0].lower() != "utf-8": |
There was a problem hiding this comment.
P3: Malformed filename* responses are now rejected in several new branches, but the added sync/async tests only generate valid UTF-8 percent encoding or omit filename* entirely. Adding cases for an invalid escape (for example %GG), unsupported charset, and invalid UTF-8 bytes would protect this protocol-error behavior and verify both clients surface DaytonaError as intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk-python/src/daytona/common/file_transfer.py, line 93:
<comment>Malformed `filename*` responses are now rejected in several new branches, but the added sync/async tests only generate valid UTF-8 percent encoding or omit `filename*` entirely. Adding cases for an invalid escape (for example `%GG`), unsupported charset, and invalid UTF-8 bytes would protect this protocol-error behavior and verify both clients surface `DaytonaError` as intended.</comment>
<file context>
@@ -13,6 +15,100 @@
+ return name, parameters.get("filename")
+
+ extended_parts = extended_filename.split("'", 2)
+ if len(extended_parts) != 3 or extended_parts[0].lower() != "utf-8":
+ raise DaytonaError("Invalid Content-Disposition filename* parameter")
+
</file context>
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
|
I have read the CLA Document and I hereby sign the CLA |
fs.download_filefails on Windows sandboxes when the remote path uses backslashes:The same file downloads fine spelled
C:/Windows/Temp/x, and uploads accept backslash paths — only downloads break.Root cause. The bulk-download response is multipart; each part echoes the requested path in its
Content-Dispositionso the SDK can match parts back to requests by exact string lookup. That header was parsed withpython_multipart.parse_options_header, which deliberately strips a backslash-containing filename down to its basename:The lookup for
'x'then raisesKeyError— the cryptic'x'in the error message is that KeyError leaking through the error-prefix decorator.Fix: Parse
Content-Dispositionin the SDK instead, preferring the RFC 5987filename*parameter the daemon already sends (percent-encoded, so it round-trips any path byte-exactly) and falling back to plainfilenamewith quoted-pair unescaping. Preferringfilename*also fixes a second latent bug: plainfilenameis Latin-1-only and the daemon substitutes_for characters outside it, so non-ASCII paths could never match either. Part header values are now decoded as Latin-1 (per HTTP) rather than UTF-8-with-ignore, which silently dropped bytes.An alternative considered and rejected: matching parts to requests positionally (the daemon writes one part per requested path, in order). It would sidestep header parsing entirely, but gives up self-identifying parts and would silently mis-pair files if the response order ever changed.
Verification.
pytest tests/test_filesystem.py— 45 passed. New coverage: backslash and forward-slash paths, non-ASCII paths viafilename*, thefilename-only fallback, mixed success/error parts, and malformedfilename*rejection, for both the sync and async clients.Summary by cubic
Fixes bulk download path matching by correctly decoding multipart Content-Disposition labels, so Windows backslash and non-ASCII paths download reliably in both sync and async clients.
daytona.common.file_transferthat prefers RFC 5987filename*, falls back to quotedfilename, and strictly validates percent-encoding to reject malformed labels while preserving exact paths (including backslashes).filename*(including malformed),filenamefallback, mixed success/error parts, and truncated responses in sync and async clients.Written for commit e184686. Summary will update on new commits.