Skip to content

app: use explicit ValueError instead of assert in projects_dir_validate (fixes #5403)#5448

Open
adityachilka1 wants to merge 1 commit into
platformio:developfrom
adityachilka1:fix/projects-dir-validate-no-assert
Open

app: use explicit ValueError instead of assert in projects_dir_validate (fixes #5403)#5448
adityachilka1 wants to merge 1 commit into
platformio:developfrom
adityachilka1:fix/projects-dir-validate-no-assert

Conversation

@adityachilka1

Copy link
Copy Markdown

Fixes #5403.

The bug

platformio/app.py:

def projects_dir_validate(projects_dir):
    assert os.path.isdir(projects_dir)
    return os.path.abspath(projects_dir)

Python drops assert statements under -O (and when the PYTHONOPTIMIZE
environment variable is set). On production images and distro packages that run
Python in optimised mode, any string - even a fake path - gets through the
validator untouched. The error then surfaces downstream (e.g. when the projects
directory is later globbed), where it is harder to diagnose as a config issue.

The fix

Replace the assert with an explicit raise ValueError. One-line diff:

 def projects_dir_validate(projects_dir):
-    assert os.path.isdir(projects_dir)
+    if not os.path.isdir(projects_dir):
+        raise ValueError("Not a directory: %s" % projects_dir)
     return os.path.abspath(projects_dir)

Why ValueError

Caller (sanitize_setting, platformio/app.py:168) wraps any exception from
validators into exception.InvalidSettingValue via raise ... from exc. So
the user-facing error path is identical. ValueError is idiomatic for
"argument has the correct type but wrong value".

Tests

Adds tests/misc/test_app.py with three cases:

Verification (local)

python -O -c "from platformio import app; app.projects_dir_validate('/nope')"
# Traceback (most recent call last):
# ...
# ValueError: Not a directory: /nope

Also verified that the pre-fix code (replicated in a sandbox) silently returned
an invalid path under -O.

No behavioural regression

  • Normal (non-optimised) runs: previously assert raised AssertionError
    (a subclass of Exception) which sanitize_setting caught. Now raises
    ValueError which it still catches. No user-facing difference.
  • Optimised (-O) runs: was a silent pass-through. Now correctly raises.

Related

The CONTRIBUTING.md requires a CLA. I'm filing this as an individual contributor
(@adityachilka1) and will sign the CLA through CLA-assistant when the bot prompts.

…te (fixes platformio#5403)

`projects_dir_validate()` used `assert os.path.isdir(pr)] to reject
invalid paths. Python drops all `assert` statements under `-O`
(and the PYTHONOPTIMIZE envvar), so invalid `projects_dir` values were
silently accepted in optimised builds and the failure later surfaced
in unrelated codepaths.

Raise `ValueError` explicitly so the check survives `-O`. The caller
(`sanitize_setting()`) catches any exception from a validator and
converts it to `InvalidSettingValue`, so the user-facing error path is
unchanged.

Adds `tests/misc/test_app.py` with three cases:
  * existing dir returns the absolute path
  * nonexistent path raises ValueError
  * file (not-a-dir) raises ValueError
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

fix: validation relies on assert, which is stripped in optimized python mode

3 participants