Skip to content

feat: add DSH plugin security scanner - #116

Open
EchoOfZion wants to merge 15 commits into
GoPlusSecurity:mainfrom
EchoOfZion:codex/dsh-mvp
Open

feat: add DSH plugin security scanner#116
EchoOfZion wants to merge 15 commits into
GoPlusSecurity:mainfrom
EchoOfZion:codex/dsh-mvp

Conversation

@EchoOfZion

@EchoOfZion EchoOfZion commented Aug 15, 2026

Copy link
Copy Markdown

Summary

Add AgentGuard for DeepSeek Harness (DSH): a read-only Phase 1 installation-time scanner for plugins, bundles, profiles, client extensions, active agent instructions, and Cordis composition.

The new agentguard dsh-scan command and native agentguard_dsh_scan DSH tool accept a local directory or an HTTPS GitHub repository and produce explainable JSON, Markdown, or self-contained HTML reports. Scanned packages are never installed or executed, lifecycle scripts are not run, and Cordis !!js values remain inert data.

Phase 1 boundary

This PR is an installation decision aid, not a DSH runtime firewall. It does not intercept commands, automatically block plugin behavior, resolve npm names/tarballs, or prove that a repository matches a published package. Runtime attribution and per-plugin allow/warn/approve/block enforcement remain Phase 2 work.

Risk-rule semantics are frozen at 83db977 for the phase1-rc1 baseline. Later commits add regression infrastructure and documentation rather than tuning outcomes to individual plugins.

User interfaces

agentguard dsh-scan ./path/to/plugin
agentguard dsh-scan https://github.com/owner/plugin --format json
agentguard dsh-scan ./path/to/plugin --format html --output report.html

Native DSH installation:

dsh plugin --profile web add @goplus/agentguard

DSH then exposes the read-only agentguard_dsh_scan tool. Critical CLI reports return exit code 2; lower risk levels return 0, while automation can apply stricter policy to JSON fields.

Architecture and report model

  • Detect dsh.bundle.patch, dsh.profile.bundles, dsh.client, current standalone manifests, and Cordis files.
  • Parse Cordis YAML strictly with bounded AST traversal; preserve ordinary scalar types and keep !!js inert.
  • Distinguish inserted rows from structured replacements of security-relevant core rows.
  • Infer plugin kind, capability profile, and DSH impact layers.
  • Report conservative full-repository risk and a separate installed runtime-surface risk.
  • Preserve findings from tests, examples, documentation, data, generated bundles, and active instruction files with explicit source/relevance context.
  • Provide independent reviewPriority, installation recommendations, artifact hash, Git revision, and project diagnostics.
  • Aggregate repeated findings by rule and file with occurrenceCount without changing severity.
  • Keep stable additive schemaVersion: 1 JSON plus portable Markdown and escaped standalone HTML.

Detection calibration

  • Computed local/package imports use HIGH DYNAMIC_MODULE_LOADING; only remote acquisition combined with execution uses CRITICAL REMOTE_LOADER.
  • Eval-like primitives use DYNAMIC_CODE_EXECUTION; encoded/packed indicators remain OBFUSCATION.
  • AUTO_UPDATE requires nearby acquisition and install/execute evidence instead of file-wide keyword co-occurrence.
  • Prompt injection requires an active instruction artifact or a recognized prompt-delivery surface.
  • Keychain access requires concrete credential APIs rather than labels containing the word “keychain”.
  • Executable files under data/ or assets/ remain reviewable so directory names cannot hide behavior.

Security properties

  • No package or configuration code evaluation.
  • Per-file and file-count scan limits; Cordis AST capped at 20,000 nodes and 64 levels.
  • GitHub repository URLs are canonicalized, remote HEAD is resolved first, that exact SHA is fetched without hooks or submodules, local HEAD is verified, and temporary checkouts are removed.
  • Output paths create parents explicitly; HTML escapes artifact-controlled values.
  • Malformed/oversized manifests and Cordis structures produce diagnostics instead of silent partial metadata.
  • Public benchmark snapshots contain rule names/counts, not matched secret values or source snippets.

