Skip to content

feat(sdk): add strict Dockerfile sandbox launch - #16

Open
Peng-YM wants to merge 4 commits into
inclusionAI:mainfrom
Peng-YM:feat/dockerfile-sandbox-launch
Open

feat(sdk): add strict Dockerfile sandbox launch#16
Peng-YM wants to merge 4 commits into
inclusionAI:mainfrom
Peng-YM:feat/dockerfile-sandbox-launch

Conversation

@Peng-YM

@Peng-YM Peng-YM commented Aug 6, 2026

Copy link
Copy Markdown

Design RFC: #17

What changed

This PR adds a backend-neutral, strict Dockerfile direct-launch path to the
Python SDK. Sandbox(context=...) uses the existing public Sandbox,
Commands, and Filesystem facades: it parses one Dockerfile, creates a
sandbox from FROM, applies the explicit supported instructions, and exposes
the optional background startup command as sandbox.startup_command.

The branch has been rebased onto the backend-neutral upstream main
architecture. The implementation keeps backend-native conversions behind the
existing boundary rather than coupling Dockerfile launch to either backend.

The public surface includes DockerContext, DockerContextEntry,
LocalDockerContext, parse_dockerfile, check_direct_launch,
apply_dockerfile, typed parse/check
and apply results, and constructor options context, auto_start_cmd, and
build_run_timeout. The SDK README and maintained
sdk/python/examples/dockerfile_launch.py now document and exercise the
current contract.

Why

Small single-stage Dockerfiles are a common way to describe a development
environment. Direct launch supports a narrow, auditable subset without
requiring BuildKit, a Docker daemon, or a registry push. It is intentionally
not a Docker build replacement: every launch reruns supported RUN, COPY,
and ADD instructions and creates no cache or snapshot.

Supported contract

FROM is deliberately rootfs-only. The base image supplies the sandbox root
filesystem, while its OCI ENV, USER, WORKDIR, CMD, and ENTRYPOINT
configuration is not inherited. The Dockerfile must state required runtime
settings explicitly.

The supported subset is exactly one literal FROM with optional AS alias;
shell-form RUN; shell-form local COPY/ADD sources for files, directories,
., and wildcards; literal --chown; literal local tar extraction; literal
ENV; absolute WORKDIR; named USER values such as app or root;
EXPOSE metadata; and exec- or shell-form CMD/ENTRYPOINT with normalized
OCI-style argv combination. Group-qualified and numeric UID/GID USER forms
are rejected because the current command facade cannot preserve them faithfully.

Multi-stage input, COPY --from, ARG, remote ADD URLs, JSON COPY/ADD,
exec-form RUN, relative WORKDIR, build-time variable expansion, unsupported
flags, and unsupported instructions are rejected. Execution paths fail closed:
Sandbox(context=...) parses strictly, check_direct_launch() returns False
with reason codes, and apply_dockerfile() rejects parsed unsupported items.
Non-strict parsing is diagnostics only.

After instructions complete, the SDK polls sandbox readiness and dispatches the
resolved startup argv in the background. A non-None CommandHandle confirms
dispatch, not process longevity or application health. Callers can wait() or
kill() the handle and own their application-specific health check. Dockerfiles
without a startup command, or launches with auto_start_cmd=False, expose
None.

Security and correctness

The context implementation validates the complete structured manifest and
materializes all file inputs before any sandbox operation. DockerContextEntry
represents files, directories, permission modes, and empty directories for both
local and custom contexts. COPY restores child entry modes non-recursively,
applies root .dockerignore, filters reserved Dockerfile metadata, rejects
local symbolic links and context traversal errors, and validates paths and
collisions. Dockerfile source wildcards use the strict no-escape subset of Go
filepath.Match: ^ negates character classes, ! remains literal, malformed
classes fail closed before file reads, and wildcard expansions with multiple
top-level sources require a destination ending in /. A matched directory
contributes its contents without retaining that matched root name. Local tar
extraction
accepts only regular files and directories with safe paths and always executes
with builder/root ownership, independently of the accumulated USER.
--chown is literal, quoted, and limited to outputs created by the current
instruction. Remote ADD is rejected by design, avoiding network retrieval and
host-side SSRF. Every runner command also receives the explicit Dockerfile
working directory, including the baseline /, rather than a backend default.

