Reject non-ASCII whitespace in idn-email and strengthen email format tests

[el-psy-kongroo-d]

Follow-up to #1267, applying the review suggestions from that PR.

Changes

• Extract the non-ASCII-whitespace check into Strings.containsNonAsciiWhitespace
  and reuse it from both EmailFormat and IdnEmailFormat (instead of a private
  helper in a single class).
• idn-email: IdnEmailFormat shares EmailFormat's delegation and had the
  same bug — a leading U+00A0 was accepted. It now rejects non-ASCII whitespace
  too. Adds a new IdnEmailFormatTest (which also asserts a non-ASCII letter
  is still accepted, since idn-email allows those) and a license header to
  IdnEmailFormat, which previously had none.
• Strengthen EmailFormatTest: assert a quoted local part with an ASCII
  space ("joe bloggs"@example.com) stays valid, and generalize the negative
  case over U+00A0, U+2003 and U+3000.

Scope

Deliberately narrow: only non-ASCII whitespace is rejected. Non-ASCII
letters remain valid (important for idn-email). Zero-width format characters
such as U+200B and U+FEFF (Unicode category Cf) are out of scope and unchanged
from previous behavior.

Notes on license headers

• IdnEmailFormat had no header; the added one matches its sibling
  EmailFormat.java (Copyright (c) 2016 Network New Technologies Inc.).
• EmailFormatTest / IdnEmailFormatTest use Copyright (c) 2025 the original author or authors, matching every other test file in the format package
  (TimeFormatTest, UriFormatTest, ...). That is why the Reject non-ASCII whitespace in email format #1267 header nit was
  not switched to the main-source form — it would diverge from the test siblings.

Testing

Full test suite passes (8477 tests, 0 failures).

[el-psy-kongroo-d]

Thanks for the quick review and merge on #1267, @stevehu 🙏

This follow-up applies the suggestions from that review:

• Extracted the check into Strings.containsNonAsciiWhitespace and applied it to idn-email as well (with a test that a non-ASCII letter still validates).
• Added the quoted-local-part positive test and generalized the negative case to U+2003 / U+3000.

Two judgment calls I flagged in the description: I kept EmailFormatTest's the original author or authors header (all format test files use it, so changing only this one would diverge from its siblings), and kept the scope to whitespace only. Happy to adjust either if you'd prefer.

[stevehu]

Local checkout restored to master, working tree clean.

Review: PR #1268 — Reject non-ASCII whitespace in idn-email

networknt/json-schema-validator · followup/1267-email-idn-whitespace → master · +146 / −18 across 5 files · by @el-psy-kongroo-d

Overview

Follow-up to #1267, which fixed email rejecting a leading U+00A0 but left the sibling idn-email with the identical bug. This PR:

• Extracts the private helper into Strings.containsNonAsciiWhitespace and reuses it from both formats.
• Applies the guard to IdnEmailFormat, which shares EmailFormat's EmailValidator delegation and had the same hole.
• Adds IdnEmailFormatTest, strengthens EmailFormatTest, and adds a missing license header to IdnEmailFormat.

Verification

I built the branch and ran things rather than reading only:

• Full suite: 8477 tests, 0 failures, 30 skipped — exactly matches the PR's claim, and includes the official JSON-Schema-Test-Suite, so there's no conformance regression on format.
• The idn-email fix is load-bearing. I removed the three added lines from IdnEmailFormat.matches and reran: Tests run: 3, Failures: 1. The new test fails without the fix, passes with it.
• The guard's Unicode coverage is correct. Probed Character directly across the relevant code points:

┌──────────────┬────────┬────────┬────────┬────────┬──────────┬────────┬────────┐
│ │ U+00A0 │ U+2007 │ U+202F │ U+1680 │ U+2028/9 │ U+3000 │ U+00FC │
├──────────────┼────────┼────────┼────────┼────────┼──────────┼────────┼────────┤
│ isWhitespace │ ✗ │ ✗ │ ✗ │ ✓ │ ✓ │ ✓ │ ✗ │
├──────────────┼────────┼────────┼────────┼────────┼──────────┼────────┼────────┤
│ isSpaceChar │ ✓ │ ✓ │ ✓ │ ✓ │ ✓ │ ✓ │ ✗ │
├──────────────┼────────┼────────┼────────┼────────┼──────────┼────────┼────────┤
│ guard │ ✓ │ ✓ │ ✓ │ ✓ │ ✓ │ ✓ │ ✗ │
└──────────────┴────────┴────────┴────────┴────────┴──────────┴────────┴────────┘

