Skip to content

fix: restrict project config trust - #1565

Merged
thymikee merged 4 commits into
mainfrom
agent/restrict-project-config-trust
Aug 3, 2026
Merged

fix: restrict project config trust#1565
thymikee merged 4 commits into
mainfrom
agent/restrict-project-config-trust

Conversation

@thymikee

@thymikee thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member

Summary

Restrict automatically discovered ./agent-device.json files to an explicit allowlist of project-safe automation defaults. New flags are operator-only by default; connection, credential, tenant, lease, provider, cloud, Metro, header, and local-path settings require an operator-controlled source such as user config, explicit config, CLI arguments, environment variables, or a saved remote profile.

Carry daemon authentication in a typed transport context instead of serialized RPC flags, including direct React DevTools dispatch, while preserving supported request-token and HTTP authentication behavior. Register the token with centralized diagnostics redaction at the CLI boundary.

Update configuration help, documentation, tests, and the changelog to explain the trust boundary and CI migration.

Credit

Reported responsibly by @glmgbj233. Thank you for the clear report and isolated reproduction.

Validation

  • pnpm check:tooling
  • pnpm check:fallow
  • Focused config/auth/client/daemon regression suites
  • Built CLI/package-path inspection
  • Independent final-diff review; findings resolved
  • pnpm check:affected --run: all static/type/build gates passed and 2,446/2,447 related tests passed. The sole failure was the known Android provider lifecycle timeout under aggregate host load; that exact suite passed 12/12 in isolation. Pushed with explicit maintainer approval for this flaky-gate exception.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-03 13:48 UTC

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.93 MB 1.93 MB +1.9 kB
JS gzip 619.2 kB 620.1 kB +856 B
npm tarball 738.9 kB 739.7 kB +788 B
npm unpacked 2.59 MB 2.59 MB +2.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 18.6 ms 18.7 ms +0.1 ms
CLI --help 44.8 ms 43.7 ms -1.1 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/agent-device-client.js +224 B +89 B
dist/src/cli.js +164 B +52 B
dist/src/cli-help.js +94 B +46 B
dist/src/registry.js +30 B +26 B

@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Reviewed exact head c423667; not merge-ready.

  1. Security blocker: CONFIG_TRUST_BY_FLAG still marks reporter as project-safe. A repository agent-device.json can provide a custom reporter path, and agent-device test dynamically imports that module before daemon dispatch, preserving project-config code execution across the new trust boundary. reportJunit, saveScript, and launchConsole are likewise local-path write sinks still marked project-safe. Reclassify these as user/explicit-only (or use value-sensitive handling where a safe boolean form exists) and add project-config rejection/counterfactual coverage.

  2. Remote-auth regression: the public client stores daemonAuthToken against the original request in a WeakMap, but the normal non-JSON createCliDaemonTransport branch spreads/clones that request to add progress metadata. The clone loses the auth association after the token has been removed from serializable flags, so explicit/config/CLI tokens no longer authenticate ordinary human-output remote commands; JSON or environment fallback can mask it. Preserve the auth association across that wrapper and add a non-JSON loopback test proving auth headers/RPC token are present while serialized flags remain secret-free.

The direct endpoint-redirection path is otherwise closed, the trust table is exhaustive, and exact-head CI is green.

@thymikee
thymikee marked this pull request as ready for review August 3, 2026 11:36
@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 96af6d7. The non-JSON remote-auth blocker is fixed by the typed transport context and regression coverage, and exact-head CI is green. One security blocker remains: project config still permits reporter, reportJunit, saveScript, and launchConsole. A repository-controlled reporter reaches the custom reporter’s dynamic import() before daemon dispatch (host code execution); the other three are repository-controlled local write paths. Remove/reclassify these keys from project-safe config, align the docs, and add rejection/no-side-effect counterfactuals. Until then this is not merge-ready.

@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Addressed in 689fb2985.

  • Removed reporter, reportJunit, saveScript, and launchConsole from the project-config allowlist; they remain supported from user/explicit config and CLI/env sources.
  • Added counterfactual coverage proving a project reporter is rejected before module import or daemon dispatch, and each local write sink is rejected before dispatch/file creation. The four tests failed against the previous policy and now pass.
  • Updated configuration docs and the changelog.

Validation: focused config suite 52/52 green. The affected aggregate passed all static/type/build gates and 2,448 tests; three provider tests timed out under host load, then both affected files passed 33/33 in isolation. Pushed under the maintainer-approved flaky-gate exception.

@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 689fb29. The prior security blocker is fixed: project config no longer permits reporter, reportJunit, saveScript, or launchConsole; the new counterfactuals prove no custom-module import, daemon dispatch, or target-file creation, and docs/changelog now match the boundary. I found no bypass in the production route. Code review is clean. Readiness remains pending because required Coverage is red, but its log shows no test assertion/application failure: all changed trust tests passed and Vitest timed out terminating the untouched fuzz corpus worker, then emitted a worker-pool error. Treat that as a CI/test-harness flake and rerun Coverage.

@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Addressed. The comment identified a CI harness flake rather than a source defect, so no code change was made. I reran the failed Coverage job from CI run 30815238040; attempt 2 passed in 4m10s, including the changed-line coverage gate and contention-retry policy. The complete PR check matrix is now green.

@thymikee
thymikee merged commit 99967c7 into main Aug 3, 2026
46 of 47 checks passed
@thymikee
thymikee deleted the agent/restrict-project-config-trust branch August 3, 2026 13:48
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.

1 participant