🐛(dnscheck) relax DKIM whitespace check for DNS configs - #778
Conversation
Whitespace is ignored in DKIM keys but that wasn't implemented in our validator, which showed "Invalid" in the admin UI for some keys
📝 WalkthroughWalkthroughDKIM parsing accepts records without ChangesDKIM behavior
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/backend/core/services/dns/check.py`:
- Around line 41-47: Reject duplicate DKIM tag names in the parser around the
tags construction in src/backend/core/services/dns/check.py lines 41-47 by
returning None when a parsed key already exists before assignment. Add coverage
for duplicate p and v tags in src/backend/core/tests/dns/test_check.py lines
836-849, asserting both records are rejected.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: a501f0fe-150f-43a6-b1ac-b6b236c9fbf1
📒 Files selected for processing (2)
src/backend/core/services/dns/check.pysrc/backend/core/tests/dns/test_check.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/services/dns/check.py (1)
42-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed DKIM tag segments.
parse_dkim_tags()accepts strings with missing=separators, empty tag names, and duplicate tags that later satisfy the parser;v; v=DKIM1; k=rsa; p=abcandk=rsa; p=abc; trailingare treated as correct DKIM records. ReturnNonefor any segment without=and for empty tag names, then call the semantic check on malformed strings as negative cases because any valid found DKIM result can bypass_check_dkim_semantic().🤖 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 `@src/backend/core/services/dns/check.py` around lines 42 - 53, Update parse_dkim_tags to return None immediately for any non-empty segment lacking “=” and for segments whose stripped key is empty, while preserving duplicate-tag rejection. Add negative semantic-check coverage for malformed records such as “v; v=DKIM1; k=rsa; p=abc” and “k=rsa; p=abc; trailing”, ensuring malformed results cannot bypass _check_dkim_semantic().
🤖 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.
Outside diff comments:
In `@src/backend/core/services/dns/check.py`:
- Around line 42-53: Update parse_dkim_tags to return None immediately for any
non-empty segment lacking “=” and for segments whose stripped key is empty,
while preserving duplicate-tag rejection. Add negative semantic-check coverage
for malformed records such as “v; v=DKIM1; k=rsa; p=abc” and “k=rsa; p=abc;
trailing”, ensuring malformed results cannot bypass _check_dkim_semantic().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd7c5248-63e1-4f95-a82b-caa62caa40c5
📒 Files selected for processing (2)
src/backend/core/services/dns/check.pysrc/backend/core/tests/dns/test_check.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/backend/core/services/dns/check.py`:
- Around line 43-51: Update parse_dkim_tags() in
src/backend/core/services/dns/check.py at lines 43-51 to remove only one
optional trailing empty field, reject leading or interior empty
tag-specifications, and validate each tag name against ALPHA followed by zero or
more ALNUM or underscore characters. Add focused tests in
src/backend/core/tests/dns/test_check.py at lines 850-862 covering a leading
semicolon, an interior empty tag-specification, and invalid tag names.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 1b21b5a6-ab05-4887-8574-0872fc7440aa
📒 Files selected for processing (2)
src/backend/core/services/dns/check.pysrc/backend/core/tests/dns/test_check.py
| # A trailing semicolon is valid, hence dropping empty segments up front. | ||
| parts = [p.strip() for p in value.split(";") if p.strip()] | ||
| if not parts: | ||
| return None | ||
| # v= must be first | ||
| first = parts[0] | ||
| if not first.startswith("v=") or first.split("=", 1)[1].strip() != "DKIM1": | ||
| return None | ||
| tags = {} | ||
| for part in parts: | ||
| if "=" not in part: | ||
| continue | ||
| return None | ||
| key, val = part.split("=", 1) | ||
| tags[key.strip()] = val.strip() | ||
| key = key.strip() | ||
| if not key or key in tags: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the complete DKIM tag-list grammar.
parse_dkim_tags() discards empty fields and accepts any non-empty tag name. This permits malformed DKIM records to pass semantic comparison. RFC 6376 permits only one optional trailing semicolon and defines tag names as ALPHA *ALNUMPUNC. (rfc-editor.org)
src/backend/core/services/dns/check.py#L43-L51: Remove at most one trailing empty field. Reject leading or interior empty fields. Require tag names to match[A-Za-z][A-Za-z0-9_]*.src/backend/core/tests/dns/test_check.py#L850-L862: Add focused cases for a leading semicolon, an interior empty tag-spec, and invalid tag names.
Proposed fix
- parts = [p.strip() for p in value.split(";") if p.strip()]
+ parts = value.split(";")
+ if parts[-1].strip() == "":
+ parts.pop()
+ if not parts:
+ return None
+
tags = {}
for part in parts:
+ part = part.strip()
+ if not part or "=" not in part:
+ return None
- if "=" not in part:
- return None
key, val = part.split("=", 1)
key = key.strip()
- if not key or key in tags:
+ if not re.fullmatch(r"[A-Za-z][A-Za-z0-9_]*", key) or key in tags:
return NoneAs per coding guidelines, “Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases.”
📝 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.
| # A trailing semicolon is valid, hence dropping empty segments up front. | |
| parts = [p.strip() for p in value.split(";") if p.strip()] | |
| if not parts: | |
| return None | |
| # v= must be first | |
| first = parts[0] | |
| if not first.startswith("v=") or first.split("=", 1)[1].strip() != "DKIM1": | |
| return None | |
| tags = {} | |
| for part in parts: | |
| if "=" not in part: | |
| continue | |
| return None | |
| key, val = part.split("=", 1) | |
| tags[key.strip()] = val.strip() | |
| key = key.strip() | |
| if not key or key in tags: | |
| # A trailing semicolon is valid, hence dropping empty segments up front. | |
| parts = value.split(";") | |
| if parts[-1].strip() == "": | |
| parts.pop() | |
| if not parts: | |
| return None | |
| tags = {} | |
| for part in parts: | |
| part = part.strip() | |
| if not part or "=" not in part: | |
| return None | |
| key, val = part.split("=", 1) | |
| key = key.strip() | |
| if not re.fullmatch(r"[A-Za-z][A-Za-z0-9_]*", key) or key in tags: | |
| return None | |
| tags[key] = val.strip() |
📍 Affects 2 files
src/backend/core/services/dns/check.py#L43-L51(this comment)src/backend/core/tests/dns/test_check.py#L850-L862
🤖 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 `@src/backend/core/services/dns/check.py` around lines 43 - 51, Update
parse_dkim_tags() in src/backend/core/services/dns/check.py at lines 43-51 to
remove only one optional trailing empty field, reject leading or interior empty
tag-specifications, and validate each tag name against ALPHA followed by zero or
more ALNUM or underscore characters. Add focused tests in
src/backend/core/tests/dns/test_check.py at lines 850-862 covering a leading
semicolon, an interior empty tag-specification, and invalid tag names.
Sources: Coding guidelines, MCP tools
1 participant
Whitespace is ignored in DKIM keys but that wasn't implemented in our validator, which showed "Invalid" in the admin UI for some keys
Summary by CodeRabbit