This validates the isSpaceChar half of the disjunction — the three non-breaking separators are precisely the ones isWhitespace alone misses, and U+00A0 was the reported bug. The javadoc explaining why both predicates are needed is accurate and worth keeping.

• End-to-end probe through the real validator confirmed the documented scope boundary holds for both formats: U+00A0 / U+2007 / U+202F rejected; U+200B, U+FEFF, U+180E accepted (Cf, out of scope as stated); "joe bloggs"@example.com stays valid.
• U+0085 NEL is not caught by the guard (it's Cc, so neither predicate fires) — but I checked, and the underlying EmailValidator rejects it anyway on both formats. Not a gap; noting it so it doesn't get "found" again later.

Code quality

Genuinely good. Specific things done right:

• The extraction is justified, not gratuitous. I grepped for other isWhitespace/isSpaceChar uses in src/main — there are none, so Strings is now the single home for this and there's no leftover duplication.
• Strings is already a public final utility with public statics (isInteger, split), so the new method fits the class's existing contract rather than widening it in spirit.
• Using codePoints() with the int overloads of Character.isWhitespace/isSpaceChar is the correct pairing (the char overloads would be a latent surrogate bug even though no whitespace lives above the BMP).
• null returns false rather than throwing — matches how matches is invoked and avoids changing null-handling behavior.
• The test helper change to JsonMapperFactory.getInstance().writeValueAsString(email) is necessary, not cosmetic: the new quoted-local-part case contains " characters that the old string concatenation would have emitted as malformed JSON. (Jackson 3's tools.jackson exceptions are unchecked, so no throws clause is needed — worth knowing this only compiles because the project is on Jackson 3.)
• Building NBSP via Character.toChars(0x00A0) instead of pasting an invisible character into the source is the right call and is commented as such.

Suggestions

1. Asymmetric coverage between the two test classes. EmailFormatTest generalizes over U+2003 and U+3000 with @ParameterizedTest; IdnEmailFormatTest only tests U+00A0. Since the entire premise is that the two formats share the delegation, the idn side is the one where drift is more likely to go unnoticed. Reusing the same @valuesource(ints = { 0x2003, 0x3000 }) case in IdnEmailFormatTest is a two-line addition.

2. The two test helpers are near-identical. validateEmail and validateIdnEmail differ only in the format name. Separate classes per format does match the package convention (TimeFormatTest, UriFormatTest), so this is defensible as-is — but if a third format ever needs the same guard, a shared helper parameterized on format name would be the place to consolidate.

3. Optional micro-optimization. codePoints().anyMatch(...) allocates a stream per validation call, now on two format paths. Because every Zs/Zl/Zp character is in the BMP and surrogates are never whitespace, an indexed charAt loop would be equally correct and allocation-free. Only worth doing if format validation shows up in profiling — correctness is not affected either way, and the stream version reads better.

4. License header year. IdnEmailFormat was added 2023-05-20 (Adds support for validating idn-hostname and idn-email #775), so Copyright (c) 2016 predates the file by seven years. The PR's reasoning — match the sibling EmailFormat.java — is defensible and header years in this repo are already inconsistent, so this is a maintainer preference call, not a defect. The PR description already pre-empts the test-header question with the right evidence.

Risks

Low. The behavior change is narrow and in the stricter direction, applied to a format that was accepting input the spec doesn't permit. The main compatibility consideration is that anyone relying on idn-email accepting a leading non-breaking space now gets a validation error — which is the point of the fix. The deliberate exclusion of Cf characters (U+200B, U+FEFF) keeps the blast radius small and is documented in both the PR body and the javadoc.

No security concerns: no new parsing, no regex (so no ReDoS surface), no I/O, and the guard is a pure function that runs before the existing validator rather than replacing it.

Recommendation: approve. Suggestion 1 is the only one I'd bother asking for, and it's optional.

[el-psy-kongroo-d]

Thanks for the thorough review. This was my first open-source contribution, and going through the whole cycle — issue, a narrowly-scoped fix, review, and a follow-up applying the suggestions — taught me a lot about how contribution actually works.

Added the parameterized U+2003 / U+3000 case to IdnEmailFormatTest, mirroring EmailFormatTest (suggestion 1). Left 2–4 as-is per the review's reasoning.

Looking forward to contributing more.

[stevehu]

Local checkout restored to master, working tree clean.

Re-review: PR #1268 after commit 13a0af4

networknt/json-schema-validator · followup/1267-email-idn-whitespace → master · +160 / −18 across 5 files · by @el-psy-kongroo-d

What changed since my last review

Exactly one commit, doing exactly what suggestion 1 asked and nothing else:

13a0af4 Generalize idn-email whitespace test over U+2003 and U+3000

+14 / −0 in IdnEmailFormatTest.java — two imports and the mirrored @ParameterizedTest @valuesource(ints = { 0x2003, 0x3000 }) case. No main-source changes, so the behavior surface is identical to the version I already verified. Suggestions 2–4 were left alone, which is the right call — each was explicitly marked optional or a maintainer-preference item.

Verification

• Full suite: 8479 tests, 0 failures, 30 skipped. Up exactly 2 from the 8477 I measured before, matching the two new parameter cases. No collateral movement.
• The new cases are load-bearing, not incidental. I removed the three-line guard from IdnEmailFormat.matches and reran: Tests run: 10, Failures: 3 — the NBSP case and both new parameterized cases fail. So the underlying EmailValidator genuinely accepts U+2003 and U+3000 in idn-email; these aren't tests that would pass without the fix. Restored and recompiled afterward.
• Behavior parity now holds end-to-end. Probed both formats through the real validator across the relevant code points:

┌───────────────────────────────────┬─────────┬───────────┐
│ input │ email │ idn-email │
├───────────────────────────────────┼─────────┼───────────┤
│ "joe bloggs"@example.com │ valid │ valid │
├───────────────────────────────────┼─────────┼───────────┤
│ non-ASCII letter U+00FC │ valid │ valid │
├───────────────────────────────────┼─────────┼───────────┤
│ leading U+00A0 / U+2003 / U+3000 │ invalid │ invalid │
├───────────────────────────────────┼─────────┼───────────┤
│ leading U+2007 / U+202F / U+1680 │ invalid │ invalid │
├───────────────────────────────────┼─────────┼───────────┤
│ leading U+200B / U+FEFF (Cf) │ valid │ valid │
├───────────────────────────────────┼─────────┼───────────┤
│ leading U+0085 NEL (Cc) │ invalid │ invalid │
├───────────────────────────────────┼─────────┼───────────┤
│ trailing U+00A0, U+00A0 in domain │ invalid │ invalid │
└───────────────────────────────────┴─────────┴───────────┘

The two columns are now identical for every case, which is the whole point of the PR. The Cf row is the documented scope boundary holding, and the NEL row is the underlying validator catching what the guard deliberately doesn't.

• No leftover duplication: isWhitespace/isSpaceChar appear nowhere else in src/main except the single Strings method.

Remaining notes (none blocking)

• Minor test asymmetry, opposite direction. EmailFormatTest has the quoted-local-part positive case; IdnEmailFormatTest doesn't. Lower value than the gap that was just closed — the negative cases are where drift between the two delegating formats would actually hide — so I'd leave it.
• One spec nuance worth recording, not fixing. "joe<U+00A0>bloggs"@example.com is now rejected for idn-email too. Under RFC 6531 an SMTPUTF8 quoted string can legitimately carry non-ASCII, so the guard is a hair over-strict inside quoted local parts. It's an obscure construction, format assertions aren't required to be exact validators, and tightening it would mean parsing the local part — well outside this PR's stated scope. Noting it so it isn't rediscovered as a bug later.

Risks

Unchanged from my last pass: low. Narrow, stricter-direction behavior change on input the spec doesn't permit; no new parsing, no regex, no I/O; pure function running ahead of the existing validator.

Recommendation: approve and merge. The follow-up addressed the one suggestion I'd have asked for, the fix is verified load-bearing by mutation, and the full suite is green.

Also worth saying: for a first open-source contribution, splitting the narrow fix from the follow-up refactor, pre-empting the license-header question with evidence in the PR body, and building invisible characters from code points instead of pasting them into source are all things regular contributors get wrong.