๐ก๏ธ Sentinel: [HIGH] Fix DSN secret redaction boundary and encoding vulnerabilities - #665
๐ก๏ธ Sentinel: [HIGH] Fix DSN secret redaction boundary and encoding vulnerabilities#665seonghobae wants to merge 1 commit 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. |
๐ WalkthroughWalkthroughDSN ๋น๋ฐ๋ฒํธ ์ํฌ๋ฆฟ ํ๋ณด๊ฐ ๋์ฝ๋ฉ๋ ๊ฐ๊ณผ ์ฌ๋ฌ URL ์ธ์ฝ๋ฉ ๋ณํ์ ํฌํจํ๋๋ก ๋ณ๊ฒฝ๋์์ต๋๋ค. ์ํฌ๋ฆฟ ์ ๊ท์ ๊ฒฝ๊ณ๋ ์ ๋ ๋ฌธ์์ ์์ซ์ ์ฌ๋ถ์ ๋ฐ๋ผ ์กฐ๊ฑด๋ถ๋ก ์ ์ฉ๋๋ฉฐ, ๊ด๋ จ ์ทจ์ฝ ์ฌ๋ก์ ํ ์คํธ ์ง์นจ์ด ๋ฌธ์ํ๋์์ต๋๋ค. ChangesDSN ์ํฌ๋ฆฟ ๋ฆฌ๋คํฌ์
Estimated code review effort: 2 (Simple) | ~10 minutes ๐ฅ 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.
Actionable comments posted: 2
๐ค 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/dsn_redaction.py`:
- Around line 93-95: Update the boundary checks used to build pattern in the DSN
redaction logic so their character classification matches the ASCII [A-Za-z0-9]
lookarounds. Replace the Unicode-aware secret[0].isalnum() and
secret[-1].isalnum() checks with an ASCII-consistent approach, preserving the
existing behavior for empty secrets and the compiled pattern structure.
- Around line 62-65: Update the DSN password-candidate handling around the
normal parsing and malformed-DSN fallback so both paths use a shared helper to
add the raw value plus unquote_plus, quote, and quote_plus variants. Ensure the
fallback currently handling raw_password and unquote also generates the encoding
variants, including cases such as p%20ass versus p+ass, while preserving
existing redaction behavior.
๐ช Autofix (Beta)
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: 129aa56d-1c4f-45d9-a4d6-961428e2ab46
๐ Files selected for processing (2)
.jules/sentinel.mdbackend/app/dsn_redaction.py
| decoded = unquote_plus(password) | ||
| candidates.add(decoded) | ||
| candidates.add(quote(decoded, safe="")) | ||
| candidates.add(quote_plus(decoded, safe="")) |
There was a problem hiding this comment.
๐ Security & Privacy | ๐ Major | โก Quick win
malformed DSN fallback์๋ ๋์ผํ ํ๋ณด ๋ณํ์ ์ ์ฉํ์ธ์.
urlsplit์ด ValueError๋ฅผ ๋ฐํํ๋ฉด password๊ฐ ์ค์ ๋์ง ์์ ์ด ๋ธ๋ก์ด ์คํ๋์ง ์๊ณ , fallback์ raw_password์ unquote๋ง ์ถ๊ฐํฉ๋๋ค(Line 67โ73). ์๋ฅผ ๋ค์ด p%20ass๊ฐ ๋๋ผ์ด๋ฒ ์ค๋ฅ์์ p+ass๋ก ์ถ๋ ฅ๋๋ฉด ํด๋น ํ๋ณด๊ฐ ์์ด secret์ด ๋จ์ ์ ์์ต๋๋ค. ์ ์ ํ์ฑ๊ณผ fallback์ด ๊ณตํต helper๋ฅผ ์ฌ์ฉํด unquote_plus, quote, quote_plus ๋ณํ์ ๋ชจ๋ ์์ฑํ๋๋ก ์์ ํด์ผ ํฉ๋๋ค.
์ ์ ์์
+def _add_password_candidates(candidates: set[str], raw_password: str) -> None:
+ decoded = unquote_plus(raw_password)
+ candidates.update(
+ {
+ raw_password,
+ unquote(raw_password),
+ decoded,
+ quote(decoded, safe=""),
+ quote_plus(decoded, safe=""),
+ }
+ )
+
if password:
- candidates.add(password)
- decoded = unquote_plus(password)
- candidates.add(decoded)
- candidates.add(quote(decoded, safe=""))
- candidates.add(quote_plus(decoded, safe=""))
+ _add_password_candidates(candidates, password)
...
- candidates.add(raw_password)
- candidates.add(unquote(raw_password))
+ _add_password_candidates(candidates, raw_password)๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| decoded = unquote_plus(password) | |
| candidates.add(decoded) | |
| candidates.add(quote(decoded, safe="")) | |
| candidates.add(quote_plus(decoded, safe="")) | |
| def _add_password_candidates(candidates: set[str], raw_password: str) -> None: | |
| decoded = unquote_plus(raw_password) | |
| candidates.update( | |
| { | |
| raw_password, | |
| unquote(raw_password), | |
| decoded, | |
| quote(decoded, safe=""), | |
| quote_plus(decoded, safe=""), | |
| } | |
| ) | |
| ... | |
| if password: | |
| _add_password_candidates(candidates, password) | |
| ... | |
| _add_password_candidates(candidates, raw_password) |
๐ค 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/dsn_redaction.py` around lines 62 - 65, Update the DSN
password-candidate handling around the normal parsing and malformed-DSN fallback
so both paths use a shared helper to add the raw value plus unquote_plus, quote,
and quote_plus variants. Ensure the fallback currently handling raw_password and
unquote also generates the encoding variants, including cases such as p%20ass
versus p+ass, while preserving existing redaction behavior.
| left_bound = r"(?<![A-Za-z0-9])" if secret and secret[0].isalnum() else "" | ||
| right_bound = r"(?![A-Za-z0-9])" if secret and secret[-1].isalnum() else "" | ||
| pattern = re.compile(rf"{left_bound}{re.escape(secret)}{right_bound}") |
There was a problem hiding this comment.
๐ฏ Functional Correctness | ๐ก Minor | โก Quick win
๊ฒฝ๊ณ ํ์ ์ ๋ฌธ์ ์งํฉ์ ์ ๊ท์๊ณผ ์ผ์น์ํค์ธ์.
str.isalnum()์ Unicode ์์ซ์๊น์ง True๋ฅผ ๋ฐํํ์ง๋ง ์ค์ lookaround๋ ASCII [A-Za-z0-9]๋ง ๊ฒ์ฌํฉ๋๋ค. ๋ฐ๋ผ์ Unicode ๋ฌธ์๊ฐ ์ธ์ ํ secret์์๋ ์๋ํ ๊ฒฝ๊ณ๊ฐ ์ ์ฉ๋์ง ์์ ์ ์์ต๋๋ค. ASCII ์ ์ฉ ๋์์ด๋ผ๋ฉด edge ํ์ ๋ ASCII ๊ธฐ์ค์ผ๋ก ์ ํํ๊ณ , Unicode๋ฅผ ์ง์ํ๋ค๋ฉด ํ์ ๊ณผ lookaround์ ๋์ผํ ๋ฌธ์ ์งํฉ์ ์ฌ์ฉํ์ธ์.
๐งฐ Tools
๐ช ast-grep (0.44.1)
[warning] 94-94: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(rf"{left_bound}{re.escape(secret)}{right_bound}")
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
๐ค 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/dsn_redaction.py` around lines 93 - 95, Update the boundary
checks used to build pattern in the DSN redaction logic so their character
classification matches the ASCII [A-Za-z0-9] lookarounds. Replace the
Unicode-aware secret[0].isalnum() and secret[-1].isalnum() checks with an
ASCII-consistent approach, preserving the existing behavior for empty secrets
and the compiled pattern structure.
|
Closed as superseded security/agent finding; share snapshot redaction already on main, reversing-spec redaction tracked in #681. Duplicate/overlapping Sentinel noise. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
๐จ Severity: HIGH
๐ก Vulnerability: The DSN redaction logic leaked secrets in error messages if the password contained percent-encoded characters (like
%20or+) or if it started/ended with non-alphanumeric characters (like=or&). This occurred becauseurlsplitdoes not fully decode the password, and the redaction regex incorrectly applied strict alphanumeric word boundaries to all secrets, skipping valid matches.๐ฏ Impact: Sensitive database credentials could be leaked in raw unredacted error messages or server logs, potentially allowing unauthorized access to the databases if logs are exposed.
๐ง Fix: Updated
_password_candidates_from_dsnto explicitly decode URL-encoded passwords usingunquote_plusand dynamically add all encoding variations (unquote_plus,quote,quote_plus) to the redaction candidates set. Modified_redact_secret_occurrencesto dynamically apply word boundaries only when the corresponding edge character of the secret is alphanumeric, preventing regex match failures when secrets start or end with special characters.โ Verification: Tested against edge cases via manual python execution and successfully ran all existing tests via
cd backend && uv run pytest. Logged critical security learning in.jules/sentinel.md.PR created automatically by Jules for task 2582297972876054861 started by @seonghobae
Summary by CodeRabbit
๋ฒ๊ทธ ์์
๋ฌธ์