security(dbml): enforce identifier-to-DDL trust boundary - #833
security(dbml): enforce identifier-to-DDL trust boundary#833seonghobae wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughDBML 식별자 검증과 PostgreSQL 인용 처리를 공통화했습니다. 잘못된 식별자와 경로는 파싱 단계에서 거부합니다. DBML API는 파싱 오류를 HTTP 422로 반환합니다. 인용 내부의 세미콜론과 주석 표식은 보존합니다. ChangesDBML 식별자 경계
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DBML_API
participant parse_dbml
participant quote_identifier
participant split_statements
DBML_API->>parse_dbml: DBML 입력 전달
parse_dbml->>quote_identifier: 검증된 식별자 인용 요청
quote_identifier-->>parse_dbml: PostgreSQL 식별자 토큰 반환
parse_dbml-->>DBML_API: DDL 반환
DBML_API->>split_statements: SQL 문장 분할 요청
split_statements-->>DBML_API: 인용 영역을 보존한 문장 목록 반환
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Exact-current-head stacked review request for @opencode-agent @cwl-noema-review Please review this unchanged exact head and provide a qualifying non-author formal verdict. Current-head CI |
|
@coderabbitai review Please perform a full review of exact current head |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/spec/dbml_import.py (1)
230-235: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win4,096자 초과 DBML 줄에서 fail-closed 처리를 적용하십시오.
Line 233은 길이 초과 줄을
continue로 건너뜁니다. 유효한 테이블 뒤에 길이 초과의 잘못된Ref또는Table줄을 넣으면parse_dbml은 부분 snapshot을 반환합니다.convert_dbml은 이 결과로 응답과 선택적 DDL을 생성합니다.길이 초과 시
DbmlParseError를 발생시키십시오.test_pathological_long_line_is_skipped_fast도 예외 및 HTTP 422 계약을 검증하도록 변경하십시오.🤖 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/spec/dbml_import.py` around lines 230 - 235, Update parse_dbml’s overlong-line guard to raise DbmlParseError instead of continuing, ensuring oversized DBML input fails closed rather than producing a partial snapshot. Update test_pathological_long_line_is_skipped_fast to assert the parser exception and the convert_dbml HTTP 422 response contract.backend/app/spec/index_design.py (1)
171-182: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win인덱스 이름을 PostgreSQL의 바이트 제한에 맞게 생성하십시오.
_index_name()은 63개 문자로 자르지만quote_identifier()는 UTF-8 바이트 수를 검사합니다. 유효한 Unicode 이름에서는 결과가 63바이트를 초과하여SqlIdentifierError가 발생합니다. 단순 절단은 서로 다른 입력을 같은 인덱스 이름으로 만들 수도 있습니다. UTF-8 경계를 보존하는 안정적 해시 접미사 방식으로 축약하고, 최대 길이 Unicode 입력 및 충돌 회피 테스트를 추가하십시오.🤖 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/spec/index_design.py` around lines 171 - 182, Update _index_name() to enforce PostgreSQL’s 63-byte UTF-8 identifier limit rather than a 63-character limit, preserving complete Unicode code points. When truncation is required, append a deterministic hash suffix derived from the full input so distinct long inputs remain distinguishable; add tests covering maximum-length Unicode names and collision avoidance.
🤖 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.
Inline comments:
In `@backend/app/spec/dbml_import.py`:
- Around line 46-51: Update the parsing logic using _REF_RE and _INLINE_REF_RE
so reference paths are validated completely: use full-string matching for
standalone Ref values, and require inline ref: matches to end at an allowed
settings delimiter or the string boundary. Add regression tests covering
trailing invalid tokens for both standalone and inline references, ensuring no
foreign key is created.
In `@docs/security/dbml-identifier-boundary.md`:
- Around line 62-69: Update the References section of
dbml-identifier-boundary.md to add relevant academic literature supporting the
DBML identifier-boundary security feature. Include complete citations,
accessible links, and a brief summary for each source, while retaining the
existing OWASP and PostgreSQL references.
---
Outside diff comments:
In `@backend/app/spec/dbml_import.py`:
- Around line 230-235: Update parse_dbml’s overlong-line guard to raise
DbmlParseError instead of continuing, ensuring oversized DBML input fails closed
rather than producing a partial snapshot. Update
test_pathological_long_line_is_skipped_fast to assert the parser exception and
the convert_dbml HTTP 422 response contract.
In `@backend/app/spec/index_design.py`:
- Around line 171-182: Update _index_name() to enforce PostgreSQL’s 63-byte
UTF-8 identifier limit rather than a 63-character limit, preserving complete
Unicode code points. When truncation is required, append a deterministic hash
suffix derived from the full input so distinct long inputs remain
distinguishable; add tests covering maximum-length Unicode names and collision
avoidance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5887249-a111-4a3e-b1c4-830e562ad6b8
📒 Files selected for processing (10)
CHANGELOG.mdbackend/app/api/dbml.pybackend/app/ddl/export.pybackend/app/ddl/identifiers.pybackend/app/pg_introspect/forward_ddl.pybackend/app/snowflake_introspect/introspect.pybackend/app/spec/dbml_import.pybackend/app/spec/index_design.pybackend/tests/test_dbml_import.pydocs/security/dbml-identifier-boundary.md
|
Addressed all four CodeRabbit findings at exact head
Test-first evidence: the new boundary tests first failed as expected (2 reference failures, then 3 oversized-line/index-name failures). After the fixes, the focused suites passed ( |
|
@coderabbitai review Please re-review exact current head |
|
Exact-head independent review request for @opencode-agent @cwl-noema-review Please review this unchanged exact head and provide a formal non-author verdict. Local evidence is |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested_head="8eeaa36bc45ddf0d0dc0ef0e0ae9f037c51bf879"
requested_base="5bf6cc56ca4e80d89fed852362a6ae4f8717be98"
echo "== Revision verification =="
git rev-parse HEAD
git cat-file -t "$requested_head"
git cat-file -t "$requested_base"
echo "merge-base:"
git merge-base "$requested_head" "$requested_base"
echo
echo "== Changed files: base...head =="
git diff --name-status "$requested_base...$requested_head"
echo
echo "== Changed files: predecessor...requested head =="
git diff --name-status "c3435f71ed4819769dedd1197c91afbfd6c1ca24...$requested_head"
echo
echo "== Outline: DBML parser =="
ast-grep outline backend/app/spec/dbml_import.py --items all --view compact
echo
echo "== Outline: DBML API route and identifier helpers =="
ast-grep outline backend/app/api/dbml.py backend/app/ddl/identifiers.py --items all --view compact
echo
echo "== Focused definitions and regression references =="
rg -n -C 4 -P '(^\s*(def|class)\s+.*(reference|identifier|line|index)|MAX_.*(LINE|IDENTIFIER)|DbmlParseError|SqlIdentifierError|sha256|4096|4097|Su.?Wassermann|Ray.?Ligatti)' \
backend/app/spec/dbml_import.py backend/app/api/dbml.py backend/app/ddl/identifiers.py \
backend/tests/test_dbml_import.py docs/security/dbml-identifier-boundary.md || true
echo
echo "== Exact remediation diff =="
git diff --find-renames --find-copies --unified=80 \
"c3435f71ed4819769dedd1197c91afbfd6c1ca24...$requested_head" -- \
backend/app/spec/dbml_import.py \
backend/app/api/dbml.py \
backend/tests/test_dbml_import.py \
docs/security/dbml-identifier-boundary.md \
CHANGELOG.mdLength of output: 2891 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="8eeaa36bc45ddf0d0dc0ef0e0ae9f037c51bf879"
previous="c3435f71ed4819769dedd1197c91afbfd6c1ca24"
echo "== AST outlines =="
ast-grep outline backend/app/spec/dbml_import.py --items all --view signatures
ast-grep outline backend/app/spec/index_design.py --items all --view signatures
ast-grep outline backend/app/ddl/identifiers.py --items all --view signatures
ast-grep outline backend/app/api/dbml.py --items all --view signatures
echo
echo "== Remediation delta =="
git diff --find-renames --find-copies --unified=100 "$previous...$head" -- \
backend/app/spec/dbml_import.py \
backend/app/spec/index_design.py \
backend/tests/test_dbml_import.py \
backend/tests/test_index_design.py \
docs/security/dbml-identifier-boundary.md
echo
echo "== DBML parser: reference, line-boundary, and index-name logic =="
rg -n -C 10 -P '(DbmlParseError|4096|len\(.*line|splitlines|reference|Ref:|_parse.*ref|constraint|index.*name|sha256|utf.?8|encode\()' \
backend/app/spec/dbml_import.py
echo
echo "== DBML parser source, selected range =="
nl -ba backend/app/spec/dbml_import.py | sed -n '1,460p'
echo
echo "== Index design and shared SQL identifier renderer =="
nl -ba backend/app/spec/index_design.py | sed -n '1,360p'
nl -ba backend/app/ddl/identifiers.py | sed -n '1,220p'
echo
echo "== Tests: full matching test definitions and test fixtures =="
rg -n -C 12 -P '^\s*(async\s+)?def test_.*(reference|line|index|identifier|quote|unicode|oversiz|malformed)' \
backend/tests/test_dbml_import.py backend/tests/test_index_design.py
echo
echo "== Documentation: citations and identifier/reference contract =="
nl -ba docs/security/dbml-identifier-boundary.md | sed -n '1,260p'
echo
echo "== SQL identifier quote/render call sites =="
rg -n -C 4 -P '\b(quote_identifier|validate_identifier|_q)\s*\(' backend/appLength of output: 59412 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="5bf6cc56ca4e80d89fed852362a6ae4f8717be98"
head="8eeaa36bc45ddf0d0dc0ef0e0ae9f037c51bf879"
echo "== Current DBML parser =="
sed -n '1,470p' backend/app/spec/dbml_import.py
echo
echo "== Current HTTP route and DDL renderer/splitter =="
sed -n '1,180p' backend/app/api/dbml.py
sed -n '1,360p' backend/app/ddl/export.py
sed -n '1,240p' backend/app/ddl/identifiers.py
sed -n '1,240p' backend/app/pg_introspect/forward_ddl.py
echo
echo "== Current regression tests =="
sed -n '1,380p' backend/tests/test_dbml_import.py
sed -n '1,180p' backend/tests/test_index_design.py
echo
echo "== All changes in stack boundary =="
git diff --check "$base...$head"
git diff --stat "$base...$head"
git diff --unified=35 "$base...$head" -- \
backend/app/api/dbml.py \
backend/app/ddl/export.py \
backend/app/ddl/identifiers.py \
backend/app/pg_introspect/forward_ddl.py \
backend/app/snowflake_introspect/introspect.py \
backend/app/spec/dbml_import.py \
backend/app/spec/index_design.py \
backend/tests/test_dbml_import.py \
backend/tests/test_index_design.py
echo
echo "== Identifier interpolation and quote renderer use sites =="
rg -n -C 5 -P '(CREATE\s+(TABLE|INDEX)|ALTER\s+TABLE|REFERENCES|CONSTRAINT|DROP\s+TABLE|quote_identifier\(|def _q\()' \
backend/app/ddl backend/app/spec backend/app/snowflake_introspect backend/app/pg_introspect
echo
echo "== Static probes for reference-pattern acceptance =="
python3 - <<'PY'
import re
q = r'"(?:""|[^"])+"'
path = rf'(?:{q}|\w+)(?:\.(?:{q}|\w+))*'
standalone = re.compile(
r"ref\s*(?:\w+\s*)?:?\s*"
rf"(?P<from>{path})\s*(?P<op>[<>-])\s*(?P<to>{path})",
re.IGNORECASE,
)
inline = re.compile(
rf"ref:\s*(?P<op>[<>-])\s*(?P<to>{path})(?=\s*(?:,|$))",
re.IGNORECASE,
)
for text in (
"Ref: posts.user_id > users.id trailing",
"Ref: posts.user_id > users.id",
"ref: > users.id trailing",
"ref: > users.id, not null",
"ref: > users.id]",
"ref: > users.id , note: 'x'",
):
print(repr(text), "standalone_fullmatch=", bool(standalone.fullmatch(text)),
"inline_search=", bool(inline.search(text)))
PYLength of output: 137010
The exact head and stacked base match the requested commits. I verified the four remediations by static inspection:
I could not independently execute the test suite in this sandbox. The reported test results remain unverified here. You are interacting with an AI system. |
|
@coderabbitai review Exact head |
|
Exact-head independent review request for @opencode-agent @cwl-noema-review Please provide a formal non-author verdict for this unchanged head. Local evidence is |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head="575b07fde5d88ba0ddba1ca3728fe8aa67c90f76"
base="5bf6cc56ca4e80d89fed852362a6ae4f8717be98"
previous="8eeaa36bc45ddf0d0dc0ef0e0ae9f037c51bf879"
echo "== Revision verification =="
printf 'checked-out HEAD: '; git rev-parse HEAD
printf 'requested head type: '; git cat-file -t "$head"
printf 'requested base type: '; git cat-file -t "$base"
printf 'merge-base: '; git merge-base "$head" "$base"
printf 'previous-to-head ancestry: '; git merge-base --is-ancestor "$previous" "$head" && echo yes || echo no
echo
echo "== Exact remediation delta =="
git diff --check "$previous...$head"
git diff --unified=100 "$previous...$head" -- \
backend/app/spec/dbml_import.py \
backend/tests/test_dbml_import.py \
docs/security/dbml-identifier-boundary.md
echo
echo "== Parser and API outlines =="
ast-grep outline backend/app/spec/dbml_import.py --items all --view signatures
ast-grep outline backend/app/api/dbml.py --items all --view signatures
ast-grep outline backend/app/ddl/identifiers.py --items all --view signatures
echo
echo "== Reference parsing, identifier validation, and DDL rendering sites =="
rg -n -C 12 -P '(_REF|reference|Ref:|DbmlParseError|quote_identifier|validate_identifier|_split_statements|4096|sha256)' \
backend/app/spec/dbml_import.py \
backend/app/api/dbml.py \
backend/app/ddl/identifiers.py \
backend/app/ddl/export.py \
backend/app/pg_introspect/forward_ddl.py \
backend/tests/test_dbml_import.py \
docs/security/dbml-identifier-boundary.md
echo
echo "== Selected parser source =="
nl -ba backend/app/spec/dbml_import.py | sed -n '1,460p'
echo
echo "== Test definitions for references and identifier boundary =="
rg -n -C 15 -P '^\s*(async\s+)?def test_.*(ref|reference|identifier|quote|unicode|line|oversiz|statement)' \
backend/tests/test_dbml_import.pyLength of output: 74992 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="575b07fde5d88ba0ddba1ca3728fe8aa67c90f76"
base="5bf6cc56ca4e80d89fed852362a6ae4f8717be98"
previous="8eeaa36bc45ddf0d0dc0ef0e0ae9f037c51bf879"
echo "== Changed files in the full stacked range =="
git diff --name-status "$base...$head"
git diff --check "$base...$head"
echo
echo "== Current reference parser and constraint adapter =="
sed -n '38,80p' backend/app/spec/dbml_import.py
sed -n '174,440p' backend/app/spec/dbml_import.py
echo
echo "== Quote-aware statement splitter =="
sed -n '174,245p' backend/app/pg_introspect/forward_ddl.py
echo
echo "== Shared renderer and DDL call paths =="
sed -n '1,120p' backend/app/ddl/identifiers.py
rg -n -C 3 -P '\bquote_identifier\s*\(|\b_q\s*\(' \
backend/app/ddl backend/app/spec backend/app/snowflake_introspect
echo
echo "== Reference and statement regression coverage =="
sed -n '45,115p' backend/tests/test_dbml_import.py
sed -n '145,310p' backend/tests/test_dbml_import.py
echo
echo "== Exact named-block remediation diff =="
git diff --unified=50 "$previous...$head" -- \
backend/app/spec/dbml_import.py backend/tests/test_dbml_import.py
echo
echo "== Static grammar probes through grep-visible test inputs =="
rg -n -P 'Ref\s+\w+\s*\{|Ref:.*trailing|malformed reference identifier|_REF_RE\.fullmatch' \
backend/app/spec/dbml_import.py backend/tests/test_dbml_import.pyLength of output: 48433
I found no new issues. The named one-line block reference is now accepted by the documented grammar:
The existing identifier boundary remains consistent:
I verified that the checked-out I could not execute the focused or full test suites in this sandbox. The reported
|
1 participant
Security outcome
Stack
This PR is based on #832. The branch was synchronized without history rewriting by merge commit
c3435f71ed4819769dedd1197c91afbfd6c1ca24, whose second parent is exact live base5bf6cc56ca4e80d89fed852362a6ae4f8717be98. It must not merge before #832. Review the DBML identifier-boundary commits relative to that live base.Verification
Combined exact-tree evidence after synchronizing #832:
220 passed591 passed, 1 skippedSuccess: no issues found in 69 source filesgit diff --check: cleanb04a35f402c9b65f8a3f269cec2620bb6fa85ddeThe earlier DBML-only owned production statement/branch coverage was
100.00%(414statements,64branches). Exact-head CI, Security Scan, Semgrep, zero unresolved valid findings, and qualifying independent approval remain mandatory before merge.No dependency, lockfile, database migration, frontend, or workflow changes.
Closes #747
Summary by CodeRabbit
새로운 기능
버그 수정
문서