fix(skills-sandbox): reject inputs that poison a sandbox's replay log - #6668
Draft
arsenyinfo wants to merge 3 commits into
Draft
fix(skills-sandbox): reject inputs that poison a sandbox's replay log#6668arsenyinfo wants to merge 3 commits into
arsenyinfo wants to merge 3 commits into
Conversation
Three validation gaps let a value that later fails Rust replay conversion get persisted into the append-only sandbox log, permanently bricking a conversation's default sandbox (every later run_command/download_file fails). Fixed at the input boundary so no new poison is persisted; the Rust replay boundary is left as-is (tightening it would strand already-persisted mounts). - upload/artifact paths: reject a `.` path segment as well as `..`. A path like /home/sandbox/. resolves to a directory, so the replayed `base64 -d > <path>` redirect fails on every run. Fixed in both the TS twin (resolveArtifactPath) and the Rust validators (validate_upload_path, validate_artifact_path), with the mirrored vector tables kept in sync. - skill name: reject a name that is `.` or contains `/` or `..` at parseSkillManifest, mirroring the Rust skill_root_path mount boundary. - resource file path: reject empty, `.`, and `..` segments in SkillFileInputSchema (was: only leading-/ and `..`). Forward-only: existing rows with poison names/paths are tracked as a separate data-migration follow-up.
…asses Review of the poison-fix diff surfaced three issues: - B1/C7 over-rejected: rejecting every `.` segment wrongly refused conventional relative paths (`./result.txt`, `/home/sandbox/./x`, `a//b.py`) that resolve to a regular file and the shell handles fine. Narrow to a TERMINAL `.` or empty segment (the only forms that resolve to a directory) in the upload/ artifact validators (TS + Rust) and the resource-path schema. Mirrored vector tables updated to accept the non-terminal cases. - C6 bypass: built-in skills seeded with a white-label app name skip parseSkillManifest, so an app name with `/` or `..` produced a poison skill name. Add validateSkillMountName at the mount chokepoint, mirroring the Rust skill_root_path boundary, so no source can persist an unreplayable mount. - C7 bypass: GitHub import built resource paths without the schema. Extract a shared isSafeSkillResourcePath predicate and apply it in both the input schema and the importer. Also: distinct `artifact_path_directory` reject reason for the terminal-`.` case. Existing poison rows and the app-name→built-in-skill-name normalization are tracked as follow-ups (forward-only fix).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three input-validation gaps let a value that passes create/update/upload validation but later fails Rust replay conversion get persisted into the append-only
skill_sandbox_replay_eventslog, permanently bricking a conversation's default sandbox (every laterrun_command/download_filefails). Fixed at the input boundary so no new poison is persisted; the Rust replay boundary (validate_snapshot_file_path) is left unchanged — tightening it would strand already-persisted mounts.Fixes
.(/home/sandbox/.resolves to a directory, so the replayedbase64 -d > <path>redirect fails forever). Non-terminal.(./x,a/./b) resolves to a regular file and is allowed. Fixed in the TS twin (resolveArtifactPath) and Rust (validate_upload_path/validate_artifact_pathvia a newresolves_to_directoryhelper), with the mirrored vector tables kept in sync..or contains/or..atparseSkillManifest, and — the universal chokepoint — atmountSkill(validateSkillMountName), mirroring the Rustskill_root_pathboundary. The mount-side check closes a bypass where a white-label app name is spliced into a built-in skill name without going through the parser..., and terminal empty/.segments via a sharedisSafeSkillResourcePathpredicate, applied by bothSkillFileInputSchemaand the GitHub importer (which previously skipped the schema).Scope
Forward-only: existing rows with poison names/paths, the app-name→built-in-skill-name normalization, and a pre-existing trailing-slash artifact-download friendliness gap (non-poisoning) are tracked as follow-ups.
Validation
pnpm type-check,cargo test -p sandbox_core,cargo clippy -p sandbox_core --all-targets -- -D warnings, and the affected backend suites (parser, create-skill route, skill-sandbox path vectors + mount-name, github-import) all pass; biome clean.Review
Two adversarial review rounds. Round 1 caught an over-rejection regression (rejecting every
.segment, now narrowed to terminal) and two persistence bypasses (built-in white-label name, GitHub import paths), all fixed. Round 2 confirmed no remaining security blocker.