For unsupported Dockerfiles or build-once reuse, callers externally pre-build
an image, use Sandbox(image=...), and explicitly launch the desired command.
The SDK does not auto-start image configuration on that path.

Testing

The current unit gate contains 214 tests and passes with Ruff and mypy,
including under umask 0002. It covers strict parser behavior, Go-compatible
character classes, malformed pattern rejection, wildcard expansion counts,
startup argv resolution and cleanup, context and
.dockerignore filtering, pre-upload manifest materialization, collisions,
path validation, local tar traversal, ownership validation, and remote ADD
rejection. The maintained example adds independent sections for filtered
COPY ., BuildKit-compatible wildcard directory COPY, file and directory modes,
empty-directory copies, explicit root cwd, finite
CMD and ENTRYPOINT+CMD startup handles, COPY --chown, builder/root local
tar ADD after USER, and no-sandbox fail-closed prechecks.

Real-sandbox follow-up

The maintained five-section example passed against a real standalone runsc
deployment through the backend-neutral facade using openyuanrong-sdk:

  • filtered COPY . honored .dockerignore; the ignored secret was absent,
    executable mode 0755 was preserved, top-level, nested, and literal empty
    directories were created with the expected modes, and COPY wild/* produced
    /srv/wild/dir2/foo without retaining the matched dir1 root;
  • explicit RUN/ENV/WORKDIR/USER state reached a finite CMD, and its
    startup_command.wait() completed successfully;
  • RUN and exec-form ENTRYPOINT + CMD used explicit root cwd /; the
    startup argv merged and produced the expected marker;
  • COPY --chown produced the expected owner; local tar ADD after USER app
    still extracted with builder/root semantics and produced the expected tree;
  • remote ADD, group-qualified USER, and numeric USER were rejected during
    pre-check without creating a sandbox.

Additional live regressions placed pre-existing root-owned files in both COPY
and tar destinations. Those files remained root:root; only files and
directories created by the current instruction became myuser:myuser.

The same code is backend-neutral and has unit coverage for both backend
sessions. The default openyuanrong-sandbox backend could not be runtime-tested
with the current public standalone image: it lacks the backend's
/api/sandbox/v1 route and returns HTTP 404 before sandbox creation.

Licensing

The parser dependency is dockerfile-parse (BSD-3-Clause); context matching
uses pathspec (MIT). The value-level parsing approach was informed by the E2B
Python SDK (MIT). No Docker engine, BuildKit, or registry component is added.

@Peng-YM
Peng-YM force-pushed the feat/dockerfile-sandbox-launch branch 5 times, most recently from 4412924 to 445cecf Compare August 7, 2026 05:36

@tianyuzhou95 tianyuzhou95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting together the RFC and the reference implementation. The overall direction is useful, and the submitted unit tests, Ruff, and mypy checks pass locally. However, the current implementation has several correctness and security issues that need to be addressed before merge.

Blocking findings

  1. SDK-side ADD URL creates an SSRF boundary violation. _dockerfile_runner.py lines 234-244 downloads remote URLs with urllib in the SDK process and then uploads the response into the sandbox. An agent-supplied Dockerfile can therefore read loopback, link-local, cloud metadata, or private-network endpoints reachable from the SDK host and retrieve the response from inside the sandbox. Restricting redirect schemes does not address the host-level SSRF. The fetch should happen inside the sandbox, or remote ADD must be an explicit trusted-input-only opt-in with an enforceable network policy.

  2. The build context bypasses .dockerignore and does not implement consistent Docker context semantics. _dockerfile_runner.py lines 246-275 passes a local directory directly to copy_from_local, so COPY . ... can upload files such as credentials, .env, or .git even when they are excluded by .dockerignore. DockerContext.walk() is not used, and non-local contexts cannot correctly support directories or wildcards. Please build a filtered context manifest, apply .dockerignore, expand source patterns, and use the same behavior for local and remote contexts.

  3. Valid Dockerfile syntax is accepted as directly launchable but executed incorrectly. _dockerfile.py lines 433-487 parses JSON-form COPY/ADD with shlex, so COPY ["a b", "/dest/"] becomes source "[a b," and destination "/dest/]". Exec-form RUN is also treated as a shell string, and flags such as --chmod and --link are silently discarded. In addition, _dockerfile_runner.py lines 129-139 resets relative WORKDIR values instead of resolving them against the previous directory and ignores ARG, even though build arguments affect FROM, later instructions, and RUN. Unsupported syntax must either be implemented or rejected by check_direct_launch; it must not return success and then run with different semantics.

  4. The evaluator does not inherit the base image configuration. It initializes build state as empty environment, /, and root, and resolves CMD/ENTRYPOINT only from instructions in the current Dockerfile. A Dockerfile containing only FROM nginx, for example, is reported as launchable but does not start the inherited nginx command; inherited USER, WORKDIR, CMD, and ENTRYPOINT are likewise lost. This needs OCI image-config plumbing or a narrower explicitly documented contract. Without it, the feature cannot claim that the resulting sandbox behaves like the Dockerfile.

  5. The documented warning policy is silent in the main API. sandbox.py lines 292-303 discards the DockerfileApplyResult, so warnings for ignored instructions never reach callers of Sandbox(context=...). Unknown instructions also remain warnings under strict=True. Please reject unsupported behavior by default, or emit/expose the warnings and make strict mode consistently reject every ignored instruction.

  6. CMD/ENTRYPOINT resolution and readiness do not match the stated contract. _dockerfile.py lines 154-167 drops a shell-form CMD when paired with an exec-form ENTRYPOINT, whereas Docker appends /bin/sh -c .... The launcher also infers shell form from whether a single argument contains a space, which breaks valid one-element exec-form commands. Finally, the readiness check only confirms that the sandbox is alive before starting the background command, and the returned process handle is discarded, so an immediately failing application can still produce a successful constructor. Please preserve the parsed command form explicitly and either define an application-start check or describe this as sandbox readiness rather than application readiness.

I recommend revising the RFC around a strict, trusted Dockerfile subset and making unsupported constructs fail closed. The PR also needs to be rebased onto the current backend-neutral SDK architecture before these changes can be integrated.

Add a backend-neutral direct-launch path for a deliberately strict
Dockerfile subset. The FROM image supplies only the sandbox root
filesystem; explicitly declared RUN, COPY, ADD, ENV, WORKDIR, USER,
CMD, and ENTRYPOINT behavior is applied through the public sandbox
facades without BuildKit, a Docker daemon, or a registry push.

Validate Dockerfiles and build contexts before remote side effects.
Unsupported syntax fails closed, remote ADD URLs are rejected, and a
filtered manifest applies .dockerignore consistently to local and
custom contexts. Local paths use no-follow directory-relative opens,
all COPY and ADD inputs are materialized before sandbox operations,
tar members are restricted to safe regular files and directories, and
ownership changes are limited to outputs of the current instruction.

Expose the background CMD or ENTRYPOINT CommandHandle while defining
constructor success as sandbox readiness and successful dispatch rather
than application health. Integrate cleanup with the backend-neutral
BackendSession lifecycle and document the rootfs-only and no-snapshot
contract.

Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
@Peng-YM
Peng-YM force-pushed the feat/dockerfile-sandbox-launch branch from 445cecf to 4f2a35e Compare August 12, 2026 07:44
@Peng-YM Peng-YM changed the title feat(sdk): add Dockerfile sandbox-launch path with in-sandbox execution feat(sdk): add strict Dockerfile sandbox launch Aug 12, 2026
@Peng-YM

Peng-YM commented Aug 12, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed review. I addressed each blocking finding in commit 4f2a35e, and rebased the branch onto the current upstream/main. Here is the point-by-point resolution.

1. SDK-side ADD URL / SSRF

Resolved by removing remote downloads from the execution path entirely.

  • Any URL source in ADD is now classified as remote_add by check_direct_launch().
  • Strict parsing rejects it before a backend session or sandbox is created.
  • The runner no longer contains an urllib download path.
  • Non-strict parsing remains diagnostics-only and cannot be passed to execution when unsupported items are present.

This is intentionally fail-closed rather than an opt-in host-side fetch.

2. .dockerignore and consistent context semantics

Resolved with a shared, filtered context manifest used by both local and custom DockerContext implementations.

  • The manifest is built from DockerContext.walk() and reads file content only through DockerContext.open().
  • Root .dockerignore rules are applied with Docker-style ordering, negation, directory, and wildcard behavior.
  • COPY/ADD support literal files, directories, ., and wildcard expansion with deterministic ordering.
  • Dockerfile and .dockerignore are reserved and are never copied by COPY ..
  • Local and non-local contexts now use the same source-selection and destination logic.
  • Invalid, duplicate, escaping, colliding, and ignored-only sources fail before sandbox mutation.
  • LocalDockerContext.open() uses descriptor-relative, no-follow opens and rejects symlinks, preventing context escape and symlink races.

3. Accepted syntax executed with different semantics

Resolved by narrowing the executable subset and making unsupported syntax fail closed.

check_direct_launch() and strict parsing now reject, among other cases:

  • JSON-form COPY/ADD
  • exec-form RUN
  • RUN flags such as --mount, --network, and unknown flags
  • relative WORKDIR
  • ARG
  • FROM flags or variable expansion
  • COPY/ADD --chmod, --link, --from, and unknown flags
  • unsupported build-time variable expansion
  • ignored or unknown instructions

Supported shell-form instructions retain their defined semantics. Unsupported constructs produce stable reasons such as unsupported_syntax, remote_add, or multi_stage; they are never converted into executable build instructions.

4. Base image configuration inheritance

Resolved by explicitly narrowing and documenting the contract: FROM supplies the root filesystem only.

The direct-launch path does not claim to inherit OCI image ENV, USER, WORKDIR, CMD, or ENTRYPOINT. A directly launched Dockerfile must declare any configuration it needs. check_direct_launch() reports this rootfs-only limitation in its successful result, and the README, RFC issue, and PR description all state it explicitly.

Full OCI image-config inheritance remains outside the scope of this change rather than being approximated incorrectly.

5. Warning policy and strict execution

Resolved by separating diagnostics from execution.

  • Sandbox(context=...) always invokes parse_dockerfile(..., strict=True) before backend creation.
  • Strict mode rejects every unsupported or ignored instruction.
  • apply_dockerfile() independently rejects a parsed object containing unsupported items, so a non-strict diagnostic result cannot be executed accidentally.
  • Non-strict parsing is only for check_direct_launch() and diagnostic inspection.
  • The constructor stores the returned startup handle instead of discarding the relevant apply result.

There is therefore no silent-warning execution path in the main API.

6. CMD/ENTRYPOINT resolution and readiness

Resolved by preserving command form explicitly and implementing the merge matrix rather than inferring form from whitespace.

  • Shell forms normalize to ('/bin/sh', '-c', command).
  • Exec ENTRYPOINT + exec CMD concatenates argument vectors.
  • Exec ENTRYPOINT + shell CMD appends /bin/sh -c <command>.
  • Shell ENTRYPOINT ignores CMD, matching Docker behavior.
  • One-element exec-form commands remain exec form.
  • Launching uses shlex.join(argv) without whitespace heuristics.

The readiness contract is now precise: construction guarantees sandbox readiness and successful background command dispatch, not application health. The returned CommandHandle is exposed as Sandbox.startup_command, allowing callers to inspect or wait for the process. Dispatch failure raises DockerfileBuildError and triggers constructor rollback.

Backend-neutral rebase and cleanup

The branch is rebased onto the backend-neutral SDK architecture and now creates SandboxSpec, loads the selected backend, and operates through BackendSession, Commands, and Filesystem. Any parse/apply/dispatch failure terminates and closes the session, including detached sandboxes, while preserving the original exception.

Validation

  • make sdk-check: 196 tests passed, Ruff passed, mypy passed for 24 source files.
  • Live standalone coverage passed for .dockerignore + COPY ., shell RUN, ENV/WORKDIR/USER, exec ENTRYPOINT + CMD, COPY --chown, local-tar ADD, remote-ADD preflight rejection, and precise ownership behavior without recursive changes to pre-existing files.
  • The current public standalone image does not expose /api/sandbox/v1 for the default openyuanrong-sandbox backend, so live validation used the optional openyuanrong-sdk backend through the same public SDK facade; this limitation is disclosed in the PR and RFC.

Could you please take another look when convenient?

@tianyuzhou95 tianyuzhou95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough update. The six findings from my previous review are
materially addressed by this revision, and the narrower fail-closed contract is
much clearer. I reran the new Dockerfile/context tests and reviewed the updated
RFC against the implementation. The following correctness issues still block
the direct-launch path.

Blocking findings

  1. The explicit root working directory is delegated to the backend default.
    _dockerfile_runner.py lines 253-257
    passes cwd=None whenever the accumulated Dockerfile WORKDIR is /, and
    the startup path repeats this at lines 741-746. None means "use the backend
    default", not /; the actor backend creates a temporary default directory
    and resolves a missing cwd to it. Consequently, FROM ubuntu followed by
    RUN pwd runs under /tmp/sandbox_*, and a Dockerfile without WORKDIR
    starts its CMD there as well, despite the RFC defining / as the baseline.
    Please pass the explicit / through for RUN, startup, tar, and ownership
    commands instead of converting it to None.

  2. COPY drops source file metadata, including the executable bit.
    _materialize()
    recreates every selected source with open(..., "wb"), but neither the
    context abstraction nor the copy plan records its mode. In a minimal local
    reproduction, a 0755 entrypoint.sh was staged as 0664; no later chmod
    restores it. Thus the supported pattern
    COPY entrypoint.sh /usr/local/bin/entrypoint commonly produces a
    non-executable startup file.
    Docker's supported COPY semantics preserve file metadata and permissions.
    The context/manifest needs to carry and apply the relevant metadata, or the
    advertised COPY contract must fail closed for semantics it cannot preserve.

  3. Advertised directory COPY cannot represent empty directories.
    DockerContext.walk()
    enumerates files only, and the manifest derives directories only from those
    file paths. For a context containing an empty empty/ directory,
    check_direct_launch() reports COPY empty/ /srv/empty/ as launchable, but
    walk() is empty and _validate_copy_plan() fails with "sources select no
    files". Since literal directories are explicitly in the strict subset, the
    manifest must represent directory entries and the runner must create selected
    empty directories (including empty directories nested under non-empty trees).

  4. A USER before local tar ADD makes the supported ADD fail.
    _extract_tar()
    creates the destination through the root-owned filesystem facade and then
    wraps tar xf with the accumulated non-root USER. For example, USER app
    followed by ADD app.tar /opt/app/ attempts to extract as app into the
    newly root-owned /opt/app, so it normally fails with permission denied.
    COPY does not switch to this user either. COPY/ADD should run with builder
    ownership semantics, with the validated --chown applied afterward, rather
    than applying runtime USER to tar extraction.

  5. Valid USER user:group and numeric USER forms are silently changed.
    wrap_user()
    unconditionally discards the group. USER app:staff therefore runs with
    app's default group instead of staff. USER 1000:1001 is also reported
    as directly launchable, but becomes runuser -u 1000; runuser treats that
    as a user name and fails when no passwd entry named 1000 exists, whereas a
    numeric Docker USER does not require such an entry. Please preserve these
    forms, or reject the unsupported forms during strict parsing rather than
    changing their meaning at execution time.

I ran the 117 directly affected unit tests on Python 3.10, 3.11, and 3.12; all
pass. The focused reproductions above cover cases not present in that suite.

Preserve the explicit root working directory and represent Docker build
context entries with file type and permission metadata so COPY retains
executable modes and empty directories. Run archive extraction with builder
ownership and reject USER forms that cannot be represented faithfully by the
current command facade.

Add focused regressions for directory targets, modes, ownership, strict USER
validation, and live-example coverage for the corrected behavior.

Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
@Peng-YM

Peng-YM commented Aug 12, 2026

Copy link
Copy Markdown
Author

Thanks for the focused follow-up. I addressed all five findings in a separate commit, cc53726, without rewriting the original implementation commit.

1. Explicit root working directory

The Dockerfile runner now passes the accumulated absolute workdir to every commands.run() call, including the baseline /. This covers RUN, startup dispatch, tar path checks, tar extraction, chmod, and chown; it no longer delegates / to a backend-specific cwd=None default.

Regression coverage asserts that every runner command receives / when no WORKDIR is declared. The live example also verifies both RUN pwd and the resolved ENTRYPOINT + CMD startup process execute in /.

2. COPY file metadata and executable mode

The public context protocol now exposes a frozen DockerContextEntry(path, kind, mode) for each file and directory. mode is constrained to a native integer permission value from 0o000 through 0o777; non-native integer subclasses are rejected before they can reach shell formatting.

LocalDockerContext.walk() records stat.S_IMODE for regular files and directories. Custom contexts use the same structured protocol. COPY materializes file content during preflight and then restores each selected child entry's mode through an exact, non-recursive, root-owned chmod. The literal source-directory root remains a content container and does not overwrite the destination container's mode, matching Docker's directory-copy semantics.

The unit suite covers 0755, ordinary file modes, directory modes, multiple literal directories, and root destinations. The real-sandbox example confirms an executable remains 0755.

3. Empty directories in directory COPY

The context manifest now represents directory entries directly instead of inferring them only from files. Local contexts enumerate all directories deterministically, including top-level and nested empty directories; custom contexts must also provide every directory ancestor explicitly. Missing ancestors, duplicate paths, file-as-ancestor conflicts, symlinks, and special files fail closed.

Literal directories, ., and wildcard expansion retain selected directory entries. The runner creates empty and nested directories while preserving child directory modes. It also handles COPY src /, COPY empty /, and multiple literal sources sharing one destination marker without modifying / itself.

.dockerignore filtering now applies to directory entries as well. Unit tests cover ignored empty directories, top-level and nested empty directories, literal-directory root targets, and local/custom context parity. The maintained live example exercises both COPY . and an explicit empty-directory COPY.

4. USER before local tar ADD

COPY and ADD now always use builder/root ownership semantics and no longer receive the accumulated runtime USER. Local tar extraction runs as root even after USER app; validated --chown is applied afterward to the exact outputs of the current instruction.

The previous test that expected runuser around tar xf was corrected. Unit tests now assert the wrapper is absent, --chown remains exact and non-recursive, and pre-existing destination files retain their ownership. A real standalone section runs USER app before local tar ADD and completes successfully.

5. Group-qualified and numeric USER forms

The direct-launch subset now explicitly supports named users only, such as app and root. USER app:staff, numeric UID, and UID:GID forms are classified as unsupported_syntax by non-strict diagnostics and rejected by strict parsing before backend creation. wrap_user() performs the same defensive validation and no longer strips a group or treats a numeric identity as a username.

The README, RFC, public API documentation, maintenance contract, and example prechecks now describe and exercise this narrower fail-closed rule.

Validation

  • make sdk-check: 208 tests passed; Ruff passed; mypy passed for 24 source files.
  • Independent final verification: no remaining P0/P1 findings for this review set.
  • Real standalone runsc validation through the backend-neutral facade passed all five maintained sections, including:
    • executable and directory permission preservation;
    • top-level, nested, and literal empty-directory copies;
    • root cwd for RUN and startup dispatch;
    • USER app followed by builder/root local tar extraction;
    • fail-closed remote ADD, USER app:staff, and numeric UID:GID prechecks.

The RFC issue and PR description have also been updated to reflect the corrected public contract and current test evidence. Could you please take another look when convenient?

@tianyuzhou95 tianyuzhou95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused follow-up. I verified that the five findings from the
previous review are addressed in cc53726: root cwd is now explicit, COPY
modes and empty directories are represented, tar ADD uses builder ownership,
and unsupported USER forms fail closed. Two runtime correctness issues and one
quality-gate issue remain.

Blocking findings

  1. Wildcard directory sources retain an extra directory level.
    _select_wildcard()
    prefixes every child of a matched directory with
    posixpath.basename(directory). For a context containing
    sub/dir1/dir2/foo, COPY sub/* /subdest/ therefore writes
    /subdest/dir1/dir2/foo. Docker/BuildKit treats the matched dir1 as a
    directory source and copies its contents, so the expected target is
    /subdest/dir2/foo; this exact case is covered by BuildKit's upstream
    testCopyWildcards.
    The strict subset advertises wildcard directory support, so the matched
    directory root needs destination-marker semantics rather than being added
    to each relative target.

  2. Unreadable directories are silently copied as empty directories.
    LocalDockerContext.walk()
    calls os.walk() without an onerror callback. Python consequently ignores
    a permission error while descending into a directory but still records the
    directory entry seen from its parent. In a minimal reproduction, a mode
    000 directory containing required.txt was emitted as one empty directory
    entry; COPY blocked /app then succeeded with only mkdir /app and silently
    dropped required.txt. This violates the RFC's fail-closed context contract.
    Please make traversal errors raise DockerContextError and add a regression
    covering an unreadable selected directory.

  3. The claimed 208-test gate depends on the caller's umask.
    test_local_and_memory_contexts_match
    creates local files and directories using ambient modes, while
    MemoryDockerContext hard-codes 0644/0755. With the common umask 0002,
    local entries are 0664/0775, so the complete unit gate consistently runs
    208 tests with one failure on Python 3.10, 3.11, and 3.12. The same gate
    passes after forcing umask 0022. Please assign explicit modes in this test
    so make sdk-check is hermetic.

Ruff and mypy pass locally. The updated head currently has no GitHub check runs.

Match Docker source wildcards one path segment at a time and copy the contents
of matched directories without retaining their root name. Fail closed when a
local context traversal cannot descend into an entry, and keep context parity
tests independent of the caller's umask.

Add regressions for wildcard directory targets, filepath-style double-star
matching, traversal failures, destination ownership, and the maintained live
example.

Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
@Peng-YM

Peng-YM commented Aug 13, 2026

Copy link
Copy Markdown
Author

Thanks for the latest verification. I addressed all three findings in a new, separate commit, 1f4870c; the earlier commits remain unchanged.

1. Wildcard directory sources

Wildcard-matched directory roots now use destination-marker semantics. Their contents are merged into the destination without retaining the matched root basename, while directly matched files continue to use their basename.

For the reported case:

COPY sub/* /subdest/

with sub/dir1/dir2/foo, the SDK now creates /subdest/dir2/foo and does not create /subdest/dir1. The same result is produced when the destination omits the trailing slash.

The implementation also handles mixed file/directory matches, multiple matched directories, empty matched directories, root destinations, and target collisions. Multiple directory markers may share one destination, but colliding non-marker content fails before any sandbox operation. New destination directories are included in exact --chown handling; existing destinations and / are not changed.

During independent verification, I also found and corrected a related source-pattern issue: Docker uses Go filepath.Match-style segment matching, so ** does not recursively cross /. The SDK now matches one path segment at a time. For example, modules/** matches the immediate one and two directories and copies their contents to /dest/x.py and /dest/y.txt, while src/**/*.py does not match src/a.py.

