Skip to content

fix(dotenv): handle values with both quote types and escape sequences#7139

Open
algojogacor wants to merge 1 commit into
denoland:mainfrom
algojogacor:fix/dotenv-stringify-parse-escaping
Open

fix(dotenv): handle values with both quote types and escape sequences#7139
algojogacor wants to merge 1 commit into
denoland:mainfrom
algojogacor:fix/dotenv-stringify-parse-escaping

Conversation

@algojogacor
Copy link
Copy Markdown

Why

The stringify and parse functions in @std/dotenv fail to losslessly round-trip string values that contain both single and double quotes, newlines with quotes, or backslashes with quotes. This makes it impossible to store JSON strings or other complex values in .env files.

Fixes #7055

What was wrong

stringify:

  1. Backslashes in double-quoted values were not escaped, making \" (escaped quote) ambiguous — the parser sees " as end of string
  2. Newlines were only escaped in double-quote mode, but values with newlines and no quotes fell into single-quote mode where \n is literal

parse:

  1. The regex for double-quoted values ((?:.|\r\n|\n)*?) matched any character including unescaped ", causing early termination
  2. The expandCharacters function did not un-escape \" or \\, so escaped quotes/backslashes survived round-tripping as literals

What this PR does

stringify.ts:

  • Escape backslashes BEFORE newlines in double-quoted mode (order matters: \\\\\, then \n\\n)
  • Use double quotes when the value contains newlines so escape sequences can be expanded back

