Skip to content

checks: Mark justified partial-path subprocess calls as nosec B607 - #7713

Merged
cwhite911 merged 3 commits into
OSGeo:mainfrom
cwhite911:bandit-skip-b607
Sep 6, 2026
Merged

checks: Mark justified partial-path subprocess calls as nosec B607#7713
cwhite911 merged 3 commits into
OSGeo:mainfrom
cwhite911:bandit-skip-b607

Conversation

@cwhite911

Copy link
Copy Markdown
Contributor

Skips 15 Bandit B607 (start_process_with_partial_path, note severity) code scanning alerts by adding B607 to the [tool.bandit] skips list in pyproject.toml.

All flagged sites invoke tools by bare name (git, gh, mdl, msgfmt) from developer and build-time helpers, not from runtime code handling untrusted input:

  • utils/mkdocs.py, utils/generate_release_notes.py, utils/generate_last_commit_file.py, utils/md_isvalid.py (doc build and release tooling)
  • locale/grass_po_stats.py (translation stats)
  • scripts/g.manual, python/grass/gunittest/reporters.py, python/grass/pygrass/modules/grid

GRASS resolves executables through PATH by design (using an absolute path would actually break portability across install layouts). B607 is the natural companion of B603 (subprocess_popen_with_shell_equals_false / untrusted input), which the project already skips (#3839); this follows the same rationale and the project's established preference for category skips over per-line # nosec.

Verified with bandit 1.9.4: the flagged directories report the B607 findings before the change and none after.

This is part of a larger effort to work through the open code scanning alerts, grouped into small PRs by issue type.

Written with the assistance of Claude Code.

@github-actions github-actions Bot added the Python Related code is in Python label Jul 14, 2026
@echoix

echoix commented Jul 14, 2026

Copy link
Copy Markdown
Member

The way you added the exclude here means that we will not be able to catch if a new valid violation of this rule is introduced, effectively removing the tool. It would be better for justified exclusions to write them inline in the code that uses it, so that even if a new violation in the same file appears, it should be caught.

@echoix echoix left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment

Address review feedback: instead of skipping Bandit B607 globally
(which would also stop catching newly introduced violations), suppress
it inline at each existing call site. These are developer, build, and
test helpers that invoke known tools (git, gh, svn, msgfmt, mdl, man,
grass) by name, intentionally resolved through PATH, the same rationale
as the already-skipped B603.

The g.region call in pygrass grid is addressed separately in the
shell=True cleanup (OSGeo#7703).
@cwhite911 cwhite911 changed the title checks: Skip Bandit B607 for partial executable paths checks: Mark justified partial-path subprocess calls as nosec B607 Jul 15, 2026
@cwhite911

cwhite911 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

I've reworked this to drop the global pyproject.toml skip and instead add an inline # nosec B607 at each existing call site.

The suppressed calls are developer, build, and test helpers that run known tools (git, gh, svn, msgfmt, mdl, man, grass) by name, intentionally resolved through PATH, the same rationale as the already-skipped B603.

One site, the g.region call in pygrass/modules/grid/grid.py, is left to #7703, which replaces it with grass.script.core.Popen and removes the finding there; adding a suppression here would just conflict with that PR.

@cwhite911 cwhite911 self-assigned this Jul 15, 2026
@cwhite911
cwhite911 requested a review from echoix July 15, 2026 21:19
Name the specific tool in each suppression and state why the partial
path is permanent: these are fixed external tools (git, gh, svn,
svnversion, msgfmt, mdl, grass) with no portable absolute path, and
g.manual launches the user's own system man. PATH resolution is
required by design, so the exclusion cannot become stale.
@cwhite911

cwhite911 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@echoix see below.

Is the justification permanent? Yes. Every suppressed call runs a fixed, hardcoded tool name (git, gh, svn, svnversion, msgfmt, mdl, grass, and man). None of these has a portable absolute path, they live in different places across Linux distros, macOS (Homebrew vs system), conda envs, Windows, etc., so resolving them through PATH is a hard requirement, not a temporary shortcut. That property can't change: we can never hardcode an absolute path here without breaking portability. And because the executable is a literal (not built from input), the call can't silently turn into "run an attacker-controlled program"; if someone did change it to a variable, B607 would fire again since the suppression is scoped to B607 on that single line only (other Bandit checks on the same line still run).

By context these are also not runtime-with-untrusted-input paths:

  • utils/* (mkdocs.py, generate_release_notes.py, generate_last_commit_file.py, md_isvalid.py, coverage_mapper.py) and locale/grass_po_stats.py, developer, doc-build, and release tooling run from the repo/CI.
  • python/grass/gunittest/reporters.py, records the working-copy revision (svn) during test runs.
  • scripts/g.manual (os.execlp("man", ...)), a shipped tool that intentionally launches the user's own system man via PATH (using an absolute path would be wrong here).

Wording, agreed it was too generic. I've updated each comment to name the specific tool and state the durable reason, e.g. # nosec B607: fixed external tool "git" with no portable absolute path and # nosec B607: runs the user's system "man" (PATH lookup intended).

If you'd still prefer an active resolution over a suppression for the non-shipped tooling, I can switch those to shutil.which(...), though it's the same PATH trust with extra ceremony and no added security. Happy to go that route if you think it reads better.

@cwhite911
cwhite911 enabled auto-merge (squash) September 6, 2026 01:38
@cwhite911
cwhite911 merged commit a36e184 into OSGeo:main Sep 6, 2026
27 of 28 checks passed
@github-actions github-actions Bot added this to the 8.6.0 milestone Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

general libraries module Python Related code is in Python translation Message translation related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants