🛡️ Sentinel: [CRITICAL] Fix command injection in PDF generation#225
Open
🛡️ Sentinel: [CRITICAL] Fix command injection in PDF generation#225
Conversation
…eration 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 GuideSecures LaTeX/PDF generation in CoverLetterGenerator by disabling shell escape for pdflatex/pandoc and adding timeouts with explicit process cleanup to prevent RCE/DoS and zombie processes. Sequence diagram for secured PDF compilation with timeout and fallbacksequenceDiagram
actor User
participant CoverLetterGenerator
participant pdflatex
participant pandoc
User->>CoverLetterGenerator: generate_cover_letter
CoverLetterGenerator->>CoverLetterGenerator: _compile_pdf(output_path, tex_content)
Note over CoverLetterGenerator: Primary attempt using pdflatex with -no-shell-escape
CoverLetterGenerator->>pdflatex: Popen([pdflatex, -interaction=nonstopmode, -no-shell-escape, tex_path.name])
pdflatex-->>CoverLetterGenerator: process handle
CoverLetterGenerator->>pdflatex: communicate(timeout=30)
alt pdflatex completes in time
pdflatex-->>CoverLetterGenerator: stdout, stderr, returncode
alt returncode == 0 or output_path exists
CoverLetterGenerator-->>User: PDF compiled successfully
else returncode != 0 and output_path missing
Note over CoverLetterGenerator: Try pandoc fallback
CoverLetterGenerator->>pandoc: Popen([pandoc, tex_path, -o, output_path, --pdf-engine=xelatex, --pdf-engine-opt=-no-shell-escape])
pandoc-->>CoverLetterGenerator: process handle
CoverLetterGenerator->>pandoc: communicate(timeout=30)
alt pandoc completes in time
pandoc-->>CoverLetterGenerator: stdout, stderr, returncode
alt returncode == 0 or output_path exists
CoverLetterGenerator-->>User: PDF compiled via pandoc fallback
else
CoverLetterGenerator-->>User: PDF compilation failed
end
else pandoc timeout
CoverLetterGenerator->>pandoc: kill()
pandoc-->>CoverLetterGenerator: stdout, stderr after kill
CoverLetterGenerator-->>User: PDF compilation failed
end
end
else pdflatex timeout
CoverLetterGenerator->>pdflatex: kill()
pdflatex-->>CoverLetterGenerator: stdout, stderr after kill
Note over CoverLetterGenerator: No pandoc fallback on timeout
CoverLetterGenerator-->>User: PDF compilation failed
end
Class diagram for CoverLetterGenerator PDF compilation changesclassDiagram
class CoverLetterGenerator {
+bool _compile_pdf(output_path, tex_content)
}
class SubprocessUsage {
+Popen(args, stdout, stderr, cwd)
+communicate(timeout)
+kill()
}
CoverLetterGenerator ..> SubprocessUsage : uses
class PdfLatexInvocation {
-command pdflatex
-option interaction_nonstopmode
-option no_shell_escape
-arg tex_path_name
+build_args()
}
class PandocInvocation {
-command pandoc
-arg tex_path
-option output_path
-option pdf_engine_xelatex
-option pdf_engine_opt_no_shell_escape
+build_args()
}
CoverLetterGenerator ..> PdfLatexInvocation : primary
CoverLetterGenerator ..> PandocInvocation : fallback
class TimeoutHandling {
+int timeout_seconds
+handle_timeout(process)
}
TimeoutHandling : timeout_seconds = 30
CoverLetterGenerator ..> TimeoutHandling : enforces timeout
Flow diagram for timeout and process cleanup in PDF compilationflowchart TD
A[Start _compile_pdf] --> B[Run pdflatex with -no-shell-escape via Popen]
B --> C[communicate timeout 30]
C -->|Completes| D{pdflatex success or output_path exists}
C -->|TimeoutExpired| E[Kill pdflatex]
E --> F[communicate after kill]
F --> G[Return False]
D -->|Yes| H[Set pdf_created True]
H --> Z[End]
D -->|No| I[Run pandoc fallback with --pdf-engine-opt=-no-shell-escape]
I --> J[communicate timeout 30]
J -->|Completes| K{pandoc success or output_path exists}
J -->|TimeoutExpired| L[Kill pandoc]
L --> M[communicate after kill]
M --> N[Return False]
K -->|Yes| O[Set pdf_created True]
O --> Z
K -->|No| P[Return False]
Z[End _compile_pdf]
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 handling logic for both the
pdflatexandpandocsubprocesses is duplicated; consider extracting it into a small helper function to centralize thecommunicate(timeout=...)/TimeoutExpired/kill()pattern and keep this method easier to read and maintain. - The 30-second timeout is currently hardcoded in two places; consider defining a single module-level or class-level constant (e.g.,
PDF_COMPILE_TIMEOUT_SECONDS) so future tuning doesn’t require touching multiple call sites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timeout handling logic for both the `pdflatex` and `pandoc` subprocesses is duplicated; consider extracting it into a small helper function to centralize the `communicate(timeout=...)` / `TimeoutExpired` / `kill()` pattern and keep this method easier to read and maintain.
- The 30-second timeout is currently hardcoded in two places; consider defining a single module-level or class-level constant (e.g., `PDF_COMPILE_TIMEOUT_SECONDS`) so future tuning doesn’t require touching multiple call sites.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
_compile_pdfmethod inCoverLetterGeneratorexecutedpdflatexandpandocwithout the-no-shell-escapeflag. LaTeX's shell escape feature could be abused to execute arbitrary commands if compiling malicious, unescaped, or AI-generated input (Remote Code Execution). Additionally, the subprocess call lacked a timeout, making it vulnerable to infinite compilation loops leading to Denial of Service (DoS) and zombie processes.Impact: A malicious or improperly sanitized input to the cover letter generator could result in arbitrary code execution on the host machine or resource exhaustion causing an outage.
Fix:
-no-shell-escapetopdflatexarguments.--pdf-engine-opt=-no-shell-escapetopandocfallback arguments.timeout=30inprocess.communicate()with proper process termination (process.kill()) and stream cleanup to prevent zombie processes on timeout.Verification: Ran full test suite via
pytest tests/which completed successfully. Thetests/test_cover_letter_generator.pytests executed successfully, ensuring no regressions.PR created automatically by Jules for task 9354257226934992843 started by @anchapin
Summary by Sourcery
Harden PDF compilation in the cover letter generator to mitigate command injection and denial-of-service risks.
Bug Fixes: