env2bool matches its truthy pattern as a substring, so unrelated values read as true
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 15.9k
- Forks
- 1.3k
- PR merge metrics
- No merged PRs in 30d
Description
Bug Report
dvc.utils.env2bool decides truthiness with an unanchored re.search:
def env2bool(var, undefined=False):
var = os.getenv(var, None)
if var is None:
return undefined
return bool(re.search("1|y|yes|true", var, flags=re.IGNORECASE))
Because search matches anywhere in the string, any value that merely contains 1, y, yes or true is treated as true — including values that plainly aren't boolean:
'1' -> True # intended
'yes' -> True # intended
'true' -> True # intended
'0' -> False # intended
'no' -> False # intended
'false' -> False # intended
'off' -> False # intended
'my_path' -> True # contains "y"
'anything' -> True # contains "y"
'only' -> True # contains "y"
'v1.0' -> True # contains "1"
So a variable set to a path, a branch name, a version string, or anything else containing a y or a 1 silently reads as enabled.
Impact
env2bool is used for user-facing environment variables, so the input really is arbitrary user text:
DVC_EXP_AUTO_PUSH—dvc/repo/experiments/executor/base.pyDVC_SQLALCHEMY_ECHO—dvc/database.pyDVC_IGNORE_ISATTY—dvc/progress.pyDVC_TEST—dvc/analytics.py,dvc/repo/experiments/push.py,dvc/updater.py
The most likely way to hit it is a value meant to be negative that happens to contain a matching letter (deny, nay, not_yet all read as true), or someone putting a non-boolean value in one of these by mistake and getting the feature switched on rather than an error or a sensible default.
It's also asymmetric in a confusing way: no is correctly false, but nay is true.
Expected
Only a recognised affirmative value should be true — i.e. match the whole string, not a substring. Something like
return var.strip().lower() in {"1", "y", "yes", "true"}
or re.fullmatch(r"1|y|yes|true", var.strip(), flags=re.IGNORECASE) if you'd rather keep the regex. Note that with fullmatch the yes alternative is still needed (it isn't reachable via y any more), which is presumably why search looked sufficient originally.
Reproduce
import os
from dvc.utils import env2bool
os.environ["DVC_EXP_AUTO_PUSH"] = "my_branch"
print(env2bool("DVC_EXP_AUTO_PUSH")) # True
Environment
DVC main (56e5982), Python 3.11.9, Windows.
Happy to send a PR with the fullmatch/set-membership version and a test if you agree with the direction — flagging first since tightening this is technically a behaviour change for anyone currently relying on a loose value.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at dvc.utils.env2bool and reproduce the issue with the DVC_EXP_AUTO_PUSH environment variable and a value such as my_branch. Check the existing utility test coverage, then ensure only whole, recognised affirmative values return true while unrelated values do not. Add a regression test for the substring case and run the relevant utility tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- devtools
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100