2. Unreadable context directories

LocalDockerContext.walk() now provides an onerror callback to os.walk(). Any traversal failure is immediately wrapped as DockerContextError, includes the affected relative path, and preserves the original OSError as its cause. Directory and file lstat() failures follow the same fail-closed path.

Regression tests inject PermissionError from os.scandir() for a non-empty directory containing required.txt. They verify that:

  • direct walk() fails instead of returning an empty directory entry;
  • manifest construction preserves the error and cause chain;
  • COPY blocked /app fails before any filesystem or command operation in the sandbox.

3. Umask-independent quality gate

The local/custom context parity fixture now explicitly sets context files to 0644 and directories to 0755. It does not alter process-global umask and continues to compare permission metadata.

The full quality gate was run from a subshell with umask 0002:

211 tests passed
Ruff passed
mypy passed for 24 source files

Additional validation

  • Independent final verifier: APPROVE, with no P0/P1 findings.
  • The maintained five-section example passed again against the real standalone runsc deployment.
  • Its core section now executes COPY wild/* /srv/wild/ and verifies /srv/wild/dir2/foo exists while /srv/wild/dir1 does not.
  • Existing coverage for .dockerignore, file/directory modes, empty directories, root cwd, COPY --chown, builder/root tar ADD, strict USER, and preflight side-effect prevention remains green.

The RFC issue and PR description have been updated with the wildcard matching, traversal failure, 211-test, umask, and live-example evidence. Could you please take another look when convenient?

@tianyuzhou95 tianyuzhou95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. The three findings from the previous review are addressed in this revision: wildcard-matched directory roots now have the expected destination-marker behavior, local traversal failures fail closed, and the context parity test is independent of the caller's umask. Two Dockerfile-semantics issues still need to be resolved before merge.

pattern_segments = pattern.split("/")
path_segments = path.split("/")
return len(pattern_segments) == len(path_segments) and all(
fnmatch.fnmatchcase(path_segment, pattern_segment)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: fnmatch.fnmatchcase() does not implement the Go filepath.Match semantics promised by the RFC and used by Dockerfile source patterns. In particular, Python uses ! for character-class negation while Go uses ^: with a.txt and b.txt, [!a].txt selects b.txt here but a.txt under Go, and [^a].txt has the opposite behavior. Python also treats a malformed pattern such as file[.txt as a literal match, whereas filepath.Match returns ErrBadPattern. This can silently copy the wrong context files instead of failing closed. Please use a Go-compatible matcher with malformed-pattern validation, or reject character-class syntax if it is outside the supported subset.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 5736dc1.

I replaced fnmatch.fnmatchcase() with a precompiled matcher for the strict no-escape subset of Go filepath.Match. Only leading ^ negates a class, ! remains a literal member, ranges and Unicode codepoints follow Go behavior, and */?/classes stay within one path segment. Every pattern is compiled before context entries are inspected, so malformed classes fail even for an empty context and before any file read or sandbox operation.

