Read RFC 2231 continuations in one pass and honor every charset the platform names - #233
Conversation
…latform names `extendedContinuation` looked each numbered section up with `extractHeaderParam`, which tokenizes the whole header on every call, so a sender who writes N legal `filename*N*=` sections made the parser do work that grows with N². Measured with `swiftc -O`: 1,024 sections took 0.5 s, 4,096 took 8 s, and 8,192 did not finish inside 25 s, where the previous release read the same headers in tens of milliseconds. The header is now tokenized once into `parameters(of:)`, and both the single-attribute lookup and the continuation collector read from that list. The collector also applies RFC 2231 §3 as written: sections count in decimal from 0 without leading zeroes or gaps, and the first occurrence of a repeated section stands, matching `extractHeaderParam`. `decodeExtendedBytes` whitelisted three charsets and returned nil for every other label, including the blank one. RFC 2231 §4 makes the charset field optional with the `'` delimiters present, so `filename*=''invoice.pdf` is a legal way to write `invoice.pdf`, and a filename labelled `windows-1252` or `iso-8859-15` is ordinary real-world mail; all of them lost the name and fell through to a generated one. A blank charset now decodes as UTF-8, which reads every US-ASCII value unchanged, and any other label resolves through `String.Encoding(ianaCharsetName:)`, the same lookup the encoded-word decoder already uses. A label the platform cannot name still yields nil, and the literal spelling of the parameter is used instead. The RFC 2231 helpers move to `EMLParser+RFC2231.swift`; there is no behaviour change in the move beyond the two above. Verification: `swift test` passes 624 tests in 77 suites; `swiftlint lint --strict` reports no violations. Five tests are added: blank charset, platform-named charsets (windows-1252, ISO-8859-1, and ISO-8859-15 on Darwin only: swift-corelibs-foundation names that charset but has no converter for it), unknown charset falling back to the literal spelling, section numbering rules, and a 4,096-section header that must decode in under two seconds. The blank-charset, platform-charset and linear-work tests fail on the parent commit (the last one after 34 s in a debug build); the other two pin behaviour that already held. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
6d8eae1 to
d87295e
Compare
Fork main is now upstream 64a9a86 (#232) plus one carried deviation, the RFC 2231 continuation and charset fix that is open upstream as Cocoanetics/SwiftMail#233. This moves packages.SwiftMail.revision from a2d4a94f to d87295e3, the same commit for the app and the notification extension, pulling in the twelve upstream commits the 2026-09-09 housekeeping review classified (#220 #222 #221 #224 #225 #226 #230 #229 #228 #227 #231 #232). The three receive-path collisions that review found are closed: C1 by the previous commit's app-side decode, C2 and R1 by the carried fork fix. Verification: the resolved SwiftMail checkout was proven at d87295e3 by rev-parse before and after building. App, notification extension and TabMailTests build with only the pre-existing AutoSizingHTMLView deprecation warning. The full TabMailTests target ran against the previous, code-identical fork commit 6d8eae13: 9,727 tests, two WKWebView hosting suites failed under a host load above 500 and passed on an isolated rerun; no other failures. The EML consumer suites (EmlParsing, IMAP nested-eml and render, Gmail and Exchange mocks, nested attachments; 39 tests) pass at the final pin. Fork CI is green on all platforms for d87295e3, as are the upstream PR checks. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
odrobnik
left a comment
There was a problem hiding this comment.
Requesting changes for one cross-platform RFC 2231 decoding regression: on non-Apple platforms, the new charset path can treat unsupported legacy encodings as UTF-8, corrupting valid filenames and suppressing the safe literal fallback. No builds or tests were run, per the review constraints.
One review with 1 finding, detailed in an inline comment:
- MEDIUM
Sources/SwiftMail/MIME/EMLParser+RFC2231.swift:58
To unblock the merge, address every finding: react with 👍 if it is accurate (👎 if not), reply with a brief comment on what was fixed, and mark the item as resolved.
On platforms without CoreFoundation, `String.Encoding(ianaCharsetName:)` resolves charsets it has no converter for (GBK, Big5, EUC-KR, GB2312, GB18030, KOI8-R, Macintosh) to `.utf8` as a best-effort placeholder. For an RFC 2231 extended parameter that is the wrong trade: a UTF-8 misread of GBK bytes outranked the sender's literal `filename=`, so `filename*=gbk''%C2%A3.txt` produced `£.txt` on Linux instead of the literal fallback. `decodeExtendedBytes` now accepts a `.utf8` resolution only when the label itself names UTF-8 or US-ASCII; any other label that resolves to `.utf8` is treated as unsupported and yields `nil`, so the literal spelling wins. Platforms with a real converter still decode the charset as before. New test pins the invariant on every platform: the UTF-8 misread is never the result, and on Darwin the GBK decode is. 25 tests in `MIMEParameterParsingTests` pass; SwiftLint strict clean. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
odrobnik
left a comment
There was a problem hiding this comment.
The earlier cross-platform legacy-charset corruption is addressed at the new head, and the continuation work remains linear. Requesting changes for one remaining RFC 2231 charset regression: the new UTF-8 safeguard applies narrower label normalization than SwiftCross itself, so resolver-supported UTF-8 aliases are discarded. No builds or tests were run, per the review constraints.
One review with 1 finding, detailed in an inline comment:
- MEDIUM
Sources/SwiftMail/MIME/EMLParser+RFC2231.swift:59
To unblock the merge, address every finding: react with 👍 if it is accurate (👎 if not), reply with a brief comment on what was fixed, and mark the item as resolved.
The guard added to reject the resolver's UTF-8 placeholder for unsupported legacy charsets only trimmed and lowercased the label, while `String.Encoding(ianaCharsetName:)` also folds `_` to `-`, collapses repeated hyphens and drops a `$esc` suffix before lookup. So `filename*=utf_8''…` resolved to UTF-8 and was then rejected, discarding a valid filename. `isUTF8Label` now applies the same normalization, so every spelling the resolver accepts as UTF-8 is accepted here. The US-ASCII names are dropped from the list: they resolve to `.ascii`, never `.utf8`, so they never reached the guard. An alias the list does not know still fails safe to the literal parameter rather than misreading bytes. New test covers utf-8, UTF-8, utf8, UTF8, utf8mb4, utf_8, utf--8 and utf-8$esc on every platform; it fails on the previous guard for the last three and passes now. 26 tests in `MIMEParameterParsingTests` pass; SwiftLint strict clean. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
|
Both findings are addressed and the head is now 53f27c9:
Ready for another look whenever you have a moment. |
odrobnik
left a comment
There was a problem hiding this comment.
Requesting changes for two remaining RFC 2231 charset regressions. Blank charset fields currently assign UTF-8 meaning to otherwise ambiguous non-ASCII bytes and can suppress the safe literal fallback. Separately, the UTF-8 placeholder safeguard is applied on Darwin too, where it rejects legitimate aliases that CoreFoundation resolves to UTF-8. The continuation collection change is linear and the previously reported unsupported-legacy-label and normalization cases are addressed. No builds or tests were run, per the review constraints.
One review with 2 findings, each detailed in an inline comment:
- MEDIUM
Sources/SwiftMail/MIME/EMLParser+RFC2231.swift:56 - MEDIUM
Sources/SwiftMail/MIME/EMLParser+RFC2231.swift:59
To unblock the merge, address every finding: react with 👍 if it is accurate (👎 if not), reply with a brief comment on what was fixed, and mark the item as resolved.
… UTF-8 Two remaining RFC 2231 charset regressions from the review. A blank charset field (`filename*=''…`) was decoded as UTF-8, so `filename*=''%C2%A3.txt` produced `£.txt` and outranked the sender's literal `filename="fallback.txt"`, although RFC 2231 §4 says leaving the field blank "MUST NOT be done in order to indicate a default character set" and those bytes could equally be Latin-1 `£` or GBK `拢`. The blank-charset path now decodes only US-ASCII bytes; anything else yields nil and the literal spelling wins, or no name at all when there is none. The guard that refuses SwiftCross's `.utf8` placeholder for legacy charsets it cannot convert was also applied on Apple platforms, where CoreFoundation is authoritative, so its legitimate alias `unicode-1-1-utf-8` was rejected. The guard is now compiled only where SwiftCross uses its hand table, the same platform condition SwiftCross itself tests. There it no longer mirrors the resolver's normalization, which drifted twice under review: with the resolver having already returned `.utf8`, the label is genuine exactly when its letters and digits start with `utf8`, which every accepted UTF-8 spelling does and no placeholder label (gbk, big5, euc-kr, koi8-r, macintosh, …) can. A placeholder added later is still rejected; an unusual UTF-8 alias fails safe to the literal parameter. Verification: 627 tests in 77 suites pass; `swiftlint lint --strict` is clean. New test `extendedParameterWithBlankCharsetDecodesOnlyASCII` and the Darwin `unicode-1-1-utf-8` case added to `extendedParameterUTF8AliasesAreHonored` both fail on the parent commit and pass here. `extendedParameterLegacyCharsetIsNeverMisreadAsUTF8` now covers every placeholder label and alias in SwiftCross's table. The non-Apple branch was checked against SwiftCross 1.2's alias and encoding tables by inspection; no Linux toolchain was available locally. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
|
Both findings are addressed and the head is now 9f1a2b6:
627 tests in 77 suites pass, SwiftLint strict is clean, and the new assertions are red on the previous head. Ready for another look whenever you have a moment. |
|
MissionControl review timed out for head |
odrobnik
left a comment
There was a problem hiding this comment.
Requesting changes for one build-blocking issue introduced by the RFC 2231 file split. The new file calls SwiftCross's IANA charset initializer without importing SwiftCross, so the SwiftMail target cannot compile. The continuation implementation is linear and the previously reported charset regressions are addressed. No builds or tests were run, per the review constraints.
One review with 1 finding, detailed in an inline comment:
- HIGH
Sources/SwiftMail/MIME/EMLParser+RFC2231.swift:57
To unblock the merge, address every finding: react with 👍 if it is accurate (👎 if not), reply with a brief comment on what was fixed, and mark the item as resolved.
`EMLParser+RFC2231.swift` calls `String.Encoding(ianaCharsetName:)`, which SwiftCross declares, but imported only Foundation. The target still built on every platform because Swift resolves extension members through any file in the module that imports the declaring module; that is the leaky-member-visibility behaviour `MemberImportVisibility` turns into an error, so the file now names its dependency explicitly like the other two callers do. No behaviour change; 627 tests pass, SwiftLint strict clean. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
|
Addressed; head is now 5e234fd. |
odrobnik
left a comment
There was a problem hiding this comment.
Approved. The previously reported charset and import issues are addressed at 5e234fd. I traced filename extraction and Content-Type cleanup through their parser call sites and checked SwiftCross 1.2.0's on-disk IANA resolver: the platform split and UTF-8 placeholder safeguard now match the dependency, blank charsets fail safely for non-ASCII bytes, and continuation collection remains linear with contiguous decimal semantics. No builds or tests were run, per the review constraints.
👍 No findings.
Cocoanetics/SwiftMail#233 merged upstream as 124e3cc, so the fork main was reset to it and is a pure mirror again with no deviation. App and NSE now pin packages.SwiftMail.revision to that commit instead of the fork-only d87295e3, which carried the first draft of the same fix. Behavioural difference from the previous pin on Apple platforms: an RFC 2231 extended filename with a blank charset and non-ASCII bytes now falls back to the literal filename parameter instead of being read as UTF-8. Nothing else in the receive path changed; the other review fixes affect non-Apple platforms only. Verification: resolved SwiftMail checkout proven at 124e3cc by rev-parse before and after the build; TabMail and TabMailNotificationService build with only the documented benign App Intents diagnostic; EML consumer suites (175 tests in 7 suites) pass against the new pin. The IOS-IMAP-016 record and companion memory 126 are updated with the merge and sync state. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
Cocoanetics/SwiftMail#233 merged upstream as 124e3cc, so the fork main was reset to it and is a pure mirror again with no deviation. App and NSE now pin packages.SwiftMail.revision to that commit instead of the fork-only d87295e3, which carried the first draft of the same fix. Behavioural difference from the previous pin on Apple platforms: an RFC 2231 extended filename with a blank charset and non-ASCII bytes now falls back to the literal filename parameter instead of being read as UTF-8. Nothing else in the receive path changed; the other review fixes affect non-Apple platforms only. Verification: resolved SwiftMail checkout proven at 124e3cc by rev-parse before and after the build; TabMail and TabMailNotificationService build with only the documented benign App Intents diagnostic; EML consumer suites (175 tests in 7 suites) pass against the new pin. The IOS-IMAP-016 record and companion memory 126 are updated with the merge and sync state. Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai> (cherry picked from commit cc6b90d) Signed-off-by: Kwang Moo Yi <kmyi@tabmail.ai>
Two regressions from #230, both in how
EMLParserreads RFC 2231 extended parameters. Neither touches the parameter-boundary hardening that commit added.Continuation sections cost quadratic work
extendedContinuationfound each numbered section withextractHeaderParam, and that function tokenizes the whole header on every call. A sender who writes N legalfilename*N*=sections therefore made the parser do work proportional to N². Measured withswiftc -O -wmoon the exact function source:The full
EMLParser.parsereaches this through an ordinary multipart body with folded header lines, so a small received attachment can hold a parse for seconds.The header is now tokenized once into
parameters(of:), and bothextractHeaderParamand the continuation collector read from that list. The collector also applies RFC 2231 §3 as written: sections count in decimal from 0, no leading zeroes, no gaps, and where a section repeats the first occurrence stands, matchingextractHeaderParam.Blank and non-whitelisted charsets discarded the filename
decodeExtendedBytesaccepted onlyutf-8,us-asciiandiso-8859-1and returned nil for every other label, including the blank one. RFC 2231 §4 says "it is perfectly permissible to leave either the character set or language field blank" with the'delimiters present, sofilename*=''invoice.pdfis a legal spelling ofinvoice.pdf, andwindows-1252oriso-8859-15labels are ordinary real-world mail. All of these lost the name and fell through to a generated one.A blank charset now decodes as UTF-8, which reads every US-ASCII value unchanged and still rejects bytes that are not text. Any other label resolves through
String.Encoding(ianaCharsetName:), the same lookup the encoded-word decoder already uses. A label the platform cannot name still yields nil, andextractFilenamefalls through to the literal spelling.Layout
The RFC 2231 helpers move to
EMLParser+RFC2231.swiftsoEMLParser+Parameters.swiftstays under the lint file-length limit. Nothing changes in the move beyond the two fixes above.Verification
swift test: 624 tests in 77 suites pass (619 existing plus the five below).swiftlint lint --strict: 0 violations.MIMEParameterParsingTests: blank charset;windows-1252,iso-8859-15andISO-8859-1labels; unknown charset falling back to the literal spelling; section-numbering rules; and a 4,096-section header that must decode in under two seconds. On the parent commit the blank-charset, platform-charset and linear-work tests fail (the last after 34 s in a debug build); the other two pin behaviour that already held.🤖 Generated with Claude Code
https://claude.ai/code/session_0132BGdMBGzmQwA6jkB3rPma