feat: add @swamp/container-image extension (#794) #122
Loading…
Reference in a new issue
No description provided.
Delete branch "issue-794-container-image"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
New
@swamp/container-imageextension — runtime-agnostic container imagelifecycle management via Docker, Podman, or Apple Containers.
validate,build,run,login,push,multi-platform-buildglobalArguments.binaryexportFormat/exportPathz.meta({ sensitive: true })+--password-stdin, never in argv or resourcestlsVerifyoption on login/push (Podman:--tls-verify=false, Apple Containers:--scheme http)Runtime Support Matrix
validatedocker infopodman infocontainer system statusbuilddocker buildpodman buildcontainer buildrundocker run --rmpodman run --rmcontainer run --rmlogindocker loginpodman logincontainer registry loginpushdocker pushpodman pushcontainer image pushmulti-platform-builddocker buildx buildpodman build --platform+ pushTest plan
Closes #794
CI Security Review
Medium
Missing
container-image-checkandcontainer-image-lockfileinmerge-gateneeds (.forgejo/workflows/ci.yml:1131-1164)The new
container-image-checkandcontainer-image-lockfilejobs were correctly added to theneedslists ofclaude-review(lines 759-760) andclaude-adversarial-review(lines 898-899), but they are not listed in themerge-gatejob'sneeds(lines 1131-1164). Every other extension type (vault-*,datastore-*,ssh-*,kubernetes-*,workflows-*,cve-*,software-factory-*) has its check and lockfile jobs in merge-gate's needs —container-imageis the sole exception.Impact: If
container-image-checkorcontainer-image-lockfilefails, the merge-gate will still pass. Theclaude-reviewjob depends on them, but with an!failure()condition — a failed dependency causes the review to be skipped, not failed. The merge-gate checkscontains(needs.*.result, 'failure'), which treatsskippedas passing. This means container-image code changes with broken checks or tests can be merged.Fix: Add
container-image-checkandcontainer-image-lockfileto themerge-gatejob'sneedslist, consistent with all other extension types:Notes (no findings)
needsarrays.${{ matrix.task }}inrun:blocks, but the matrix values are statically defined as[check, lint, fmt, test]— not user-controlled. This pattern is consistent with all other extension check jobs.pull_request(notpull_request_target), which is the safe pattern.actions/checkout@v6anddenoland/setup-deno@v2, both trusted publishers with tag-only pins, consistent with the rest of the workflow.contents: read— no additional permissions requested. Correct and minimal.Verdict
PASS — The changes are a straightforward, well-structured addition of CI jobs for the new
container-imageextension, following established patterns. The missing merge-gate entries are a medium-severity gap that should be fixed but do not represent an exploitable security vulnerability (thepull_requesttrigger runs without secrets in the PR context).Code Review
Blocking Issues
container-image-checkandcontainer-image-lockfilemissing frommerge-gateneeds(ci.yml:1128–1173)The new CI jobs
container-image-checkandcontainer-image-lockfileare correctly wired intoclaude-review'sneeds(lines 769–770), but they are absent from themerge-gatejob'sneedslist. Every other extension follows the pattern of listing its check and lockfile jobs directly in the merge gate (e.g.ssh-check,ssh-lockfile,vault-check, etc.). Without these entries, the gate'scontains(needs.*.result, 'failure')test cannot observe a failure from these jobs.The indirect path through
claude-reviewdoesn't rescue this: ifcontainer-image-checkfails,claude-reviewevaluates!failure()→falseand is skipped (result:'skipped'). The merge gate seesskipped, notfailure, and passes. A broken container-image extension can therefore be merged without any gate blocking it.Fix: Add
container-image-checkandcontainer-image-lockfileto the merge gate'sneedsarray alongside the other extension check jobs.Suggestions
Duplicate tag in podman buildx argv (
runner.ts:219,230)In
buildBuildxArgvfor podman,args.tags[0]is added twice: first via the-tloop over all tags (argv.push("-t", tag)) and then again asargv.push("--manifest", args.tags[0]). In podman,--manifestnames the manifest list and-ttags individual platform images; both pointing at the same value may be redundant or could produce unexpected behavior (e.g. overwriting the manifest list name with an image tag). This is worth verifying against real podman behavior.Direct
npm:specifier inschemas.tsinstead of import map alias (schemas.ts:26)import { z } from "npm:zod@4.3.6"bypasses the"zod"alias indeno.json. Usingimport { z } from "zod"would keep the import consistent with how other files in the repo consume mapped dependencies and would letdeno.jsonbe the single source of truth for the pinned version.claude-adversarial-reviewdoesn't trigger for container-image-only PRs (ci.yml:921)The adversarial review job's
ifcondition gates on vaults, datastores, issue-lifecycle, kubernetes, workflows, cve, software-factory, and codegen — but notneeds.changes.outputs.container-image. This PR therefore won't receive adversarial review, unlike changes to the other handwritten extension types. If this is intentional, a comment explaining the exclusion would help; otherwise|| needs.changes.outputs.container-image == 'true'should be added to the condition.8c43dc7114ac87de5b9eCI Security Review
Summary of Changes
This PR adds CI support for a new
container-imageextension by:container-imageto the change detection outputs andcheck_pathcallscontainer-image-check(check/lint/fmt/test matrix) andcontainer-image-lockfilejobscontainer-imageto thedeps-auditfind pathneedslists ofclaude-reviewandclaude-adversarial-reviewAnalysis
1. Prompt Injection — No new LLM interactions introduced. The new jobs are standard Deno CI tasks. The only change to LLM-invoking jobs (
claude-review,claude-adversarial-review) is adding the new jobs to theirneedsdependency lists, which does not affect prompt construction or tool scoping.2. Expression Injection — The new jobs use
${{ matrix.task }}inrun:blocks (e.g.,if [ "${{ matrix.task }}" = "fmt" ]). The matrix values are hardcoded in the workflow file as[check, lint, fmt, test]and are not attacker-controlled. This follows the same pattern as all other extension check jobs in this workflow.3. Dangerous Triggers — No trigger changes. Workflow remains on
pull_request(notpull_request_target), which runs in the PR context without automatic access to base-repo secrets.4. Supply Chain — Uses
actions/checkout@v6anddenoland/setup-deno@v2, both from trusted publishers (tag-only pins acceptable per policy). No new third-party actions, nocurl | bashpatterns.5. Permissions — New jobs inherit
contents: readfrom the workflow-level permission block. No additional permissions requested. This is appropriate for read-only check/lint/test jobs.6. Secret Exposure — The new jobs do not reference any secrets. No risk of secret leakage.
7. Auto-merge & Trust Boundaries — The new jobs are added to the dependency chains of the review gates, which is the correct pattern to ensure they must pass before merging.
Verdict
PASS — Security-neutral change. The new
container-imageCI jobs follow the exact same pattern as all other extension check/lockfile jobs in the workflow, introduce no new attack surface, and are properly wired into the merge gate dependency chain.Code Review
Blocking Issues
merge-gatedoesn't includecontainer-image-checkorcontainer-image-lockfile(ci.ymllines 1128–1173)The new
container-image-checkandcontainer-image-lockfilejobs are defined and run, but they are absent from themerge-gatejob'sneedslist. Any failure in those jobs does not block the PR from merging. Every other extension (vault, datastore, ssh, kubernetes, workflows, cve, software-factory) is wired into the gate — container-image must be too.Fix: add
container-image-checkandcontainer-image-lockfileto theneedsarray of themerge-gatejob.Comma injection in Docker OCI export
--outputflag (runner.tsline 365)Docker's
--outputparser splits its value on commas into key=value pairs.safeOptionValueblocks newlines and NUL bytes but not commas, so a user-suppliedexportPathlikeout.tar,push=truewould produce:Docker parses this as three directives:
type=oci,dest=out.tar,push=true, causing an unintended registry push. Other directives (e.g.type=registry,ref=...) could redirect output entirely.Fix: add a comma check to
safeOptionValueforexportPath, or use Docker's native--outputkey=value quoting mechanism if available, or assemble the output spec via a separate argv structure wheredestis not embedded in a comma-parsed string (Docker also accepts--output type=oci --output dest=./out.tarwith separate flags in some versions, but the cleanest fix is adding,to the blocked character set forexportPath).Stale/incorrect module docstring in
checks.tsdescribes alogin-supportedcheck that doesn't exist (checks.tslines 23–29)The module-level JSDoc says:
No
checkLoginSupportedfunction is exported, and the extension'schecksmap incontainer_image.tshas nologin-supportedentry. Worse, the description is factually wrong — the implementation does support Apple Containers for login viacontainer registry login(runner.ts line 194). Future maintainers reading this docstring will be misled into believing Apple Containers can't log in, or will expect a check that doesn't exist.Fix: remove the
login-supportedbullet from the docstring (or add the check if the behaviour was intentional). Update the description to match the actual runtime matrix.Suggestions
Podman multi-platform silently drops tags beyond the first (
runner.tslines 231, 293)buildBuildxArgvfor podman uses onlyargs.tags[0]for--manifest, andbuildPodmanManifestPushArgvonly pushesargs.tags[0]. If a caller passestags: ["registry.example.com/app:latest", "registry.example.com/app:v1.2.3"]expecting both to be pushed, the second tag is silently dropped with no error or warning. Consider validating thattags.length === 1for podman inbuildBuildxArgv, or logging a warning inrunBuildxBuild.runValidatehas no tests (container_image_test.ts)runValidatecontains non-trivial logic: it fans out tocheckRuntimeAvailable,probeVersion,probeDaemon, andcheckBuildxAvailable, then derives amethodsmap and conditionally throws.probeVersionandprobeDaemonare untested private functions. The mock executor seam (setCommandExecutor) used throughout the test file could cover these paths.claude-adversarial-reviewskipscontainer-imagechanges (ci.ymlline 921)The adversarial review's
ifcondition lists vaults, datastores, issue-lifecycle, kubernetes, workflows, cve, software-factory, and codegen — but notcontainer-image. PRs touching only the container-image extension will not receive an adversarial review, unlike every other non-model extension. Consider addingneeds.changes.outputs.container-image == 'true'to the condition.buildArgsrecord keys are not validated againstsafeOptionValue(schemas.tsline 90,runner.tsline 113)The value side is plain
z.string(), notsafeOptionValue. A key or value containing a newline would be passed as--build-arg KEY\nVALUE, which the container runtime would likely reject but the schema layer doesn't catch first. Consistent with the rest of the schema hardening, both keys and values inbuildArgsandlabelsmaps could usesafeOptionValue.CI Security Review
Analysis
This PR adds CI jobs for a new
container-imageextension, following the identical pattern used by all other extensions in this workflow. The changes are:container-imagechange detection (lines 31, 70): Adds output andcheck_pathfor thecontainer-image/directory — follows the same pattern as all other extensions.container-image-checkjob (lines 453–480): Runs check/lint/fmt/test matrix — structurally identical tosoftware-factory-check,ssh-check, etc.container-image-lockfilejob (lines 482–496): Verifies lockfile — structurally identical to other lockfile jobs.deps-auditupdate (line 720): Addscontainer-imageto thefindsearch paths for outdated dependency scanning.needslist updates (lines 761–762, 900–901, 1148–1149): Adds the new jobs toclaude-review,claude-adversarial-review, andmerge-gatedependency chains.container-imageto the trigger condition so adversarial review runs when container-image files change.Checklist evaluation:
needslists. No new vectors.${{ matrix.task }}used inrun:block (line 472), but values are hardcoded in the workflow matrix ([check, lint, fmt, test]) — not attacker-controlled. Same pre-existing pattern as all other jobs.pull_requestonly (notpull_request_target).actions/checkout@v6anddenoland/setup-deno@v2— same trusted-publisher actions as all existing jobs. No new dependencies.contents: read. No escalation.merge-gatecorrectly includes both new jobs in itsneedslist, so they must pass before merge.Verdict
PASS — Security-neutral change. The new
container-imageCI jobs are structurally identical to existing extension jobs, introduce no new secrets usage, no new triggers, no new LLM interactions, and are correctly wired into the merge gate.CI Security Review
Summary
This PR adds CI support for a new
container-imageextension by:container-image/pathcontainer-image-checkandcontainer-image-lockfilejobscontainer-imagein the dependency auditfindscopeneedslists ofclaude-review,claude-adversarial-review, andmerge-gatecontainer-imageto the adversarial review trigger conditionAll changes follow the exact same patterns established by the other extension jobs (ssh, kubernetes, software-factory, etc.).
Checklist Results
Prompt Injection: No new LLM interactions introduced. The existing review steps are unchanged in prompt construction. No findings.
Expression Injection: The new jobs use
${{ matrix.task }}inrun:blocks, but the matrix values are hardcoded to[check, lint, fmt, test]— repo-controlled, not attacker-influenced. This matches the pattern used by every other extension check job. No findings.Dangerous Triggers: No trigger changes. The workflow uses
pull_request(notpull_request_target). No findings.Supply Chain: The new jobs use
actions/checkout@v6anddenoland/setup-deno@v2— both trusted publishers acceptable with tag-only pins per policy. No new third-party actions or remote script execution introduced. No findings.Permissions: The new jobs inherit the workflow-level
contents: readdefault. No elevated permissions needed or requested. No findings.Secret Exposure: No secrets are used in the new container-image jobs. No findings.
Auto-merge & Trust Boundaries: No changes to merge gate logic beyond adding the new jobs as dependencies, which strengthens the gate. No findings.
Verdict
PASS — Security-neutral change. The new
container-imageCI jobs are a mechanical extension of existing patterns with no new attack surface.Adversarial Review
Critical / High
None.
Medium
Silent no-op when exportFormat is set without exportPath —
container-image/extensions/models/_lib/operations.ts:245The guard
if (args.exportFormat && args.exportPath)means if a user sets exportFormat to oci but forgets exportPath, the build silently succeeds without exporting. The returned resource will contain exportFormat oci and exportPath undefined, giving no indication that export was skipped.Breaking example: A workflow passes exportFormat oci without exportPath. The build succeeds, the resource says exportFormat oci, but no archive file is written. The next workflow step expecting the archive fails with a confusing file-not-found error.
Suggested fix: Add cross-field validation — either a z.refine on BuildArgsSchema requiring exportPath when exportFormat is set (and vice versa), or a runtime check in runBuild that throws early when only one of the pair is provided.
Docker silently ignores tlsVerify false for login and push —
container-image/extensions/models/_lib/runner.ts:197-199andrunner.ts:330When binary is docker and tlsVerify is false, no flag is passed to docker login or docker push. Docker does not support a per-command --tls-verify flag — it requires insecure-registries in /etc/docker/daemon.json. The user thinks TLS verification is disabled but it is not.
Breaking example: User configures binary docker with server my-http-registry:5000 and sets tlsVerify false. Login fails with a TLS error despite the explicit opt-out, because Docker ignores the flag entirely.
Suggested fix: When binary is docker and tlsVerify is false, log a warning via ctx.logger.warn explaining that Docker requires daemon-level configuration for insecure registries, or document this in the schema description for tlsVerify.
No resource written when build succeeds but export fails —
container-image/extensions/models/_lib/operations.ts:245-267If runBuildCommand returns exitCode 0 but runExportCommand fails, the function throws at line 263 without ever calling ctx.writeResource. This is inconsistent with the build-failure path (lines 231-242) which writes a resource before throwing. The build stdout/stderr is lost.
Breaking example: Build succeeds, but export fails due to a full disk. The user gets an export-failed error but has no resource to inspect the successful build output. In a build-failure case, they would have a resource with the full stdout/stderr.
Suggested fix: Write the buildResult resource before attempting the export step, so the build output is always captured regardless of export outcome.
Low
probeDaemon and probeVersion bypass the exec test seam —
container-image/extensions/models/_lib/operations.ts:82-127These functions spawn Deno.Command directly instead of going through the exec function in runner.ts. The validate success path (runtime found, daemon probed) is untestable without a real container runtime. Current tests only cover the runtime-not-found path. Not blocking — a testability concern, not a correctness issue.
stdin writer lacks try/finally for close —
container-image/extensions/models/_lib/runner.ts:90-92If writer.write throws, writer.close is never called. The pipe is cleaned up when the child process exits, and the data is always small (passwords, tiny Dockerfiles), so this is unlikely to matter in practice.
Verdict
PASS — The code is solid. Command injection is properly prevented via Deno.Command with array args and schema-level newline/NUL/comma rejection. Passwords never appear in argv or persisted resources. Error handling is mostly consistent, and tests cover the main paths with proper mock cleanup in finally blocks. The medium findings are real UX papercuts (silent no-ops, missing resources) but none are security issues, data corruption, or crash-in-production risks.
Code Review
Blocking Issues
None.
Suggestions
probeDaemonandprobeVersionbypass the mock executor seam (operations.ts:82-127). Both functions callnew Deno.Command(...)directly instead of going throughexec(), so the validate success path (runtime found, daemon reachable) cannot be exercised in CI without Docker or Podman actually installed. Every existing validate test usesnotFoundProbe(returns false), which short-circuits theif (runtimeFound)block and never reaches either probe. If you want that code path covered, extract them behind the same injectable pattern used byBinaryProbe/BuildxProbe, or accept the gap and add a// tested manuallynote.Docker's
tlsVerify=falseis silently ignored forloginandpush(runner.ts:197-200,runner.ts:330). Podman and Apple Containers both handle the flag; Docker does not (insecure registries must be configured indaemon.jsoninstead). A user who setstlsVerify: falsewithbinary: "docker"will get no warning. Adding actx.logger.warn(...)— or documenting the limitation in the schema description — would prevent a confusing silent no-op.buildwithexportFormat: "oci"andbinary: "docker"silently requires buildx (runner.ts:356-365). The OCI export path emitsdocker buildx build --output type=oci,dest=... -, which fails at runtime if the buildx plugin is absent. Thebuildx-availablepre-flight check only applies tomulti-platform-build, notbuild, so there's no early signal. Worth noting in theexportFormatschema description (e.g. "OCI export with Docker requires the buildx plugin") or adding a runtime check before spawning the command.README argument tables for
loginandpushomittlsVerify(README.md:198-202,README.md:163-166). TheLoginArgsSchemaandPushArgsSchemaboth havetlsVerify: z.boolean().default(true), but neither table documents it. Users on insecure registries need to know it exists.@systeminit/swamp-testingis imported indeno.jsonbut unused (deno.json:16). None of the test files import from this package. If it's not needed for this extension type, remove the import to keep the lockfile minimal; if conformance helpers for model extensions are planned, a TODO comment would clarify intent.