Skip to content

security: escape untrusted model output in HTML reports, harden fetch/report paths - #25

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1784930596-security-hardening
Open

security: escape untrusted model output in HTML reports, harden fetch/report paths#25
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1784930596-security-hardening

Conversation

@devin-ai-integration

Copy link
Copy Markdown

Summary

Security pass over the codebase. Four issues fixed; all inputs involved are attacker-influenceable (LLM output is derived from third-party client source code that PRSpec fetches, so prompt injection in a scanned repo can steer what lands in a report).

1. HTML injection in the single-client report (highest severity). _generate_html_report escaped issue fields but interpolated the narrative, status, file_name, and the ReportMetadata fields raw. The narrative embeds result["summary"] verbatim, so any model-produced <script> executes when the report is opened locally (file:// origin, reads local files, exfiltrates). Now every interpolation goes through html.escape; same for row.verdict / KPI numbers in the differential report.

-white-space:pre-line;">{self._build_narrative(results, metadata)}</div>
+white-space:pre-line;">{html.escape(self._build_narrative(results, metadata))}</div>

2. Report filenames could escape output/. Filenames were built from metadata.client (a CLI value) and metadata.eip_number, so --client ../../foo wrote outside the output directory. Added _slug() over both components.

3. Cache filenames could escape the cache dir. SpecFetcher/CodeFetcher built cache keys with path.replace('/', '_') only — branch, owner, repo were unsanitized. Replaced with a shared _cache_name() that maps everything outside [A-Za-z0-9._-] to _.

4. clone_repository() argument injection. url/branch were passed straight to git clone, so ext::sh -c ... or a --upload-pack= "URL" is command execution. Added _validate_clone_url (scheme allowlist, no leading -) and _validate_branch (plain ref names only).

Also added request timeouts (DEFAULT_TIMEOUT = 30) to every session.get in both fetchers — previously a slow/hostile endpoint hung the CLI indefinitely.

New tests/test_report_security.py covers all four (156 passing, ruff clean).

Reviewed and found clean

No hardcoded secrets in the tree or in git history (.env.example holds placeholders only). No SQL, no HTTP server, no CORS config, no debug endpoints, no auth surface — PRSpec is a local CLI/library. yaml.safe_load already used. pip-audit reports no known-vulnerable dependencies.

Known residual risks (not fixed here)

  • Dependencies in requirements.txt are fully unpinned (requests, pyyaml, click, ...) with no floors or hashes; a compromised upstream release is installed silently.
  • Analyzing a repo means feeding untrusted source into the prompt; the JSON contract is advisory, so report content remains attacker-influenceable even though it can no longer execute.

Link to Devin session: https://app.devin.ai/sessions/99d7c66d95a64b73b3c0a0c4686266a7
Requested by: @Fosurero

@Fosurero Fosurero self-assigned this Jul 24, 2026
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

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