treeverse / treeverse/dvc

env2bool matches its truthy pattern as a substring, so unrelated values read as true

Open Beginner friendly
#11,080 0 comments 0 reactions 0 assignees View on GitHub

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_PUSHdvc/repo/experiments/executor/base.py
  • DVC_SQLALCHEMY_ECHOdvc/database.py
  • DVC_IGNORE_ISATTYdvc/progress.py
  • DVC_TESTdvc/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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.