parse.ts:

  • Tighten regex from (?:.|\r\n|\n)*? to (?:[^\"\\]|\\.)* — only match non-quote-non-backslash chars OR backslash + any char
  • Add \"" and \\\ to expandCharacters for proper round-tripping

Verification

All 16 character-combination round-trip tests from the original issue now pass, plus 9 additional edge case tests covering JSON strings, mixed quotes, newlines, and literal backslash sequences.

The stringify and parse functions fail to losslessly round-trip values
that contain both single and double quotes, or newlines with quotes.

Root cause: stringify did not escape backslashes in double-quoted values,
making escape sequences ambiguous. Parse did not handle `"` or `\\`
escape sequences within double-quoted strings.

Changes:
- stringify: escape backslashes before newlines in double-quoted values
- stringify: use double quotes for values with newlines (needed for expansion)
- parse: tighten regex for double-quoted values to handle escape sequences
- parse: add `"` and `\\` to expandCharacters for proper unescaping

Fixes: denoland#7055
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Copy Markdown
Contributor

@lunadogbot lunadogbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is red — two failures, one of which is a regression in parse_test.ts:145 (PRIVATE_KEY_DOUBLE_QUOTED):

  1. The new KEY_VALUE_REGEXP interpolated capture (?:[^"\\]|\\.)* is greedy, and [^"\\] matches \n — so it swallows the trailing newline that the old *? + trailing \r?\n?" used to strip. The multi-line RSA key fixture now parses with an extra \n at the end. Either keep the capture non-greedy ((?:[^"\\]|\\.)*?) or exclude \r/\n from the char class and handle real newlines separately, so the trailing \r?\n? before " can still trim.

  2. stringify_test.ts:96 ("handles backslash with double quotes") asserts BS="test\\\\\\"value" but the code emits BS='test\\"value'. Single-quote mode is actually fine here: the value has " but no ' or \n, single-quoted dotenv values are literal, and it round-trips. The test expectation is wrong, not the code — drop the assertion or pick an input that forces double-quote mode (e.g. value containing ').

The parse_test.ts round-trip test step "value with backslash and quotes" passes because String.raw\test"value`contains no'or\n, so it also goes through single-quote mode — which never exercises the new \/"expansion inexpandCharacters. Worth adding a round-trip case with a '` in the value so the double-quoted escape path is actually covered.

Copy link
Copy Markdown
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this — the issue is real and the overall direction (escaped \"/\\ in double-quoted values + matching escape on stringify) is correct.

However, the PR currently does not pass deno test dotenv/. Two failures locally on 953506a:

parse() => ./dotenv/parse_test.ts:11:6
  AssertionError: Private Key Double Quoted
  -   -----END DSA PRIVATE KEY-----\n
  -   
  +   -----END DSA PRIVATE KEY-----

stringify() ... handles backslash with double quotes => ./dotenv/stringify_test.ts:96:11
  AssertionError
  -   BS='test\\"value'
  +   BS="test\\\\\\"value"

One is an existing-test regression, one is your own new test failing. Both have the same root cause and are explained in the inline comments. Please fix and re-run deno test dotenv/ before the next push.

A broader concern worth raising in the issue/PR thread: this is a behavioral change in how parse() interprets existing .env files. Previously \\ inside double quotes was preserved literally; after this PR it collapses to \. Files in the wild written against the old behavior will now decode differently. Probably acceptable as a bug fix, but worth calling out explicitly so users aren't surprised.

Comment thread dotenv/parse.ts

const KEY_VALUE_REGEXP =
/^\s*(?:export\s+)?(?<key>[^\s=#]+?)\s*=[\ \t]*('\r?\n?(?<notInterpolated>(.|\r\n|\n)*?)\r?\n?'|"\r?\n?(?<interpolated>(.|\r\n|\n)*?)\r?\n?"|(?<unquoted>[^\r\n#]*)) *#*.*$/gm;
/^\s*(?:export\s+)?(?<key>[^\s=#]+?)\s*=[\ \t]*('\r?\n?(?<notInterpolated>(?:.|\r\n|\n)*?)\r?\n?'|"\r?\n?(?<interpolated>(?:[^"\\]|\\.)*)\r?\n?"|(?<unquoted>[^\r\n#]*)) *#*.*$/gm;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression here. Switching the inner pattern from non-greedy (.|\r\n|\n)*? to greedy (?:[^"\\]|\\.)* breaks PRIVATE_KEY_DOUBLE_QUOTED in testdata/.env.test.

In JS regex, [^"\\] is a character class — it matches \n by default. So the greedy * happily consumes the trailing newline inside the quotes, leaving nothing for the outer \r?\n? to strip. Parsed value ends with an extra \n.

Two possible fixes:

  • Keep the new shape but exclude newlines from the negated class: (?:[^"\\\r\n]|\\.|\r\n|\n)* (and keep it non-greedy, or restructure so the outer \r?\n? still wins).
  • Or use (?:[^"\\]|\\.)*? (non-greedy) so the outer \r?\n? can still match the trailing newline.

Also a smaller edge case: \\. won't cross an actual \n (the . doesn't match newlines without the s flag), so a literal backslash immediately before a real newline inside a double-quoted value will fail to parse. Unlikely in practice but worth a test.

Comment thread dotenv/stringify.ts
const hasDoubleQuote = escapedValue.includes('"');

// Use double quotes when the value contains newlines (so they can be
// expanded back) or single quotes (which are safe inside double quotes).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what causes your "handles backslash with double quotes" test to fail.

For a value containing " and \ but no ' or \n (e.g. String.raw\test\"value`), neither hasNewlinenorhasSingleQuoteis true, so we fall into this branch and pick'. The single-quote branch doesn't escape backslashes — round-trip happens to work only because parse()reads the single-quoted group *without*expandCharacters. But the stringified form is 'test\"value'`, not the double-quoted form your test asserts.

If you want the double-quoted form (matching the test), the condition needs to also trigger when the value contains " and \ (so escaping is meaningful). Otherwise, drop the failing test and keep the simpler single-quote routing. Either way, the code and tests need to agree.

Comment thread dotenv/stringify_test.ts
assertEquals(
stringify({ BS: String.raw`test\"value` }),
`BS="test\\\\\\"value"`,
));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing test — see the comment on stringify.ts. Once you've decided the desired routing for values with only " + \, either update this assertion or change the branch condition so it matches.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dotenv: Cannot stringify and parse JSON strings or values with quotes and apostrophes or newlines

4 participants