From 17db2fe9d7fb165ce689bbd1158e767d315b2720 Mon Sep 17 00:00:00 2001 From: Ewald <129623145+MajoorWaldi@users.noreply.github.com> Date: Fri, 24 Jul 2026 16:16:50 +0200 Subject: [PATCH] fix(security): harden workflow path containment and bump brace-expansion - Resolve CodeQL py/path-injection alerts: enforce lexical containment (normpath + prefix check) before any filesystem access in list_workflow_versions, diff_workflow_versions, list_workflow_thumbnail_candidates and _resolve_path_under_roots, with post-resolve symlink re-validation. Also removes a file existence oracle for paths outside the allowed roots. - npm audit fix: brace-expansion DoS (GHSA-3jxr-9vmj-r5cp, dev-only). --- mjr_am_backend/features/workflows/service.py | 73 ++++++++++++++------ mjr_am_backend/routes/handlers/workflows.py | 27 ++++++-- package-lock.json | 68 +++--------------- tests/features/test_workflows_routes.py | 5 +- 4 files changed, 87 insertions(+), 86 deletions(-) diff --git a/mjr_am_backend/features/workflows/service.py b/mjr_am_backend/features/workflows/service.py index 56463a04..affae9d9 100644 --- a/mjr_am_backend/features/workflows/service.py +++ b/mjr_am_backend/features/workflows/service.py @@ -1023,6 +1023,35 @@ def is_managed_workflow_json_path(path: Path) -> bool: return False +def _resolve_managed_workflow_path(path: Path) -> Result[Path]: + """Resolve *path* while enforcing containment in the managed workflow root. + + The lexical prefix check runs before any filesystem access so untrusted + paths never reach ``resolve``/``stat`` (avoids an existence oracle and + keeps the path-injection barrier visible to static analysis), then the + containment is re-validated after symlink resolution. + """ + root = managed_workflow_root(create=False) + if root is None: + return Result.Err("FORBIDDEN", "Workflow path is not allowed") + try: + prefix = os.path.normcase(os.path.normpath(str(root))) + candidate = os.path.normcase(os.path.normpath(os.path.abspath(str(path)))) + except Exception: + return Result.Err("FORBIDDEN", "Workflow path is not allowed") + if candidate != prefix and not candidate.startswith(prefix + os.sep): + return Result.Err("FORBIDDEN", "Workflow path is not allowed") + try: + resolved = Path(candidate).resolve(strict=True) + except FileNotFoundError: + return Result.Err("NOT_FOUND", "Workflow not found") + except Exception: + return Result.Err("FORBIDDEN", "Workflow path is not allowed") + if not is_managed_workflow_json_path(resolved): + return Result.Err("FORBIDDEN", "Workflow path is not allowed") + return Result.Ok(resolved) + + def read_workflow_content(path: Path) -> Result[dict[str, Any]]: if not is_workflow_json_path(path): return Result.Err("FORBIDDEN", "Workflow path is not allowed") @@ -1291,12 +1320,10 @@ def _snapshot_workflow_version(path: Path, *, reason: str) -> Result[dict[str, A def list_workflow_versions(path: Path) -> Result[dict[str, Any]]: - if not is_managed_workflow_json_path(path): - return Result.Err("FORBIDDEN", "Workflow path is not allowed") - try: - resolved = path.resolve(strict=True) - except FileNotFoundError: - return Result.Err("NOT_FOUND", "Workflow not found") + resolved_check = _resolve_managed_workflow_path(path) + resolved = resolved_check.data + if not resolved_check.ok or resolved is None: + return Result.Err(resolved_check.code or "FORBIDDEN", resolved_check.error or "Workflow path is not allowed") history_dir = _workflow_history_dir(resolved) versions: list[dict[str, Any]] = [] if history_dir.is_dir(): @@ -1333,23 +1360,29 @@ def _flatten_json(value: Any, prefix: str = "") -> dict[str, Any]: def diff_workflow_versions(path: Path, *, version_filepath: Any = "") -> Result[dict[str, Any]]: - if not is_managed_workflow_json_path(path): - return Result.Err("FORBIDDEN", "Workflow path is not allowed") - try: - current_path = path.resolve(strict=True) - except FileNotFoundError: - return Result.Err("NOT_FOUND", "Workflow not found") + resolved_check = _resolve_managed_workflow_path(path) + current_path = resolved_check.data + if not resolved_check.ok or current_path is None: + return Result.Err(resolved_check.code or "FORBIDDEN", resolved_check.error or "Workflow path is not allowed") versions = list_workflow_versions(current_path) if not versions.ok: return versions requested = str(version_filepath or "").strip() if requested: + history_root = _workflow_history_dir(current_path) try: - version_path = Path(requested).resolve(strict=True) + history_prefix = os.path.normcase(os.path.normpath(str(history_root))) + requested_norm = os.path.normcase(os.path.normpath(os.path.abspath(requested))) + except Exception: + return Result.Err("FORBIDDEN", "Workflow version path is not allowed") + if requested_norm != history_prefix and not requested_norm.startswith(history_prefix + os.sep): + return Result.Err("FORBIDDEN", "Workflow version path is not allowed") + try: + version_path = Path(requested_norm).resolve(strict=True) except FileNotFoundError: return Result.Err("NOT_FOUND", "Workflow version not found") - history_root = _workflow_history_dir(current_path).resolve(strict=False) - if version_path != history_root and history_root not in version_path.parents: + history_resolved = history_root.resolve(strict=False) + if version_path != history_resolved and history_resolved not in version_path.parents: return Result.Err("FORBIDDEN", "Workflow version path is not allowed") else: items = (versions.data or {}).get("versions") or [] @@ -1707,12 +1740,10 @@ def set_workflow_info(path: Path, *, info: dict[str, Any] | None = None) -> Resu def list_workflow_thumbnail_candidates(path: Path, *, limit: int = 12) -> Result[list[dict[str, Any]]]: - if not is_managed_workflow_json_path(path): - return Result.Err("FORBIDDEN", "Workflow path is not allowed") - try: - resolved = path.resolve(strict=True) - except FileNotFoundError: - return Result.Err("NOT_FOUND", "Workflow not found") + resolved_check = _resolve_managed_workflow_path(path) + resolved = resolved_check.data + if not resolved_check.ok or resolved is None: + return Result.Err(resolved_check.code or "FORBIDDEN", resolved_check.error or "Workflow path is not allowed") workflow = _safe_read_workflow_json(resolved) if workflow is None: diff --git a/mjr_am_backend/routes/handlers/workflows.py b/mjr_am_backend/routes/handlers/workflows.py index 25535491..e1f726d8 100644 --- a/mjr_am_backend/routes/handlers/workflows.py +++ b/mjr_am_backend/routes/handlers/workflows.py @@ -188,20 +188,35 @@ async def _audit_workflow_write( def _resolve_path_under_roots(user_path: Path, safe_roots: list[Path]) -> Result[Path]: + # Lexical containment check before any filesystem access (no existence + # oracle outside the safe roots), then re-validated after symlink + # resolution. try: - resolved = user_path.resolve(strict=True) - except FileNotFoundError: - return Result.Err("NOT_FOUND", "Thumbnail not found") + candidate = os.path.normcase(os.path.normpath(os.path.abspath(str(user_path)))) except Exception: return Result.Err("FORBIDDEN", "Thumbnail path is not allowed") for root in safe_roots: try: - root_resolved = root.resolve(strict=False) + prefix = os.path.normcase(os.path.normpath(str(root))) except Exception: continue - if resolved == root_resolved or resolved.is_relative_to(root_resolved): - return Result.Ok(resolved) + if candidate != prefix and not candidate.startswith(prefix + os.sep): + continue + try: + resolved = Path(candidate).resolve(strict=True) + except FileNotFoundError: + return Result.Err("NOT_FOUND", "Thumbnail not found") + except Exception: + return Result.Err("FORBIDDEN", "Thumbnail path is not allowed") + for allowed_root in safe_roots: + try: + root_resolved = allowed_root.resolve(strict=False) + except Exception: + continue + if resolved == root_resolved or resolved.is_relative_to(root_resolved): + return Result.Ok(resolved) + return Result.Err("FORBIDDEN", "Thumbnail path is not allowed") return Result.Err("FORBIDDEN", "Thumbnail path is not allowed") diff --git a/package-lock.json b/package-lock.json index b79c199e..5c8016c3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -506,9 +506,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -526,9 +523,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -546,9 +540,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -566,9 +557,6 @@ "riscv64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -586,9 +574,6 @@ "riscv64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -606,9 +591,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -626,9 +608,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -646,9 +625,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -895,9 +871,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -912,9 +885,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -929,9 +899,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -946,9 +913,6 @@ "riscv64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -963,9 +927,6 @@ "riscv64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -980,9 +941,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -997,9 +955,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1014,9 +969,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -2346,16 +2298,16 @@ } }, "node_modules/brace-expansion": { - "version": "5.0.6", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.6.tgz", - "integrity": "sha512-kLpxurY4Z4r9sgMsyG0Z9uzsBlgiU/EFKhj/h91/8yHu0edo7XuixOIH3VcJ8kkxs6/jPzoI6U9Vj3WqbMQ94g==", + "version": "5.0.8", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.8.tgz", + "integrity": "sha512-JZyDyq3D4AUifKTPOB7DELf6XsB3WdPuNxCtob1vFXPsSXhdAiHBWJ/tJ8HAc9aH84BK+5JFZLNkJKx3G9kzQg==", "dev": true, "license": "MIT", "dependencies": { "balanced-match": "^4.0.2" }, "engines": { - "node": "18 || 20 || >=22" + "node": "20 || >=22" } }, "node_modules/buffer-image-size": { @@ -2534,9 +2486,9 @@ "license": "MIT" }, "node_modules/editorconfig/node_modules/brace-expansion": { - "version": "2.0.3", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.0.3.tgz", - "integrity": "sha512-MCV/fYJEbqx68aE58kv2cA/kiky1G8vux3OR6/jbS+jIMe/6fJWa0DTzJU7dqijOWYwHi1t29FlfYI9uytqlpA==", + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.2.tgz", + "integrity": "sha512-w5JZcKgdhDOgOwm8H+KgbosopHMuGcl6qbulwjtz3SM7I7P3yW1eAjzMPLrIE+NQ9vjgANKHWeMHnrT0OXW1oA==", "dev": true, "license": "MIT", "dependencies": { @@ -2977,9 +2929,9 @@ "license": "MIT" }, "node_modules/glob/node_modules/brace-expansion": { - "version": "2.0.3", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.0.3.tgz", - "integrity": "sha512-MCV/fYJEbqx68aE58kv2cA/kiky1G8vux3OR6/jbS+jIMe/6fJWa0DTzJU7dqijOWYwHi1t29FlfYI9uytqlpA==", + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.2.tgz", + "integrity": "sha512-w5JZcKgdhDOgOwm8H+KgbosopHMuGcl6qbulwjtz3SM7I7P3yW1eAjzMPLrIE+NQ9vjgANKHWeMHnrT0OXW1oA==", "dev": true, "license": "MIT", "dependencies": { diff --git a/tests/features/test_workflows_routes.py b/tests/features/test_workflows_routes.py index 531fb388..c4d4dd9e 100644 --- a/tests/features/test_workflows_routes.py +++ b/tests/features/test_workflows_routes.py @@ -151,8 +151,11 @@ async def test_workflow_save_forwards_info_fields(monkeypatch): @pytest.mark.asyncio async def test_workflow_thumbnail_missing_returns_not_found(monkeypatch, tmp_path): - missing = tmp_path / "workflows" / "missing.png" + workflow_dir = tmp_path / "workflows" + workflow_dir.mkdir() + missing = workflow_dir / "missing.png" + monkeypatch.setenv("MJR_AM_WORKFLOW_DIRECTORY", str(workflow_dir)) monkeypatch.setattr(workflows_routes, "is_workflow_thumbnail_path", lambda _path: True) app = _build_app()