🛡️ Sentinel: [CRITICAL] Fix RCE and DoS risks in PDF compilation#215
Open
🛡️ Sentinel: [CRITICAL] Fix RCE and DoS risks in PDF compilation#215
Conversation
Added `-no-shell-escape` flags for `pdflatex` and `pandoc` in `CoverLetterGenerator` and `PDFConverter`. Added 30-second timeouts to all `subprocess.communicate()` calls for PDF generation to prevent hanging. Appended learning to `.jules/sentinel.md`. 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 GuideHardened LaTeX→PDF compilation in the cover letter generator and PDF converter by forbidding shell escapes, adding timeouts and proper cleanup for subprocess invocations, and documenting the security issue in the Sentinel log. Sequence diagram for pdflatex compilation with timeout and no_shell_escapesequenceDiagram
participant CoverLetterGenerator
participant PDFConverter
participant subprocess
participant pdflatex
rect rgb(230,230,250)
CoverLetterGenerator->>subprocess: Popen([pdflatex, -interaction=nonstopmode, -no-shell-escape, tex_path.name])
activate subprocess
subprocess->>pdflatex: start process in working_dir
activate pdflatex
subprocess->>pdflatex: communicate(timeout=30)
pdflatex-->>subprocess: stdout, stderr or long-running
alt completes_before_timeout
subprocess-->>CoverLetterGenerator: returncode, stdout, stderr
else TimeoutExpired
subprocess->>pdflatex: kill
pdflatex-->>subprocess: process terminated
subprocess-->>CoverLetterGenerator: TimeoutExpired handled, stdout, stderr
end
deactivate pdflatex
deactivate subprocess
end
rect rgb(240,255,240)
PDFConverter->>subprocess: Popen([pdflatex, -interaction=nonstopmode, -no-shell-escape, tex_path.name])
activate subprocess
subprocess->>pdflatex: start process in working_dir
activate pdflatex
subprocess->>pdflatex: communicate(timeout=30)
pdflatex-->>subprocess: stdout, stderr or long-running
alt completes_before_timeout
subprocess-->>PDFConverter: returncode, stdout, stderr
else TimeoutExpired
subprocess->>pdflatex: kill
pdflatex-->>subprocess: process terminated
subprocess-->>PDFConverter: TimeoutExpired handled, stdout, stderr
end
deactivate pdflatex
deactivate subprocess
end
Sequence diagram for pandoc compilation with timeout and no_shell_escapesequenceDiagram
participant CoverLetterGenerator
participant PDFConverter
participant subprocess
participant pandoc
rect rgb(230,230,250)
CoverLetterGenerator->>subprocess: Popen([pandoc, tex_path, -o, output_path, --pdf-engine=xelatex, --pdf-engine-opt=-no-shell-escape])
activate subprocess
subprocess->>pandoc: start process
activate pandoc
subprocess->>pandoc: communicate(timeout=30)
pandoc-->>subprocess: stdout, stderr or long-running
alt completes_before_timeout
subprocess-->>CoverLetterGenerator: returncode, stdout, stderr
else TimeoutExpired
subprocess->>pandoc: kill
pandoc-->>subprocess: process terminated
subprocess-->>CoverLetterGenerator: TimeoutExpired handled, stdout, stderr
end
deactivate pandoc
deactivate subprocess
end
rect rgb(240,255,240)
PDFConverter->>subprocess: Popen([pandoc, tex_path, -o, output_path, --pdf-engine=xelatex, --pdf-engine-opt=-no-shell-escape])
activate subprocess
subprocess->>pandoc: start process
activate pandoc
subprocess->>pandoc: communicate(timeout=30)
pandoc-->>subprocess: stdout, stderr or long-running
alt completes_before_timeout
subprocess-->>PDFConverter: returncode, stdout, stderr
else TimeoutExpired
subprocess->>pandoc: kill
pandoc-->>subprocess: process terminated
subprocess-->>PDFConverter: TimeoutExpired handled, stdout, stderr
end
deactivate pandoc
deactivate subprocess
end
File-Level Changes
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 left some high level feedback:
- The timeout/TimeoutExpired handling around
subprocess.Popen(...).communicate()is duplicated in multiple places; consider extracting a small helper (e.g.,run_with_timeout(cmd, cwd, timeout=30)) to keep the pattern consistent and easier to maintain. - The hard-coded
timeout=30is used in several locations; defining a single module-level constant (e.g.,PDF_COMPILE_TIMEOUT_SECONDS) would make it easier to tune this behavior and keep the values in sync. - On timeout you currently kill the process but ignore the failure outcome; depending on how this is surfaced to callers, you might want to explicitly treat timeouts as a failed compilation (e.g., via a distinct return value or error message) so consumers can react appropriately.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timeout/TimeoutExpired handling around `subprocess.Popen(...).communicate()` is duplicated in multiple places; consider extracting a small helper (e.g., `run_with_timeout(cmd, cwd, timeout=30)`) to keep the pattern consistent and easier to maintain.
- The hard-coded `timeout=30` is used in several locations; defining a single module-level constant (e.g., `PDF_COMPILE_TIMEOUT_SECONDS`) would make it easier to tune this behavior and keep the values in sync.
- On timeout you currently kill the process but ignore the failure outcome; depending on how this is surfaced to callers, you might want to explicitly treat timeouts as a failed compilation (e.g., via a distinct return value or error message) so consumers can react appropriately.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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: CRITICAL
💡 Vulnerability: The
CoverLetterGeneratorandPDFConvertercompiled LaTeX to PDF without enforcing the-no-shell-escapeflag forpdflatexandpandoc. In addition, the blocking calls tosubprocess.Popen.communicate()lacked a timeout parameter.🎯 Impact: This oversight created two critical security risks:
\write18into templates to execute arbitrary shell commands on the host server.pdflatexorpandocto hang indefinitely, tying up system resources and causing a DoS.🔧 Fix:
"-no-shell-escape"topdflatexcommands."--pdf-engine-opt=-no-shell-escape"topandoccommands.timeout=30toprocess.communicate()calls using the standard Python pattern to safely catchsubprocess.TimeoutExpired, kill the process, and clean up to prevent zombie processes.✅ Verification:
pytest tests/test_pdf_security.pypasses. All tests run locally pass. Added an entry to.jules/sentinel.mdcapturing these learnings.PR created automatically by Jules for task 8044524251336920935 started by @anchapin
Summary by Sourcery
Harden PDF compilation by disabling LaTeX shell escapes and enforcing timeouts on external compilation processes to mitigate RCE and DoS risks.
Enhancements:
Documentation: