🛡️ Sentinel: [High] Fix RCE and DoS in PDF compilation#272
Open
🛡️ Sentinel: [High] Fix RCE and DoS in PDF compilation#272
Conversation
- Add `-no-shell-escape` flags to `pdflatex` and `pandoc` subprocess calls in `cli/pdf/converter.py` and `cli/generators/cover_letter_generator.py`. - Add 30-second timeouts to `process.communicate()` to prevent infinite loops. - Add and update tests in `tests/test_pdf_security.py`. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideHarden PDF compilation in PDFConverter and CoverLetterGenerator against RCE and DoS by enforcing no-shell-escape flags, adding timeouts and cleanup for subprocess communication, and adding tests plus Sentinel documentation for the security fix. Sequence diagram for secured pdflatex and pandoc compilationsequenceDiagram
participant Caller
participant PDFConverter
participant Subprocess
participant Pdflatex
participant Pandoc
Caller->>PDFConverter: _compile_pdflatex(tex_path, output_path, working_dir)
PDFConverter->>Subprocess: Popen([pdflatex,-interaction=nonstopmode,-no-shell-escape,tex_path.name],cwd=working_dir)
Subprocess-->>Pdflatex: start process
PDFConverter->>Subprocess: communicate(timeout=30)
alt pdflatex_completes_in_time
Subprocess-->>PDFConverter: stdout, stderr
alt success_or_output_exists
PDFConverter-->>Caller: True
else failure_and_no_output
PDFConverter-->>Caller: False
end
else pdflatex_timeout
Subprocess-->>PDFConverter: TimeoutExpired
PDFConverter->>Subprocess: kill()
PDFConverter->>Subprocess: communicate()
Subprocess-->>PDFConverter: stdout, stderr
PDFConverter-->>Caller: False
end
Caller->>PDFConverter: _compile_pandoc(tex_path, output_path, working_dir)
PDFConverter->>Subprocess: Popen([pandoc,tex_path,-o,output_path,--pdf-engine=xelatex,--pdf-engine-opt=-no-shell-escape],cwd=working_dir)
Subprocess-->>Pandoc: start process
PDFConverter->>Subprocess: communicate(timeout=30)
alt pandoc_completes_in_time
Subprocess-->>PDFConverter: stdout, stderr
alt success_or_output_exists
PDFConverter-->>Caller: True
else failure_and_no_output
PDFConverter-->>Caller: False
end
else pandoc_timeout
Subprocess-->>PDFConverter: TimeoutExpired
PDFConverter->>Subprocess: kill()
PDFConverter->>Subprocess: communicate()
Subprocess-->>PDFConverter: stdout, stderr
PDFConverter-->>Caller: False
end
Flow diagram for secured cover letter PDF compilation with fallbackflowchart TD
Start([Start _compile_pdf])
A[Write tex_content to temporary tex_path]
B[Run pdflatex with -interaction=nonstopmode and -no-shell-escape]
C{pdflatex communicate timeout?}
D[Kill pdflatex process
and call communicate again]
E{pdflatex succeeded
or output_path exists?}
F[Set pdf_created to True
and return]
G[Log failure and
attempt pandoc fallback]
H[Run pandoc with --pdf-engine=xelatex
and --pdf-engine-opt=-no-shell-escape]
I{pandoc communicate timeout?}
J[Kill pandoc process
and call communicate again]
K{pandoc succeeded
or output_path exists?}
L[Set pdf_created to True
and return]
M[Set pdf_created to False
and return]
Start --> A --> B --> C
C -- Yes --> D --> E
C -- No --> E
E -- Yes --> F
E -- No --> G --> H --> I
I -- Yes --> J --> K
I -- No --> K
K -- Yes --> L
K -- No --> M
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The timeout and
-no-shell-escapehandling aroundsubprocess.Popenis now duplicated in bothPDFConverterandCoverLetterGenerator; consider extracting a small shared helper (e.g.,run_latex_command(...)) so the security-critical behavior is defined in one place and easier to keep consistent. - The 30-second timeout is currently an inline literal in multiple places; introducing a single module-level constant (or configuration) for the PDF compile timeout would make it easier to tune and ensure all call sites stay aligned.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timeout and `-no-shell-escape` handling around `subprocess.Popen` is now duplicated in both `PDFConverter` and `CoverLetterGenerator`; consider extracting a small shared helper (e.g., `run_latex_command(...)`) so the security-critical behavior is defined in one place and easier to keep consistent.
- The 30-second timeout is currently an inline literal in multiple places; introducing a single module-level constant (or configuration) for the PDF compile timeout would make it easier to tune and ensure all call sites stay aligned.
## Individual Comments
### Comment 1
<location path="tests/test_pdf_security.py" line_range="107" />
<code_context>
+ self.assertIn("--pdf-engine-opt=-no-shell-escape", command)
+ self.assertIn("pandoc", command)
+
+ @patch("subprocess.Popen")
+ def test_cover_letter_pdflatex_timeout_and_args(self, mock_popen):
+ process_mock = MagicMock()
</code_context>
<issue_to_address>
**issue (testing):** Patch target for `Popen` in `CoverLetterGenerator` tests is incorrect and may call the real subprocess implementation
Because `cover_letter_generator.py` imports `subprocess` directly, the patch must target the symbol as used in that module (e.g. `@patch("cli.generators.cover_letter_generator.subprocess.Popen")`). Using `@patch("subprocess.Popen")` won’t intercept the call inside `_compile_pdf` and can trigger the real `pdflatex` during tests. Please update the patch target accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.assertIn("--pdf-engine-opt=-no-shell-escape", command) | ||
| self.assertIn("pandoc", command) | ||
|
|
||
| @patch("subprocess.Popen") |
There was a problem hiding this comment.
issue (testing): Patch target for Popen in CoverLetterGenerator tests is incorrect and may call the real subprocess implementation
Because cover_letter_generator.py imports subprocess directly, the patch must target the symbol as used in that module (e.g. @patch("cli.generators.cover_letter_generator.subprocess.Popen")). Using @patch("subprocess.Popen") won’t intercept the call inside _compile_pdf and can trigger the real pdflatex during tests. Please update the patch target accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚨 Severity: HIGH
💡 Vulnerability: The
PDFConverterandCoverLetterGeneratormodules were executingpdflatexandpandocviasubprocess.Popenwithout enforcing the-no-shell-escapeflag or timeouts on thecommunicate()calls.🎯 Impact: This allowed potential Remote Code Execution (RCE) via LaTeX's
\write18{...}feature and Denial of Service (DoS) via infinite compilation loops when handling untrusted user input or maliciously crafted job descriptions/templates.🔧 Fix:
-no-shell-escape(and--pdf-engine-opt=-no-shell-escapefor pandoc) to all external compilation commands incli/pdf/converter.pyandcli/generators/cover_letter_generator.py.process.communicate(), catchingsubprocess.TimeoutExpiredto explicitly kill hanging processes and clear the I/O buffers.tests/test_pdf_security.py.✅ Verification: Ran the full
pytestsuite ensuring all tests passed, and specifically verifiedtests/test_pdf_security.pysuccessfully caught the expected mock behaviors without regressions.PR created automatically by Jules for task 11426473784438085786 started by @anchapin
Summary by Sourcery
Harden PDF generation against command injection and hangs by securing LaTeX/Pandoc subprocess execution and adding regression tests.
Bug Fixes:
Documentation:
Tests: