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

bug: 0.16 doesn't pass current shell environment variables to command #1876

Closed
garry-harthill-cko opened this issue Feb 3, 2025 · 13 comments · Fixed by #1879
Closed

bug: 0.16 doesn't pass current shell environment variables to command #1876

garry-harthill-cko opened this issue Feb 3, 2025 · 13 comments · Fixed by #1879
Labels

Comments

@garry-harthill-cko
Copy link

garry-harthill-cko commented Feb 3, 2025

Describe the Bug

Shells environment variables aren't set in the context of the command that asdf execs

Steps to Reproduce

Set an environment variable that the command requires

Run the command

Application displays an error about missing environment variable

Expected Behaviour

I would expect the parent shell environment to be passed to the exec'd binary.

Actual Behaviour

The command doesn't recognise environment variables.

However manually running asdf env terraform echo $SOME_ENVAR does actually show the contents of the variable.

Environment

OS:
Darwin MAC-G6WN34MH7H 23.6.0 Darwin Kernel Version 23.6.0: Fri Nov 15 15:13:15 PST 2024; root:xnu-10063.141.1.702.7~1/RELEASE_ARM64_T6000 arm64

SHELL:
zsh 5.9 (x86_64-apple-darwin23.0)

BASH VERSION:
5.2.37(1)-release

ASDF VERSION:
v0.16.0

ASDF INTERNAL VARIABLES:
ASDF_DEFAULT_TOOL_VERSIONS_FILENAME=.tool-versions
ASDF_DATA_DIR=/Users/user/.asdf
ASDF_CONFIG_FILE=/Users/user/.asdfrc

ASDF INSTALLED PLUGINS:
terraform https://github.com/asdf-community/asdf-hashicorp.git c44c314b220f36790bbee4f6869e16a95ac36bfc

asdf plugins affected (if relevant)

No response

@garry-harthill-cko
Copy link
Author

garry-harthill-cko commented Feb 3, 2025

After some digging it appeared that is was just when the environment variable value has additional =. eg:
MYVAR=some things = with = in it

The culprit is likely in execenv.SliceToMap:

	for _, envVar := range env {
		varValue := strings.Split(envVar, "=")
		if len(varValue) == 2 { // <<<< silently ignoring it when there's more `=` than 1
			envMap[varValue[0]] = varValue[1]
		}
	}

This seems to fix it for me. But i'm not sure of the implications of this change:

// SliceToMap converts an env map to env slice suitable for syscall.Exec
func SliceToMap(env []string) map[string]string {
	envMap := map[string]string{}

	for _, envVar := range env {
		varValue := strings.Split(envVar, "=")
		if len(varValue) >= 2 {
			envMap[varValue[0]] = strings.Join(varValue[1:], "=")
		}
	}

	return envMap
}

@Stratus3D
Copy link
Member

Hi @garry-harthill-cko , thanks for the additional info! I was testing this myself and confirmed again that env vars in general do work:

$ export MYVAR="some string val"

$ asdf exec irb
irb(main):001> ENV['MYVAR']
=> "some string val"
irb(main):002>

But as you've identified the env var will not be set if the value of it contains =.

$ export MYVAR="some string=val"

asdf exec irb
irb(main):001> ENV['MYVAR']
=> nil
irb(main):002> 

And of course it gets traced back to my custom env var parsing code that I wrote so I could work with env vars as a map rather than a slice. I'll get this fixed and get a patch release out this week. I might try to find a library that does this as I imagine with env vars and quoting issues there may be other bugs lurking in this function.

@Stratus3D Stratus3D changed the title bug: 0.16 doesn't pass current shell environment variables to comman bug: 0.16 doesn't pass current shell environment variables to command Feb 3, 2025
Stratus3D added a commit that referenced this issue Feb 4, 2025
The old code did not allow the equals sign in the environment variable
value. But the equals sign is a valid character in value.

Fixes #1876
@Stratus3D
Copy link
Member

@garry-harthill-cko I believe I've fixed this in #1879 just now. Would you be able to build latest master and see if the issue is fixed for you?

@leiicamundi
Copy link

Thanks @Stratus3D
Do you have any plan to publish a patch version?

@Stratus3D
Copy link
Member

Yes, today or tomorrow hopefully. A fix for #1860 also needs to be included.

@leiicamundi
Copy link

Thank you for this great tool

@garry-harthill-cko
Copy link
Author

Yes this fixes it. Thanks!

@Stratus3D
Copy link
Member

Awesome! Thank you for confirming. I plan to release the patch in version 0.16.1 tonight.

@NickNeck
Copy link

@Stratus3D thank you for your work.
Environment variables with multiple lines are also broken:

 $> echo $MYVAR
one
two
three
 $> asdf exec irb
irb(main):001:0> ENV['MYVAR']
=> "one"
irb(main):002:0>

I thought it would fit with this issue.

@duckworth
Copy link

I am also seeing variables ending in '=" are excluded (again?) and this is very common with base64 credentials:

❯ asdf --version
asdf version 0.16.3

❯ echo $MYVAR
abc=

❯ ruby -e "p ENV['MYVAR']"
nil

@Stratus3D
Copy link
Member

Hi @duckworth. Thanks for reporting. I believe my PR here fixes that - #1977. Can you test it when you get a chance?

@NickNeck interesting. I'm not even sure Go's os.Environ function properly handles environment variable values with newlines in them. I did however add a test here for values with newlines and my execenv.SliceToMap function seems to handle them without a problem - https://github.com/asdf-vm/asdf/pull/1977/files#diff-c633b9b4982e6638a48e2e424beff929ffe0071a8e7a61252cc79580bbe2e0c8R109-R112 But if os.Environ() doesn't give me the right value there isn't much I can do.

@duckworth
Copy link

Hi @duckworth. Thanks for reporting. I believe my PR here fixes that - #1977. Can you test it when you get a chance?

Yes, I just tried #1977 locally and it fixes environment variables with equals sign in value issue.

@NickNeck
Copy link

It works with the main branch. Thank you.

Projects/forks/asdf> echo $MYVAR
one
two
three
Projects/forks/asdf> ./asdf exec iex
iex(1)> System.get_env("MYVAR") |> IO.puts
one
two
three
:ok
iex(2)>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants