diff --git a/.changeset/inconsistent-naming-dedup-collision.md b/.changeset/inconsistent-naming-dedup-collision.md new file mode 100644 index 00000000..41a8777f --- /dev/null +++ b/.changeset/inconsistent-naming-dedup-collision.md @@ -0,0 +1,5 @@ +--- +'dotenv-diff': patch +--- + +fix an inconsistent naming warning being silently dropped when a key contained the character `|`. diff --git a/packages/cli/src/core/detectInconsistentNaming.ts b/packages/cli/src/core/detectInconsistentNaming.ts index 7e43a91f..df9b5d5a 100644 --- a/packages/cli/src/core/detectInconsistentNaming.ts +++ b/packages/cli/src/core/detectInconsistentNaming.ts @@ -20,8 +20,11 @@ export function detectInconsistentNaming( // Skip if either key is undefined if (!key1 || !key2) continue; - // Create a sorted pair key to avoid duplicate checking - const pairKey = [key1, key2].sort().join('|'); + // Create a sorted pair key to avoid duplicate checking. JSON.stringify + // keeps the encoding injective — a plain `join('|')` would let keys that + // themselves contain the separator collide (e.g. `("||","_")` and + // `("|","_|")` both map to `_|||`), silently dropping a real warning. + const pairKey = JSON.stringify([key1, key2].sort()); if (processedPairs.has(pairKey)) continue; processedPairs.add(pairKey); diff --git a/packages/cli/test/property/core/detectInconsistentNaming.property.test.ts b/packages/cli/test/property/core/detectInconsistentNaming.property.test.ts new file mode 100644 index 00000000..d63cba51 --- /dev/null +++ b/packages/cli/test/property/core/detectInconsistentNaming.property.test.ts @@ -0,0 +1,136 @@ +import { describe, test, expect } from 'vitest'; +import fc from 'fast-check'; +import { detectInconsistentNaming } from '../../../src/core/detectInconsistentNaming.js'; + +/** + * Property-based ("fuzz") tests for the inconsistent-naming detector. + * + * Two keys are "inconsistently named" when they are equal after lower-casing and + * stripping underscores, yet not identical (e.g. `API_KEY` vs `APIKEY`). These + * tests throw thousands of random key sets at the detector — heavy on the + * characters that create near-collisions (case, underscores, and the `|` + * separator the dedup key is built from) — to prove it never throws, only + * reports genuine inconsistencies (soundness), reports every one of them + * (completeness), and does not depend on the order the keys arrive in. + * Property-based testing is also what OpenSSF Scorecard recognises as fuzzing + * for JS/TS. + */ + +/** Reference normalizer: the exact notion of "same key" the detector uses. */ +const norm = (k: string) => k.toLowerCase().replace(/_/g, ''); + +/** Stable, collision-free key for an unordered pair of strings. */ +const pairId = (a: string, b: string) => JSON.stringify([a, b].sort()); + +/** + * Keys drawn from a tiny alphabet on purpose: short strings over {a,A,_,|} + * maximise both genuine inconsistencies and delimiter collisions in the dedup + * key (`|` is the separator a naive `join('|')` would have used). + */ +const trickyKey = fc.stringMatching(/^[aA_|]{0,8}$/); + +/** A more realistic env-style key generator. */ +const envKey = fc.stringMatching(/^[A-Za-z][A-Za-z0-9_]{0,10}$/); + +// Weight heavily toward the tricky alphabet so collisions are actually hit. +const keyArb = fc.oneof( + { weight: 4, arbitrary: trickyKey }, + { weight: 1, arbitrary: envKey }, +); + +/** All genuinely-inconsistent unordered pairs among the distinct input keys. */ +function expectedPairs(keys: string[]): Set { + const uniq = [...new Set(keys.filter((k) => k))]; + const pairs = new Set(); + for (let i = 0; i < uniq.length; i++) { + for (let j = i + 1; j < uniq.length; j++) { + const a = uniq[i]!; + const b = uniq[j]!; + if (a !== b && norm(a) === norm(b)) pairs.add(pairId(a, b)); + } + } + return pairs; +} + +describe('detectInconsistentNaming (property-based)', () => { + test('never throws on arbitrary key arrays', () => { + fc.assert( + fc.property(fc.array(fc.string({ unit: 'binary' })), (keys) => { + detectInconsistentNaming(keys); + }), + { numRuns: 2000 }, + ); + }); + + test('soundness: every warning is a genuine, distinct inconsistency', () => { + fc.assert( + fc.property(fc.array(keyArb, { maxLength: 12 }), (keys) => { + const set = new Set(keys.filter((k) => k)); + for (const w of detectInconsistentNaming(keys)) { + expect(w.key1).not.toBe(w.key2); + expect(norm(w.key1)).toBe(norm(w.key2)); + expect(set.has(w.key1)).toBe(true); + expect(set.has(w.key2)).toBe(true); + } + }), + { numRuns: 3000 }, + ); + }); + + test('completeness: every distinct inconsistent pair is reported exactly once', () => { + fc.assert( + fc.property(fc.array(keyArb, { maxLength: 12 }), (keys) => { + const out = detectInconsistentNaming(keys); + const reported = out.map((w) => pairId(w.key1, w.key2)); + // No duplicate unordered pairs. + expect(new Set(reported).size).toBe(reported.length); + // Reported set equals the expected set. + expect(new Set(reported)).toEqual(expectedPairs(keys)); + }), + { numRuns: 15000 }, + ); + }); + + test('order-independence: the set of reported pairs ignores input order', () => { + fc.assert( + fc.property( + fc.array(keyArb, { maxLength: 12 }), + fc.array(fc.integer()), + (keys, seed) => { + // Deterministic shuffle driven by the generated seed array. + const shuffled = keys + .map((k, i) => ({ k, s: seed[i] ?? i })) + .sort((a, b) => a.s - b.s) + .map((x) => x.k); + const a = new Set( + detectInconsistentNaming(keys).map((w) => pairId(w.key1, w.key2)), + ); + const b = new Set( + detectInconsistentNaming(shuffled).map((w) => + pairId(w.key1, w.key2), + ), + ); + expect(b).toEqual(a); + }, + ), + { numRuns: 15000 }, + ); + }); + + test('suggestion is always one of the two keys, and the underscored one when unambiguous', () => { + fc.assert( + fc.property(fc.array(keyArb, { maxLength: 12 }), (keys) => { + for (const w of detectInconsistentNaming(keys)) { + expect([w.key1, w.key2]).toContain(w.suggestion); + const has1 = w.key1.includes('_'); + const has2 = w.key2.includes('_'); + if (has1 !== has2) { + // Exactly one side is snake_case → that side must be suggested. + expect(w.suggestion.includes('_')).toBe(true); + } + } + }), + { numRuns: 3000 }, + ); + }); +}); diff --git a/packages/cli/test/unit/core/detectInconsistentNaming.test.ts b/packages/cli/test/unit/core/detectInconsistentNaming.test.ts index f8118e02..f1742bfb 100644 --- a/packages/cli/test/unit/core/detectInconsistentNaming.test.ts +++ b/packages/cli/test/unit/core/detectInconsistentNaming.test.ts @@ -67,8 +67,7 @@ describe('detectInconsistentNaming', () => { it('skips undefined keys in the array', () => { const keys = ['API_KEY', undefined, 'APIKEY', undefined] as ( - | string - | undefined + string | undefined )[]; const warnings = detectInconsistentNaming(keys as string[]); @@ -93,6 +92,17 @@ describe('detectInconsistentNaming', () => { expect(warnings[0]!.suggestion).toContain('API_KEY'); }); + it('does not drop a real inconsistency when keys contain the dedup separator', () => { + // Regression: with a `join('|')` dedup key, the pair ("||","_") and the + // pair ("|","_|") collided on "_|||", so the genuine "|"/"_|" inconsistency + // (both normalise to "|") was silently skipped. + const keys = ['||', '|', '_|', '_']; + const warnings = detectInconsistentNaming(keys); + + const pairs = warnings.map((w) => [w.key1, w.key2].sort()); + expect(pairs).toContainEqual(['_|', '|']); + }); + it('avoids duplicate warnings when same key appears multiple times', () => { const keys = ['API_KEY', 'APIKEY', 'API_KEY']; const warnings = detectInconsistentNaming(keys);