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

Removed proxy variable feature. (#60) #148

Closed
wants to merge 1 commit into from
Closed

Removed proxy variable feature. (#60) #148

wants to merge 1 commit into from

Conversation

mrogaski
Copy link

@mrogaski mrogaski commented Oct 15, 2017

Expanding variables automatically on a read is an anti-pattern.
Variable expansion by the shell should only be done when the
value is inserted into the environment, but the vlaue should be
treated as opaque data. Any processing or interpretation of the
variable should be done by the appliaction, not by the access
method.

Expanding variables automatically on a read is an anti-pattern.
Variable expansion by the shell should only be done when the
value is inserted inot the environment, but the vlaue should be
treated as opaque data.  Any processing or interpretation of the
variable should be done by the appliaction, not by the access
method.
@coveralls
Copy link

coveralls commented Oct 15, 2017

Coverage Status

Coverage decreased (-0.09%) to 88.342% when pulling d5afbb2 on mrogaski:bugfix/60_proxy_var into 628ed38 on joke2k:develop.

@nwjlyons
Copy link

nwjlyons commented Nov 1, 2017

FYI I added a way to make this optional #145

@mrogaski
Copy link
Author

mrogaski commented Nov 1, 2017

Interpolation of environment variables on read is a very risky behavior. Even if there's a valid use case for it (which is debatable), it should be disabled by default.

@joke2k joke2k added duplicate This issue or pull request already exists Edit and removed duplicate This issue or pull request already exists Edit labels Jun 5, 2018
@joke2k joke2k added this to the Road to 0.4.5 milestone Jun 5, 2018
@joke2k
Copy link
Owner

joke2k commented Jun 5, 2018

Thank you

A lot of people use the proxy values, so we can't remove without a lot of issues of backports.
But I understand your point.
We should use the quoting to catch or not the proxy value, for example:

# .end file:
# FOO="$BRIAN"
# BAR='$BRIAN'
# ZAP=$BRIAN
# BRIAN=brian

# if quoted the proxy is not evaluated
>>> print env('FOO'), env('BAR')
('$BRIAN', '$BRIAN')
>>> print env('ZAP')
brian

To do that we should edit read_env() method adding a dict that records if a variable is quoted or not.
Then in get_value() we can check if this variable is not quoted and finally apply the proxy solution.

See #145

@joke2k joke2k added the help wanted Extra attention is needed label Jun 5, 2018
@joke2k joke2k added the ci/cd CI/CD related stuff label Jun 5, 2018
@mrogaski
Copy link
Author

mrogaski commented Jun 6, 2018

This is a reasonable approach. Thank you.

@fdemmer
Copy link
Contributor

fdemmer commented Feb 9, 2019

another aspect of this, that i'd consider broken with proxy variables are default values and that would not be improved with the suggested approach. consider the following test:

self.assertEqual('$@u#c4w=%k', self.env('not_present', default='$@u#c4w=%k'))

here the environment variable not_present does not exist and the default value happens to start with a $. this is assumed to be a "proxy variable" and looked up (using the same value as default again), which leads to an infinite recursion.

came across this issue when using django's get_random_secret_key() for a safe default:

SECRET_KEY = env.str('SECRET_KEY', default=get_random_secret_key())

it seems to me like a feature, that does more harm, than good and should be explicitly enabled; would suggest a semantic major version update with breaking change.

@mrogaski
Copy link
Author

mrogaski commented Feb 9, 2019

@fdemmer The SECRET_KEY example is what got me looking at this in the first place. ;)

@kov
Copy link

kov commented Mar 26, 2021

I just got hit by this and was very confused. The secret key that got generated for me started with a $. Took me a while to figure it out because I had not yet read about proxy variables on the docs. And I am now a little more confused about the state of this issue, since both this and #145 are both still open I assume the only fix for my case is to change my SECRET_KEY? This may be fine for new setups but may make it impossible for me to deploy django-environ on some projects where variables will necessarily have values starting with $...

@sergeyklay sergeyklay removed the ci/cd CI/CD related stuff label Aug 31, 2021
@sergeyklay sergeyklay removed this from the Road to 0.4.5 milestone Sep 1, 2021
@sergeyklay sergeyklay deleted the branch joke2k:develop September 4, 2021 12:00
@sergeyklay sergeyklay closed this Sep 4, 2021
@sergeyklay
Copy link
Collaborator

PR was closed by mistake. I can't reopen it since the repository that submitted this PR has been deleted. I'll review this PR and give feedback soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants