Skip to content

fix: remediate five open CodeQL alerts - #871

Merged
haofeif merged 3 commits into
mainfrom
fix/codeql-open-alerts
Oct 3, 2026
Merged

haofeif merged 3 commits into
mainfrom
fix/codeql-open-alerts

Conversation

@haofeif

@haofeif haofeif commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Remediate the five open CodeQL alerts remaining after #869. This changes the flagged code and assertions, not scan thresholds, workflow configuration, or alert dispositions.

Alert Change
253 Make plugin source-kind inference purely syntactic on the CLI and both HTTP entry points; remove the request-controlled filesystem probe.
254 Replace a test-only hostname-substring assertion over policy prose with exact canonical schema-source URL and URL/ID assertions. Runtime schema loading remains offline.
289 Canonicalize and confine Kiro policy-file reads to the agent directory, rejecting escaping symlinks before reading and surfacing the error at launch.
301 Check footer tips within their own escape-delimited colour segments instead of using the flagged combined regex.
303 Match only the swarm glyph/state prefix, parse detail separately, and enforce the single-row boundary without overlapping suffix repetitions.

The five HIGH labels are CodeQL's classifications. Triage found existing safeguards and false-positive diagnostics; this PR does not claim five demonstrated HIGH-severity exploits.

Compatibility

  • Explicit local plugin paths (./, ../, /, or ~) remain local, including Git-shaped filenames. Ambiguous SCP-shaped local names must use an explicit path prefix; classification no longer changes when a same-named directory exists.
  • Preserve Kiro's historical filename flattening, symlinks to in-directory files, and symlinked agent directories. The target must be a strict descendant: an escaping path or a link to the directory itself is an error, not a missing-policy result. A narrow KiroAgentPathError maps these refusals to HTTP 400 on creation/run-step requests while leaving missing-resource 404 responses intact.
  • Preserve Kimi footer colour ownership, native swarm states and details, quoted/tool-content boundaries, and normal output extraction. No transcript-length cap or source-specific exception was added.
  • Update docs/agent-plugins.md, docs/kiro-cli.md, docs/kimi-cli.md, and CHANGELOG.md in the same commit.

Validation

  • Red-first regressions: 20 failed / 45 passed against the unchanged production code, covering filesystem probes, escaping symlinks, and progress-parser failure boundaries.
  • Focused CLI/API, resolver/schema, Kiro installation/path, and Kimi provider/transcript suites: 1,507 passed / 8 skipped, with fresh HOME and CAO_HOME_DIR.
  • Additional Kiro launch/enforcement checks: 13 passed.
  • After the hosted scan exposed an unchecked directory-root alternative, a new boundary regression failed on the first head. The corrected Kiro installation, CLI launch, and path suites then passed 211 tests, with fresh application state.
  • The coupled API status-code regression failed before correction. API terminal/handoff, installation, and CLI launch suites then passed 251 tests, including durable run-step failure recording and unchanged missing-session 404 handling.
  • Bounded parser cases exercise matching and nonmatching inputs at 32,768 and 131,072 repeated elements, with child-process timeouts of five seconds.
  • Canonical schema assertions reject seven alternate-host, scheme, path, and query mutations.
  • The original affected modules passed mypy. The API-inclusive check reports one unchanged, pre-existing MemoryArchiveBackend call-argument error and zero new diagnostics. Black/isort, Markdown links, and patch checks passed.

Base: 55dd4a0c839e087771c999a4760bf5502b6d1c21

Head: decd6dea1916ff9ca13b59ad059b1c08ac3cbc76

Hosted acceptance

The first full-branch scan (37120974948, head ba17f0426eca241a04385336db771be1c22a40fc) removed four targeted constructs but still reported the Kiro read as alert 304. Its path == base alternative could reach the read without the descendant check. The follow-up commit rejects that invalid agent-file target and makes the normalized containment guard mandatory before every read.