The tests cover [!a], [^a], ranges, Unicode ?, single-segment **, empty/unclosed classes, and malformed ranges. An independent verifier also compared 1,368 pattern/name cases against Go filepath.Match with no mismatch. Backslash escapes remain explicitly outside the documented strict subset.

or selection.has_directories
or ins.dest.endswith("/")
)
must_use_directory = selection.has_directories or len(selection.entries) > 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this guard cannot reject wildcard expansions containing directories, because selection.has_directories makes both must_use_directory and directory_target true; the later not directory_target condition is therefore false. For a context containing one/a and two/b, COPY * /target currently succeeds and writes /target/a and /target/b. Docker requires the destination to end in / whenever a wildcard expands to multiple sources, just as it does for multiple explicit sources. Please retain/count the top-level wildcard matches and reject a multi-source expansion without a trailing slash, while preserving the valid single-directory case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 5736dc1.

_ContextSelection now records the number of top-level wildcard sources after .dockerignore filtering: each matched directory root counts once regardless of descendants, and each directly matched file counts once. The runner rejects an expansion count greater than one when the destination does not end in /, before materialization or any sandbox operation.

Regression coverage includes two directories, two files, and a mixed directory/file expansion, with no-trailing-slash rejection and trailing-slash success. A single directory or single file without a trailing slash remains valid, and marker, mode, collision, and --chown behavior remains covered.

