From 106ed2d135ff5c5a98452cbce093d24efcde493e Mon Sep 17 00:00:00 2001 From: Christian Munk-Nissen Date: Fri, 24 Jul 2026 18:09:27 +0200 Subject: [PATCH] fix: false warning in sveltekit rules --- .../sveltekit-lib-server-false-positive.md | 5 + .../cli/src/core/frameworks/sveltekitRules.ts | 5 +- .../property/core/envLine.property.test.ts | 191 +++++++++++++ .../frameworkRules.property.test.ts | 263 ++++++++++++++++++ .../core/frameworks/sveltekitRules.test.ts | 24 ++ 5 files changed, 487 insertions(+), 1 deletion(-) create mode 100644 .changeset/sveltekit-lib-server-false-positive.md create mode 100644 packages/cli/test/property/core/envLine.property.test.ts create mode 100644 packages/cli/test/property/core/frameworks/frameworkRules.property.test.ts diff --git a/.changeset/sveltekit-lib-server-false-positive.md b/.changeset/sveltekit-lib-server-false-positive.md new file mode 100644 index 00000000..abb80ba1 --- /dev/null +++ b/.changeset/sveltekit-lib-server-false-positive.md @@ -0,0 +1,5 @@ +--- +'dotenv-diff': patch +--- + +fix a false positive SvelteKit warning for `process.env` used inside the `$lib/server` (`src/lib/server`) directory. diff --git a/packages/cli/src/core/frameworks/sveltekitRules.ts b/packages/cli/src/core/frameworks/sveltekitRules.ts index b2997541..cc92dd54 100644 --- a/packages/cli/src/core/frameworks/sveltekitRules.ts +++ b/packages/cli/src/core/frameworks/sveltekitRules.ts @@ -29,7 +29,10 @@ export function applySvelteKitRules( // hooks.server.ts / handle.server.ts etc. /\/(hooks|handle)\.server\.(ts|js)$/.test(normalizedFile) || // generic server.ts fallback - /\/server\.(ts|js)$/.test(normalizedFile); + /\/server\.(ts|js)$/.test(normalizedFile) || + // $lib/server (src/lib/server) — SvelteKit's enforced server-only directory, + // never bundled to the client, so process.env there is legitimate. + /(^|\/)lib\/server\//.test(normalizedFile); const isClientFile = !normalizedFile.includes('.server.') && diff --git a/packages/cli/test/property/core/envLine.property.test.ts b/packages/cli/test/property/core/envLine.property.test.ts new file mode 100644 index 00000000..d77a0582 --- /dev/null +++ b/packages/cli/test/property/core/envLine.property.test.ts @@ -0,0 +1,191 @@ +import { describe, test, expect } from 'vitest'; +import fc from 'fast-check'; +import { + stripBom, + splitEnvLines, + parseEnvLine, +} from '../../../src/core/envLine.js'; + +/** + * Property-based ("fuzz") tests for the low-level dotenv line helpers. + * + * `parseEnvLine` / `splitEnvLines` / `stripBom` sit under the whole tool — the + * diff, the duplicate scanner and the expiration reader all build on them — so + * these feed thousands of random inputs (control chars, unicode, stray `=`/`#`, + * BOMs, mixed line endings) to prove they never throw and always uphold their + * documented shape: parsed keys are non-empty and trimmed, values are trimmed, + * splitting round-trips, and re-parsing a parsed line is stable (idempotent). + * Property-based testing is also what OpenSSF Scorecard recognises as fuzzing + * for JS/TS. + */ + +const BOM = ''; + +describe('parseEnvLine (property-based)', () => { + test('never throws on arbitrary unicode / control chars', () => { + fc.assert( + fc.property(fc.string({ unit: 'binary' }), (line) => { + parseEnvLine(line); + }), + { numRuns: 3000 }, + ); + }); + + test('a returned pair always has a non-empty, trimmed, `=`-free key and a trimmed value', () => { + fc.assert( + fc.property(fc.string({ unit: 'binary' }), (line) => { + const parsed = parseEnvLine(line); + if (parsed === null) return; + expect(parsed.key.length).toBeGreaterThan(0); + expect(parsed.key).toBe(parsed.key.trim()); + expect(parsed.key).not.toContain('='); + expect(parsed.value).toBe(parsed.value.trim()); + }), + { numRuns: 3000 }, + ); + }); + + test('blank, whitespace-only and comment lines are always rejected', () => { + const blank = fc.stringMatching(/^\s*$/); + const comment = fc + .tuple(fc.stringMatching(/^\s*$/), fc.string()) + .map(([ws, rest]) => `${ws}#${rest.replace(/[\r\n]/g, ' ')}`); + fc.assert( + fc.property(fc.oneof(blank, comment), (line) => { + expect(parseEnvLine(line)).toBeNull(); + }), + { numRuns: 2000 }, + ); + }); + + test('a line with no `=` or an empty key is rejected', () => { + // No '=' anywhere, and not a comment/blank → must be null. + const noEquals = fc + .string({ unit: 'binary' }) + .filter( + (s) => + !s.includes('=') && s.trim().length > 0 && !s.trim().startsWith('#'), + ); + fc.assert( + fc.property(noEquals, (line) => { + expect(parseEnvLine(line)).toBeNull(); + }), + { numRuns: 2000 }, + ); + }); + + test('re-parsing a parsed line is stable (idempotent)', () => { + fc.assert( + fc.property(fc.string({ unit: 'binary' }), (line) => { + const first = parseEnvLine(line); + if (first === null) return; + const second = parseEnvLine(`${first.key}=${first.value}`); + expect(second).toEqual(first); + }), + { numRuns: 5000 }, + ); + }); + + test('round-trips well-formed KEY=VALUE lines', () => { + const key = fc + .stringMatching(/^[A-Za-z_][A-Za-z0-9_]*$/) + .filter((k) => k.length > 0); + // Trimmed, newline-free values (including the empty value). + const value = fc + .string() + .filter((v) => !/[\r\n]/.test(v)) + .map((v) => v.trim()); + fc.assert( + fc.property(key, value, (k, v) => { + expect(parseEnvLine(`${k}=${v}`)).toEqual({ key: k, value: v }); + }), + { numRuns: 2000 }, + ); + }); + + test('splits only on the first `=`', () => { + const key = fc + .stringMatching(/^[A-Za-z_][A-Za-z0-9_]*$/) + .filter((k) => k.length > 0); + const value = fc + .string() + .filter((v) => !/[\r\n]/.test(v)) + .map((v) => v.trim()) + .filter((v) => v.includes('=')); + fc.assert( + fc.property(key, value, (k, v) => { + const parsed = parseEnvLine(`${k}=${v}`); + expect(parsed).toEqual({ key: k, value: v }); + }), + { numRuns: 1000 }, + ); + }); +}); + +describe('splitEnvLines (property-based)', () => { + test('is total and never returns an empty array', () => { + fc.assert( + fc.property(fc.string({ unit: 'binary' }), (input) => { + const lines = splitEnvLines(input); + expect(Array.isArray(lines)).toBe(true); + expect(lines.length).toBeGreaterThanOrEqual(1); + // No element retains a '\n'. A lone '\r' (classic-Mac ending, not part + // of a '\r\n') is intentionally left intact — split is on /\r?\n/. + for (const l of lines) expect(l.includes('\n')).toBe(false); + }), + { numRuns: 3000 }, + ); + }); + + test('round-trips arrays of newline-free lines joined by \\n', () => { + const lineNoBreaks = fc + .string({ unit: 'binary' }) + .filter((s) => !/[\r\n]/.test(s)); + fc.assert( + fc.property( + fc.array(lineNoBreaks, { minLength: 1, maxLength: 30 }), + (lines) => { + // Guard: a leading BOM on the first element would be stripped. + fc.pre(!lines[0]!.startsWith(BOM)); + expect(splitEnvLines(lines.join('\n'))).toEqual(lines); + }, + ), + { numRuns: 2000 }, + ); + }); + + test('line count equals the number of newline separators plus one', () => { + fc.assert( + fc.property(fc.string({ unit: 'binary' }), (input) => { + const stripped = stripBom(input); + const separators = (stripped.match(/\r?\n/g) ?? []).length; + expect(splitEnvLines(input).length).toBe(separators + 1); + }), + { numRuns: 2000 }, + ); + }); +}); + +describe('stripBom (property-based)', () => { + test('is idempotent and removes at most one leading BOM', () => { + fc.assert( + fc.property(fc.string({ unit: 'binary' }), (input) => { + const once = stripBom(input); + expect(stripBom(once)).toBe(once); + // Only a *leading* BOM is affected; length shrinks by 0 or 1. + expect(input.length - once.length).toBeGreaterThanOrEqual(0); + expect(input.length - once.length).toBeLessThanOrEqual(1); + }), + { numRuns: 3000 }, + ); + }); + + test('strips exactly one BOM when the input starts with one', () => { + fc.assert( + fc.property(fc.string({ unit: 'binary' }), (rest) => { + expect(stripBom(`${BOM}${rest}`)).toBe(rest); + }), + { numRuns: 2000 }, + ); + }); +}); diff --git a/packages/cli/test/property/core/frameworks/frameworkRules.property.test.ts b/packages/cli/test/property/core/frameworks/frameworkRules.property.test.ts new file mode 100644 index 00000000..08db239a --- /dev/null +++ b/packages/cli/test/property/core/frameworks/frameworkRules.property.test.ts @@ -0,0 +1,263 @@ +import { describe, test, expect } from 'vitest'; +import fc from 'fast-check'; +import type { + EnvUsage, + FrameworkWarning, +} from '../../../../src/config/types.js'; +import { applyNextJsRules } from '../../../../src/core/frameworks/nextJsRules.js'; +import { applyNuxtRules } from '../../../../src/core/frameworks/nuxtRules.js'; +import { applySvelteKitRules } from '../../../../src/core/frameworks/sveltekitRules.js'; +import { normalizePath } from '../../../../src/core/helpers/normalizePath.js'; + +/** + * Property-based ("fuzz") tests for the per-framework validation rules. + * + * Each `apply*Rules` function inspects one env-var usage and pushes at most one + * warning. They are branchy (path regexes, prefix checks, a sensitive-keyword + * matcher), so these throw thousands of random usages — odd file paths, unicode + * variables, every access pattern, arbitrary `$env` imports — at all three to + * prove the invariants that must hold whatever the rule decides: they never + * throw, never emit more than one warning per usage, always short-circuit for + * `node_modules`, are deterministic, and every warning they emit is well-formed + * (its variable/line come from the usage, its file is normalised, its framework + * tag is correct). Property-based testing is also what OpenSSF Scorecard + * recognises as fuzzing for JS/TS. + */ + +const PREFIXES = [ + 'NEXT_PUBLIC_', + 'PUBLIC_', + 'VITE_', + 'NUXT_PUBLIC_', + 'NUXT_', + '', +]; +const SUFFIXES = [ + 'SECRET', + 'PRIVATE', + 'PASSWORD', + 'secret', + 'API_KEY', + 'URL', + '', +]; + +const variableArb = fc.oneof( + fc + .tuple(fc.constantFrom(...PREFIXES), fc.constantFrom(...SUFFIXES)) + .map(([p, s]) => `${p}${s}`), + fc.string({ unit: 'binary' }), +); + +/** File paths that deliberately land on (and near) the framework path regexes. */ +const fileArb = fc.oneof( + fc.constantFrom( + 'src/routes/+page.svelte', + 'src/routes/+page.ts', + 'src/routes/+layout.server.ts', + 'src/hooks.server.ts', + 'src/lib/server/db.ts', + 'svelte.config.js', + 'pages/index.tsx', + 'pages/api/users.ts', + 'app/page.tsx', + 'nuxt.config.ts', + 'server/api/foo.ts', + 'components/Foo.server.vue', + 'node_modules/pkg/index.js', + 'a\\b\\pages\\api\\x.ts', + 'weird.server.data/x.ts', + '', + ), + fc.string({ unit: 'binary' }), +); + +const importsArb = fc.option( + fc.subarray([ + '$env/dynamic/private', + '$env/dynamic/public', + '$env/static/private', + '$env/static/public', + '$env/whatever', + ]), + { nil: undefined }, +); + +const usageArb: fc.Arbitrary = fc.record({ + variable: variableArb, + file: fileArb, + line: fc.nat({ max: 100000 }), + column: fc.nat({ max: 100000 }), + pattern: fc.constantFrom( + 'process.env', + 'import.meta.env', + 'sveltekit', + 'vite', + 'docker-compose', + ), + imports: importsArb, + context: fc.string(), + isLogged: fc.option(fc.boolean(), { nil: undefined }), +}) as fc.Arbitrary; + +type Rule = ( + u: EnvUsage, + warnings: FrameworkWarning[], + fileContentMap?: Map, +) => void; + +const RULES: Array<{ name: string; fn: Rule; framework: string }> = [ + { name: 'nextjs', fn: applyNextJsRules, framework: 'nextjs' }, + { name: 'nuxt', fn: applyNuxtRules as Rule, framework: 'nuxt' }, + { + name: 'sveltekit', + fn: applySvelteKitRules as Rule, + framework: 'sveltekit', + }, +]; + +const run = (fn: Rule, u: EnvUsage): FrameworkWarning[] => { + const w: FrameworkWarning[] = []; + fn(u, w); + return w; +}; + +describe.each(RULES)('$name rules (property-based)', ({ fn, framework }) => { + test('never throws on arbitrary usages', () => { + fc.assert( + fc.property(usageArb, (u) => { + run(fn, u); + }), + { numRuns: 4000 }, + ); + }); + + test('emits at most one warning per usage', () => { + fc.assert( + fc.property(usageArb, (u) => { + expect(run(fn, u).length).toBeLessThanOrEqual(1); + }), + { numRuns: 4000 }, + ); + }); + + test('every emitted warning is well-formed', () => { + fc.assert( + fc.property(usageArb, (u) => { + for (const w of run(fn, u)) { + expect(w.variable).toBe(u.variable); + expect(w.line).toBe(u.line); + expect(w.file).toBe(normalizePath(u.file)); + expect(w.framework).toBe(framework); + expect(typeof w.reason).toBe('string'); + expect(w.reason.length).toBeGreaterThan(0); + } + }), + { numRuns: 4000 }, + ); + }); + + test('never warns for files under node_modules', () => { + fc.assert( + fc.property(usageArb, (u) => { + if (normalizePath(u.file).includes('/node_modules/')) { + expect(run(fn, u)).toEqual([]); + } + }), + { numRuns: 4000 }, + ); + }); + + test('is deterministic', () => { + fc.assert( + fc.property(usageArb, (u) => { + expect(run(fn, u)).toEqual(run(fn, u)); + }), + { numRuns: 3000 }, + ); + }); + + test('does not mutate the usage object', () => { + fc.assert( + fc.property(usageArb, (u) => { + const snapshot = JSON.stringify(u); + run(fn, u); + expect(JSON.stringify(u)).toBe(snapshot); + }), + { numRuns: 2000 }, + ); + }); +}); + +describe('applySvelteKitRules — $lib/server is server context (property-based)', () => { + test('process.env in a $lib/server file never triggers the client-usage warning', () => { + const serverPath = fc + .array(fc.stringMatching(/^[a-z]{1,8}$/), { minLength: 0, maxLength: 3 }) + .map((segs) => ['src/lib/server', ...segs, 'mod.ts'].join('/')); + fc.assert( + fc.property(variableArb, serverPath, (variable, file) => { + const u = { + variable, + file, + line: 1, + column: 1, + pattern: 'process.env', + context: '', + } as EnvUsage; + const w: FrameworkWarning[] = []; + applySvelteKitRules(u, w); + expect( + w.some( + (x) => + x.reason === 'process.env should only be used in server files', + ), + ).toBe(false); + }), + { numRuns: 2000 }, + ); + }); +}); + +describe('applyNextJsRules — fileContentMap handling (property-based)', () => { + test('a "use client" file forces the NEXT_PUBLIC_ requirement', () => { + const nonPublicVar = fc + .tuple(fc.constantFrom('', 'FOO_', 'API_'), fc.constantFrom('KEY', 'URL')) + .map(([p, s]) => `${p}${s}`) + .filter((v) => !v.startsWith('NEXT_PUBLIC_')); + // A plain non-route file so only the client-directive drives the result. + fc.assert( + fc.property( + nonPublicVar, + fc.constantFrom('src/a.ts', 'lib/b.tsx'), + (variable, file) => { + const u = { + variable, + file, + line: 1, + column: 1, + pattern: 'process.env', + context: '', + } as EnvUsage; + const map = new Map([[file, '"use client"\nconst x = 1']]); + const w: FrameworkWarning[] = []; + applyNextJsRules(u, w, map); + expect(w).toHaveLength(1); + expect(w[0]!.reason).toBe( + 'Server-only variable accessed from client code', + ); + }, + ), + { numRuns: 1000 }, + ); + }); + + test('never throws with arbitrary file-content maps', () => { + fc.assert( + fc.property(usageArb, fc.string({ unit: 'binary' }), (u, content) => { + const w: FrameworkWarning[] = []; + applyNextJsRules(u, w, new Map([[u.file, content]])); + }), + { numRuns: 2000 }, + ); + }); +}); diff --git a/packages/cli/test/unit/core/frameworks/sveltekitRules.test.ts b/packages/cli/test/unit/core/frameworks/sveltekitRules.test.ts index 32af8178..f9f96a6b 100644 --- a/packages/cli/test/unit/core/frameworks/sveltekitRules.test.ts +++ b/packages/cli/test/unit/core/frameworks/sveltekitRules.test.ts @@ -66,6 +66,30 @@ describe('applySvelteKitRules', () => { expect(warnings).toHaveLength(0); }); + it('allows process.env in $lib/server directory', () => { + // Regression: src/lib/server is SvelteKit's enforced server-only directory, + // so process.env there must not be flagged as client-side usage. + applySvelteKitRules( + { ...baseUsage, file: 'src/lib/server/db.ts' }, + warnings, + ); + expect(warnings).toHaveLength(0); + }); + + it('allows process.env in a nested $lib/server file', () => { + applySvelteKitRules( + { ...baseUsage, file: 'src/lib/server/auth/tokens.ts' }, + warnings, + ); + expect(warnings).toHaveLength(0); + }); + + it('still warns for process.env in a non-server $lib file', () => { + applySvelteKitRules({ ...baseUsage, file: 'src/lib/utils.ts' }, warnings); + expect(warnings).toHaveLength(1); + expect(warnings[0]!.reason).toMatch(/process\.env should only be used/); + }); + it('allows process.env in svelte.config.js', () => { applySvelteKitRules({ ...baseUsage, file: 'svelte.config.js' }, warnings); expect(warnings).toHaveLength(0);