From 55a5b70fa12f1101e93416506b206f2f4cd1ab00 Mon Sep 17 00:00:00 2001 From: St3451 Date: Thu, 28 May 2026 18:42:03 +0200 Subject: [PATCH 1/2] fix: quote paths inside ChimeraX --cmd script to handle spaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous argv-list refactor bypassed the OS shell but the path tokens were still inlined unquoted into the ChimeraX --cmd script. ChimeraX's own command parser still split on whitespace, so an output dir or build folder containing a space (e.g. user-chosen output paths) would break "open" and "save" commands silently — no PNG, no Python-level error. Wrap pdb_path, attr_file_path, and output_path in double quotes before interpolation so ChimeraX treats each as a single token. Paths with embedded double quotes are not supported; build-datasets wouldn't accept them either. Also tightened the _run_chimerax docstring to accurately describe what the argv-list change does and doesn't cover. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/plotting/chimerax_plot.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/scripts/plotting/chimerax_plot.py b/scripts/plotting/chimerax_plot.py index 193d2d9..68738e1 100644 --- a/scripts/plotting/chimerax_plot.py +++ b/scripts/plotting/chimerax_plot.py @@ -21,8 +21,10 @@ def _run_chimerax(command, label): """Run a ChimeraX subprocess. Surface failures as warnings so one bad image doesn't abort the whole loop, but the user actually sees something went wrong. - `command` is an argv list (no shell interpretation), so paths and labels - with spaces or shell metacharacters pass through verbatim. + `command` is an argv list passed with shell=False, so the OS shell never + interprets the tokens (spaces, metacharacters, etc. in argv elements are + preserved as-is). Path quoting *inside* the ChimeraX --cmd script is + handled separately by `get_chimerax_command`. """ # Discard stdout (never read) but keep stderr for the failure warning; # capture_output=True would buffer both into memory across many genes. @@ -132,11 +134,17 @@ def get_chimerax_command(chimerax_bin, palette = get_palette(intervals, type="diverging") if attribute == "logscore" else get_palette(intervals, type="sequential") transparent_bg_suffix = " transparentBackground true" if transparent_bg else "" + # Quote paths so ChimeraX's own command parser treats each as a single + # token even when the path contains spaces. Paths with embedded double + # quotes are not supported (and would be rejected by build-datasets too). + pdb_arg = f'"{pdb_path}"' + attr_arg = f'"{attr_file_path}"' + chimerax_script = ( - f"open {pdb_path}; " + f"open {pdb_arg}; " "set bgColor white; " "color lightgray; " - f"open {attr_file_path}; " + f"open {attr_arg}; " f"color byattribute {attribute} palette {palette}; " f"key {palette} :{intervals[0]} :{intervals[1]} :{intervals[2]} :{intervals[3]} :{intervals[4]} pos 0.35,0.03 fontSize 4 size 0.3,0.02;" f"2dlabels create label text '{labels[attribute]}' size 6 color darkred xpos 0.34 ypos 0.065;" @@ -156,10 +164,11 @@ def get_chimerax_command(chimerax_bin, cluster_tag = "" output_path = os.path.join(chimera_output_path, f"{cohort}_{i}_{gene}_{attribute}{cluster_tag}.png") - chimerax_script += f"save {output_path} pixelSize {pixelsize} supersample 3{transparent_bg_suffix};exit" + chimerax_script += f'save "{output_path}" pixelSize {pixelsize} supersample 3{transparent_bg_suffix};exit' - # Return as an argv list so the caller can use subprocess.run(..., shell=False) - # and avoid shell interpretation of paths/labels with spaces or metacharacters. + # Return as an argv list so the caller can use subprocess.run(..., shell=False), + # bypassing the OS shell entirely. Paths embedded inside the ChimeraX + # --cmd script are also quoted so ChimeraX's own parser handles spaces. return [chimerax_bin, "--nogui", "--offscreen", "--silent", "--cmd", chimerax_script] From cbe61afae81eecf0b3fb63932db0d406b9e12857 Mon Sep 17 00:00:00 2001 From: St3451 Date: Thu, 28 May 2026 18:58:32 +0200 Subject: [PATCH 2/2] fix: validate paths for ChimeraX commands to prevent injection of unsafe characters --- scripts/plotting/chimerax_plot.py | 94 ++++++++++++++++++++++--------- 1 file changed, 66 insertions(+), 28 deletions(-) diff --git a/scripts/plotting/chimerax_plot.py b/scripts/plotting/chimerax_plot.py index 68738e1..b281bc4 100644 --- a/scripts/plotting/chimerax_plot.py +++ b/scripts/plotting/chimerax_plot.py @@ -17,6 +17,30 @@ logger = daiquiri.getLogger(__logger_name__ + ".plotting.chimerax_plot") +# Characters that would break the ChimeraX command parser (or, worse, allow +# injecting extra ChimeraX commands) if interpolated raw into the --cmd script. +# Double quotes are how we delimit path arguments; newlines / carriage returns +# would let an attacker (or a malformed config) end a command and start new ones. +_CHIMERAX_UNSAFE_PATH_CHARS = ('"', '\n', '\r') + + +def _validate_chimerax_path(path, role): + """Reject paths that contain characters unsafe for the ChimeraX --cmd script. + + Real-world paths in this codebase don't contain these characters (build-datasets + rejects them, and HPC/output paths are normally clean), but interpolating them + raw would silently produce broken or attacker-controlled ChimeraX command + strings. Surface a clear failure instead. + """ + text = str(path) + bad = [c for c in _CHIMERAX_UNSAFE_PATH_CHARS if c in text] + if bad: + raise ValueError( + f"{role} path contains characters unsafe for the ChimeraX command " + f"script ({', '.join(repr(c) for c in bad)}): {text!r}" + ) + + def _run_chimerax(command, label): """Run a ChimeraX subprocess. Surface failures as warnings so one bad image doesn't abort the whole loop, but the user actually sees something went wrong. @@ -134,9 +158,14 @@ def get_chimerax_command(chimerax_bin, palette = get_palette(intervals, type="diverging") if attribute == "logscore" else get_palette(intervals, type="sequential") transparent_bg_suffix = " transparentBackground true" if transparent_bg else "" - # Quote paths so ChimeraX's own command parser treats each as a single - # token even when the path contains spaces. Paths with embedded double - # quotes are not supported (and would be rejected by build-datasets too). + # Validate paths *before* interpolating them: the double-quote wrapping below + # protects against whitespace splitting but doesn't escape embedded " or + # newlines, which would otherwise let a path inject extra ChimeraX commands. + _validate_chimerax_path(pdb_path, "PDB") + _validate_chimerax_path(attr_file_path, "attribute file") + + # Quote paths so ChimeraX's own command parser treats each as a single token + # even when the path contains spaces. pdb_arg = f'"{pdb_path}"' attr_arg = f'"{attr_file_path}"' @@ -164,6 +193,7 @@ def get_chimerax_command(chimerax_bin, cluster_tag = "" output_path = os.path.join(chimera_output_path, f"{cohort}_{i}_{gene}_{attribute}{cluster_tag}.png") + _validate_chimerax_path(output_path, "output") chimerax_script += f'save "{output_path}" pixelSize {pixelsize} supersample 3{transparent_bg_suffix};exit' # Return as an argv list so the caller can use subprocess.run(..., shell=False), @@ -260,39 +290,47 @@ def generate_chimerax_plot(output_dir, intervals = get_intervals(attribute_vector, attribute) logger.debug("Generating 3D images..") - chimerax_command = get_chimerax_command(chimerax_bin, - pdb_path, - chimera_plots_path, - attr_file_path, - attribute, - intervals, - gene, - uni_id, - labels, - i, - f, - cohort, - pixelsize=pixel_size, - transparent_bg=transparent_bg) - _run_chimerax(chimerax_command, f"{gene} ({attribute})") - logger.debug(shlex.join(chimerax_command)) - - if attribute == "score" or attribute == "logscore": - chimerax_command = get_chimerax_command(chimerax_bin, - pdb_path, - chimera_plots_path, - attr_file_path, - attribute, - intervals, + try: + chimerax_command = get_chimerax_command(chimerax_bin, + pdb_path, + chimera_plots_path, + attr_file_path, + attribute, + intervals, gene, uni_id, labels, i, f, cohort, - clusters=clusters, pixelsize=pixel_size, transparent_bg=transparent_bg) + except ValueError as exc: + logger.warning(f"Skipping {gene} ({attribute}): {exc}") + continue + _run_chimerax(chimerax_command, f"{gene} ({attribute})") + logger.debug(shlex.join(chimerax_command)) + + if attribute == "score" or attribute == "logscore": + try: + chimerax_command = get_chimerax_command(chimerax_bin, + pdb_path, + chimera_plots_path, + attr_file_path, + attribute, + intervals, + gene, + uni_id, + labels, + i, + f, + cohort, + clusters=clusters, + pixelsize=pixel_size, + transparent_bg=transparent_bg) + except ValueError as exc: + logger.warning(f"Skipping {gene} ({attribute}, clusters): {exc}") + continue _run_chimerax(chimerax_command, f"{gene} ({attribute}, clusters)") logger.debug(shlex.join(chimerax_command))