fix(kubernetes): prevent kubectl flag injection in pod exec #6
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/kubernetes-exec-flag-injection"
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
Security fix for kubectl flag injection vulnerability in the
@swamp/kubernetespod exec method, plus a CI fix for the Write tool blocking in review agents.Flag injection in pod exec (
kubernetes/extensions/models/pod.ts)The
execmethod passesargs.podNamedirectly into the kubectl args array at the second position (immediately after"exec"). BecauseDeno.Commandpasses each element verbatim to the OS, a pod name starting with-is interpreted by kubectl as a flag rather than a positional argument.Example exploit: A pod name like
--as=system:masterswould cause kubectl to impersonate a cluster-admin identity:The
--separator at the end of the args only separates kubectl options from the exec command — it does not protect the positional pod name argument that comes before it.Fix: Added a Kubernetes DNS label validation guard (
/^[a-z0-9][a-z0-9\-.]*$/) at the top of the exec method. This rejects any pod name that doesn't conform to Kubernetes naming rules, which inherently blocks flag-like values starting with-. This is the correct approach because:-,.)-CI review agent Write tool blocking (
.forgejo/workflows/ci.yml)The
--allowedToolslist for the three CI review jobs (claude-review, claude-adversarial-review, claude-ci-security-review) did not includeWrite. The review prompts instruct Claude to useteeto write output, but the model sometimes chooses theWritetool instead. SinceWritewasn't in the allowed list, it triggered an approval prompt — which blocks indefinitely in non-interactive CI mode, preventing the review from completing.Fix: Added
Writeto--allowedToolsfor all three review jobs.Version bump
All 15 model files and
manifest.yamlbumped from2026.05.27.1→2026.05.27.2with upgrade entries added to each model.Changed files
kubernetes/extensions/models/pod.ts— security fix + version bump + upgrade entrykubernetes/extensions/models/{configmap,deployment,event,hpa,ingress,job,namespace,netpol,node,pod_summary,pvc,rbac,secret,service}.ts— version bump + upgrade entrykubernetes/manifest.yaml— version bump.forgejo/workflows/ci.yml— add Write to allowedToolsTest plan
deno check extensions/models/*.ts— all 15 models passdeno lint extensions/models/— cleandeno fmt --check extensions/models/— clean-at runtimeIssue originally identified by the CI adversarial code review agent.
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
CI Security Review
Critical / High
Writetool included in LLM--allowedToolsis overly broad —.forgejo/workflows/ci.yml:730, 844, 935Vulnerability: All three Claude review jobs (
claude-review,claude-adversarial-review,claude-ci-security-review) includeWritein their--allowedToolslist. TheWritetool provides unrestricted file system write access within the runner workspace.Attack scenario: The LLM reads the content of every changed file during review. An attacker submits a PR containing a file with embedded prompt injection in its source code (e.g., comments, string literals, or docstrings designed to override the system prompt). If injection succeeds, the attacker-controlled LLM can use
Writeto:review-body.mdwith a fake "PASS — no issues found" verdict, causing the review gate to pass on malicious code.The
Bashtool is correctly scoped to onlyteeandtouchfor review output — butWritebypasses that scoping entirely, since it can write to any file path.Fix: Remove
Writefrom--allowedToolsin all three Claude review steps. The agent only needs to produce review output, which is already covered by the scoped Bash commands (tee ${OUTPUT_DIR}/review-body.md:*andtouch ${OUTPUT_DIR}/review-failed). The corrected tools list:Medium
pull-requests: writepermission may be unnecessary —.forgejo/workflows/ci.yml:679, 793, 884The three Claude review jobs declare
pull-requests: writeat the job level. However, review comments are posted via direct Forgejo API calls usingsecrets.BOT_TOKEN, not via theGITHUB_TOKEN/ runner token. Ifpull-requests: writeis not consumed by any step using the runner token, it grants a broader permission scope than needed. Verify whether the Forgejo runner token actually requires this permission for the API calls; if not, remove it to follow least-privilege.Claude binary fetched via
curlfrom remote URL —.forgejo/workflows/ci.yml:714, 829, 920The Claude CLI is downloaded at runtime via
curlfromdownloads.claude.ai. This is mitigated by SHA256 checksum verification (good), version pinning (good), andcurl -fto fail on HTTP errors. This is comparable in security to SHA-pinned actions. However, vendoring the binary or using a first-party action would eliminate the runtime dependency on an external download server entirely. Noting as medium because the current checksumming is adequate but the pattern could be hardened further.Low
No low-severity findings.
Security Positives
pull_request(notpull_request_target) — PR code runs in the PR context, not the base repo context with secrets.run:blocks. TheCHANGED_FILESvariable contains only file paths fromgit diff.BASE_SHAandHEAD_SHAare passed viaenv:and quoted in the shell, not interpolated directly — correct mitigation for expression injection..forgejo/prompts/review.md,adversarial.md,ci-security.md) include security preambles instructing the model to treat PR content as untrusted data.Bashtool is restricted togit diff,git log,tee, andtouch— nocurl,wget, or arbitrary command execution.actions/checkout@v6anddenoland/setup-deno@v2are from trusted publishers with tag pins, acceptable per policy.ANTHROPIC_API_KEYandBOT_TOKENare scoped to individual steps, not entire jobs.contents: read: Sets a restrictive baseline; jobs that need write permissions override explicitly.cancel-in-progress: trueto prevent stale reviews from posting.Verdict
FAIL — The
Writetool in the LLM--allowedToolslist is overly broad and should be removed from all three Claude review jobs. This is straightforward to fix (deleteWrite,from the three--allowedToolsstrings). Once fixed, this workflow has solid security posture.Code Review
Blocking Issues
None.
Suggestions
Field selector injection in
event.tsandnode.tsIn
event.ts, three methods build Kubernetes field selector strings by directly interpolating user-supplied arguments:Similarly in
node.ts:509:If a caller passes a name containing
,or=(e.g.,"mypod,involvedObject.kind=Node"), the injected text is silently added to the field selector, overriding the intendedkindfilter or introducing unexpected constraints. Kubernetes resource names are supposed to follow DNS subdomain rules (no,/=), but the code places no guard here.The blast radius is limited — worst case is empty or mis-filtered results within the caller's own cluster, not privilege escalation — so this is not blocking. A simple defensive check (
if (/[,=]/.test(name)) throw new Error(...)) would close the gap.containerargument inpod.tsexec is not validatedThe PR correctly adds pod-name validation at
pod.ts:477to prevent names starting with-from being interpreted as kubectl flags. Thecontainerparameter (used as-c <container>) has no corresponding validation. BecauseDeno.Commandreceives it as a discrete array element (not via shell), there is no injection risk, but the asymmetry is confusing to future readers. Adding a similar^[a-z0-9][a-z0-9.-]*$check (or just a "must not start with-" guard) forcontainerwould make the fix consistent.Pod name validation regex allows invalid trailing characters
The regex at
pod.ts:477is^[a-z0-9][a-z0-9\-.]*$. Kubernetes pod names must not end with-or.per DNS subdomain rules, but this regex permits it (e.g.,"mypod-"is accepted). The security goal — preventing leading-— is fully achieved, but the looser-than-spec allowance could cause confusing errors from the Kubernetes API rather than a clean validation failure. Tightening to^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$would bring it in line with the spec.codegen-checkCI matrix omitstest— new snapshot tests not enforcedcodegen/shared/zodGenerator_test.tsis a new test file with 14 test cases (snapshot + unit). Thecodegen-checkjob matrix only includes[check, lint, fmt], so these tests are never executed in CI. The snapshot file (codegen/shared/__snapshots__/zodGenerator_test.ts.snap) is committed, so the tests would pass if run manually. Addingtestto the codegen matrix (along with appropriate--allow-read --allow-write --allow-envflags) would ensure regressions are caught automatically.Adversarial Review
Critical / High
No critical or high severity findings.
Medium
Codegen: bare
{type: "object"}in resource schema now emitsz.string()instead ofz.record(z.string(), z.unknown())—codegen/shared/zodGenerator.ts:390generateSimplifiedZodForProperty(used exclusively for resource schemas — line 107) now returnsz.string()for objects with noproperties,patternProperties, oradditionalProperties. Previously it returnedz.record(z.string(), z.unknown()).Breaking scenario: If any CloudFormation resource has a property typed as
{type: "object"}with no sub-schema, and the cloud provider returns it as an actual JSON object in the resource state, the generated resource schema will reject it at runtime —z.string()cannot validate{"key": "value"}.Example: A hypothetical CF property
Metadata: {type: "object"}where CloudControl returns{"Metadata": {"foo": "bar"}}. The old schema (z.record) would accept it; the new schema (z.string()) would reject it with a Zod validation error.Mitigating factor: Most freeform JSON fields in CloudFormation schemas have
additionalPropertiesdefined (which hits line 354-357 and still emitsz.record) or are typed asjson(line 322-325, correctly handled). Truly bare{type: "object"}with nothing else may be rare. However, the existing test (test 15) was specifically written to cover this case, suggesting it was a known concern.Test coverage deleted: Test 15 (
"bare object with no properties emits z.record") was removed without a replacement test for the new behavior (zodGenerator_test.ts, lines 437-475 deleted). The newz.string()behavior for bare objects has zero test coverage. At minimum, a test asserting the new behavior should be added.Same change in
generateObjectZod(zodGenerator.ts:540) affects the input schema path identically.Suggested fix: Either (a) restore
z.record(z.string(), z.unknown())for bare objects in the resource schema path (line 390), or (b) add a test proving this case doesn't occur with real schemas and document whyz.string()is correct. For the input schema path (line 540),z.string()may be intentional if CloudControl accepts JSON-as-string — but it should still have a test.Low
Pod name regex allows
.but Kubernetes pod names are DNS labels —kubernetes/extensions/models/pod.ts:477The regex
/^[a-z0-9][a-z0-9\-.]*$/allows dots, but Kubernetes pod names must conform to RFC 1123 DNS labels:[a-z0-9]([-a-z0-9]*[a-z0-9])?(max 63 chars, no dots). Dots are only valid in DNS subdomain names (max 253 chars), used for some other Kubernetes resource names but not pods.This is overly permissive, not a security issue — a pod name with dots would simply fail kubectl lookup. But tightening the regex to match actual pod name rules would provide better error messages (fail at validation, not at kubectl).
Suggested fix:
/^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$/and optionally.max(63)on the Zod schema.CI:
Writetool added to review agent allowed tools without path restriction —.forgejo/workflows/ci.yml:730,845,936The
Writetool was added alongsideReadandGrepin the--allowedToolsfor all three review agents. Unlike theBash(tee ...)which is path-scoped to${OUTPUT_DIR}/review-body.md,Writeis unrestricted — the review agent can write to any file in the checkout. Since the agent processes untrusted PR content, a prompt injection in the PR diff could attempt to get the agent to write files.Mitigating factor: The CI runner is ephemeral and the review agent's system prompt explicitly warns against following instructions in PR content. The blast radius is limited to the CI workspace.
This is not blocking, but the principle of least privilege suggests restricting
Writeto the output directory if the tool supports path scoping.Verdict
PASS — The security fix in
pod.tsis correct and well-structured (array-basedDeno.Commandavoids shell injection,--separator protects the command array, pod name validation prevents flag injection). The kubernetes version bumps and upgrade stubs are mechanical and correct. The codegen change to bare-object handling warrants attention (medium finding #1) but is mitigated by the rarity of truly bare{type: "object"}schemas and should be verified against real generated output before merge.Adversarial Review
High
codegen/shared/zodGenerator.ts:196, 390, 539 — Reverts a deliberate bug fix, reintroducing the exact bug from issue #457
This PR changes three fallback paths from z.record(z.string(), z.unknown()) back to z.string(). This is a direct revert of commit
a3b6cfdd("fix(codegen): emit z.record for freeform-JSON properties in input schemas (#457)"), which was specifically merged to fix a production failure: CloudControl rejects String for bare-object properties like PolicyDocument — only JSONObject is accepted.The previous fix's commit message explicitly documented:
Breaking example: A user sets PolicyDocument on AWS::IAM::RolePolicy as a YAML object. With z.string(), Zod validation rejects the object (expected string, received object), so the user cannot create the resource — the exact bug from issue #457.
Three affected paths:
Impact scope: The previous fix affected 390 model files (182 properties across ~96 AWS services, ~39 GCP services). This revert affects the same scope.
Suggested fix: Do not revert these three lines. Keep z.record(z.string(), z.unknown()) as established by the fix in
a3b6cfdd.codegen/shared/zodGenerator_test.ts:401-433 — Test 15 deleted, test 14 asserts the opposite of its previous assertion
Test 15 ("bare object with no properties emits z.record") was specifically added by commit
a3b6cfddto prevent regression. Deleting it removes the safety net.Test 14 was changed from asserting z.record(z.string(), z.unknown()) for JSON-type input schemas to asserting z.string() — directly contradicting the previous test's documented rationale ("CloudControl expects JSONObject, not String").
Suggested fix: Restore test 15 and the original assertions in test 14.
Medium
kubernetes/extensions/models/pod.ts:477 — container argument not validated in exec method
The PR adds pod name validation to prevent kubectl flag injection, but the container field (line 493-494) has no equivalent validation. While Deno.Command array-style arg passing and the -c flag prefix make exploitation harder than the positional podName, the inconsistency is worth addressing for defense-in-depth. A container name like a whitespace string " " would pass the truthy check and be forwarded to kubectl.
Suggested fix: Add the same regex validation for args.container when it is provided.
Low
kubernetes/extensions/models/pod.ts:477 — Pod name regex allows trailing hyphens and dots
The regex allows names like "my-pod-" or "my-pod.", which are invalid per RFC 1123 (labels must end with an alphanumeric character). Kubernetes itself would reject these, so this is not a security issue — just an overly permissive validation that could produce confusing error messages (Kubernetes error instead of the clearer custom error).
Verdict
FAIL — The codegen changes revert a deliberate, well-documented bug fix (commit
a3b6cfdd/ issue #457), reintroducing validation failures for freeform-JSON properties across ~96 AWS and ~39 GCP services. The Kubernetes security fix in pod.ts is correct and valuable but should be separated from the codegen regression.CI Security Review
Analysis
This PR hardens the Claude Code review sandbox across three review jobs (
claude-review,claude-adversarial-review,claude-ci-security-review) in.forgejo/workflows/ci.ymlby adding:--add-dir "$OUTPUT_DIR"— scopes the agent's filesystem access to the review output directory--disallowedTools "Write" "Edit"— explicitly blocks file-modification tools, forcing the agent to use onlyteevia the already-scopedBashallowlistThe three prompt files (
.forgejo/prompts/review.md,ci-security.md,adversarial.md) are updated to reinforce that the Write tool is unavailable, reducing the chance of the agent attempting disallowed tool calls.Full-file review of
ci.ymlagainst the security checklist:Prompt Injection: No attacker-controlled event data (PR title, body, comments) is interpolated into LLM prompts. Changed file paths from
git diffare included, which is a minimal and acceptable surface. All three prompts contain security preambles instructing the model to treat PR content as untrusted. Tool scoping is tight — onlyRead,Grep,Bash(git diff:*),Bash(git log:*), and two specificBash(tee ...)/Bash(touch ...)patterns are allowed. No broadBash(curl:*)or unrestricted Bash access.Expression Injection: Matrix values (
matrix.task,matrix.vault, etc.) used inrun:blocks are all hardcoded in the workflow — not attacker-controlled. SHAs (github.event.pull_request.base.sha,head.sha) are passed viaenv:variables (lines 40–41), which is the safe pattern.github.event.pull_request.numberis also passed via env var. The merge-gatecontains()expression (line 1015) produces only a boolean — safe.Dangerous Triggers: Uses
pull_requesttrigger only (notpull_request_target). Noissue_commentorworkflow_dispatchtriggers. Safe.Supply Chain:
actions/checkout@v6anddenoland/setup-deno@v2are from trusted publishers with tag pins — acceptable per policy. The Claude CLI binary is downloaded via curl but verified with a SHA-256 checksum before execution (lines 713–715, 829–831, 923–925). This is not acurl | bashpattern — it's a download-and-verify pattern, which is secure.Permissions: Workflow-level
contents: read(line 9) applies to all non-review jobs, which is minimal and correct. The three Claude review jobs specify job-level permissions (contents: read+pull-requests: write), which is the minimum needed to post review comments. Noid-token: writeanywhere.Secret Exposure:
ANTHROPIC_API_KEYis scoped to only the Claude invocation steps.BOT_TOKENis scoped to only the comment-posting steps. Neither is logged or used in command substitution that could leak values.Auto-merge & Trust Boundaries: No auto-merge behavior. The workflow posts comments and optionally fails the job. The merge gate aggregates job results but does not merge anything.
Medium
(none)
Low
(none)
Verdict
PASS — This PR is a pure security hardening change. It adds
--disallowedToolsand--add-dirto constrain the Claude review agents, and updates prompts to match. No new vulnerabilities are introduced. The full workflow file passes all checklist items.Code Review
Blocking Issues
None.
Suggestions
codegen/shared/zodGenerator_test.ts— snapshot files not committedTests 1–12 use
assertSnapshot, which requires snapshot files to exist. Those snapshot files do not appear in the changed-files list. If this file is newly added,deno testwill fail on first run (Deno requires--updateto generate snapshots). Because thecodegen-checkCI job only runscheck,lint, andfmt— nevertest— this won't be caught by CI, but the tests themselves are non-functional without the snapshots. Either commit the snapshot files or confirm they already exist from a prior revision.kubernetes/extensions/models/event.ts(lines 170, 199, 228) — user input interpolated intofieldSelectorstringargs.podName,args.deploymentName, andargs.serviceNameare interpolated directly into the Kubernetes field selector string:A value containing a comma (e.g.
"nginx,involvedObject.kind=Warning") would silently add an extra selector clause and could return unexpected events. Since the namespace is already bound, this is information-disclosure at worst (not RCE), but it could produce surprising results for callers. Consider percent-encoding the value or validating that it contains no commas or=signs before interpolation.kubernetes/extensions/models/netpol.ts(lines 304–308) — double cast suggests type mismatchThe
as unknown aspattern bypasses the type system entirely. Either thespec: Record<string, unknown>shape doesn't match the Kubernetes client's expected type (pointing to a missing intermediate interface), or the cast could hide a subtle field mismatch. At minimum, annotate why the cast is necessary so a future reader knows it's intentional.kubernetes/extensions/models/deployment.ts(line 353) andpod.ts(line 295) —let body;without explicit type annotationIn TypeScript strict mode,
let body;without an initializer relies on control-flow inference for its type. If a future path is inserted between declaration and assignment, TypeScript may widen the type toany. Explicitly annotating these variables (e.g. asRecord<string, unknown>) is more defensive and stays cleaner under the project's no-anyrule.kubernetes/extensions/models/namespace.ts(line 917) — vacuous-truth health checkAn empty namespace (0 deployments, 0 pods, 0 services, 0 warnings) evaluates to
healthy: true. This is likely the intended behaviour for an empty namespace, but it may surprise callers seeing a brand-new or fully-drained namespace marked healthy. A short comment explaining the intentional vacuous-truth behaviour would prevent future confusion.Adversarial Review
Medium
Resource schema regression: bare object now emits z.string() instead of z.record() -- codegen/shared/zodGenerator.ts:390
In generateSimplifiedZodForProperty (which generates resource state schemas -- the read/describe side), a bare type object with no properties, patternProperties, or additionalProperties now emits z.string(). Previously it emitted z.record(z.string(), z.unknown()).
The design doc (codegen/designs/aws.md:686-688) explicitly states that the StateSchema should accept objects because AWS CloudControl returns these properties as already-deserialized objects. While the design doc discusses the json type (which is unchanged and still emits z.union), the same principle applies to bare objects in resource responses -- CloudControl returns them as actual JSON objects, not strings.
Breaking example: If a CloudFormation schema has type object with no property definitions, and CloudControl returns a parsed object like a PolicyDocument, Zod validation will throw Expected string, received object.
Mitigating factor: This is a narrow edge case -- most bare objects would have additionalProperties set (line 353-357 handles that and still emits z.record). The more common json type path is unaffected.
Test coverage gap: Test 15 (bare object with no properties emits z.record) was deleted entirely. No replacement test verifies the new bare-object behavior in resource schemas. Even if z.string() is the intended behavior, the test deletion leaves this path uncovered.
Suggested fix: Keep z.string() for the input schema paths (lines 196 and 540 -- these match the design doc), but restore z.record(z.string(), z.unknown()) at line 390 for the resource schema path. Add a test verifying the asymmetry.
--disallowedTools syntax may only block Write, not Edit -- .forgejo/workflows/ci.yml:731 (also lines 848, 941)
The flag is invoked as two separate shell words: --disallowedTools "Write" "Edit"
But --allowedTools in the same command uses comma-separated format: --allowedTools "Read,Grep,Bash(...)"
If --disallowedTools expects a single comma-separated argument (like --allowedTools), then Edit is a dangling positional argument and the Edit tool is NOT actually blocked. This would leave the review sandbox weaker than intended.
Mitigating factor: The prompt text also instructs the model not to use Write/Edit, providing defense in depth. And if Claude Code CLI does accept multiple space-separated values for --disallowedTools, this works fine.
Suggested fix: Use consistent syntax: --disallowedTools "Write,Edit" to match the comma-separated pattern of --allowedTools, or verify the CLI accepts the space-separated form.
Low
Pod name regex allows trailing dash and dot -- kubernetes/extensions/models/pod.ts:477
Regex: /^[a-z0-9][a-z0-9-.]*$/
Kubernetes DNS subdomain names must start AND end with an alphanumeric character. This regex allows my-pod- or my-pod. which are invalid pod names. The primary purpose of this validation is preventing kubectl flag injection (names starting with dash), which it does correctly. kubectl would reject invalid trailing characters anyway.
Suggested fix (optional): /^a-z0-9?$/ to also enforce the trailing character constraint.
container argument not validated -- kubernetes/extensions/models/pod.ts:493-494
args.container is passed directly to kubectlArgs.push without validation. A container name like --namespace=kube-system would be treated as a literal -c argument value by Deno.Command array-based execution (no shell involved), so this is not exploitable for flag injection. Still, applying the same pattern as pod name validation would be consistent defense-in-depth.
Verdict
PASS -- The pod exec security fix is sound and the CI hardening is a clear improvement. The codegen z.string() change is correct for input schemas (matching the design doc) but questionable for the resource schema bare-object path. The resource schema concern and the --disallowedTools syntax inconsistency are worth addressing but neither is blocking given their limited blast radius.