Skip to content

fix(isISO6346): anchor regex alternation and remove stray comma#2771

Open
DucMinhNe wants to merge 1 commit into
validatorjs:masterfrom
DucMinhNe:fix-iso6346-regex-anchoring
Open

fix(isISO6346): anchor regex alternation and remove stray comma#2771
DucMinhNe wants to merge 1 commit into
validatorjs:masterfrom
DucMinhNe:fix-iso6346-regex-anchoring

Conversation

@DucMinhNe

Copy link
Copy Markdown

The isISO6346 validator accepts strings it should reject because of an un-grouped regex alternation.

The bug

const isISO6346Str = /^[A-Z]{3}(U[0-9]{7})|([J,Z][0-9]{6,7})$/;

| has the lowest precedence, and the two branches are not wrapped in a group, so the pattern actually parses as:

(^[A-Z]{3}(U[0-9]{7}))   OR   (([J,Z][0-9]{6,7})$)

The first branch is anchored only at the start and the second only at the end. As a result:

  • a valid prefix followed by arbitrary trailing characters matches the first branch, and
  • arbitrary leading characters followed by a valid suffix match the second branch.

Additionally, the character class [J,Z] matches a literal comma, which is never a valid ISO 6346 category identifier.

Reproduction

const validator = require("validator");

validator.isISO6346("ABCU1234567GARBAGE"); // => true  (expected false)
validator.isISO6346("GARBAGEZ123456");     // => true  (expected false)
validator.isISO6346("XYZ,123456");         // => true  (expected false)

These all return true today. Because the checksum block only runs when str.length === 11, the un-anchored false positives bypass validation entirely.

The fix

Wrap the alternation in the existing group and drop the stray comma, so the whole string is anchored:

const isISO6346Str = /^[A-Z]{3}(U[0-9]{7}|[JZ][0-9]{6,7})$/;

This is the same class of fix already applied to other anchored-alternation validators in this project.

Tests

  • Added regression cases (ABCU1234567GARBAGE, GARBAGEZ123456, XYZ,123456) to the isISO6346 / isFreightContainerID invalid lists.
  • All existing valid cases (including the J/Z short forms like QJRZ123456) still pass.
  • Full validators suite passes (249 passing) and eslint src test is clean.

The isISO6346 pattern had an un-grouped alternation:

  /^[A-Z]{3}(U[0-9]{7})|([J,Z][0-9]{6,7})$/

Because | has the lowest precedence and the two branches were not
wrapped in a group, the regex parsed as:

  (^[A-Z]{3}(U[0-9]{7}))  OR  (([J,Z][0-9]{6,7})$)

The first branch was anchored only at the start and the second only at
the end, so strings with trailing garbage after a valid prefix, or
leading garbage before a valid suffix, passed validation. The character
class [J,Z] also matched a literal comma, which is never valid in an
ISO 6346 category identifier.

Wrap the alternation in the existing group and drop the comma so the
whole string is anchored:

  /^[A-Z]{3}(U[0-9]{7}|[JZ][0-9]{6,7})$/

Add regression cases for the previous false positives.
@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7fdc788) to head (b3516be).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2771   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          114       114           
  Lines         2587      2587           
  Branches       656       656           
=========================================
  Hits          2587      2587           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant