util: fix parseEnv handling of invalid lines #57798

AugustinMauroy · 2025-04-08T18:05:32Z

This PR fixes an issue with util.parseEnv() where invalid lines in the input were being incorrectly concatenated into key names instead of being skipped. The fix ensures that lines without an equals sign are properly skipped during parsing

Fixes: #56775

I lost my old pr due to a git failure 😅

#56778

nodejs-github-bot · 2025-04-08T18:05:37Z

Review requested:

@nodejs/config

test/parallel/test-dotenv-edge-cases.js

src/node_dotenv.cc

anonrig · 2025-04-08T18:37:40Z

src/node_dotenv.cc

@@ -257,13 +285,13 @@ void Dotenv::ParseContent(const std::string_view input) {
        // Example: KEY=value # comment
        // The value pair should be `value`
        if (hash_character != std::string_view::npos) {
-          value = content.substr(0, hash_character);
+          value = value.substr(0, hash_character);


Why did you make this change? Please add a test that covers this.

src/node_dotenv.cc

codecov · 2025-04-08T18:54:29Z

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (1540fc6) to head (bae0194).
Report is 87 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_dotenv.cc	81.81%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57798      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         630      630              
  Lines      185288   185294       +6     
  Branches    36344    36341       -3     
==========================================
- Hits       167203   167202       -1     
- Misses      11006    11017      +11     
+ Partials     7079     7075       -4

Files with missing lines	Coverage Δ
src/node_dotenv.cc	`84.53% <81.81%> (+0.93%)`	⬆️

... and 27 files with indirect coverage changes

🚀 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.

AugustinMauroy · 2025-04-08T20:26:25Z

I had added some test case for parseEnv to handle all trimming. So we can analyse if behavior is right

nodejs-github-bot · 2025-04-11T17:33:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/66203/ 💛

jasnell · 2025-04-12T17:52:46Z

src/node_dotenv.cc

+// Returns an empty string_view if the input is empty.
+// Example:
+//   trim_spaces("  hello  ") -> "hello"
+//   trim_spaces("") -> ""


Not new to this PR but it occurs to me that this function overlooks the often-overlooked vertical tab (0x0B).

I maybe can help on that, but I don't know what your are talking about

nodejs-github-bot · 2025-04-21T06:46:46Z

Landed in 64d9b7c

PR-URL: #57798 Fixes: #56775 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>

util: fix parseEnv handling of invalid lines

83f47a6

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Apr 8, 2025