Phase 1 RC regression baseline

Two complementary gates are included:

  1. An offline labeled synthetic corpus covering safe themes, expected capabilities, deceptive packages, test-only evidence, active instructions, generated runtime code, executable data paths, prompt/keychain false positives, dynamic loading, auto-update locality, and core overrides.
  2. A networked real-world benchmark pinned to exact commits and artifact hashes for:
    • dsh-deep-whale — MEDIUM full / LOW runtime
    • superdesign-skill — HIGH full / MEDIUM runtime
    • dsh-open-in-vscode — HIGH / HIGH
    • dsh-vision-router — CRITICAL / CRITICAL / URGENT
    • MisakaNet — CRITICAL / CRITICAL / URGENT

npm run benchmark:dsh exits non-zero with field-level differences when current results diverge from the committed snapshot. Snapshot updates require a documented scanner change and human review of new or removed HIGH/CRITICAL runtime tags.

Manual source reviews document important qualifications: expected guarded self-update behavior in vision-router, generated dependency execution in open-in-vscode, real webhook/keyring/subprocess capability plus sensitive historical material in MisakaNet, and superdesign as a clean runtime control. The notes deliberately do not reproduce credential values.

Validation

  • npm run build
  • npm test466 passed, 0 failed
  • npm run test:dsh-e2e — real profile composition, temporary loopback boot, and installed tool execution passed
  • npm run benchmark:dsh5/5 exact-commit cases stable
  • git diff --check
  • AgentGuard PR Review workflow — passed

Known limitations

  • Static analysis cannot prove safety or detect every native, computed, packed, or runtime-downloaded behavior.
  • Runtime relevance does not yet resolve package-manager publish sets, every export/import edge, third-party provenance, or full Cordis reachability.
  • Pattern evidence can require human interpretation (for example a machine-learning model's .eval() method versus language-level eval).
  • GitHub scans follow a resolved current HEAD; user-supplied tag/branch/commit selectors and npm tarball comparison are future supply-chain work.
  • The scanner evaluates one artifact rather than every interaction in a final composed profile.

Documentation

  • docs/dsh.md — architecture, report contract, risk model, operations, and limitations
  • docs/dsh-phase1-rc.md — frozen boundary and acceptance gates
  • benchmarks/dsh/README.md — exact-commit benchmark and snapshot-update policy
  • benchmarks/dsh/manual-reviews.md — source-level review conclusions

Review history

Earlier automated review findings were addressed across the branch: development-path evidence is preserved, Cordis parsing is bounded and structured, malformed manifests produce diagnostics, GitHub scans verify exact resolved revisions, output directories are created safely, and patch overrides derive from parsed rows rather than free-form snippets. The current PR has no unresolved line-level review comments.

@github-actions

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown

AgentGuard PR Review

I found several actionable issues in the DSH scanner patch.

  1. severity: high — src/dsh/scan.ts / src/dsh/classify-plugin.ts

    • What can go wrong: scanDshPlugin() classifies harmlessMismatch as high-risk by unconditionally appending DSH_THEME_ELEVATED_CAPABILITY to riskTags, and classifyDshPlugin() treats any label matching theme/ui/... plus elevated capabilities as a theme/ui plugin. This can silently downgrade or misrepresent a clearly malicious package as a benign theme while still producing only a generic high-risk recommendation, which weakens the security signal and can hide the true plugin kind in reports.
    • Fix: separate benign-label detection from primary kind classification. Keep plugin kind based on manifest/behavior first, and emit a distinct mismatch finding without overriding the inferred kind; ensure the summary and recommendation explicitly reflect the elevated behavior.
  2. severity: high — src/dsh/scan.ts

    • What can go wrong: riskTags is built from scanner evidence, then DSH_THEME_ELEVATED_CAPABILITY is appended for mismatch cases, but calculateDshRisk() only looks at tags/severity and not at whether evidence actually exists for that tag. This means a crafted package can trigger risk escalation solely via label heuristics, including critical-seeming recommendations, even when no corresponding security rule matched.
    • Fix: derive risk only from real findings or from a separately tracked, explicitly justified mismatch category; do not mix heuristic mismatch tags into the same risk-tag set used for severity calculation.
  3. severity: medium — src/dsh/parse-package.ts

    • What can go wrong: JSON.parse(raw) is not guarded against malformed or non-object package.json content. A syntactically invalid manifest throws and is swallowed, returning empty metadata with no diagnostics. That can cause the scanner to miss DSH detection and silently under-report risk on corrupted or intentionally malformed packages.
    • Fix: capture parse failures as diagnostics and propagate them into the report so the scan is marked incomplete instead of silently falling back to empty metadata.
  4. severity: medium — src/dsh/source.ts

    • What can go wrong: GitHub repository cloning uses the user-provided URL directly in git clone ... -- <input>, but the regex allows only github.com URLs and does not pin to a commit or verify the repository content after clone. Because the scan follows the current default branch at clone time, reports are not reproducible and can change between runs or after a force-push.
    • Fix: record and require an immutable reference when scanning GitHub sources, or at minimum include the fetched commit SHA prominently and reject ambiguous inputs that do not resolve to a pinned revision.
  5. severity: low — src/cli.ts

    • What can go wrong: the new dsh-scan command always exits with code 0 for low/medium/high risk and only uses 2 for critical. That makes shell automation unable to enforce a stricter gate based on the scan result, despite the docs claiming recommendations should drive policy decisions.
    • Fix: add an option to control exit-code policy or provide a non-zero exit for at least high risk when requested by automation.

@EchoOfZion

Copy link
Copy Markdown
Author

Addressed all four review items in 5413eca.

  1. Test-like path under-reporting — fixed. Removed development-path filtering from both capability inference and evidence/risk calculation. Dangerous code under tests, fixtures, *.spec.*, or *.test.* now affects capabilities, findings, risk, and the installation recommendation. The regression test now requires a test-path shell call to produce high risk.

  2. Cordis YAML API — verified and hardened. yaml@2.9.0 does accept SchemaOptions.schema: string, but the old failsafe schema parsed ordinary booleans as strings. The parser now uses the documented core schema with an explicit tag:yaml.org,2002:js scalar resolver that returns inert text. This preserves !!js without evaluation while retaining normal boolean/number semantics. Added a representative cordis.yml test with disabled: false, numeric config, and !!js.

  3. GitHub revision pinning — fixed. The resolver now canonicalizes the validated owner/repository URL, resolves remote HEAD first, fetches that exact SHA at depth 1 into an initialized temporary repository, checks it out detached, and verifies local HEAD before scanning. A live GitHub scan confirmed the resolved and reported revisions match.

  4. Nested output path — fixed. --output now creates missing parent directories explicitly. Added a CLI regression test that writes JSON into a nested, previously absent directory.

Validation after the fixes:

  • npm run build
  • npm test: 428 passed, 0 failed
  • Focused DSH tests: 13 passed, 0 failed
  • Live GitHub exact-SHA DSH scan
  • npm pack --dry-run
  • git diff --check

@EchoOfZion

Copy link
Copy Markdown
Author

Additional hardening from the updated review is in 9efa5a1:

  • Cordis resource bounds: removed whole-document toJS() materialization. The parser validates the YAML AST iteratively, caps it at 20,000 nodes and 64 levels, and reads only the map/sequence/scalar fields needed for Cordis rows. Malformed YAML remains rejected under strict: true. Added a deep-nesting rejection test.
  • Install instructions metadata: documented in the public report type and schema guide that project.hasInstallInstructions is informational README metadata only. It is not consumed by risk calculation or installation recommendations.
  • Git dependency diagnostics: GitHub scans now check git --version first and return a specific missing-PATH error. Local scans remain available without Git metadata when Git is unavailable. The exact-SHA fetch and checkout verification from 5413eca remains in place.
  • Theme mismatch summary: added and prioritized DSH_THEME_ELEVATED_CAPABILITY in the summary reason list, with a regression assertion.

Latest validation:

  • npm run build
  • npm test: 429 passed, 0 failed
  • Focused DSH tests: 14 passed, 0 failed
  • git diff --check

@EchoOfZion

Copy link
Copy Markdown
Author

The latest review items are addressed in a15e4c6:

  • Theme mismatch precision: network-only UI/theme behavior now remains a normal medium network finding. The derived mismatch is limited to shell execution, file writes, runtime mutation, or the environment-plus-network exfiltration combination. Added a network-only regression test.
  • Derived mismatch evidence: the DSH_THEME_ELEVATED_CAPABILITY finding now carries an explicit message and snippet containing the package identity and exact unexpected capability names. The summary prioritizes the same reason.
  • Cordis structural validation: top-level rows, inserted rows, and nested config.patches must have the expected sequence/mapping shapes. Invalid structures produce diagnostics and do not return a partial row set. Added a malformed-row regression test.
  • GitHub URL contract: the normalizer and docs now explicitly support https://github.com/owner/repo, optional .git, and one trailing slash, while rejecting repository subpaths. Added unit coverage for all forms.
  • README metadata naming: renamed the report field to hasReadmeInstallInstructions, broadened common README variants, and documented that it is informational only and never affects risk or recommendations.

Final validation for this revision:

  • npm run build
  • npm test: 432 passed, 0 failed
  • Focused DSH tests: 17 passed, 0 failed
  • git diff --check

@EchoOfZion

Copy link
Copy Markdown
Author

Final deterministic improvements are in ee9e156:

  • Malformed or oversized package.json now produces diagnostics.packageParseError without aborting the scan. Missing manifests remain a valid no-manifest case. Added regression coverage.
  • DSH_PATCH_OVERRIDE is no longer accepted or rejected by parsing a free-form scanner snippet. Findings and risk tags are built directly from parsed Cordis rows with operation === "replace" and a security-relevant structured row ID.

Two review statements refer to code that is no longer present: the current parser does not call document.toJS(), and GitHub scans do not call git clone; they resolve HEAD and fetch the exact SHA into a detached checkout. Risk remains evidence/rule-driven rather than being raised directly from the coarser capability profile, because doing the latter would make benign descriptive strings security findings. Remote scanning is already explicit in the dsh-scan <repo-or-path> argument and documented as a static fetch; adding an interactive prompt would break CI and JSON automation.

Validation at ee9e156:

  • npm run build
  • npm test: 433 passed, 0 failed
  • Focused DSH tests: 18 passed, 0 failed
  • git diff --check

@EchoOfZion

Copy link
Copy Markdown
Author

Phase 1 RC stabilization is complete in a6e1edc.

  • Risk semantics are frozen at 83db977 (phase1-rc1); the latest commit adds regression infrastructure and documentation, not sample-specific retuning.
  • Full suite: 466 passed, 0 failed.
  • Native DSH E2E: profile composition, loopback boot, and installed agentguard_dsh_scan execution passed.
  • Exact-commit real-world benchmark: 5/5 stable across LOW-through-CRITICAL runtime postures.
  • Added deterministic snapshots, field-level diff failure, snapshot update policy, and offline validation of benchmark assets.
  • Added source-level reviews for dsh-vision-router, MisakaNet, dsh-open-in-vscode, and superdesign-skill, including documented evidence qualifications and known false-positive examples without reproducing credential values.
  • Current AgentGuard PR Review check passes and there are no unresolved inline comments.

The PR body now contains the complete Phase 1 boundary, architecture, calibration history, security properties, acceptance gates, benchmark results, and known limitations. Runtime enforcement remains explicitly deferred to Phase 2.

@EchoOfZion

Copy link
Copy Markdown
Author

Phase 1 RC delivery follow-up is ready in 840a5f1:

  • adds scanner provenance to JSON, Markdown, HTML, and the DSH tool result (version, phase1-rc1, frozen rules baseline 83db977a566d8a853568a2d2903b142106d80196)
  • keeps schema-v1 backward compatibility and labels provenance as unavailable when rendering older reports
  • documents install/update/remove, profile verification, report interpretation, and ERR_CONNECTION_REFUSED troubleshooting
  • adds a clean temporary-profile lifecycle smoke test covering initialize → link install → Cordis composition → scan → uninstall

Validation after the change:

  • npm test: 466/466 passed
  • npm run test:dsh-lifecycle: passed
  • npm run test:dsh-e2e: passed (HTTP 200 and scanner tool execution)
  • npm run benchmark:dsh: all 5 pinned real-world cases stable

No Phase 1 detection rule or risk threshold changed in this follow-up.

@EchoOfZion

Copy link
Copy Markdown
Author

Release-artifact validation follow-up is ready in 55224ee.

The new npm run test:dsh-package gate builds the exact npm tarball and validates it in a clean temporary DSH profile: required runtime/type/Cordis/docs assets → tarball install → Cordis composition → scanner execution → package update → uninstall. It never publishes the package.

The first run caught that files: ["dist"] shipped all compiled tests. The package now explicitly excludes dist/tests:

  • entries: 457 → 353
  • packed size: 492,102 → 389,842 bytes
  • unpacked size: 2,451,003 → 1,636,026 bytes

Final validation:

  • npm test: 466/466 passed
  • clean link-profile lifecycle: passed
  • exact npm tarball lifecycle: passed
  • DSH runtime E2E: HTTP 200 and scanner execution passed
  • 5 pinned real-world benchmarks: stable
  • git diff --check: passed

No scanner rule, severity, recommendation, or Phase 1 boundary changed.

@Mr-Lucky

Copy link
Copy Markdown
Contributor

New findings:

  1. Remote repository resource exhaustion — High
    [High] Add repository-level resource limits before git fetch

This path accepts an attacker-controlled GitHub repository and performs a shallow fetch and checkout, but there is no limit on repository size, Git object count, disk usage, or total downloaded bytes. The later file-count and file-size limits only apply after checkout has completed, so a large repository can exhaust network, disk, memory, or CPU before scanning starts.

Please add acquisition-level limits, or use a bounded partial/sparse checkout, and abort safely when the repository exceeds the configured budget.

  1. Symlink escape — High
    [High] Do not follow symlinks outside the scan root

glob() can return symlinked files, and the subsequent stat() / readFile() calls follow those symlinks. A malicious GitHub repository could include a symlink such as leak.js pointing outside the checkout, causing the scanner to read local files that are not part of the repository. The contents may then appear in findings or reports.

Please use lstat() and reject symlinks, or resolve the real path and verify that it remains within the scan root before reading.

  1. Markdown report prompt injection — Medium/High
    [Medium/High] Escape all artifact-controlled values in Markdown output

The Markdown renderer only escapes | and newlines, while the package name, description, repository metadata, and detection signals can be controlled by the scanned repository. These values are returned through the DSH tool as model-visible text, so a malicious package could inject Markdown/HTML or prompt-injection instructions into the scan result.

The HTML renderer escapes these fields, but the Markdown and JSON/tool-output paths still expose them as trusted-looking text. Please escape all untrusted fields and clearly mark artifact content as untrusted data, or return structured fields instead of embedding them into a natural-language report.

  1. Cordis parse failures can cause false negatives — Medium/High
    [Medium/High] Fail closed when Cordis parsing fails

When Cordis parsing fails or the file exceeds the parser limits, parseCordisConfigs() returns no rows and records only a diagnostic. Later, scanDshPlugin() removes regex-based DSH_PATCH_OVERRIDE findings and calculates risk from successfully parsed rows. As a result, a malformed or unsupported Cordis patch could hide a security-relevant core-row replacement while still producing a low-risk recommendation.

Please make parse errors and unsupported/truncated Cordis structures raise the review status or minimum risk level. The scanner should not return safe-to-try when security-relevant configuration was not successfully understood.

  1. Insufficient dsh.client schema validation — Medium
    [Medium] Validate dsh.client instead of using truthiness

hasClientExtension is currently derived from Boolean(client). This means any truthy object, including { "dsh": { "client": {} } }, is treated as a valid DSH client extension. That can skew plugin classification, capability inference, and risk recommendations.

Please validate the expected dsh.client schema and only set hasClientExtension when the required fields are present and valid. Invalid metadata should produce an explicit diagnostic.
Overall review comment
Overall, the feature direction is reasonable, but I do not recommend merging until the scanner has bounded remote acquisition, rejects symlink escapes, and fails closed on unsupported or malformed security-relevant metadata.

These issues affect the scanner itself rather than the scanned plugin, and could lead to resource exhaustion, local file disclosure, prompt injection, or false-negative security recommendations.

@EchoOfZion

Copy link
Copy Markdown
Author

Addressed the five new scanner-self-protection findings in 367227c; c1231fc records the resulting phase1-rc2 provenance.

  1. Remote acquisition bounds

    • GitHub scans now use a depth-one, blob-less fetch.
    • Fetch and checkout are monitored against a 256 MiB on-disk budget and are killed on overflow.
    • Git object count is checked before and after checkout with a 100,000-object limit.
    • Existing timeouts, exact-HEAD resolution, detached checkout verification, disabled hooks, and no-submodule behavior remain.
  2. Symlink containment

    • Every scanner read now resolves the real path and verifies it remains inside the real scan root.
    • A symlink escaping the root aborts the scan; an in-repository symlink remains supported (the pinned superdesign-skill case uses one).
    • Package, Cordis, README, capability, rule-scan, and artifact-hash reads all use the same containment helper.
  3. Markdown / model-output trust boundary

    • Artifact-controlled metadata is removed from the Markdown title and rendered as JSON-escaped quoted data under an explicit untrusted-data boundary. Finding paths are JSON-quoted and Markdown-escaped.
    • The native DSH tool renderer no longer returns the target-controlled detailed report as model-visible prose. It renders a scanner-generated decision summary containing only enum decisions plus a warning that detailed content is untrusted. Raw Markdown/JSON remains output data for explicit consumers.
  4. Cordis fail closed

    • Malformed, oversized, unsupported, or unreadable Cordis/package metadata produces the new high-severity DSH_SCAN_INCOMPLETE finding.
    • Both risk views are at least HIGH, review priority is HIGH, and both recommendations are expert-review-required; safe-to-try is impossible when security metadata is incomplete.
  5. dsh.client validation

    • A client extension now requires a non-empty platform string and, when present, a string-array inject field. Invalid truthy objects are not classified as clients and enter the fail-closed path with an explicit diagnostic.

Validation:

  • full suite: 470 passed, 0 failed
  • clean link-profile lifecycle: passed
  • exact npm tarball install/update/remove lifecycle: passed
  • native DSH HTTP E2E: passed
  • real GitHub URL scan using the bounded acquisition path: passed
  • 5/5 exact-commit real-world benchmark cases: stable (no outcome changes for successfully parsed artifacts)
  • git diff --check: passed

The security baseline is now phase1-rc2 at 367227cc2b8bc064af369bf41e4490f6c4d3ea8b.

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