src: improve node::Dotenv trimming #56983

dario-piotrowicz · 2025-02-09T22:13:39Z

the trimming functionality that the dotenv parsing uses currently only takes into consideration plain spaces (' '), other type of space characters such as tabs and newlines are not trimmed, this can cause subtle bugs, so the changes here make sure that such characters get trimmed as well

Fixes #56686

src/node_dotenv.cc

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

src/node_dotenv.cc

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

anonrig · 2025-02-10T00:20:56Z

Looking good. After these changes, we can land this pull-request. @dario-piotrowicz

the trimming functionality that the dotenv parsing uses currently only takes into consideration plain spaces (' '), other type of space characters such as tabs and newlines are not trimmed, this can cause subtle bugs, so the changes here make sure that such characters get trimmed as well Co-authored-by: Yagiz Nizipli <[email protected]>

dario-piotrowicz · 2025-02-10T00:43:26Z

@anonrig thank you very much for your review and help here 🙏 (and sorry for the silly mistakes 😓)

I've applied all your suggested changes 🙂

nodejs-github-bot · 2025-02-10T01:36:09Z

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

codecov · 2025-02-10T01:48:29Z

Codecov Report

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

Project coverage is 89.13%. Comparing base (d1f8ccb) to head (c261fb4).
Report is 24 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56983      +/-   ##
==========================================
- Coverage   89.13%   89.13%   -0.01%     
==========================================
  Files         665      665              
  Lines      193167   193177      +10     
  Branches    37190    37193       +3     
==========================================
+ Hits       172185   172192       +7     
- Misses      13731    13743      +12     
+ Partials     7251     7242       -9

Files with missing lines	Coverage Δ
src/node_dotenv.cc	`91.25% <60.00%> (-1.53%)`	⬇️

... and 27 files with indirect coverage changes

anonrig · 2025-02-10T01:56:37Z

@dario-piotrowicz the tests are failing

remove pos_start == pos_end check

dario-piotrowicz · 2025-02-10T09:56:48Z

@dario-piotrowicz the tests are failing

Sorry about that, I've addressed it 🙂 (e3ee074)

The issue was this new check:

if (pos_start == pos_end) {
  return "";
}

since if pos_start is the same as pos_end that shouldn't result in an empty string but in a single character at that position (input.at(post_start))

I removed the check completely (instead of returning input.at(pos_start)) since when pos_start and post_end are the same return input.substr(pos_start, pos_end - pos_start + 1); should work just fine (as it would resolve to return input.substr(pos_start, 1); meaning that it'd take a single character starting from pos_start)

do you agree?

PS: that's not the only issue 🤔

add check for when no value is present

nodejs-github-bot · 2025-02-10T13:01:57Z

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

jasnell · 2025-02-10T15:00:48Z

src/node_dotenv.cc

-  if (input.front() == ' ') {
-    input.remove_prefix(input.find_first_not_of(' '));
+
+  auto pos_start = input.find_first_not_of(" \t\n");


should this also include \r and \f?

sounds good to me 🙂, but I'm not sure how to test that, would you be ok with addition without dedicated tests for it?

nodejs-github-bot · 2025-02-11T22:26:52Z

Landed in 43ffcf1

the trimming functionality that the dotenv parsing uses currently only takes into consideration plain spaces (' '), other type of space characters such as tabs and newlines are not trimmed, this can cause subtle bugs, so the changes here make sure that such characters get trimmed as well Co-authored-by: Yagiz Nizipli <[email protected]> PR-URL: #56983 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

the trimming functionality that the dotenv parsing uses currently only takes into consideration plain spaces (' '), other type of space characters such as tabs and newlines are not trimmed, this can cause subtle bugs, so the changes here make sure that such characters get trimmed as well Co-authored-by: Yagiz Nizipli <[email protected]> PR-URL: nodejs#56983 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

the trimming functionality that the dotenv parsing uses currently only takes into consideration plain spaces (' '), other type of space characters such as tabs and newlines are not trimmed, this can cause subtle bugs, so the changes here make sure that such characters get trimmed as well Co-authored-by: Yagiz Nizipli <[email protected]> PR-URL: #56983 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 Feb 9, 2025

dario-piotrowicz force-pushed the dario/56686/spaces-tabs-in-env-file branch 6 times, most recently from 3dba125 to ff0ffe4 Compare February 9, 2025 22:52

anonrig reviewed Feb 9, 2025

View reviewed changes

src/node_dotenv.cc Outdated Show resolved Hide resolved

test/parallel/test-dotenv-edge-cases.js Outdated Show resolved Hide resolved

src/node_dotenv.cc Show resolved Hide resolved

dario-piotrowicz force-pushed the dario/56686/spaces-tabs-in-env-file branch 2 times, most recently from b5dd12f to 00d9449 Compare February 10, 2025 00:14

dario-piotrowicz requested a review from anonrig February 10, 2025 00:14

anonrig reviewed Feb 10, 2025

View reviewed changes

src/node_dotenv.cc Outdated Show resolved Hide resolved

src/node_dotenv.cc Show resolved Hide resolved

src/node_dotenv.cc Outdated Show resolved Hide resolved

test/parallel/test-dotenv-edge-cases.js Outdated Show resolved Hide resolved

dario-piotrowicz force-pushed the dario/56686/spaces-tabs-in-env-file branch from 1eb6236 to eabaf64 Compare February 10, 2025 00:36

dario-piotrowicz force-pushed the dario/56686/spaces-tabs-in-env-file branch from eabaf64 to 47d1a76 Compare February 10, 2025 00:36

anonrig approved these changes Feb 10, 2025

View reviewed changes

anonrig 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 Feb 10, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 10, 2025

fixup! src: improve node::Dotenv trimming

e3ee074

remove pos_start == pos_end check

fixup! src: improve node::Dotenv trimming

c261fb4

add check for when no value is present

dario-piotrowicz force-pushed the dario/56686/spaces-tabs-in-env-file branch from 6d32733 to c261fb4 Compare February 10, 2025 10:37

anonrig approved these changes Feb 10, 2025

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 10, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 10, 2025

anonrig requested a review from jasnell February 10, 2025 13:38

jasnell reviewed Feb 10, 2025

View reviewed changes

jasnell approved these changes Feb 10, 2025

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2025

nodejs-github-bot merged commit 43ffcf1 into nodejs:main Feb 11, 2025
57 checks passed

dario-piotrowicz deleted the dario/56686/spaces-tabs-in-env-file branch February 12, 2025 00:11

dario-piotrowicz mentioned this pull request Feb 12, 2025

test: remove unnecessary assert requiring from tests #57008

Merged

github-actions bot mentioned this pull request Feb 25, 2025

2025-02-26, Version 23.9.0 (Current) #57207

Merged

aduh95 mentioned this pull request Apr 11, 2025

2025-04-23, Version 22.15.0 'Jod' (LTS), @UlisesGascon prepared by @RafaelGSS #57840

Merged

Uh oh!

src: improve node::Dotenv trimming #56983

src: improve node::Dotenv trimming #56983

Uh oh!

Conversation

dario-piotrowicz commented Feb 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anonrig commented Feb 10, 2025

Uh oh!

dario-piotrowicz commented Feb 10, 2025

Uh oh!

nodejs-github-bot commented Feb 10, 2025

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anonrig commented Feb 10, 2025

Uh oh!

dario-piotrowicz commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 10, 2025

Uh oh!

jasnell Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 10, 2025 •

edited

Loading

dario-piotrowicz commented Feb 10, 2025 •

edited

Loading