Skip to content

util: fix parseEnv handling of invalid lines - #57798

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
AugustinMauroy:util-fix-parseEnv-handling-of-invalid-lines
Apr 21, 2025
Merged

util: fix parseEnv handling of invalid lines#57798
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
AugustinMauroy:util-fix-parseEnv-handling-of-invalid-lines

Conversation

@AugustinMauroy

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-botnodejs-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
Comment threadtest/parallel/test-dotenv-edge-cases.js
Comment threadsrc/node_dotenv.cc Outdated
Comment threadsrc/node_dotenv.cc Outdated
Comment threadsrc/node_dotenv.cc
// 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);

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.

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

Comment threadsrc/node_dotenv.cc
Comment threadsrc/node_dotenv.cc
Comment threadsrc/node_dotenv.cc
Comment threadsrc/node_dotenv.cc
Comment threadsrc/node_dotenv.cc
Comment threadsrc/node_dotenv.cc
Comment threadsrc/node_dotenv.cc
@codecov

codecovBot commented Apr 8, 2025

Copy link
Copy Markdown

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 linesPatch %Lines
src/node_dotenv.cc81.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 linesCoverage Δ
src/node_dotenv.cc84.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

Copy link
Copy Markdown
MemberAuthor

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

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

nodejs-github-bot commented Apr 11, 2025

Copy link
Copy Markdown
Collaborator

Comment threadsrc/node_dotenv.cc
// Returns an empty string_view if the input is empty.
// Example:
// trim_spaces(" hello ") -> "hello"
// trim_spaces("") -> ""

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.

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

@marco-ippolitomarco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2025
@marco-ippolitomarco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 21, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 21, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 64d9b7c into nodejs:mainApr 21, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 64d9b7c

@AugustinMauroy
AugustinMauroy deleted the util-fix-parseEnv-handling-of-invalid-lines branch April 26, 2025 09:40
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 19, 2025
PR-URL: #57798Fixes: #56775
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@ghostghost mentioned this pull request Jun 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.configIssues or PRs related to the config subsystemneeds-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

util.parseEnv creates keys from invalid, newline-separated lines

6 participants

@AugustinMauroy@nodejs-github-bot@jasnell@anonrig@aduh95@marco-ippolito