Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

util: fix parseEnv incorrectly splitting multiple ‘=‘ in value #57421

Merged
merged 6 commits into from
Apr 6, 2025
Merged

util: fix parseEnv incorrectly splitting multiple ‘=‘ in value #57421

merged 6 commits into from
Apr 6, 2025

Conversation

rayark1
Copy link
Contributor

@rayark1 rayark1 commented Mar 12, 2025

Previously, parseEnv would create multiple environment variables if a single line contained multiple ‘=‘ characters (e.g. A=B=C would become { A: ‘B=C’, B: ‘C’ }).
This commit ensures that only the first ‘=‘ is used as the key-value delimiter, and the rest of the line is treated as the value.

Fixes: #57411

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config
  • @nodejs/performance

@nodejs-github-bot 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 Mar 12, 2025
@rayark1
util: fix parseEnv incorrectly splitting multiple ‘=‘ in value
be4ee4d 
Previously, parseEnv would create multiple environment
variables if a single line contained multiple ‘=‘ characters
(e.g. A=B=C would become { A: ‘B=C’, B: ‘C’ }).
This commit ensures that only the first ‘=‘ is used as
the key-value delimiter, and the rest of the line is treated
as the value.

Fixes: nodejs#57411
@codecov Codecov
Copy link

codecov bot commented Mar 12, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (c3ed292) to head (1ac38ad).
Report is 163 commits behind head on main.

Files with missing lines Patch % Lines
src/node_dotenv.cc 75.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57421      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         630      629       -1     
  Lines      185219   184859     -360     
  Branches    36248    36215      -33     
==========================================
- Hits       167140   166811     -329     
+ Misses      11048    11018      -30     
+ Partials     7031     7030       -1
Files with missing lines Coverage Δ
src/node_dotenv.cc 83.59% <75.00%> (-2.12%) ⬇️

... and 61 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.

@Armanidashh

This comment was marked as off-topic.

Sign in to view
rayark1 and others added 2 commits March 14, 2025 04:14
anonrig
anonrig reviewed Mar 13, 2025
View reviewed changes
@aduh95
Copy link
Contributor

aduh95 commented Mar 14, 2025

Also, we should keep a test case for the unquoted version A=B=C, either to verify it throws, or whatever behavior that we can get consensus on.

@rayark1 rayark1 force-pushed the fix/57411 branch 2 times, most recently from c7267a5 to 255c299 Compare March 14, 2025 18:37
@rayark1
Copy link
Contributor Author

rayark1 commented Mar 23, 2025

I have incorporated all the review comments and added the corresponding test cases. Please let me know if there are any further suggestions or improvements needed.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 23, 2025
mcollina
mcollina approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 5, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 5, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/57421
✔  Done loading data for nodejs/node/pull/57421
----------------------------------- PR info ------------------------------------
Title      util: fix parseEnv incorrectly splitting multiple ‘=‘ in value (#57421)
Author     HEESEUNG <[email protected]> (@rayark1)
Branch     rayark1:fix/57411 -> nodejs:main
Labels     c++, author ready, needs-ci, config
Commits    6
 - util: fix parseEnv incorrectly splitting multiple ‘=‘ in value
 - Update test/fixtures/dotenv/valid.env
 - Update benchmark/fixtures/valid.env
 - refactor: apply comment
 - test: add test case
 - add: test case for DOUBLE_QUOTES
Committers 2
 - Heeseung <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/57421
Fixes: https://github.com/nodejs/node/issues/57411
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/57421
Fixes: https://github.com/nodejs/node/issues/57411
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 12 Mar 2025 05:31:31 GMT
   ✔  Approvals: 2
   ✔  - Daniel Lemire (@lemire): https://github.com/nodejs/node/pull/57421#pullrequestreview-2679120207
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/57421#pullrequestreview-2722574029
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-04-05T17:46:19Z: https://ci.nodejs.org/job/node-test-pull-request/66056/
- Querying data for job/node-test-pull-request/66056/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 57421
From https://github.com/nodejs/node
 * branch                  refs/pull/57421/merge -> FETCH_HEAD
✔  Fetched commits as 214e3d4affab..1ac38ad56bfb
--------------------------------------------------------------------------------
[main 2c84c9c79c] util: fix parseEnv incorrectly splitting multiple ‘=‘ in value
 Author: Heeseung <[email protected]>
 Date: Tue Mar 11 11:15:13 2025 -0700
 5 files changed, 33 insertions(+), 8 deletions(-)
[main de0c2f0a1a] Update test/fixtures/dotenv/valid.env
 Author: HEESEUNG <[email protected]>
 Date: Fri Mar 14 04:14:01 2025 +0900
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 0b421493f7] Update benchmark/fixtures/valid.env
 Author: HEESEUNG <[email protected]>
 Date: Fri Mar 14 04:14:10 2025 +0900
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 162d087172] refactor: apply comment
 Author: Heeseung <[email protected]>
 Date: Thu Mar 13 21:28:56 2025 -0700
 3 files changed, 9 insertions(+), 13 deletions(-)
[main f7e1303268] test: add test case
 Author: Heeseung <[email protected]>
 Date: Fri Mar 14 11:25:53 2025 -0700
 4 files changed, 8 insertions(+)
[main e4f0160a43] add: test case for DOUBLE_QUOTES
 Author: Heeseung <[email protected]>
 Date: Mon Mar 17 08:57:43 2025 -0700
 1 file changed, 1 insertion(+)
   ✔  Patches applied
There are 6 commits in the PR. Attempting autorebase.
Rebasing (2/12)
Executing: git node land --amend --yes
   ⚠  Found Fixes: https://github.com/nodejs/node/issues/57411, skipping..
--------------------------------- New Message ----------------------------------
util: fix parseEnv incorrectly splitting multiple ‘=‘ in value

Previously, parseEnv would create multiple environment

variables if a single line contained multiple ‘=‘ characters

(e.g. A=B=C would become { A: ‘B=C’, B: ‘C’ }).

This commit ensures that only the first ‘=‘ is used as

the key-value delimiter, and the rest of the line is treated

as the value.


Fixes: #57411

PR-URL: #57421

Reviewed-By: Daniel Lemire <[email protected]>

Reviewed-By: Matteo Collina <[email protected]>


[detached HEAD 22bb0e7ac2] util: fix parseEnv incorrectly splitting multiple ‘=‘ in value

Author: Heeseung <[email protected]>

Date: Tue Mar 11 11:15:13 2025 -0700

5 files changed, 33 insertions(+), 8 deletions(-)

Rebasing (3/12)

Rebasing (4/12)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update test/fixtures/dotenv/valid.env


Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #57421

Fixes: #57411

Reviewed-By: Daniel Lemire <[email protected]>

Reviewed-By: Matteo Collina <[email protected]>


[detached HEAD 6f945c6411] Update test/fixtures/dotenv/valid.env

Author: HEESEUNG <[email protected]>

Date: Fri Mar 14 04:14:01 2025 +0900

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (5/12)

Rebasing (6/12)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update benchmark/fixtures/valid.env


Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #57421

Fixes: #57411

Reviewed-By: Daniel Lemire <[email protected]>

Reviewed-By: Matteo Collina <[email protected]>


[detached HEAD 0d93b43c2f] Update benchmark/fixtures/valid.env

Author: HEESEUNG <[email protected]>

Date: Fri Mar 14 04:14:10 2025 +0900

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (7/12)

Rebasing (8/12)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

refactor: apply comment


PR-URL: #57421

Fixes: #57411

Reviewed-By: Daniel Lemire <[email protected]>

Reviewed-By: Matteo Collina <[email protected]>


[detached HEAD ced1bf15e9] refactor: apply comment

Author: Heeseung <[email protected]>

Date: Thu Mar 13 21:28:56 2025 -0700

3 files changed, 9 insertions(+), 13 deletions(-)

Rebasing (9/12)

Rebasing (10/12)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

test: add test case


PR-URL: #57421

Fixes: #57411

Reviewed-By: Daniel Lemire <[email protected]>

Reviewed-By: Matteo Collina <[email protected]>


[detached HEAD 5257bc7670] test: add test case

Author: Heeseung <[email protected]>

Date: Fri Mar 14 11:25:53 2025 -0700

4 files changed, 8 insertions(+)

Rebasing (11/12)

Rebasing (12/12)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

add: test case for DOUBLE_QUOTES


PR-URL: #57421

Fixes: #57411

Reviewed-By: Daniel Lemire <[email protected]>

Reviewed-By: Matteo Collina <[email protected]>


[detached HEAD cbf1234b3d] add: test case for DOUBLE_QUOTES

Author: Heeseung <[email protected]>

Date: Mon Mar 17 08:57:43 2025 -0700

1 file changed, 1 insertion(+)

Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/14286187364

@aduh95 aduh95 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. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 6, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 6, 2025
@nodejs-github-bot nodejs-github-bot merged commit e6a0d77 into nodejs:main Apr 6, 2025
75 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e6a0d77

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

@lemire lemire lemire approved these changes

@jasnell jasnell jasnell left review comments

@anonrig anonrig anonrig left review comments

@aduh95 aduh95 aduh95 left review comments

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs 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-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. config Issues or PRs related to the config subsystem 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 additional environment variables when equal sign is in value
8 participants
@rayark1 @nodejs-github-bot @Armanidashh @aduh95 @mcollina @lemire @jasnell @anonrig