Match Dockerfile source patterns with Go filepath-compatible character
classes and reject malformed patterns before context files are read. Track
top-level wildcard expansions so multi-source copies require a directory
destination.

Add regressions for class negation, malformed ranges, Unicode matching,
ignored sources, and wildcard expansion counts.

Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
@Peng-YM

Peng-YM commented Aug 14, 2026

Copy link
Copy Markdown
Author

Thanks for the follow-up. Both blocking inline comments are addressed in a new, separate commit, 5736dc1; no earlier commit was rewritten.

Go-compatible source patterns

The SDK no longer delegates Dockerfile source matching to Python fnmatch. It now precompiles the strict no-escape subset of Go filepath.Match before inspecting context entries:

  • only a leading ^ negates a character class;
  • ! is an ordinary class character;
  • *, ?, classes, ranges, Unicode codepoints, and single-segment ** follow Go behavior;
  • malformed or empty classes and invalid ranges fail closed, including for an empty context;
  • malformed patterns fail before context file reads and before sandbox operations;
  • backslash escapes remain explicitly rejected by the documented strict subset.

The new unit coverage includes [!a].txt, [^a].txt, ranges, Unicode ?, **, and multiple malformed forms. Independent verification compared 1,368 pattern/name combinations against Go filepath.Match without a result or error-state mismatch.

Multi-source wildcard destinations

_ContextSelection now retains the top-level wildcard expansion count after .dockerignore filtering. A matched directory root counts once regardless of how many descendants it contains; each directly matched file counts once.

When one wildcard expands to multiple top-level sources, the runner now requires the destination to end in / and rejects the Dockerfile before materialization or any sandbox side effect. Tests cover:

  • two directories;
  • two files;
  • one directory plus one file;
  • valid trailing-slash variants;
  • valid single-directory and single-file expansions without a trailing slash;
  • ignored sources, destination markers, collisions, modes, and exact --chown behavior.

Validation

  • (umask 0002; make sdk-check): 214 tests passed; Ruff passed; mypy passed for 24 source files.
  • Independent final verifier: APPROVE, no P0/P1/P2 findings.
  • The maintained five-section example passed again on the real standalone runsc deployment through the backend-neutral facade.
  • The RFC issue and PR description now document character-class semantics, malformed-pattern rejection, top-level expansion counting, and the 214-test result.

I also replied directly to both inline discussions. Could you please take another look when convenient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants