From b345dc49936d53d5c1b635c488fd4ee43ae1d001 Mon Sep 17 00:00:00 2001 From: Josh Hudson <313875020+hudsonwa@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:48:35 +0100 Subject: [PATCH] fix(extractors/bash): resolve dirname cmdsubst and dotdot suffix in source paths (#2596) Extends variable-built `source` resolution to two path-construction forms that were silently dropped: `source "$(dirname "$VAR")/lib/x.sh"` (command substitution in the source argument) and `source "$VAR/../lib/x.sh"` (a `..` suffix when the leading var is a tracked var_bases entry). Co-Authored-By: Claude Opus 4.8 (1M context) --- graphify/extractors/bash.py | 123 +++++++++++++++++++++++++++--------- tests/test_extract.py | 92 +++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 29 deletions(-) diff --git a/graphify/extractors/bash.py b/graphify/extractors/bash.py index 984676b..f1d5e41 100644 --- a/graphify/extractors/bash.py +++ b/graphify/extractors/bash.py @@ -1,6 +1,7 @@ """Bash extractor. Moved verbatim from graphify/extract.py.""" from __future__ import annotations +import os import re from pathlib import Path from typing import Any @@ -17,18 +18,33 @@ _BASH_LEADING_EXPANSION = re.compile( ) -def _bash_source_suffix(raw: str) -> str | None: +def _bash_source_suffix(raw: str, allow_dotdot: bool = False) -> str | None: """Return the literal path suffix of a variable-built `source` argument, or - None when the remainder is empty, still holds an expansion, or escapes upward - with ``..``. ``"${DIR}/lib/x.sh"`` -> ``"lib/x.sh"``.""" + None when the remainder is empty or still holds an expansion. + + ``"${DIR}/lib/x.sh"`` -> ``"lib/x.sh"``. + + When *allow_dotdot* is False (the default — the base is a script-dir guess), + ``..`` segments are rejected to prevent traversal outside the project tree. + When True (the base is a tracked var_bases entry — a known directory), + ``..`` is permitted and the caller normalises with ``os.path.normpath``. + """ suffix = _BASH_LEADING_EXPANSION.sub("", raw, count=1).lstrip("/") if not suffix or "$" in suffix: return None - if ".." in suffix.split("/"): + if not allow_dotdot and ".." in suffix.split("/"): return None return suffix +# Recognise ``$(dirname "$VAR")`` (or ``$(dirname "${VAR}")``) at the start of +# a ``source`` argument, capturing the variable name so the source resolver +# can treat the whole construct as ``var_bases[VAR].parent`` (#2596 form 3). +_BASH_SOURCE_DIRNAME = re.compile( + r'\$\(dirname\s+"\$\{?([A-Za-z_][A-Za-z0-9_]*)\}?"\)' +) + + # Name of the leading variable of a `source` argument: `${ROOT}/lib/x.sh` -> ROOT. _BASH_LEADING_VAR = re.compile( r"^\$\{([A-Za-z_][A-Za-z0-9_]*)[^}]*\}|^\$([A-Za-z_][A-Za-z0-9_]*)" @@ -295,33 +311,82 @@ def extract_bash(path: Path) -> dict: # emit INFERRED (the expansion can't be proven # statically) only when it resolves to a real file, # never a dead id. - suffix = _bash_source_suffix(raw) - if suffix: - # Resolve against the variable's tracked base when - # we know it, else fall back to the script's own - # directory as before (#2172). - base = path.parent + + # Form 3 (#2596): `source "$(dirname "$VAR")/lib/x.sh"` + # — command substitution in the source argument. + # Recognise the dirname idiom on the source line the + # same way _bash_assignment_base does on assignment + # lines: treat `$(dirname "$VAR")` as + # `var_bases[VAR].parent` (falling back to + # `script_dir.parent` when VAR is untracked), then + # resolve the literal suffix against it. + dirname_match = _BASH_SOURCE_DIRNAME.match(raw) + if dirname_match: + var_name = dirname_match.group(1) + if var_name in var_bases: + base = var_bases[var_name].parent + else: + # Untracked var: fall back to the script's + # own directory, so dirname resolves to its + # parent — matching the existing untracked-var + # fallback semantics. + base = path.parent.parent + # Strip the $(dirname ...) prefix and any leading + # slash to get the literal suffix. + suffix = raw[dirname_match.end():].lstrip("/") + if suffix and "$" not in suffix: + resolved = (base / suffix).resolve() + if resolved.is_file(): + add_edge(file_nid, _make_id(str(resolved)), + "imports_from", line, + confidence="INFERRED", context="import", + target_file=str(resolved)) + bash_sources.append({ + "target_path": str(resolved), + "source_file": str_path, + "source_location": f"L{line}", + }) + else: + # Check if the leading variable is tracked + # before deciding whether to allow `..` in + # the suffix (Form 4, #2596). When the base + # is a known var_bases entry, `..` is safe. var_match = _BASH_LEADING_VAR.match(raw) + allow_dotdot = False if var_match: - var_name = var_match.group(1) or var_match.group(2) - if var_name in var_bases: - base = var_bases[var_name] - resolved = (base / suffix).resolve() - if resolved.is_file(): - add_edge(file_nid, _make_id(str(resolved)), - "imports_from", line, - confidence="INFERRED", context="import", - target_file=str(resolved)) - # Integration (#2141 + #2079): record the resolved - # sourced file so calls into its functions resolve - # too, not just the source edge. target_path is - # absolute here; resolve_bash_source_edges takes it - # as-is. - bash_sources.append({ - "target_path": str(resolved), - "source_file": str_path, - "source_location": f"L{line}", - }) + _vn = var_match.group(1) or var_match.group(2) + if _vn in var_bases: + allow_dotdot = True + suffix = _bash_source_suffix(raw, allow_dotdot=allow_dotdot) + if suffix: + # Resolve against the variable's tracked base + # when we know it, else fall back to the + # script's own directory as before (#2172). + base = path.parent + if var_match: + var_name = var_match.group(1) or var_match.group(2) + if var_name in var_bases: + base = var_bases[var_name] + if var_match and var_name in var_bases: + resolved = Path(os.path.normpath(base / suffix)) + else: + resolved = (base / suffix).resolve() + if resolved.is_file(): + add_edge(file_nid, _make_id(str(resolved)), + "imports_from", line, + confidence="INFERRED", context="import", + target_file=str(resolved)) + # Integration (#2141 + #2079): record + # the resolved sourced file so calls + # into its functions resolve too, not + # just the source edge. target_path is + # absolute here; resolve_bash_source_edges + # takes it as-is. + bash_sources.append({ + "target_path": str(resolved), + "source_file": str_path, + "source_location": f"L{line}", + }) else: # Bare `source lib.sh` (no leading ./ or /). Bash # itself resolves such a name via $PATH at runtime, diff --git a/tests/test_extract.py b/tests/test_extract.py index 16f2319..e30a081 100644 --- a/tests/test_extract.py +++ b/tests/test_extract.py @@ -2725,6 +2725,98 @@ def test_extract_bash_var_source_untracked_var_keeps_script_dir_guess(tmp_path): assert (lib / "y.sh").resolve() in targets, targets +def test_extract_bash_source_dirname_cmdsubst_in_argument(tmp_path): + """Form 3 (#2596): `source "$(dirname "$VAR")/lib/y.sh"` — command + substitution in the source argument. The dirname idiom on the *source* + line should resolve the same way it does on an assignment line: treat + `$(dirname "$VAR")` as `var_bases[VAR].parent` (or `script_dir.parent` + when VAR is untracked), then resolve the literal suffix against it. + """ + (tmp_path / "lib").mkdir() + (tmp_path / "lib" / "y.sh").write_text( + "#!/usr/bin/env bash\ny_fn() { :; }\n", encoding="utf-8" + ) + script = tmp_path / "bin" / "x.sh" + script.parent.mkdir(parents=True) + # SCRIPT_DIR is tracked by the existing idiom, so dirname("$SCRIPT_DIR") + # should resolve to tmp_path, and tmp_path/lib/y.sh is the target. + script.write_text( + '#!/usr/bin/env bash\n' + 'SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"\n' + 'source "$(dirname "$SCRIPT_DIR")/lib/y.sh"\n', + encoding="utf-8", + ) + result = extract_bash(script) + targets = [Path(s["target_path"]).resolve() for s in result["bash_sources"]] + assert (tmp_path / "lib" / "y.sh").resolve() in targets, targets + + +def test_extract_bash_source_dirname_cmdsubst_untracked_var(tmp_path): + """Form 3 with an untracked variable: `source "$(dirname "$SCRIPT_DIR")/lib/y.sh"` + where SCRIPT_DIR is NOT assigned via the recognised idiom. The fallback + is the script's own directory (same as the existing untracked-var path), + so dirname of the script dir is the parent. + """ + (tmp_path / "lib").mkdir() + (tmp_path / "lib" / "y.sh").write_text( + "#!/usr/bin/env bash\ny_fn() { :; }\n", encoding="utf-8" + ) + script = tmp_path / "bin" / "x.sh" + script.parent.mkdir(parents=True) + # No SCRIPT_DIR assignment — the var is untracked, so the extractor + # falls back to script_dir.parent (= tmp_path) for dirname. + script.write_text( + '#!/usr/bin/env bash\n' + 'source "$(dirname "$SCRIPT_DIR")/lib/y.sh"\n', + encoding="utf-8", + ) + result = extract_bash(script) + targets = [Path(s["target_path"]).resolve() for s in result["bash_sources"]] + assert (tmp_path / "lib" / "y.sh").resolve() in targets, targets + + +def test_extract_bash_source_dotdot_suffix_with_tracked_var(tmp_path): + """Form 4 (#2596): `source "$VAR/../lib/y.sh"` — `..` in the literal + suffix. When the base comes from a tracked var_bases entry, `..` is + safe (it's a known directory, not a guess), so resolve via normpath + and the existing is_file() gate. The `..` rejection should only apply + on the script-dir-guess path where the base is uncertain. + """ + (tmp_path / "lib").mkdir() + (tmp_path / "lib" / "y.sh").write_text( + "#!/usr/bin/env bash\ny_fn() { :; }\n", encoding="utf-8" + ) + script = tmp_path / "bin" / "x.sh" + script.parent.mkdir(parents=True) + # SCRIPT_DIR is tracked (= bin/), so $SCRIPT_DIR/../lib/y.sh resolves + # to tmp_path/lib/y.sh. + script.write_text( + '#!/usr/bin/env bash\n' + 'SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"\n' + 'source "$SCRIPT_DIR/../lib/y.sh"\n', + encoding="utf-8", + ) + result = extract_bash(script) + targets = [Path(s["target_path"]).resolve() for s in result["bash_sources"]] + assert (tmp_path / "lib" / "y.sh").resolve() in targets, targets + + +def test_extract_bash_source_dotdot_suffix_script_dir_guess_still_rejected(tmp_path): + """Form 4 guard: `..` in the suffix must still be rejected when the base + is the script-dir *guess* (no tracked variable). Otherwise a path like + `source "${UNTRACKED}/../../etc/passwd"` could traverse outside the tree. + """ + script = tmp_path / "run.sh" + script.write_text( + '#!/usr/bin/env bash\n' + 'source "${UNTRACKED}/../evil.sh"\n', + encoding="utf-8", + ) + result = extract_bash(script) + targets = [Path(s["target_path"]).resolve() for s in result["bash_sources"]] + assert not targets, f"untracked var with .. should not resolve: {targets}" + + # --------------------------------------------------------------------------- # JSON extractor tests (#866) # ---------------------------------------------------------------------------