util: fix parseEnv handling of invalid lines #56778

AugustinMauroy · 2025-01-26T23:39:33Z

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

anonrig

Nice work!

Even though this test pass, I strongly recommend adding as much as comment possible while simplifying your code. Our parser is already complex (due to not using a regex) and it would help a lot if we know why such decision is taken in the parser.

test/parallel/test-dotenv-invalid-syntax.js

src/node_dotenv.cc

anonrig · 2025-01-27T00:49:35Z

src/node_dotenv.cc

        continue;
+      } else {
+        break;


Please add a comment here. Explain it as much as you can. Possibly with examples so it is more readable/understandable to reviewers/contributors.

codecov · 2025-01-27T01:28:14Z

Codecov Report

Attention: Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (ea7ab16) to head (db57145).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_dotenv.cc	85.71%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56778      +/-   ##
==========================================
- Coverage   89.24%   89.22%   -0.03%     
==========================================
  Files         662      663       +1     
  Lines      191919   191990      +71     
  Branches    36911    36921      +10     
==========================================
+ Hits       171284   171297      +13     
- Misses      13525    13566      +41     
- Partials     7110     7127      +17

Files with missing lines	Coverage Δ
src/node_dotenv.cc	`92.85% <85.71%> (+0.07%)`	⬆️

... and 44 files with indirect coverage changes

AugustinMauroy · 2025-01-27T18:28:08Z

Yup yagizz, I had tried to apply yours suggestions. I'm reaching my limit in C++. So I hope that this time if it's good.

anonrig · 2025-01-27T18:59:29Z

src/node_dotenv.cc

    if (content.front() == '\n' || content.front() == '#') {
      auto newline = content.find('\n');
      if (newline != std::string_view::npos) {
        content.remove_prefix(newline + 1);
+        // Trim spaces after skipping comments or empty lines to handle
+        // cases where there might be trailing whitespace


Can you give an example to this statement?

Not sure but:

Suggested change

// cases where there might be trailing whitespace

// cases where there might be trailing whitespace

// Example: " # comment\n KEY=value"

anonrig · 2025-01-27T19:00:55Z

src/node_dotenv.cc

+    // Example:
+    //   # This is a comment
+    //
+    //   KEY=value
    if (content.front() == '\n' || content.front() == '#') {
      auto newline = content.find('\n');


If content.front() is \n this will always return 0 (the first character). I wonder why we did it like this...

We can simply replace line 144 with if (content.front() == '\n') and remove the content.find('\n') check I guess...

src/node_dotenv.cc

anonrig · 2025-01-27T19:03:00Z

src/node_dotenv.cc

+    auto pos = content.find_first_of("=\n");
+
+    // If we found nothing or found a newline before equals, the line is invalid
+    if (pos == std::string_view::npos || content[pos] == '\n') {


Suggested change

if (pos == std::string_view::npos || content[pos] == '\n') {

if (pos == std::string_view::npos || content.at(pos) == '\n') {

what is diff bettween [xxx] and .at(xxx) ?

anonrig · 2025-01-27T19:03:59Z

src/node_dotenv.cc

-    if (equal == std::string_view::npos) {
+    // Find the next equals sign or newline in a single pass
+    // This optimizes the search by avoiding multiple iterations
+    auto pos = content.find_first_of("=\n");


Can we change the variable name to be something more readable?

Suggested change

auto pos = content.find_first_of("=\n");

auto equal_or_newline = content.find_first_of("=\n");

anonrig · 2025-01-27T19:05:16Z

src/node_dotenv.cc

    if (key.empty()) {
+      auto newline = content.find('\n');
+      if (newline != std::string_view::npos) {
+        content.remove_prefix(newline + 1);


if newline is the last character, this will literally clear the content and we don't need to call trim_spaces afterwards.

anonrig · 2025-01-27T19:07:45Z

src/node_dotenv.cc

    }

-    // SAFETY: Content is guaranteed to have at least one character
+    // If content is empty after the equals sign, store empty value and break


This comment is unneccessary. It explains the code, not how it got end up here. I recommend this article: https://swimm.io/learn/code-collaboration/comments-in-code-best-practices-and-mistakes-to-avoid

Specifically:

Explain the reasoning behind the code, not just what the code is doing.

anonrig · 2025-01-27T19:08:24Z

src/node_dotenv.cc

      store_.insert_or_assign(std::string(key), "");
      break;
    }

-    // Expand new line if \n it's inside double quotes
-    // Example: EXPAND_NEWLINES = 'expand\nnew\nlines'
+    // Handle different types of value formats (quoted, multi-line, etc)


Can you revert this code change. The previous comment was better for understanding where we are at the parser.

anonrig · 2025-01-27T19:08:57Z

src/node_dotenv.cc

      if (closing_quote == std::string_view::npos) {
-        // Check if newline exist. If it does, take the entire line as the value
-        // Example: KEY="value\nKEY2=value2


Can you revert comments for the changes that are unrelated to your code? This change for example doesn't make things better.

anonrig · 2025-01-27T19:09:40Z

Yup yagizz, I had tried to apply yours suggestions. I'm reaching my limit in C++. So I hope that this time if it's good.

I see lots of changes unrelated to your PR. Can you revert everything that is unrelated to your change?

util: fix parseEnv handling of invalid lines

4ca51e8

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 26, 2025

tools: fix lint and format issues

8dd566f

AugustinMauroy force-pushed the util-fix-parseEnv-handling-of-invalid-lines branch from de3dd86 to 8dd566f Compare January 26, 2025 23:53

anonrig requested changes Jan 27, 2025

View reviewed changes

AugustinMauroy added 3 commits January 27, 2025 18:59

test: put everythings on test-dotenv-edge-cases.js

7a89bdb

src: add more inline docs

f12d4a1

src: update logic based on feedback

ddf3b76

test: remove useless changes

65e42e3

anonrig requested changes Jan 27, 2025

View reviewed changes

AugustinMauroy added 3 commits January 28, 2025 10:50

src: clean + revert useless changes

ba372fb

doc: add example

f1c07c7

update from feedback

db57145

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: fix parseEnv handling of invalid lines #56778

util: fix parseEnv handling of invalid lines #56778

AugustinMauroy commented Jan 26, 2025

anonrig left a comment

anonrig Jan 27, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading

AugustinMauroy commented Jan 27, 2025

anonrig Jan 27, 2025

AugustinMauroy Jan 28, 2025

anonrig Jan 27, 2025

anonrig Jan 27, 2025

anonrig Jan 27, 2025

AugustinMauroy Jan 28, 2025

anonrig Jan 27, 2025

anonrig Jan 27, 2025

anonrig Jan 27, 2025

AugustinMauroy Jan 28, 2025

anonrig Jan 27, 2025

anonrig Jan 27, 2025

anonrig commented Jan 27, 2025

	// cases where there might be trailing whitespace
	// cases where there might be trailing whitespace
	// Example: " # comment\n KEY=value"

	if (pos == std::string_view::npos \|\| content[pos] == '\n') {
	if (pos == std::string_view::npos \|\| content.at(pos) == '\n') {

	auto pos = content.find_first_of("=\n");
	auto equal_or_newline = content.find_first_of("=\n");

util: fix parseEnv handling of invalid lines #56778

Are you sure you want to change the base?

util: fix parseEnv handling of invalid lines #56778

Conversation

AugustinMauroy commented Jan 26, 2025

anonrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

AugustinMauroy commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonrig commented Jan 27, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading