[rplanel]

Written by an AI agent on behalf of @rplanel. @rplanel has not reviewed it yet.

Problem

Galaxy tool wrappers commonly interpolate user-controlled values — collection element identifiers, group_tag and data_column values, free-text parameters — directly into <command> templates. Where parameter sanitization is disabled (<sanitizer sanitize="false"/>, <options sanitize="false"/>) or reconfigured to let shell metacharacters through, such interpolations can allow command injection. Finding these patterns today is a manual, expert-driven review process; nothing in the tool-development toolchain points maintainers at the highest-risk idioms.

What changed

This PR adds an opt-in, informational security linter module, planemo/linters/security.py, with one Linter class per check, following planemo's existing linter conventions:

Class	Check
SecurityRawIdentifier	Raw element_identifier of a dataset/collection interpolated into the command
SecurityRawCollectionName	Collection .name / .keys() used raw in the command
SecurityGroupTag	group_tag param value used in the command (never sanitized by Galaxy)
SecurityDataColumn	data_column param value used in the command (never sanitized)
SecuritySanitizerDisabled	Per-parameter <sanitizer sanitize="false"/>
SecurityToolSanitizeOff	Tool-wide <options sanitize="false"/>
SecurityPermissiveSanitizer	Sanitizer declarations that let shell metacharacters survive (including a shell-metacharacter invalid_char)
SecurityUnquotedParam	text params interpolated unquoted, unless a regex validator restricts the value to a safe character set

Key properties:

• INFO level only. Findings never fail planemo lint; the module runs only when the new --security flag is passed (available on both planemo lint and planemo shed lint through the shared linter option group).
• Skippable. The whole module can be skipped with --skip security (module name added to the known-skip list in planemo/lint.py), and each class individually, e.g. --skip SecurityRawIdentifier.
• Semantics aligned with Galaxy's ToolParameterSanitizer. The permissive-sanitizer check models Galaxy's preset resolution (string.* constants, default/none presets, <valid> add/remove, <mapping> add/remove, invalid_char), so findings reflect what actually survives sanitization.
• False-positive mitigations. The command scanner recognizes common mitigations and stays quiet about them: values cleaned via re.sub with a safe whitelisted character class (e.g. re.sub(r'[^\w.-]', '_', ...)) or shlex.quote; quoted interpolation; Cheetah condition directives (#if/#elif/#while/#unless) whose contents are evaluated but not rendered; bash comments (the rest of their line is inert, later lines still run); and text params constrained by an anchored safe-character-set regex validator.
• Macro-aware. Macro expansion performed by tool loading means <expand>/<token> content is fully visible to the checks, including Cheetah #for loop-variable binding (loop variables are tracked to the collections they iterate).
• Params declared via argument="..." (without an explicit name) are indexed the same way Galaxy derives parameter names.
• For raw collection identifiers, the message points to Galaxy 26.2's safe_element_identifier accessor (PR Implement Tool Pattern for Using Filename Safe Collection Identifiers galaxy#23735) and, for older servers, the re.sub cleaning fallback.

Docs: --security appears in the regenerated docs/commands/lint.rst and docs/commands/shed_lint.rst; docs/planemo.linters.rst lists the new module.

Evidence

planemo lint --security on the new fixture (other lint messages trimmed; note the exit code — INFO findings do not fail the run):

$ planemo lint --security --report_level all tests/data/tools/security_raw_identifier.xml
Linting tool tests/data/tools/security_raw_identifier.xml
[... standard lint output unchanged ...]
.. INFO (SecurityRawCollectionName): $collection1.name is the raw element identifier of the collection; consider $collection1.safe_element_identifier (Galaxy 26.2+) or clean it before interpolating
.. INFO (SecurityRawCollectionName): $pair.name is the raw element identifier of the collection; consider $pair.safe_element_identifier (Galaxy 26.2+) or clean it before interpolating
.. INFO (SecurityRawIdentifier): Raw value of $collection1.element_identifier reaches the command; consider $collection1.safe_element_identifier (Galaxy 26.2+) or clean it, e.g. re.sub(r'[^\w.-]', '_', ...), before interpolating
$ echo $?
0

Suppression paths, on the same fixture:

$ planemo lint --security -s security ... # module skip
# → no Security messages

$ planemo lint ...                        # without --security (plain lint)
# → no Security messages, exit 0

A second fixture (security_clean_tool.xml) uses cleaned identifiers, quoted interpolation, and restricted sanitizers and produces zero security findings; a third (security_unquoted_param.xml) contains two unquoted text params of which only the one without a safe regex validator is reported.

Tests: unit tests in tests/test_planemo_linters.py (exact message assertions per fixture, clean-tool silence, module-name skip, loop-scope and comment semantics) and a CLI test test_lint.py::LintTestCase::test_lint_security covering the flag and the module skip. flake8, black, isort, ruff and mypy pass on the changed files; the two pre-existing toil-engine test failures on master are unrelated (they fail identically without this change).

The change was reviewed in three rounds by an AI reviewer (Claude Opus 5.5); all 41 findings were fixed and pinned by tests. That review log is collapsed in the comments below.

Details / limitations

• This is a heuristic, static analysis: Cheetah logic is not evaluated, so a finding is a prompt for human review, not proof of exploitability. Message wording reflects this ("consider ... or clean it").
• Deliberately out of scope for this first version (noted in the module docstring): language-level analysis of <configfile> contents (shell/R/Python injection inside generated scripts), eval()-style sinks and tool launchers, and values derived from file content (e.g. BED column parsing).
• The linter reports at most one finding per (check, parameter) to keep output actionable.

ELI5

Some Galaxy tools paste user-supplied names and text straight into shell commands. If the usual "cleaning" of those values is switched off or too permissive, a crafted name can run commands on the machine running the tool. This PR adds an optional planemo lint --security mode that points out these patterns — as hints for maintainers to review, never as failures — and stays quiet when the wrapper already quotes or cleans the value.

Known, not fixed

Problems I know of and have deliberately left in this PR, for the reviewer to rule on:

1. Deferred check families (by design, documented in the module docstring): language-level analysis of <configfile> contents (shell/R/Python injection inside generated scripts), eval()-style sinks and tool launchers, helper/generated scripts, and values derived from file content (e.g. BED column parsing) are out of scope for this first version.
2. Class naming: the spec table called the data-column check SecuritySampleSheetColumns; it is implemented as SecurityDataColumn (consistent with data_column param type naming).
3. Beyond the stated v1 rule list (deliberate, conservative direction): the dangerous_invalid_char finding and recognition of shlex.quote(...) as a safe cleaning idiom.
4. Eight Linter subclasses repeat an identical dispatch shape (a loop + string filter each) — kept to mirror the established linter-module patterns (urls.py, doi.py).
5. docs/planemo.linters.rst gains an autodoc entry for the pre-existing planemo.linters.util module — a regeneration side effect, not part of this feature.
6. No systematic per-class false-positive measurement. Findings are prompts for human review, not verdicts; rates were not systematically measured across a broad corpus.