GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — util: fix parseEnv handling of invalid lines by AugustinMauroy · Pull Request #57798 · nodejs/node

Conversation

@AugustinMauroy

@AugustinMauroy AugustinMauroy commented Apr 8, 2025

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and mertcanaltin reacted with rocket emoji
All reactions
  • 🚀 2 reactions
@AugustinMauroy
util: fix parseEnv handling of invalid lines
83f47a6
@nodejs-github-bot

nodejs-github-bot commented Apr 8, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/config
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues and PRs related to Node.js configuration and feature settings. needs-ci PRs that need a full CI run. labels Apr 8, 2025
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread test/parallel/test-dotenv-edge-cases.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/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);

@anonrig anonrig Apr 8, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Apr 8, 2025
View reviewed changes
Comment thread src/node_dotenv.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@codecov

codecov Bot commented Apr 8, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AugustinMauroy
util(parseEnv): add more test cases
bae0194
@AugustinMauroy

AugustinMauroy commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Apr 10, 2025
View reviewed changes
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 10, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 10, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Apr 11, 2025 •
edited by jasnell
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Apr 12, 2025
View reviewed changes
Comment thread src/node_dotenv.cc
// Returns an empty string_view if the input is empty.
// Example:
// trim_spaces(" hello ") -> "hello"
// trim_spaces("") -> ""

@jasnell jasnell Apr 12, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AugustinMauroy AugustinMauroy Apr 12, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell approved these changes Apr 12, 2025
View reviewed changes
@AugustinMauroy
AugustinMauroy requested a review from aduh95 April 18, 2025 09:47
marco-ippolito
marco-ippolito approved these changes Apr 21, 2025
View reviewed changes
@marco-ippolito marco-ippolito added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Apr 21, 2025
@marco-ippolito marco-ippolito added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Apr 21, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 21, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 64d9b7c into nodejs:main Apr 21, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 21, 2025

Copy link
Copy Markdown
Collaborator

Landed in 64d9b7c
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@AugustinMauroy @RafaelGSS
util: fix parseEnv handling of invalid lines
ab5ae9e 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
2025-05-06, Version 24.0.0 (Current) #57609
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@AugustinMauroy @RafaelGSS
util: fix parseEnv handling of invalid lines
7e3503f 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@AugustinMauroy @aduh95
util: fix parseEnv handling of invalid lines
e880614 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@AugustinMauroy @aduh95
util: fix parseEnv handling of invalid lines
8540ee7 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@AugustinMauroy @RafaelGSS
util: fix parseEnv handling of invalid lines
8ed12b0 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@AugustinMauroy @aduh95
util: fix parseEnv handling of invalid lines
b10f0d9 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@AugustinMauroy @aduh95
util: fix parseEnv handling of invalid lines
ab9f0dc 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@AugustinMauroy @aduh95
util: fix parseEnv handling of invalid lines
c648dd1 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@github-actions github-actions Bot mentioned this pull request May 19, 2025
2025-05-21, Version 22.16.0 'Jod' (LTS) #58388
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@AugustinMauroy @aduh95
util: fix parseEnv handling of invalid lines
2df7ce9 
PR-URL: #57798
Fixes: #56775
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@ghost ghost mentioned this pull request Jun 8, 2025
### Notable Changes autofix-ci/action#28
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Aug 9, 2026
chore: lighten e18e-flagged dependencies aaditagrawal/openmtp#1
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@anonrig anonrig anonrig approved these changes
@marco-ippolito marco-ippolito marco-ippolito approved these changes
@aduh95 aduh95 Awaiting requested review from aduh95

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. config Issues and PRs related to Node.js configuration and feature settings. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@AugustinMauroy @nodejs-github-bot @jasnell @anonrig @aduh95 @marco-ippolito
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.