Skip to content

Track remaining qa-changes security related follow-ups #212

@enyst

Description

@enyst

Context

Follow-up from PR #135 (feat: add qa-changes plugin for automated PR QA validation) and the blast-radius review comment here:

This is intended as a follow-up tracking issue, not a blocker for merging the current PR.

Already addressed in PR #135

These items from the original list are already handled and should not be tracked here as remaining work:

  • ✅ Disable shared uv cache by default in qa-changes (8b89af1)
  • ✅ Tell the QA prompt to treat all PR-derived content as untrusted, not just the PR body (81a9527)
  • ✅ Gracefully skip fork PR runs when secrets are unavailable instead of failing (9774ad8)

Remaining follow-ups worth tracking

These are the items from the original blast-radius list that still seem relevant after the fixes above, especially for secret-bearing runs on same-repo PRs or maintainer-triggered runs:

  • Do not load repo-controlled guidance from the PR head for secret-bearing QA runs

    • Load AGENTS.md / project skills from the base branch or another maintainer-controlled source instead of the checked-out PR head.
  • Decide whether secret-bearing QA should be manual / maintainer-triggered only

    • The original recommendation was to prefer qa-this / reviewer-request style triggering over automatic secret-bearing execution.
  • Document the runner trust model more explicitly

    • The example workflow currently uses GitHub-hosted runners, but docs should clearly say this workflow should not be used on self-hosted/shared runners without stronger sandboxing.
  • Make telemetry opt-in in the example workflow

    • lmnr-api-key is optional in the action, but the example workflow still wires in the Laminar secret by default.
  • Review and minimize credential scope

    • Prefer the default workflow GITHUB_TOKEN over broader PAT/App tokens.
    • Re-check whether the current permissions are the minimum required (for example, whether issues: write is still needed in addition to review posting).
  • Separate execution from privileged reporting

    • Longer-term, the runner that executes PR code should ideally not share the same trust domain / credentials as the component that posts the final QA result.
  • Reduce raw secret exposure to subprocesses

    • Today the QA agent can use shell + gh api directly. A narrower harness-owned reporting primitive would reduce the blast radius.
  • Add network / egress restrictions guidance

    • During setup/exercise, ideally allow only the minimum outbound destinations needed (for example package registries, localhost, GitHub API).
  • Add security rails / analyzers for obvious exfiltration paths

    • Examples: env, printenv, reading credential files, arbitrary curl/wget, browsing attacker-controlled non-localhost URLs surfaced by the PR, etc.
  • Consider a stronger executor isolation model

    • Examples from the original list: two-sandbox split, fresh isolated executor, no persistent trust/caches/host mounts, tighter containment for long-term hardening.

Notes

The fork-specific concern from the original comment is now less urgent because the action skips cleanly when secrets are unavailable on fork PRs. The items above are the ones that still appear to matter for broader rollout or for any run that executes untrusted PR code while secrets are present.

This issue was created by an AI agent (OpenHands) on behalf of the user.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions