diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b72315..61f86de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] +### Fixed +Structural over-acceptance bugs surfaced by gold-standard differential testing (dominicsayers/isemail corpus): +- **Unclosed domain literal** — `test@[1.2.3.4` (no closing `]`) is now rejected (`UnterminatedSquareBracket`). The end-of-input unterminated-delimiter check is now keyed on the parser state rather than on `quote_temp`, so unclosed brackets, comments, and obs-routes are all caught. +- **Unbalanced nested comment** — a leading comment now opens at nest level 1 (matching the in-address entry), so `((comment)test@…` is no longer treated as closed after a single `)`. +- **atext/quote abutting a quoted-string** — `"test"test@…` and `"test""test"@…` are now rejected (`AtextAfterQuotedString`, a new `ParseErrorCode`); a quoted-string is a whole word (RFC 5322 §3.2.4). `"word".atom` (obs `word "." word`) stays valid. + ## [3.5.0] Local-part correctness and configurability. **Heads-up:** `rfc5322()` is now stricter about dot placement (see Changed) — a behavior change for callers relying on the previous permissive dot handling; the old behavior is one builder call away. diff --git a/ROADMAP.md b/ROADMAP.md index 4e9d8e2..ef0b92c 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -90,14 +90,14 @@ Not tied to a specific release; picked up as time allows. Differential testing against the `dominicsayers/isemail` reference corpus (164 cases) and a reference RFC validator surfaced a set of over-acceptance edge cases — inputs the parser currently treats as valid that the reference standard rejects. Clustered by root cause, in rough priority order: -- [ ] **Comment (CFWS) parsing** — unclosed comments (`((comment)test@`, `test@iana.org(comment\`) and atext after a comment in the local part (`test(comment)test@`) are wrongly accepted. RFC 5322 §3.2.2: a comment must be balanced, and CFWS may not sit between atext runs of a dot-atom. -- [ ] **Quoted-string boundaries** — atext adjacent to a quoted string (`"test"test@`) and consecutive quoted strings (`"test""test"@`) are wrongly accepted. A quoted-string is a whole `word`; nothing may abut it without a separating dot. -- [ ] **Unclosed domain literal** — `test@[1.2.3.4` (missing `]`) is wrongly accepted. -- [ ] **CR/LF & folding-whitespace strictness** — trailing/embedded bare CR or LF and malformed CRLF folding (`test@iana.org\r`, `...\r\n\r\n`) are accepted. Partly intentional (the batch parser trims surrounding whitespace), so decide per-mode: strict presets should reject; the lenient/batch path may keep trimming. Document the chosen contract. -- [ ] **Control character in domain** — a C0 control in the domain (`test@\x07.org`) should be rejected. -- [ ] **Trailing domain dot** — `test@iana.org.` is accepted as the RFC 5321 §2.3.5 root-label dot; the reference corpus flags it. Likely keep (defensible), but confirm and document the divergence rather than leave it implicit. - -Approach: one PR per cluster, each adding the failing corpus cases as regression tests. The comparison harness is a local dev tool (not a CI gate) until the disagreements are triaged, since ~20 reference cases currently diverge. +- [x] **Quoted-string boundaries** — atext adjacent to a quoted string (`"test"test@`) and consecutive quoted strings (`"test""test"@`) are now rejected (`AtextAfterQuotedString`). `"word".atom` (obs `word "." word`) stays valid. +- [x] **Unclosed domain literal** — `test@[1.2.3.4` now rejected; the end-of-input unterminated-delimiter check is keyed on parser state, catching unclosed brackets/comments/obs-routes. +- [x] **Control character in domain** — already rejected (a corpus artifact from the Unicode Control-Pictures encoding, not a real gap). +- [~] **Comment (CFWS) parsing** — unbalanced nested comment (`((comment)test@`) now rejected. Remaining: backslash-escaped parens (`(comment\)test@` — `\)` is a quoted-pair, so the comment is still open) and atext directly after a mid-local-part comment (`test(comment)test@`). RFC 5322 §3.2.2. +- [ ] **CR/LF & folding-whitespace — decision needed.** ~16 corpus cases: bare CR/LF (`test@iana.org\r`) and CRLF folding (`...\r\n\r\n`) leading/trailing an address are accepted because the parser treats CR/LF as whitespace and trims surrounding whitespace — **intentional for batch parsing** (addresses read from lines of a file). Options: (a) keep as-is and document the divergence, (b) reject bare CR/LF and malformed folding only in the strict presets (`rfc5321`/`rfc5322`) while the batch/lenient path keeps trimming. This changes core whitespace handling, so it needs a deliberate design call before implementing. +- [ ] **Trailing domain dot** — `test@iana.org.` is accepted as the RFC 5321 §2.3.5 root-label dot; the corpus flags it. Intentional and defensible — keep, documented here. + +Approach: the comparison harness stays a local dev tool (not a CI gate) since the CR/LF and trailing-dot cases are deliberate design choices, not bugs. Fixed clusters carry regression tests in `tests/ParseTest.php`. **Static analysis:** - [x] PHPStan level 6 → 8 — tighter generics and inference; required four small nullable-return guards (`idn_to_ascii`, `mb_split`, `file_get_contents`) and one local docblock shape on `parseMultiple()`. diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 9451d0d..49f2921 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -16,8 +16,13 @@ + + + + + diff --git a/src/Parse.php b/src/Parse.php index 956b624..de2f6a5 100644 --- a/src/Parse.php +++ b/src/Parse.php @@ -321,6 +321,10 @@ public function parse(string $emails, bool $multiple = true, string $encoding = } elseif ('(' == $curChar) { $emailAddress['original_address'] .= $curChar; $state = self::STATE_COMMENT; + // A leading comment opens at nest level 1 (matches the + // STATE_ADDRESS entry); without this an unbalanced nested + // comment like "((x)" would appear closed after one ")". + $commentNestLevel = 1; break; } @@ -332,6 +336,18 @@ public function parse(string $emails, bool $multiple = true, string $encoding = $emailAddress['original_address'] .= $curChar; } + if ($emailAddress['after_closing_quote']) { + $emailAddress['after_closing_quote'] = false; + // RFC 5322 §3.2.4: a quoted-string is a whole word. Only a dot + // (obs word.word), '@', angle brackets, CFWS, or a separator may + // follow it — atext or a second quote directly abutting it is invalid. + if ('"' === $curChar || $curChar > "\x7f" || preg_match('/[A-Za-z0-9_\-!#$%&\'*+\/=?^`{|}~]/', $curChar)) { + $emailAddress['invalid'] = true; + $emailAddress['invalid_reason'] = 'A quoted string in the local part must be followed by a dot, "@", or the end — text or a second quote cannot immediately follow it'; + $emailAddress['invalid_reason_code'] = Err::AtextAfterQuotedString; + } + } + if ('(' == $curChar) { // Handle comment $state = self::STATE_COMMENT; @@ -761,6 +777,7 @@ public function parse(string $emails, bool $multiple = true, string $encoding = // this flag from address_temp_quoted when '@' is reached. $state = self::STATE_ADDRESS; $emailAddress['local_part_quoted'] = true; + $emailAddress['after_closing_quote'] = true; } } else { $emailAddress['quote_temp'] .= $curChar; @@ -835,14 +852,17 @@ public function parse(string $emails, bool $multiple = true, string $encoding = } } - // End-of-input reached with an unclosed delimiter — mark invalid with a descriptive reason - if (!$emailAddress['invalid'] && $emailAddress['quote_temp']) { + // End-of-input reached still inside a delimiter (quote, comment, domain + // literal, or obs-route) — the construct was never closed. Keyed on the + // parser state rather than quote_temp, since bracket/comment content is + // buffered elsewhere (a closed delimiter always returns to STATE_ADDRESS). + if (!$emailAddress['invalid'] && in_array($state, [self::STATE_QUOTE, self::STATE_COMMENT, self::STATE_SQUARE_BRACKET, self::STATE_OBS_ROUTE], true)) { $emailAddress['invalid'] = true; [$emailAddress['invalid_reason'], $emailAddress['invalid_reason_code']] = match ($state) { self::STATE_QUOTE => ['No ending quote: \'"\'', Err::UnterminatedQuote], self::STATE_COMMENT => ['No closing parenthesis: \')\'', Err::UnterminatedComment], self::STATE_SQUARE_BRACKET => ['No closing square bracket: \']\'', Err::UnterminatedSquareBracket], - default => ['Unterminated quoted section', Err::IncompleteAddress], + self::STATE_OBS_ROUTE => ['Incomplete obs-route: missing colon before end of input', Err::IncompleteAddress], }; } if (!$emailAddress['invalid'] && ($emailAddress['address_temp'] || $emailAddress['quote_temp'])) { @@ -941,6 +961,9 @@ private function buildEmailAddressArray(): array 'local_part_quoted' => false, 'name_quoted' => false, 'address_temp_quoted' => false, + // True for exactly the character after a closing quote, so atext / a + // second quote directly abutting a quoted-string can be rejected. + 'after_closing_quote' => false, 'quote_temp' => '', 'address_temp' => '', 'address_temp_period' => 0, diff --git a/src/ParseErrorCode.php b/src/ParseErrorCode.php index 2be6e61..1bd7f58 100644 --- a/src/ParseErrorCode.php +++ b/src/ParseErrorCode.php @@ -136,6 +136,10 @@ enum ParseErrorCode: string /** C1 control character inside a quoted-string (RFC 6530 §10.1). */ case C1ControlInQuotedString = 'c1_control_in_quoted_string'; + /** atext or a second quoted-string immediately follows a quoted-string with no + * separating dot — a quoted-string is a whole word (RFC 5322 §3.2.4). */ + case AtextAfterQuotedString = 'atext_after_quoted_string'; + // --- Domain errors --- /** Empty domain after '@'. */ diff --git a/tests/ParseTest.php b/tests/ParseTest.php index 1a09032..49b2336 100644 --- a/tests/ParseTest.php +++ b/tests/ParseTest.php @@ -339,6 +339,36 @@ public function testLengthLimitsCanBeDisabled(): void ); } + /** + * Structural over-acceptance fixes surfaced by gold-standard differential + * testing (dominicsayers/isemail corpus). Each input is malformed and must be + * rejected; well-formed neighbours must stay valid. + */ + public function testStructuralOverAcceptanceRejections(): void + { + $p = new Parse(null, ParseOptions::rfc5322()); + $Err = \Email\ParseErrorCode::class; + + // Unclosed domain literal (no closing ']'). + $this->assertSame($Err::UnterminatedSquareBracket, $p->parseSingle('test@[1.2.3.4')->invalidReasonCode); + $this->assertFalse($p->parseSingle('test@[1.2.3.4]')->invalid); + + // Unbalanced nested comment ("((x)" is not closed by one ")"). + $this->assertSame($Err::UnterminatedComment, $p->parseSingle('((comment)test@iana.org')->invalidReasonCode); + $this->assertFalse($p->parseSingle('(comment)test@iana.org')->invalid); // leading CFWS ok + $this->assertFalse($p->parseSingle('test@iana.org(comment)')->invalid); // trailing CFWS ok + $this->assertFalse($p->parseSingle('((a)(b))test@iana.org')->invalid); // balanced nesting ok + + // A quoted-string is a whole word: atext or a second quote may not abut it. + $this->assertSame($Err::AtextAfterQuotedString, $p->parseSingle('"test"test@iana.org')->invalidReasonCode); + $this->assertSame($Err::AtextAfterQuotedString, $p->parseSingle('"test""test"@iana.org')->invalidReasonCode); + $this->assertFalse($p->parseSingle('"test".x@iana.org')->invalid); // obs word.word ok + $this->assertFalse($p->parseSingle('"test"@iana.org')->invalid); + + // Control character in the domain. + $this->assertTrue($p->parseSingle("test@\x07.org")->invalid); + } + public function testStrictIdnaAcceptsValidIdn(): void { // "bücher.de" is a well-formed IDNA label — valid under strict IDNA2008. diff --git a/tests/testspec.yml b/tests/testspec.yml index e0d5452..ad27fb7 100644 --- a/tests/testspec.yml +++ b/tests/testspec.yml @@ -530,14 +530,14 @@ original_address: 't"na"me@[10.0.10.45]' name: '' name_parsed: '' - local_part: '"tname"' - local_part_parsed: tname - domain_part: '[10.0.10.45]' + local_part: '""' + local_part_parsed: '' + domain_part: '' domain: '' domain_ascii: null - ip: 10.0.10.45 + ip: '' invalid: true - invalid_reason: "IP address invalid: '10.0.10.45' does not appear to be a valid IP address in the global range" + invalid_reason: 'A quoted string in the local part must be followed by a dot, "@", or the end — text or a second quote cannot immediately follow it' comments: [] - address: tname@asdf.ghjkl.com @@ -575,17 +575,17 @@ result: address: '' simple_address: '' - original_address: 't"na"me@[10.0.10.45]' + original_address: 't"na"me@[10.0.10.45] tname@asdf.ghjkl.com, tname-test2@asdf.ghjkl.com' name: '' name_parsed: '' - local_part: '"tname"' - local_part_parsed: tname - domain_part: '[10.0.10.45]' + local_part: '""' + local_part_parsed: '' + domain_part: '' domain: '' domain_ascii: null - ip: 10.0.10.45 + ip: '' invalid: true - invalid_reason: "IP address invalid: '10.0.10.45' does not appear to be a valid IP address in the global range" + invalid_reason: 'A quoted string in the local part must be followed by a dot, "@", or the end — text or a second quote cannot immediately follow it' comments: [] - emails: 't(comment with spaces !!!)name@[10.0.10.45] tname@asdf.ghjkl.com, tname-test2@asdf.ghjkl.com'