🛡️ Sentinel: [MEDIUM] Fix control character injection in schemas - #701
🛡️ Sentinel: [MEDIUM] Fix control character injection in schemas#701seonghobae wants to merge 1 commit into
Conversation
This patch mitigates a potential log/terminal injection vulnerability by strictly rejecting ASCII control characters from user-provided identifier and name strings in FastAPI request schemas using a regex pattern.
|
👋 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. |
📝 WalkthroughWalkthrough
Changes입력 스키마 검증
Estimated code review effort: 1 (간단) | ~5분 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/schemas.py (1)
193-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win새 정규식 동작에 대한 집중 테스트를 추가하세요.
이 변경은 네 입력 필드의 거부 동작을 변경합니다. 제공된 PR 목표에는
backend/app/schemas.py만 수정된 것으로 되어 있어, 새 검증을 직접 확인하는 테스트 변경이 보이지 않습니다.각 필드에
\x00,\x1F,\x7F, 탭, 캐리지 리턴, 줄바꿈이 포함된 값이ValidationError를 발생시키는지 테스트하세요. 일반 Unicode 값과 최소·최대 길이 경계도 유지되는지 확인하세요.TableAnnotationUpsertIn테스트에는 유효한body를 함께 제공하세요.As per coding guidelines:
**/*.{py,ts,tsx}: 동작 변경 시 집중 테스트를 추가하거나 갱신해야 합니다.Also applies to: 217-218, 305-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/schemas.py` at line 193, 새 정규식 검증을 확인하는 집중 테스트를 추가하세요. 해당 네 입력 필드에 NUL(\x00), \x1F, DEL(\x7F), 탭, 캐리지 리턴, 줄바꿈이 포함되면 ValidationError가 발생하는지 검증하고, 일반 Unicode 값과 최소·최대 길이 경계의 유효성은 유지되는지 확인하세요. TableAnnotationUpsertIn 테스트에서는 유효한 body를 함께 제공하세요.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/app/schemas.py`:
- Line 193: 새 정규식 검증을 확인하는 집중 테스트를 추가하세요. 해당 네 입력 필드에 NUL(\x00), \x1F,
DEL(\x7F), 탭, 캐리지 리턴, 줄바꿈이 포함되면 ValidationError가 발생하는지 검증하고, 일반 Unicode 값과 최소·최대
길이 경계의 유효성은 유지되는지 확인하세요. TableAnnotationUpsertIn 테스트에서는 유효한 body를 함께 제공하세요.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cecc461-9661-4fb4-86de-93e482ff97f7
📒 Files selected for processing (1)
backend/app/schemas.py
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head7ae2bacab3bc6d0df6bbf9cebbf8188b07db41e0. -
Head SHA:
7ae2bacab3bc6d0df6bbf9cebbf8188b07db41e0 -
Workflow run: 30732788459
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend: schemas.py"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend: schemas.py"]
R1 --> V1["backend tests"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend: schemas.py"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend: schemas.py"]
R1 --> V1["backend tests"]
|
|
Closing as superseded by #699, which applies the same schema hardening and adds exhaustive valid/invalid input regression coverage plus the multiline annotation-body guard. Consolidating on the tested PR avoids duplicate merge conflicts. |
Understood. Acknowledging that this work is now superseded by #699 and stopping work on this task. |
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: User-provided string fields in Pydantic request models (like project names, schema names, and API key names) lacked strict validation against ASCII control characters, only relying on string length constraints.
🎯 Impact: If left unvalidated, injecting control characters (like null bytes, tabs, or carriage returns) into these fields could lead to log injection (CRLF injection), terminal spoofing, or unexpected downstream parsing behavior when interacting with databases or system logs.
🔧 Fix: Implemented explicit regex validation
pattern=r"^[^\x00-\x1F\x7F]+$"across multiple string fields inbackend/app/schemas.py(DiagramViewCreateIn.name,TableAnnotationUpsertIn.schema_name,TableAnnotationUpsertIn.relation_name, andApiKeyCreateIn.key_name) to strictly reject any ASCII control characters.✅ Verification:
cd backend && uv run pytestensuring no regression in test coverage.cd backend && uv run ruff check .ensuring no linting issues were introduced.schemas.pywas modified and changes are well under the 50 lines constraint.PR created automatically by Jules for task 10293526622766420780 started by @seonghobae
Summary by CodeRabbit