The second full-branch scan at 4fd8f11bb7a1ab40afff7498ada7b425025aeff3 passed all four jobs. Python analysis 1885845670 removed exactly the five targeted findings, including replacement diagnostic 304; the six previously dismissed findings were unchanged. CI at that head also passed.

Review then identified the coupled HTTP 404/400 issue, corrected in the current head. Final PR CI succeeded with all 20 jobs, including all four CodeQL language jobs. All 25 PR checks are successful, including the native CodeQL check.

The final full-branch scan also succeeded at decd6dea1916ff9ca13b59ad059b1c08ac3cbc76:

Language Analysis ID Full SARIF results
Python 1885877844 6 previously dismissed findings
Actions 1885876335 0
JavaScript/TypeScript 1885877003 0
Rust 1885881500 0

Compared with default-branch Python analysis 1885232648 (11 results), the final full SARIF removes exactly alerts 253, 254, 289, 301, and 303. The six previously dismissed results are unchanged; there are no new findings or replacement diagnostic 304. Comparison uses rule ID, file URI, and primaryLocationLineHash, so the API edit's line shifts do not hide changes. No analysis errors or warnings were reported.

The API 400/404 review thread is fixed and resolved. The latest Copilot carriage-return note describes pre-existing behavior: the baseline swarm regex's .* also accepts carriage returns, and the reported row produces identical state/detail parsing before and after this change. No unrelated terminal-redraw behavior change is included.

Required maintainer review remains outstanding. No alerts have been dismissed, no protections have been weakened, and nothing has been merged by this task. Default-branch alert closure is expected only after maintainer merge and successful analysis of main.

Keep plugin source inference free of filesystem probes, confine Kiro policy reads, remove flagged Kimi regex constructs, and validate canonical schema metadata. Preserve existing behavior with focused regressions and update the affected documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 3, 2026 11:51
Comment thread src/cli_agent_orchestrator/services/install_service.py Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Security-sensitive path and parser changes still depend on the pending full-branch CodeQL validation.

Review effort: Balanced
Findings: None

What changed in this PR

Remediates five CodeQL findings involving plugin inference, Kiro path confinement, schema assertions, and Kimi transcript parsing.

Changes:

  • Makes plugin source inference syntactic and confines Kiro policy reads.
  • Reworks Kimi footer/swarm parsing to avoid problematic regex behavior.
  • Adds regression tests and updates relevant documentation.
File Description
src/​cli_agent_orchestrator/​cli/​commands/​agent_plugin.py Removes filesystem-dependent source inference.
src/​cli_agent_orchestrator/​services/​install_service.py Confines Kiro policy reads to the agent directory.
src/​cli_agent_orchestrator/​providers/​kimi_transcript.py Reworks footer and swarm parsing.
test/​agent_plugins/​test_cli.py Tests syntactic CLI inference.
test/​agent_plugins/​test_api.py Tests both HTTP entry points.
test/​agent_plugins/​test_schema_pin_property.py Enforces canonical schema metadata.
test/​services/​test_install_service.py Covers Kiro flattening and symlinks.
test/​cli/​commands/​test_launch.py Verifies launch-time path errors.
test/​providers/​test_kimi_transcript_whitespace.py Adds bounded parser regressions.
test/​providers/​test_kimi_swarm.py Covers swarm states and boundaries.
docs/​agent-plugins.md Documents inference and schema checks.
docs/​kiro-cli.md Documents policy-path confinement.
docs/​kimi-cli.md Documents parser behavior.
CHANGELOG.md Records the security remediation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Reject a symlink to the agent directory itself as well as escaping targets. Every policy read must pass the canonical descendant check; add the boundary regression and document the explicit failure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 3, 2026 12:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Swarm rows containing carriage-return redraw boundaries can still absorb ordinary content as progress chrome.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject carriage returns to prevent misclassifying assistant content

src/​cli_agent_orchestrator/​providers/​kimi_transcript.py:1776

