Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Address regions are not reliably bounded, allowing cross-part rewrites or missed domain sanitization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Scopes email sanitization by detected address part to avoid rewriting matching text elsewhere.
Changes:
- Adds
partmetadata to detections. - Applies region-scoped punycode replacement.
- Adds sanitizer regression coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
lib/homographic_spoofing/detector/detection.rb |
Adds detection-part metadata. |
lib/homographic_spoofing/detector/email_address.rb |
Tags detections by address part. |
lib/homographic_spoofing/sanitizer/base.rb |
Scopes replacements by region. |
test/sanitizer/email_address_test.rb |
Tests email scoping behavior. |
test/sanitizer/idn_test.rb |
Verifies standalone IDN behavior. |
Review details
Suppressed comments (1)
lib/homographic_spoofing/sanitizer/base.rb:36
:localand:nameare still treated as the same region, so a detection can rewrite the other component. For example,tᴡitter <tᴡitter@twitter.com>has a safe display name (the quoted-string detector does not flag this text) and a spoofed local part, but the:localreplacement runs over the entire prefix and punycodes both occurrences. This contradicts the part-aware contract; carry/use exact name and local spans rather than grouping everything before@.
when :local, :name
in_region(source, nil, source.rindex("@")) { |head| replace_label(head, label) }
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eac69ab537
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
eac69ab to
4be9599
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4be9599f17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4be9599 to
9a17578
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a17578d75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9a17578 to
fa08e5a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa08e5a6ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fa08e5a to
45478dc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45478dccae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review threads: 19 resolved (16 fixed, 3 declined with the reasoning in-thread). This was the final iterated review round for this PR. Instrument reassessment. Every round after the first was a variation on one theme: the sanitizer needs the character offsets of the parsed components, and Follow-up candidates (not for this PR):
CI is green on 027d192; the base |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c21f490509
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3372382d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d440b5e5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b34ce3dde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 027d19279d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
786ffaa to
f3b9bf3
Compare
f3b9bf3 to
311a537
Compare
… from
A domain-label detection replaced every matching component across the whole
email address, so a benign local part equal to a spoofed domain label was
punycoded too — sanitize_email_address("з@мир.з.example.com") rewrote the "з"
mailbox because it equals the spoofed "з" domain label. This is by-design
Sanitizer::Base behavior that per-label domain detection newly made reachable
with a benign mailbox.
Carry on each Detection the [offset, length] span of the component it came
from, and have the sanitizer punycode each label only within its span, splicing
right-to-left so earlier offsets stay valid. The spans come from parser token
boundaries: the addr-spec built from the parser's raw local and domain (CFWS
kept, unlike the stripped forms) is a contiguous substring of the field, so its
offset pins the mailbox and host exactly even when a comment or stray
whitespace sits inside the address, and it is resolved against the angle-address
so a display name that repeats the addr-spec cannot divert the recipient's
replacement onto the name. Only the display name is still located by text, since
a mislocated name misplaces a cosmetic fix, never the recipient. A genuine spoof
on both sides still reports a detection per component, so both are punycoded;
detections from a standalone string carry no span, so sanitize_idn is unchanged.
A quoted display name or a comment may spell out the bracketed addr-spec itself, and the first textual "<local@domain>" then sits inside the name rather than at the real angle-address, so the domain detection rewrote the name and left the recipient domain spoofed. Locate the angle-address at a structural "<" instead.
…lay name outside comments
An obsolete route ("<@relay:local@domain>") keeps "<local@domain>" from
occurring literally, so the unrestricted fallback search landed on a
display name's copy of the addr-spec and left the recipient domain spoofed.
Leading CFWS that repeats a spoofed display name was likewise punycoded in
place of the name itself.
…ing the angle-address A double quote inside a comment is plain text, a ">" inside a domain literal does not close the angle-address, and a bare addr-spec is located outside comments — each otherwise let a leading comment or route text take the recipient's replacement and leave the spoofed domain in place.
… a whole component
A comment before an obsolete route can repeat the addr-spec, and a comment
attached to an offending domain label kept it from matching as a whole
component, sending the fallback substring replacement into a benign sibling
label ("магазин" around "з").
…lay name keep its span A comment's own text may carry dots, which split the label boundary and sent the fallback replacement into a benign sibling; and a display name the parser takes from a trailing comment lost its span to the comment exclusion, widening a name detection into a rewrite of an accepted mailbox.
The parser's raw domain token carries a trailing comment, so the addr-spec span covered the comment a display name was taken from and the name could never be located; a comment is never the mailbox or host, so a commented occurrence is accepted there when none exists outside.
Label matching set comments aside with "\([^)]*\)", which cannot nest: for
"user@магазин.з(outer(inner).note).example.com" it removed "(outer(inner)",
left ".note)" behind, and the offending label no longer matched as a whole
component, so the fallback substring replacement also rewrote the "з" inside
the benign sibling "магазин". The dot-splitting lookahead rescanned to the end
of the region at every dot, and the leading-comment pattern backtracked on runs
of "(", and every detection re-split the whole region.
Scan each region once, character by character, tracking nested comments,
quoted strings and backslash escapes as RFC 5322 has them. Each component
records its label with CFWS set aside and where that label starts, and all
detected labels are matched against the components in that one pass, so
sanitizing stays linear however many labels were detected.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… linearly
A display name the parser reads from a comment inside the host's token has a
span nested in the domain's span. Splicing the name first lengthened the text
under the domain's span, so a spoofed domain label after the comment fell
outside it and was left as is ("user@safe.(á́́)а.com"). Splice inner spans
first and grow the span around them by each edit.
A display name the parser unescapes ("á\́́") is not in the field as written,
so it got no span, and no span meant the whole field: the replacement landed
on a matching mailbox instead. Confine an unlocated name to the text before
the angle-address.
Mail::Address#local keeps an obsolete route in front of the mailbox, so the
local detections carry "@relay:local" while the span held only "local", and a
spoofed mailbox behind a route was left unsanitized. Widen the local span to
start at the route.
Each structural search converted a character offset by walking the field from
its start, and asked the field for its character count, so a long quoted name
full of "<" took seconds. Search the field's bytes with offset tables built
once.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mains
A display name located by text could start inside a comment and run past its
closing parenthesis into the mailbox ('"á́́)\user" <(á́́)user@example.com>'),
so its replacement rewrote the recipient. Accept an occurrence only when it
lies wholly outside comments or wholly inside one, and search each such run of
the field once, which also keeps a name that overlaps itself linear to find.
Mail::Address#local keeps an obsolete route in front of the mailbox, and
punycoding that text as one label encoded the route's "@" and ":" into the
mailbox. Check the mailbox with the route set aside, and check each route
domain as a domain within its own span, so each part is punycoded on its own
and the route syntax is kept. An address is no longer reported as a spoof just
for carrying a route.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t spans A display name matched inside a comment could take the comment's closing parenthesis with it, and punycoding the match moved the parenthesis, pushing the encoded text out of the comment and into the mailbox. Searchable comment runs now exclude the parentheses that open and close each comment. Route domains were split with a regular expression that broke a domain at whitespace between its labels and kept comments in the domain text, so a spoofed label after the whitespace went unchecked. Split the route in one pass on "," and ":" outside comments and check each domain with comments and whitespace set aside. With one span per route domain, splicing each span and then walking every remaining span grew with the square of the route count. Arrange spans into a tree once and rebuild the field in a single pass. Also stop recounting the display name's characters for every comment run searched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A display name could be matched starting right after a backslash in a comment, and punycoding it left the backslash escaping the encoded text instead of the character it escaped, opening an unmatched comment. Searchable runs now skip quoted pairs; the parser unescapes them in the name it reports, so a name that carries one is not in the field as written anyway. When the parser took the display name from a comment inside the local part, rewriting the name changed the text the local detection was matched by, and a spoofed mailbox beside it was left as is. Check the mailbox with its comments set aside; comments are not part of the mailbox, so an address is no longer reported as a spoof just for carrying one there. Return early when nothing was detected, so sanitizing nil returns nil again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Confining an unlocatable name to the text before the angle-address still let
the substring fallback rewrite across a quoted pair ("á́́\\\\"), leaving the
closing quote escaped. Give such a name an empty span: the detection is still
reported and the field is left as written.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A domain label PublicSuffix reports with whitespace inside it ("а ") never
equalled a component with its whitespace set aside, so it fell back to a
substring replacement over the whole region: one pass per label, and in an
address the first match could be a comment rather than the label itself.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
311a537 to
5811357
Compare
…l part The fallback split the angle-address at its first structural @, which belongs to an obsolete route when there is one, so the domain span took in the route and mailbox and a flagged domain label beside the mailbox was left as is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ocal part Outside an angle-address the fallback located the domain by its text, which a comment inside the domain hides, so the domain got no span and was rewritten across the whole field: the matching text before the @ was punycoded and the recipient's label was left as is. The domain is everything after the @. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For some comment placements the parser reports a domain that carries the
mailbox and its @ ("а.b@safe.com"), so a flagged label before the @ fell
outside the span after it. When the reported domain carries an @, the
boundary is unknown: let the domain detections span the whole field.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test built its name from combining marks, which also drives the display name check through a path that is quadratic on some Ruby releases (3.4.5 here, and on main), so it timed out in CI regardless of the search it guards. Build the name from a precomposed letter, which repeats the same way. Give the unclosed-comments timing test an assertion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The bug
sanitize_email_addressreplaced every address component that matched a detection, across the whole address. So a benign mailbox that merely equals a spoofed domain label was rewritten too:The fix
Each detection carries the
span([offset, length]) of the address component it came from, and the sanitizer punycodes only inside that span. A spoof genuinely present on both sides is still punycoded on both (tᴡitter@tᴡitter.com).Locating components (
Detector::EmailAddress). The mailbox and host spans come from the parser's raw tokens, located inside the structural angle-address. A copy of the address in a display name or comment can't take the replacement. The display name is located by text, and only accepted where it lies wholly outside comments and the address, or wholly inside one comment. Comment delimiters and backslash escapes are never part of a match. A name that is not in the field as written (the parser unescapes quoted pairs) gets an empty span: it is still reported, and nothing is rewritten. When the parser yields no raw address, a structural fallback splits at the address's own@(the last one inside<…>, past any obsolete route). When the parser folds the mailbox into the domain, the domain spans the whole field.Obsolete routes and mailbox comments.
Mail::Address#localkeeps an obsolete route (<@relay:local@domain>) and any comment in front of or inside the mailbox. The mailbox is now checked with both set aside, and each route domain is checked as a domain within its own span. SoName <@example.org:tᴡitter@example.com>becomesName <@example.org:xn--titter-345b@example.com>, and the route syntax is kept.Rewriting (
Sanitizer::Base). Spans are disjoint or nested (a display name read from a comment the domain token carries), so they are arranged into a tree and the field is rebuilt in one pass. Each region is split into labels by one scan that tracks nested comments, quoted strings and backslash escapes, replacing the earlier regular expressions. A label matches its component with comments and surrounding whitespace set aside, or exactly as written when the label itself carries whitespace.Performance
All searches use byte offsets with character tables built once, because repeated character-offset searches on a multibyte string are quadratic. That includes long display names, many comments or parentheses, many route domains and names that overlap themselves. Sanitizing stays linear in the length of the address; tests guard the main shapes with a timeout. This also resolves the CodeQL polynomial-regex alert.
Behaviour changes
ン/丿labels flag (from Apply mixed-script rules per label, not over the whole subdomain chain #76).Overlap with #83
#83 changes
Sanitizer::Base#sanitize/#punycode(longest label first, literal replacement); both are covered here by component matching and literal insertion, so it will conflict in that file. Recommended order: #76, this PR, then #83 rebased down to its wildcard and private-suffix split.Follow-ups (not in this PR)
@and the domain, which the parser folds into the domain.main), the display-name check is quadratic on long runs of combining marks; newer Ruby releases are fast.main).Tests
Each reported case has a regression test that asserts the sanitized output: comments, nested comments, escapes, quoted display names repeating the address, obsolete routes, display names from comments, names crossing a comment edge, nil input, and linear-time guards. Green on Ruby 3.1–3.4.
🤖 Generated with Claude Code