fix(bay): support workspace path aliases and UTF-8 download filenames - #31
fix(bay): support workspace path aliases and UTF-8 download filenames#31lzyqwr wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
validate_relative_pathdocstring and rules mention only/workspaceas an allowed alias, but the implementation also accepts bareworkspaceandworkspace/...; consider updating the docstring/comments to clearly reflect all supported alias forms. - Normalizing a bare
workspacepath to.changes the meaning of a legitimateworkspacefile/directory name into the workspace root; if this is intentional, it may be worth explicitly guarding or documenting this ambiguity to avoid surprising behavior for paths actually namedworkspace.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `validate_relative_path` docstring and rules mention only `/workspace` as an allowed alias, but the implementation also accepts bare `workspace` and `workspace/...`; consider updating the docstring/comments to clearly reflect all supported alias forms.
- Normalizing a bare `workspace` path to `.` changes the meaning of a legitimate `workspace` file/directory name into the workspace root; if this is intentional, it may be worth explicitly guarding or documenting this ambiguity to avoid surprising behavior for paths actually named `workspace`.
## Individual Comments
### Comment 1
<location path="pkgs/bay/app/validators/path.py" line_range="65-66" />
<code_context>
+ # workspace/.... Normalize both forms before applying the traversal checks.
+ if path == "/workspace" or path == "workspace":
+ path = "."
+ elif path.startswith("/workspace/"):
+ path = path.removeprefix("/workspace/")
+ elif path.startswith("workspace/"):
+ path = path.removeprefix("workspace/")
</code_context>
<issue_to_address>
**issue:** Double slashes after `/workspace/` may cause paths to be treated as absolute and rejected
For an input like `/workspace//foo`, `removeprefix` produces `//foo`, and `PurePosixPath("//foo")` is typically treated as absolute, so it will fail the later absolute-path checks. If everything under `/workspace` should be workspace-relative, consider normalizing redundant slashes (e.g., collapsing `//` to `/`) around the prefix stripping step.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| elif path.startswith("/workspace/"): | ||
| path = path.removeprefix("/workspace/") |
There was a problem hiding this comment.
issue: Double slashes after /workspace/ may cause paths to be treated as absolute and rejected
For an input like /workspace//foo, removeprefix produces //foo, and PurePosixPath("//foo") is typically treated as absolute, so it will fail the later absolute-path checks. If everything under /workspace should be workspace-relative, consider normalizing redundant slashes (e.g., collapsing // to /) around the prefix stripping step.
|
I reproduced the same issue in a real AstrBot + Shipyard Neo deployment. For the Unicode filename case, the underlying Ship container successfully handled the file download request: but Bay then returned HTTP 500. The Bay traceback points directly to the I also reproduced the failure with Control test:
I also encountered the So this PR appears to address the same two issues I encountered in my deployment. I would be happy to test this PR in my environment if that would be helpful. |
|
I tested this PR on a real AstrBot + Shipyard Neo deployment, and the Unicode filename / workspace alias case passed end-to-end. What I verified:
Result:
This matches the failure I originally reproduced, where Ship returned 200 but Bay failed while building the Thanks for the fix. |
Summary
Fix file download failures for sandbox clients that use
/workspace-style paths or Unicode filenames.Changes
/workspace/...andworkspace/...as aliases for the sandbox workspace root.Content-Dispositionfilenames using RFC 5987 UTF-8 format.Motivation
Some clients expose sandbox files using paths such as
/workspace/file.txtorworkspace/file.txt, while the Bay API expects paths relative to the workspace root. This causes valid files to be reported as missing.The previous
Content-Dispositionheader also embedded filenames directly and could not reliably handle non-ASCII filenames.Security
Workspace aliases are normalized before traversal validation. Paths such as
/workspace/../etc/passwdandworkspace/../../etc/passwdremain rejected.Filename values are percent-encoded before being placed in the response header.
Validation
Content-Dispositionheaders.Closes #21
Summary by Sourcery
Support workspace path aliases and Unicode-safe filenames when downloading sandbox files.
New Features:
Bug Fixes:
Enhancements:
Tests: