security: sanitize model input to prevent newline injection in grep validation#192
Open
kane-review-agent[bot] wants to merge 7 commits into
Open
security: sanitize model input to prevent newline injection in grep validation#192kane-review-agent[bot] wants to merge 7 commits into
kane-review-agent[bot] wants to merge 7 commits into
Conversation
kane-review-agent
Bot
added
the
pipeline-docs:none
kane-review-agent
Bot
force-pushed
the
fix/agy-newline-injection
branch
from
cbc815f to
0b9e5c0
Compare
Address findings from code-reviewer agent (PR #191): - Replace NUL-byte sentinel '__ENUM_FAILED__' with explicit string for clarity in logs/debugging - Add inline documentation explaining eval 3-level quoting escaping to prevent future misedits - Newline-stripping already in place prevents grep -Fxq injection All unit tests pass (61/61). No functional change; code-maintainability improvements.
CI was failing because the regression test called _agy_known_model without setting up PATH="$BIN:$PATH" + AGENT_CMD=agy. Without the stub, 'agy models' enumeration fails in CI (no real agy binary), so the function returns rc 2 (best-effort pass-through) instead of rc 1 (rejected) — masking the security fix. The stub is needed because the test wants to verify the newline-stripping logic that runs ONLY when enumeration succeeds. All 62 unit tests now pass.
_agy_known_model stripped newlines on its own local copy for the grep -Fxq check, but _agy_build_model_args still forwarded the ORIGINAL value. A known model carrying a trailing newline therefore validated (rc 0 after the in-function strip) yet leaked the raw newline into agy's --model argv — a defense-in-depth gap that the PR #192 fix did not close. Strip newlines up-front in _agy_build_model_args so the SAME sanitized value is both validated and forwarded at the single choke point. Tests: add AGY-06e (run_agent round-trip; NUL-recorder asserts the forwarded --model element carries no embedded newline) and TC-AGYM-BM (direct _agy_build_model_args check). Clean-env suite: 64 pass, 0 fail.
…tion The newline strip lived only inside _agy_known_model, operating on its own local copy. A known model carrying a trailing/embedded newline would pass validation (rc 0 after the in-function strip) yet _agy_build_model_args would still forward the raw, un-sanitized original as the --model argv — leaking the newline to agy. Strip newlines up-front in _agy_build_model_args (the single choke point that both validates and forwards) so the same sanitized value is used for both. The strip in _agy_known_model stays as belt-and-suspenders for direct callers. Tests: AGY-06e (full run_agent round-trip via NUL-recorder) and TC-AGYM-BM (direct _agy_build_model_args unit) pin that a known model + trailing newline forwards the newline-free value. 64/64 pass in a clean env; shellcheck clean.
zxkane
added a commit
that referenced
this pull request
Jun 13, 2026
_agy_known_model stripped newlines on its own local copy for the grep -Fxq check, but _agy_build_model_args still forwarded the ORIGINAL value. A known model carrying a trailing newline therefore validated (rc 0 after the in-function strip) yet leaked the raw newline into agy's --model argv — a defense-in-depth gap that the PR #192 fix did not close. Strip newlines up-front in _agy_build_model_args so the SAME sanitized value is both validated and forwarded at the single choke point. Tests: add AGY-06e (run_agent round-trip; NUL-recorder asserts the forwarded --model element carries no embedded newline) and TC-AGYM-BM (direct _agy_build_model_args check). Clean-env suite: 64 pass, 0 fail.
zxkane
force-pushed
the
fix/agy-newline-injection
branch
from
d7140cd to
d9fb63d
Compare
…t just newline A known agy model id carrying a trailing carriage return validated clean (the validator already stripped [[:cntrl:]] for its grep -Fxq check) but the forward point in _agy_build_model_args stripped only \n, so the raw \r leaked into agy's --model argv. Widen both _agy_known_model and _agy_build_model_args to strip the same [[:cntrl:]] class, so the value that validates is exactly the value forwarded. Mirrors the INV-60 [[:cntrl:]] guard in post-verdict.sh so the two model sites agree. Also wrap the enum-failure sentinel in \x01 (__ENUM_FAILED__ → $'\x01__ENUM_FAILED__\x01') so no real `agy models` listing line can collide with it. Docs: INV-50 (invariants.md), agy-cli-support.md, and the agy-model-support.md test plan updated to record the [[:cntrl:]] strip at both sites and the new carriage-return coverage. Tests: AGY-06e2 + TC-AGYM-BM2 assert a \r-bearing known id forwards sanitized. Full suite 66/0, shellcheck clean. Refs #192
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a newline injection vulnerability in agy model validation (_agy_known_model function).
Vulnerability: The function used
grep -Fxqto check if a model name is in the list of known agy models. Without sanitizing the input, a model value like"Gemini 3.5 Flash (High)\nGemini 3.5 Flash (Low)"could bypass validation by matching multiple cache lines, allowing an attacker to forward an invalid composite model to agy.Fix: Strip newlines from the model parameter before passing to grep:
model="${model//\$'\n'/}"Impact:
Test Coverage:
Checklist