Reject carriage returns as row boundaries too. Production transcripts are split only on \n, so terminal redraws leave \r inside row; with the current guard, Orchestrating… detail\rordinary assistant content is accepted because that state permits arbitrary detail, causing the remainder to be classified as swarm chrome instead of content.

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review at 4fd8f11b (merge-base 55dd4a0c): P3 only.

Resolved before posting. At ba17f042 the compound != base and not startswith guard left the == base branch unguarded, so CodeQL raised high alert #304. 4fd8f11b uses the single positive startswith(base + os.sep) guard documented in workflow_spec_service.py:236-244. Results:

  • #304 is fixed on refs/pull/871/merge.
  • The python analysis at 0305d37b reports 0 results.
  • None of the five targeted alerts is open on the PR ref.
  • The branch-ref instance of #304 closes once the in-flight CodeQL (python) run finishes.

P3: the new refusal surfaces as a 404 on POST /sessions/{name}/terminals (inline).

Verified clean

  • #289: the guard now dominates the read. A link to the directory itself is refused and tested, and the docs are updated.
  • #253: inference is syntactic and shared by the CLI and the API (main.py:3032-3034). /abs/x.git now classifies as a path, which matches git_clone_target's own "use a plain directory path" advice. SCP detection follows git's no-slash-before-colon rule.
  • #301 / #303: equivalent for the parsed inputs, and linear-time. Copilot's \r note at kimi_transcript.py:1776 describes pre-existing behaviour: base's (?P<detail>.*) also accepted \r. This PR does not introduce it.
  • #254: exact canonical source_base_url and schema_id assertions.

At posting time Unit Tests, CAO MCP Apps and the CodeQL rust/python runs are still in progress at 4fd8f11b; the rest is green.

Comment thread src/cli_agent_orchestrator/services/install_service.py Outdated
Use a narrow ValueError subtype for Kiro policy containment failures and handle it in the existing API 400 paths, including durable run-step failures. Preserve missing-resource 404 responses and document the distinction.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 3, 2026 12:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Swarm parsing still accepts carriage-return-delimited content as a single progress row.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject embedded carriage returns in row-boundary classification

src/​cli_agent_orchestrator/​providers/​kimi_transcript.py:1776

This row-boundary check still permits embedded carriage returns. classify_lines() splits only on "\n", so a value such as Orchestrating… detail\rordinary output reaches this helper unchanged; the prefix matches and the unrestricted Orchestrating detail causes the whole value to be classified as swarm chrome. Reject \r too so a terminal overwrite cannot hide ordinary output or establish progress evidence as part of the same parsed row, and add it to the multiline regression cases.

@haofeif

haofeif commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator Author

Final verification is complete at decd6dea1916ff9ca13b59ad059b1c08ac3cbc76, against base 55dd4a0c839e087771c999a4760bf5502b6d1c21.

  • CI: all 20 jobs passed; all 25 PR checks are successful.
  • Full-branch CodeQL: all four languages passed. Python analysis 1885877844 removes exactly the five requested findings, leaves the same six previously dismissed results, and introduces no replacement/new finding. This is full-SARIF fingerprint comparison, not merely a green PR-delta check.
  • The API 400/404 issue is fixed in the final commit and its thread is resolved; the coupled suites passed 251 tests.

Regarding the carriage-return review note: this behavior already exists at the base SHA. Its _SWARM_STATUS_RE detail pattern uses .*, which accepts carriage returns; the reported input has identical state and detail before and after this PR. That is not a regression from the prefix/detail split, so this five-alert remediation does not add an unrelated terminal-redraw behavior change.

The PR description now records the final run/analysis evidence. Required maintainer review is still outstanding; nothing was merged and no alerts were dismissed. Default-branch alerts close only after maintainer merge and a successful main analysis.

@haofeif
haofeif merged commit 0caec2c into main Oct 3, 2026
30 checks passed
@haofeif
haofeif deleted the fix/codeql-open-alerts branch October 3, 2026 12:57
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.

3 participants