feat(analyze-problem): download attachments/screenshots behind a mandatory security gate (#725)

[pekral]

Summary

Adds reliable, security-gated downloading of issue-tracker attachments/screenshots so analyze-problem no longer runs blind. Bytes are downloaded into a 0600 quarantine with TLS validation always on, then a mandatory deterministic security gate classifies every file; only safe, allowlisted, inert types are promoted to safe/ for the analysis to read. Resolves #725.

What changed

• skills/_shared/attachments.sh — sourced download library. Fetches each inventory URL into <dest>/_quarantine/ (0600), TLS pinned to HTTPS (--proto/--proto-redir '=https', never -k), auth read only from a 0600 curl --config file (token never in argv/logs), size-capped per transfer, then writes attachments-manifest.json and invokes the scan gate.
• skills/_shared/scan-attachments.sh — standalone, deterministic security gate. Classifies from magic bytes + a bounded content sniff and blocks executables, archives, scripts, HTML, SVG with active content, polyglots, declared/actual MIME mismatches, and over-limit files; promotes only allowlisted inert types (png/jpg/gif/webp/pdf/txt/log/csv/json) to safe/. Ships --self-test.
• skills/code-review-{github,jira,bugsnag}/scripts/download-attachments.sh — thin per-tracker wrappers (auth + inventory) delegating to the shared library.
• skills/analyze-problem/SKILL.md — pre-flight now enforces inventory → download → security gate → analyse only safe/, documents per-tracker auth setup, and the block/review handling.

Security posture (rules/security/*)

• TLS validation always on; no -k / --insecure / --no-check-certificate / verify=false anywhere (pinned by test).
• No curl … | sh — responses go to --output, never an interpreter.
• Token only inside a 0600 curl --config file — never argv, never the manifest, never a log line. GitHub token sent to github.com only (-L, not --location-trusted); Bugsnag token never forwarded to third-party hosts.
• Quarantine files are 0600 under the scratchpad (not the repo), not hidden, with no background process; blocked files are reported, never opened.

Testing

• bash skills/_shared/scan-attachments.sh --self-test → benign PNG promoted; malicious SVG/HTML/polyglot blocked (the fixture proof required by the issue).
• New Pest content-pinning tests (tests/Installer/SkillsContentTest.php) cover the scripts, the no-disabled-TLS / token-out-of-argv guarantees, the gate's blocked categories + allowlist + limits, and the SKILL.md mandatory order.
• composer build green (322 tests, 100% coverage on PHP).

Note: per the project's test-isolation rule a Pest test cannot exec a real .sh, so the fixture proof lives in the script's --self-test and its asserted outcomes are pinned in the test source.

How to test

1. Set GitHub auth (gh auth login), then run on an issue with an inline screenshot:
   skills/code-review-github/scripts/download-attachments.sh <NUMBER|URL> → inspect attachments-manifest.json and safe/.
2. JIRA: export JIRA_API_EMAIL + a token source, run skills/code-review-jira/scripts/download-attachments.sh <KEY|URL>.
3. Run bash skills/_shared/scan-attachments.sh --self-test and confirm PASS.

TODO (out of scope / deferred)

• Add download-attachments.sh to the installer's bundled-script permission allowlist (InstallerClaudeSettings) so agents don't get a per-call prompt — kept out to avoid touching the pinned allowlist contract in this PR.
• SSRF on inventory URLs — resolved in code review: att_run now blocks loopback / link-local / private hosts via att_host_block_reason (opt-out ATT_ALLOW_PRIVATE_HOSTS=1).
• Polyglot sniff is bounded to the first 4096 bytes (payloads hidden later evade the polyglot heuristic, though file-based typing + allowlist still apply); revisit if deeper inspection is needed.

[pekral]

Technical report — pre-PR self-review

Code review summary

Inline self-review against the project rules during the implementation loop. The
change is bash tooling + Markdown + one PHP test; no Critical/Moderate findings
remained at PR creation. Design notes:

• Each per-tracker wrapper is thin and delegates download + scan to the single shared
  library (skills/_shared/attachments.sh) — no duplicated download logic.
• att_run re-serialises the manifest accumulator once per attachment; bounded by the
  25-attachment cap, so acceptable.
• A failed download is contained (if att_download …) and recorded as block with a
  reason rather than aborting the whole run.

The authoritative argos (quality) / athena (security) review runs on the open PR;
the PR stays Draft until that loop converges to 0 Critical + 0 Moderate.

Security review summary

Self-review against rules/security/backend.md + frontend.md
(Malicious Code & Supply-Chain Indicators, Malicious File Upload Content):

• TLS always on — --proto/--proto-redir '=https', no -k/--insecure/
  --no-check-certificate/verify=false (pinned by test).
• No curl … | sh — responses written to --output, never piped to an interpreter.
• Token isolation — auth lives only in a 0600 curl --config file; never in argv,
  the manifest, or logs. GitHub token sent to github.com only (-L, not
  --location-trusted); Bugsnag token never forwarded to third-party hosts.
• Quarantine — files 0600 under the scratchpad (not the repo), non-hidden, no
  background process; size-capped per transfer.
• Upload-content gate — magic-byte typing + bounded sniff blocks executables,
  archives, scripts, HTML, SVG-with-active-content, polyglots, MIME mismatch; only
  inert allowlisted types reach safe/. Proven by scan-attachments.sh --self-test.

Residual (documented in PR TODO): SSRF surface inherent to fetching issue-supplied
URLs is mitigated by HTTPS-only + size cap + the quarantine gate; the polyglot sniff is
bounded to the first 4096 bytes.

composer build: green — 322 tests, 100% PHP coverage.

[pekral]

Code review — resolved items (converged: 0 Critical, 0 Moderate)

Processed the review on this PR. Convergence reached in 2 loop iterations; final re-review reports 0 Critical, 0 Moderate. No reviewer review-threads were open (the CR was a general comment), so none required resolution.

• Moderate — Bugsnag SSRF: inventory URLs fetched with no host guard

  • Why: a comment-supplied URL (e.g. https://169.254.169.254/...) could drive an outbound request to an internal service from the agent host.
  • Reason: @rules/security/backend.md External Requests / security-review External Interaction (SSRF) require an allowlist + private-IP guard on outbound requests with user-controlled input; only the Bugsnag wrapper had an open outbound surface (GitHub is host-pinned by regex, JIRA contentUrl is server-issued).
  • Solution: added att_host_block_reason in skills/_shared/attachments.sh; att_run now blocks loopback / link-local (incl. 169.254.169.254) / RFC-1918 / ULA hosts before any request and records blocked host — … in the manifest, for all three trackers. Self-hosted trackers opt out with ATT_ALLOW_PRIVATE_HOSTS=1. Pinned by a new Pest test.

• Minor — 2>/dev/null on loader / gh auth token was unannotated

  • Why: the security rule's benign-use carve-out for stderr suppression requires an inline justification, which was missing.
  • Reason: @rules/security/backend.md Malicious Code & Supply-Chain Indicators.
  • Solution: added a one-line comment on each suppression in the three download wrappers (the result is validated on the next line).

• Minor — scan_manifest could orphan its per-entry temp file on jq failure

  • Why: under set -e, a failing jq … > "$tmp" && mv aborted the script leaving $tmp behind.
  • Reason: robustness (@rules/php/core-standards.mdc Design Principles — keep I/O clean).
  • Solution: rewrote to if jq … > "$tmp"; then mv; else rm -f "$tmp"; exit 3; fi so the temp file is always cleaned.

composer build: green — 323 tests, 100% PHP coverage. scan-attachments.sh --self-test: PASS.

[pekral]

Code Review — PR #726 (final diff)

Status: ✅ Converged — ready to merge
Counts: Critical 0 · Moderate 0 · Minor 0
Assignment conformance: conformant — every acceptance criterion maps to a change and every changed block traces to the assignment.

All findings from the prior review iteration were resolved (see the cr-status comment for the per-item justification):

• Moderate Bugsnag SSRF → closed by the att_host_block_reason guard in skills/_shared/attachments.sh (loopback / link-local / RFC-1918 / ULA blocked before any request, ATT_ALLOW_PRIVATE_HOSTS=1 opt-out for self-hosted trackers), pinned by a new Pest test.
• 2× Minor (unannotated 2>/dev/null; orphaned scan temp file) → resolved.

Specialized reviews self-skipped (no matching surface): api-review, mysql-problem-solver, dependency-selection, assignment-compliance-check (no gaps), ## Architecture (non-Laravel package). Coverage: changed PHP is test-only; composer build green — 323 tests, 100% coverage. scan-attachments.sh --self-test: PASS.

Summary: assignment conformance: conformant. The change keeps TLS validation on, the auth token out of argv/manifest/logs, guards every outbound URL against SSRF to non-public hosts, and quarantines every download behind the deterministic security gate before promotion to safe/. No blocking findings remain.

[pekral]

Code Review — PR #726 (re-review of the final diff after rebase)

Status: ✅ Converged — ready to merge
Counts: Critical 0 · Moderate 0 · Minor 0
Head: d28674f · Assignment conformance: conformant.

Re-reviewed after the branch was rebased onto master (which now carries the CI matrix fix from #727). The rebase introduced no conflicts and changed no reviewed code — the 9-file diff is identical to the previously converged state:

• Moderate Bugsnag SSRF → resolved by the att_host_block_reason guard in skills/_shared/attachments.sh (loopback / link-local / RFC-1918 / ULA blocked before any request; ATT_ALLOW_PRIVATE_HOSTS=1 opt-out), pinned by a Pest test.
• 2× Minor (unannotated 2>/dev/null; orphaned scan temp file) → resolved.

CI Quality Checks (PHP 8.5) is green on d28674f; scan-attachments.sh --self-test PASS. No findings remain.

Summary: assignment conformance: conformant. TLS stays on, the token never reaches argv/manifest/logs, every outbound URL is SSRF-guarded against non-public hosts, and every download is quarantined behind the deterministic security gate before promotion to safe/. No blocking findings.