diff --git a/scripts/plotting/chimerax_plot.py b/scripts/plotting/chimerax_plot.py index 692bb61..f18d3b9 100644 --- a/scripts/plotting/chimerax_plot.py +++ b/scripts/plotting/chimerax_plot.py @@ -17,20 +17,18 @@ 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') +# Paths are interpolated unquoted into the ';'-separated --cmd script (ChimeraX +# rejects quoted paths in such a --cmd on some versions), so reject characters +# that would split a path (space, tab) or terminate the command and inject new +# ones (';', quote, newline, CR). +_CHIMERAX_UNSAFE_PATH_CHARS = ('"', ' ', '\t', ';', '\n', '\r') def _validate_chimerax_path(path, role): - """Reject paths that contain characters unsafe for the ChimeraX --cmd script. + """Reject paths with characters that would break 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. + Real-world paths here are normally clean; this just surfaces a clear failure + instead of a silently broken or injected command. """ text = str(path) bad = [c for c in _CHIMERAX_UNSAFE_PATH_CHARS if c in text] @@ -42,13 +40,12 @@ def _validate_chimerax_path(path, role): 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. + """Run a ChimeraX subprocess. Failures become warnings so one bad image + doesn't abort the loop while the user still sees something went wrong. - `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`. + Paths inside the --cmd script are unquoted, so `get_chimerax_command` + validates them to be free of spaces/quotes/newlines that would break the + ChimeraX parser. """ # Discard stdout (never read) but keep stderr for the failure warning; # capture_output=True would buffer both into memory across many genes. @@ -159,22 +156,15 @@ 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 "" - # 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 before interpolating: paths go unquoted into the --cmd script. _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}"' - chimerax_script = ( - f"open {pdb_arg}; " + f"open {pdb_path}; " "set bgColor white; " "color lightgray; " - f"open {attr_arg}; " + f"open {attr_file_path}; " 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;" @@ -209,11 +199,9 @@ def get_chimerax_command(chimerax_bin, 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' + 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), - # bypassing the OS shell entirely. Paths embedded inside the ChimeraX - # --cmd script are also quoted so ChimeraX's own parser handles spaces. + # argv list for subprocess.run(shell=False) — the OS shell never sees it. return [chimerax_bin, "--nogui", "--offscreen", "--silent", "--cmd", chimerax_script] @@ -240,7 +228,8 @@ def generate_chimerax_plot(output_dir, if "Ratio_obs_sim" in result.columns: result = result.rename(columns={"Ratio_obs_sim" : "Score_obs_sim"}) result = cap_inf_scores(result) - result["Logscore_obs_sim"] = np.log(result["Score_obs_sim"]) + with np.errstate(divide="ignore"): + result["Logscore_obs_sim"] = np.log(result["Score_obs_sim"]) # Detect the AlphaFold version actually present in the dataset; the # user-supplied --af_version is used as a tiebreaker only. diff --git a/scripts/plotting/utils.py b/scripts/plotting/utils.py index 5ecf4aa..ea9ca52 100644 --- a/scripts/plotting/utils.py +++ b/scripts/plotting/utils.py @@ -55,7 +55,7 @@ def cap_inf_scores(pos_result, col="Score_obs_sim"): n_inf = int(inf_mask.sum()) logger.warning( f"Capping {n_inf} `{col}` value(s) at {cap:.4f} for plotting " - f"(originally +inf — extreme hotspot positions)" + f"(originally +inf: extreme hotspot positions)" ) # Write back the whole numeric column so the returned frame is numeric dtype. # .copy() is required: pd.to_numeric can share the caller's buffer, so an