fix(harness-opencode): allow external directory access with permissionMode: "allow-all" - #21191
Conversation
Co-authored-by: davidcolatti-eve <314377486+davidcolatti-eve@users.noreply.github.com>
ai-sdk-factory
Bot
added
harness/opencode
Bugfix reviewOutcome: changes-required Reproduction replayStatus: no-longer-reproduces The exact original reproduction completed successfully and the original bug signal did not appear. Fixes issueStatus: fully-addresses The permission check now honors allow-all before external-path rejection, and canonical path comparison handles symlink-backed work directories while rejecting symlink escapes in restrictive modes. Side effectsRisk: low The new containment predicate incorrectly rejects a narrow class of legitimate in-workdir paths under restrictive permission modes. Concerns:
PerformanceRisk: low Canonicalization adds synchronous realpath calls for restrictive permission requests, but the work is bounded by resource count and path depth and presents no material memory risk. Backwards compatibilityRisk: none The change does not read, migrate, or alter stored sessions, configuration files, or other persisted data. Breaking changesRisk: low No public API, type, export, default, or persisted format changes, but restrictive modes gain an observable false rejection for valid paths whose first relative component starts with two dots. Concerns:
ArchitectureRisk: none The implementation remains private to the OpenCode bridge, follows existing dependency direction, and introduces no cross-package source import, export, or misplaced shared abstraction. Change scopeStatus: minimal The production change, focused bridge regression tests, and patch changeset are all directly related to fixing and validating the reported permission behavior. SecurityRisk: none External access is enabled only for the documented allow-all mode, while restrictive modes now canonicalize paths and reject tested symlink escapes. TestingStatus: needs-more Tests cover allow-all external access, symlink-backed work directories, and symlink escapes, but miss the containment boundary that exposes the new false rejection. Concerns:
VerificationInspected the complete merge-base diff and permission flow, ran the full harness-opencode test suite successfully with 107 tests, passed package type checking and changed-file formatting checks, and used a focused path-containment probe that confirmed valid '..cache' and '...' child paths are classified as outside. Relevant Documentation |
Co-authored-by: davidcolatti-eve <314377486+davidcolatti-eve@users.noreply.github.com> Co-authored-by: ai-sdk-factory[bot] <305873210+ai-sdk-factory[bot]@users.noreply.github.com> Co-authored-by: Felix Arntz <3531426+felixarntz@users.noreply.github.com>
ai-sdk-factory
Bot
changed the title
|
Addressed PR comment 5757559326.
Addressed. The containment predicate now rejects only an exact |
Bugfix reviewOutcome: approved Reproduction replayStatus: no-longer-reproduces The exact original reproduction completed successfully and the original bug signal did not appear. Fixes issueStatus: fully-addresses Allow-all is evaluated before external-path rejection, while canonical containment handles symlink-backed work directories and blocks symlink escapes in restrictive modes. Side effectsRisk: low Restrictive modes now intentionally reject paths whose canonical targets escape the work directory; separator-aware traversal detection preserves legitimate dot-prefixed children such as '..cache'. PerformanceRisk: low Restrictive permission checks add bounded synchronous realpath resolution per resource and path ancestor, with no material memory-growth risk. Backwards compatibilityRisk: none The change does not read, migrate, or alter stored sessions, configuration files, or persisted data formats. Breaking changesRisk: none No public APIs, types, exports, accepted configuration shapes, defaults, output formats, or persisted formats change; the observable permission change corrects allow-all and restrictive containment semantics. ArchitectureRisk: none The implementation remains private to the OpenCode bridge, preserves package boundaries and dependency direction, and adds no cross-package source imports or misplaced shared abstractions. Change scopeStatus: minimal The bridge permission changes, focused regression tests, and patch changeset are all directly necessary to fix and verify the reported behavior. SecurityRisk: none External access is enabled only for allow-all, while restrictive modes canonicalize paths and reject symlink escapes, improving containment without exposing credentials or new attack surfaces. TestingStatus: appropriate Regression coverage verifies allow-all external access, symlink-backed work directories, symlink escapes, and valid dot-prefixed children under restrictive modes. VerificationInspected the complete merge-base diff and permission flow; the full harness-opencode suite passed with 107 tests, and package type checking, build, changed-file formatting, linting, and diff checks all passed. Relevant Documentation |
felixarntz
changed the title
permissionMode: "allow-all"
|
🚀 Published in:
|
2 participants
Background
The PR reordered permission evaluation and introduced canonical filesystem containment; review identified an overbroad traversal boundary.
Root Cause
Allow-all was evaluated after external rejection, lexical paths mishandled symlinks, and
startsWith('..')conflated traversal with valid dot-prefixed names.Summary
Allow-all now permits external access, canonical containment blocks symlink escapes, and separator-aware traversal detection preserves valid dot-prefixed children.
Testing
Regression coverage includes allow-all external access, symlink-backed workdirs, symlink escapes, and
..cachechildren. All validation passed.End-to-end Validation
Related Issues
Fixes #21187
Closes #21189