🛡️ Sentinel: [CRITICAL] Fix PDF compilation vulnerabilities#230
Open
🛡️ Sentinel: [CRITICAL] Fix PDF compilation vulnerabilities#230
Conversation
Adds `-no-shell-escape` flags and explicit timeouts to `pdflatex` and `pandoc` commands in `cli/pdf/converter.py` and `cli/generators/cover_letter_generator.py` to prevent Remote Code Execution (RCE) and Denial of Service (DoS) attacks. 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 GuideAdds shell-escape hardening and execution timeouts to all LaTeX-based PDF compilation paths (pdflatex and pandoc) to mitigate RCE and DoS risks, while preserving existing success/fallback behavior. Sequence diagram for shared PDF converter compilation helpers with timeoutsequenceDiagram
participant PdfConverter
participant pdflatex_process
participant pandoc_process
PdfConverter->>pdflatex_process: Popen([pdflatex -interaction=nonstopmode -no-shell-escape tex_path])
pdflatex_process-->>PdfConverter: communicate(timeout=30) or TimeoutExpired
alt pdflatex completes in time and succeeds
PdfConverter->>PdfConverter: return True
else pdflatex times out
PdfConverter->>pdflatex_process: kill()
pdflatex_process-->>PdfConverter: communicate()
PdfConverter->>PdfConverter: handle RuntimeError
PdfConverter->>PdfConverter: check output_path.exists()
alt PDF exists
PdfConverter->>PdfConverter: return True
else
PdfConverter->>PdfConverter: continue to pandoc fallback
end
else pdflatex fails or other error
PdfConverter->>PdfConverter: check output_path.exists()
alt PDF exists
PdfConverter->>PdfConverter: return True
else
PdfConverter->>PdfConverter: continue to pandoc fallback
end
end
PdfConverter->>pandoc_process: Popen([pandoc tex_path -o output_path --pdf-engine=xelatex --pdf-engine-opt=-no-shell-escape])
pandoc_process-->>PdfConverter: communicate(timeout=30) or TimeoutExpired
alt pandoc completes in time and succeeds
PdfConverter->>PdfConverter: return True
else pandoc times out
PdfConverter->>pandoc_process: kill()
pandoc_process-->>PdfConverter: communicate()
PdfConverter->>PdfConverter: handle RuntimeError
PdfConverter->>PdfConverter: return False
else pandoc fails or other error
PdfConverter->>PdfConverter: return False
end
Flow diagram for secure LaTeX-based PDF compilation with timeoutflowchart TD
A_start["Start compilation request"] --> B_engine["Run pdflatex with -no-shell-escape"]
B_engine --> C_timeout_check["communicate with timeout=30s"]
C_timeout_check -->|"success, returncode==0 or output_path exists"| D_pdflatex_success["pdflatex success -> PDF ready"]
D_pdflatex_success --> Z_end["Return success"]
C_timeout_check -->|"TimeoutExpired"| E_kill_pdflatex["kill pdflatex process"]
E_kill_pdflatex --> F_collect_pdflatex["collect stdout and stderr"]
F_collect_pdflatex --> G_pdflatex_runtime_error["raise and handle RuntimeError"]
C_timeout_check -->|"failure or other error"| G_pdflatex_runtime_error
G_pdflatex_runtime_error --> H_check_pdf_after_pdflatex["check if output_path exists"]
H_check_pdf_after_pdflatex -->|"exists"| I_pdf_exists_after_pdflatex["treat as success"]
I_pdf_exists_after_pdflatex --> Z_end
H_check_pdf_after_pdflatex -->|"missing"| J_pandoc_fallback["Run pandoc with --pdf-engine=xelatex and --pdf-engine-opt=-no-shell-escape"]
J_pandoc_fallback --> K_pandoc_timeout_check["communicate with timeout=30s"]
K_pandoc_timeout_check -->|"success, returncode==0 or output_path exists"| L_pandoc_success["pandoc success -> PDF ready"]
L_pandoc_success --> Z_end
K_pandoc_timeout_check -->|"TimeoutExpired"| M_kill_pandoc["kill pandoc process"]
M_kill_pandoc --> N_collect_pandoc["collect stdout and stderr"]
N_collect_pandoc --> O_pandoc_runtime_error["raise and handle RuntimeError"]
K_pandoc_timeout_check -->|"failure or other error"| O_pandoc_runtime_error
O_pandoc_runtime_error --> P_final_failure["no PDF produced -> return failure"]
P_final_failure --> Z_end["Return result"]
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 found 1 issue, and left some high level feedback:
- The timeout and
subprocess.Popenhandling logic is duplicated across multiple call sites; consider extracting a small helper (e.g.,run_latex_command(...)) to centralize the timeout,process.kill(), and error mapping behavior. - When a timeout or other failure occurs, the current code swallows
stdout/stderr; consider logging or surfacing these streams in some way to aid debugging of compilation issues while still treating the operation as failed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timeout and `subprocess.Popen` handling logic is duplicated across multiple call sites; consider extracting a small helper (e.g., `run_latex_command(...)`) to centralize the timeout, `process.kill()`, and error mapping behavior.
- When a timeout or other failure occurs, the current code swallows `stdout`/`stderr`; consider logging or surfacing these streams in some way to aid debugging of compilation issues while still treating the operation as failed.
## Individual Comments
### Comment 1
<location path="cli/generators/cover_letter_generator.py" line_range="787" />
<code_context>
if process.returncode == 0 or output_path.exists():
pdf_created = True
- except (subprocess.CalledProcessError, FileNotFoundError):
+ except (subprocess.CalledProcessError, FileNotFoundError, RuntimeError):
# Check if PDF was created anyway
if output_path.exists():
</code_context>
<issue_to_address>
**suggestion:** Catching CalledProcessError is unnecessary when using Popen without `check_*` helpers.
Because this uses `subprocess.Popen` and checks `process.returncode` manually, `CalledProcessError` is never raised (it only comes from `check_call`/`check_output`). You can remove `CalledProcessError` from the `except` tuple to simplify and clarify the control flow.
```suggestion
except (FileNotFoundError, RuntimeError):
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if process.returncode == 0 or output_path.exists(): | ||
| pdf_created = True | ||
| except (subprocess.CalledProcessError, FileNotFoundError): | ||
| except (subprocess.CalledProcessError, FileNotFoundError, RuntimeError): |
There was a problem hiding this comment.
suggestion: Catching CalledProcessError is unnecessary when using Popen without check_* helpers.
Because this uses subprocess.Popen and checks process.returncode manually, CalledProcessError is never raised (it only comes from check_call/check_output). You can remove CalledProcessError from the except tuple to simplify and clarify the control flow.
Suggested change
| except (subprocess.CalledProcessError, FileNotFoundError, RuntimeError): | |
| except (FileNotFoundError, RuntimeError): |
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: LaTeX compilation mechanisms (
pdflatexandpandoc) were executed viasubprocess.Popenwithout enforcing the-no-shell-escapeflag or a timeout.🎯 Impact: This could allow a malicious user or AI-generated output to execute arbitrary shell commands on the host machine via LaTeX's
\write18feature (RCE). It also allowed for potential Denial of Service (DoS) via infinitely looping LaTeX documents.🔧 Fix: Added
-no-shell-escapeand--pdf-engine-opt=-no-shell-escapeflags to allpdflatexandpandocinvocations. Implemented a 30-second execution timeout usingprocess.communicate(timeout=30)with proper error handling and process cleanup to kill hanging operations.✅ Verification: Ran
pytestsuite ensuring all tests, including new security tests, pass. Verified changes logically align with fixes applied previously inresume_pdf_lib.PR created automatically by Jules for task 12122897405955206167 started by @anchapin
Summary by Sourcery
Harden PDF compilation for LaTeX-based generators to mitigate command execution and hang risks.
Bug Fixes:
Enhancements: