π‘οΈ Sentinel: [MEDIUM] Fix control character injection in schemas#619
π‘οΈ Sentinel: [MEDIUM] Fix control character injection in schemas#619seonghobae wants to merge 2 commits into
Conversation
|
π 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. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds stricter validation to several Pydantic input schemas to block ASCII control characters in user-provided string fields (mitigating log/terminal injection vectors), and records the hardening guidance in the Sentinel notes.
Changes:
- Added
pattern=r"^[^\x00-\x1F\x7F]+$"to selected string fields in Pydantic request schemas. - Expanded
.jules/sentinel.mdwith an extended entry describing the control-character hardening approach.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
backend/app/schemas.py |
Adds regex-based validation to block ASCII control characters in multiple request schema string fields. |
.jules/sentinel.md |
Documents the vulnerability pattern and the prevention guidance for consistent schema hardening. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: str = Field( | ||
| min_length=1, | ||
| max_length=200, | ||
| pattern=r"^[^\x00-\x1F\x7F]+$", | ||
| ) |
| schema_name: str = Field( | ||
| min_length=1, | ||
| max_length=255, | ||
| pattern=r"^[^\x00-\x1F\x7F]+$", | ||
| ) | ||
| relation_name: str = Field( | ||
| min_length=1, | ||
| max_length=255, | ||
| pattern=r"^[^\x00-\x1F\x7F]+$", | ||
| ) |
| key_name: str = Field( | ||
| min_length=1, | ||
| max_length=128, | ||
| pattern=r"^[^\x00-\x1F\x7F]+$", | ||
| ) |
| name: str = Field( | ||
| min_length=1, | ||
| max_length=200, | ||
| pattern=r"^[^\x00-\x1F\x7F]+$", | ||
| ) |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
β¦xtend to C1 controls
Addressed all feedback in commit
19 existing tests continue to pass. |
| # Rejects ASCII C0 controls (\x00β\x1F), DEL (\x7F), and C1 controls | ||
| # (\x80β\x9F) to prevent log/terminal injection in user-supplied strings. | ||
| NO_ASCII_CONTROL_CHARS_PATTERN = r"^[^\x00-\x1F\x7F-\x9F]+$" |
| name: str = Field( | ||
| min_length=1, | ||
| max_length=200, | ||
| pattern=NO_ASCII_CONTROL_CHARS_PATTERN, | ||
| ) |
| schema_name: str = Field( | ||
| min_length=1, | ||
| max_length=255, | ||
| pattern=NO_ASCII_CONTROL_CHARS_PATTERN, | ||
| ) | ||
| relation_name: str = Field( | ||
| min_length=1, | ||
| max_length=255, | ||
| pattern=NO_ASCII_CONTROL_CHARS_PATTERN, | ||
| ) |
| key_name: str = Field( | ||
| min_length=1, | ||
| max_length=128, | ||
| pattern=NO_ASCII_CONTROL_CHARS_PATTERN, | ||
| ) |
| **Prevention:** Use explicit regex validation `pattern=r'^[^\x00-\x1F\x7F]+$'` on Pydantic string fields to strictly reject control characters. | ||
| ## 2025-02-18 - Hardening Pydantic String Fields Against Control Characters (Extended) | ||
| **Vulnerability:** Several Pydantic string fields such as schema names, relation names, and diagram view names were missing strict validation against ASCII control characters, only relying on length constraints. | ||
| **Learning:** Incomplete validation across schemas can lead to log injection, terminal manipulation, or unintended backend behaviors if these fields are logged or processed. | ||
| **Prevention:** Consistently apply the explicit regex validation `pattern=r"^[^\x00-\x1F\x7F]+$"` on all relevant Pydantic string fields, except for fields like bodies or raw scripts that legitimately require newlines and tabs. |
π¨ Severity: MEDIUM
π‘ Vulnerability:
DiagramViewCreateIn,TableAnnotationUpsertIn,ApiKeyCreateInλ± μ¬λ¬ Pydantic μ€ν€λ§μ λ¬Έμμ΄ νλκ° μ μ΄ λ¬Έμ(Control characters)μ λν κ²μ¦ μμ΄ κΈΈμ΄ μ νλ§ μ μ©λμ΄ μμμ΅λλ€.π― Impact: μ μμ μΈ μ¬μ©μκ° λ λ°μ΄νΈ(\x00)λ κ°ν λ¬Έμ, ν λ±μ μ μ΄ λ¬Έμλ₯Ό μ½μ νμ¬ λ‘κ·Έ μΈμ μ , ν°λ―Έλ μ΄μ€μΌμ΄ν μ‘°μ λλ μμ€ν μ μκΈ°μΉ μμ λμμ μ λ°ν μ μμ΅λλ€.
π§ Fix: μ μ΄ λ¬Έμκ° νμ μλ λ¬Έμμ΄ νλλ€μ λν΄ λͺ μμ μΌλ‘ μ κ·ννμ ν¨ν΄(
pattern=r"^[^\x00-\x1F\x7F]+$")μ μΆκ°νμ¬ ASCII μ μ΄ λ¬Έμμ μ λ ₯μ μ격ν μ°¨λ¨νμ΅λλ€.β Verification:
backend/app/schemas.pyμ ν¨ν΄ μμ νuv run pytestλ₯Ό μ€ννμ¬ κΈ°μ‘΄ ν μ€νΈ ν΅κ³Όλ₯Ό νμΈνμμΌλ©°, μ κ·μμ μν μ μ΄ λ¬Έμ νν°λ§ λμμ μ€ν¬λ¦½νΈλ‘ κ²μ¦νμ΅λλ€.PR created automatically by Jules for task 16772809612572038082 started by @seonghobae