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.

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

Bugs in parser around unquoted whitespace and newlines in v1.5.0 #204

Closed
andreynering opened this issue Feb 4, 2023 · 10 comments
Closed

Comments

@andreynering
Copy link

Hi there!

I'd just like to let you know that the following, which worked on v1.4.0, now breaks in v1.5.0:

MY_ENV=foo bar

It's possible to fix by wrapping into quotes, though:

MY_ENV="foo bar"

As there isn't any official dotenv specification (AFAIK), I'm not sure if the former is supposed to work in the first place, but it caught my attention and I decided to wait until at least we have a decision if this is going to be addressed or not before upgrading, because it can end up breaking for my users.

@joho
Copy link
Owner

joho commented Feb 4, 2023

It's definitely unintended, in that it wasn't an explicit decision, though I'd say it was "undefined" behaviour.

I'll go check what ruby/node do, and we'll do the same as them.

@joho
Copy link
Owner

joho commented Feb 5, 2023

Yep, so I've gone and tested against ruby. This is a regression, will fix and get a 1.5.1 out

@joho joho changed the title Possible unintended breaking change on v1.5.0: white space is not allowed anymore if unquoted Bug in parser around unquoted whitespace and newlines Feb 5, 2023
@joho joho changed the title Bug in parser around unquoted whitespace and newlines Bugs in parser around unquoted whitespace and newlines in v1.5.0 Feb 5, 2023
@joho
Copy link
Owner

joho commented Feb 5, 2023

Yeah I did a pretty ordinary job code reviewing the multiline PR and it's pulled a few regressions in, some in previously "undefined" behaviour, and some in tests that weren't updated to exercise the newer code paths.

Fix might be this week, might drag out a bit, got a hectic schedule at the moment.

@budimanjojo
Copy link

budimanjojo commented Feb 5, 2023

I'm wondering if my issue is the same, because I'm using yaml format for the test, and most people don't put " for yaml value. This test string:

env1: value1
env2: "true"
env3: 123
default: default value

using godotenv.Unmarshal(testFile)
resulted in nothing is being set at all.

    envsubst_test.go:59: got , want value1       
    envsubst_test.go:59: got , want true         
    envsubst_test.go:59: got , want 123          
    envsubst_test.go:59: got , want default value

@joho joho pinned this issue Feb 5, 2023
@joho
Copy link
Owner

joho commented Feb 5, 2023

Yeah, I suspect it's the exact same issue. I'll put in a test case on my branch that covers that in yaml style explicitly.

@joho
Copy link
Owner

joho commented Feb 5, 2023

Yep, definitely is, here's some test output from my new cases

--- FAIL: TestTrailingNewlines (0.00s)
    --- FAIL: TestTrailingNewlines/Value_with_internal_whitespace_with_trailing_newline (0.00s)
        godotenv_test.go:566: Input: "KEY=value value\n" Unexpected error:	"unexpected character \"\\n\" in variable name near \"value\\n\""
        godotenv_test.go:569: Input "KEY=value value\n" Expected:	 "KEY"/"value value"
            Got:	 map["KEY":"value"]
    --- FAIL: TestTrailingNewlines/YAML_style_-_value_with_internal_whitespace_without_trailing_newline (0.00s)
        godotenv_test.go:569: Input "KEY: value value" Expected:	 "KEY"/"value value"
            Got:	 map["":"value" "KEY":"value"]
    --- FAIL: TestTrailingNewlines/YAML_style_-_value_with_internal_whitespace_with_trailing_newline (0.00s)
        godotenv_test.go:566: Input: "KEY: value value\n" Unexpected error:	"unexpected character \"\\n\" in variable name near \"value\\n\""
        godotenv_test.go:569: Input "KEY: value value\n" Expected:	 "KEY"/"value value"
            Got:	 map["KEY":"value"]
    --- FAIL: TestTrailingNewlines/Value_with_internal_whitespace_without_trailing_newline (0.00s)
        godotenv_test.go:569: Input "KEY=value value" Expected:	 "KEY"/"value value"
            Got:	 map["":"value" "KEY":"value"]

@budimanjojo
Copy link

budimanjojo commented Feb 5, 2023

I think it's this line:

godotenv/parser.go

Lines 115 to 123 in b311b26

if !hasPrefix {
// unquoted value - read until whitespace
end := bytes.IndexFunc(src, unicode.IsSpace)
if end == -1 {
return expandVariables(string(src), vars), nil, nil
}
return expandVariables(string(src[0:end]), vars), src[end:], nil
}

Not sure what will happen if we just remove that check, is there a usecase where unquoted value can't have space? I think for both .yaml and .env format we only need to quote for "newline(\n)" and not " "?

@joho
Copy link
Owner

joho commented Feb 5, 2023

OK I think I've cracked the nut of it.

For that specific bug I had to change the rules to

  • look for the end of the line
  • work backwards to see if there was a valid end-of-line comment (and cut it)
  • then strip what was left over

I also found and fixed some bugs in the key finding/naming and sorted those at the same time.

Will merge tomorrow and re-release.

@andreynering
Copy link
Author

Thanks for the quick response and action!

joho added a commit that referenced this issue Feb 5, 2023
* Add tests to cover the regression reported in #204

* Add a comment on regex for clarity

* Remove some old code that wasn't doing anything

* Push _all_ parse code into the parser and get tests calling live code

* Add some newline specific tests

* Add some YAML tests for the newline/space split bug

* Fix incorrect terminating of lines on whitespace

* Fix most of the parser regressions

* Bring back FOO.BAR names

* remove some commented out code
@joho
Copy link
Owner

joho commented Feb 5, 2023

https://github.com/joho/godotenv/releases/tag/v1.5.1

@joho joho closed this as completed Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants