Skip to content

Autofix: Code scanning alert #2 #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Notoriousjayy wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Autofix: Code scanning alert #2 #3

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions scripts/triage_all_repos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import datetime as dt
from collections import Counter, defaultdict
from typing import Any, Dict, List, Tuple
import os

from gh_code_scanning import create_clients
from gh_code_scanning.exceptions import GitHubApiError, GitHubNotFoundError
Expand Down Expand Up @@ -160,16 +161,19 @@
md = render_markdown_report(args.owner, scanned, skipped_no_analysis, per_repo_counts, top_alerts)

# write report
import os
os.makedirs(os.path.dirname(args.write_md), exist_ok=True)
with open(args.write_md, "w", encoding="utf-8") as f:
base_dir = os.path.abspath(os.getcwd())
output_path = os.path.abspath(args.write_md)
if os.path.commonpath([base_dir, output_path]) != base_dir:
raise ValueError(f"Refusing to write report outside base directory: {output_path}")
os.makedirs(os.path.dirname(output_path), exist_ok=True)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.

Copilot Autofix

AI 4 months ago

In general, to fix uncontrolled-path issues with CLI arguments, we should (1) choose a fixed, trusted root directory; (2) normalize the user-supplied path against that root; and (3) verify after normalization that the resulting path is still within the allowed root before performing any filesystem operations. This ensures that even if the user passes absolute paths or directory traversal sequences (../..), writes remain confined to the trusted directory.

For this specific script, the simplest robust fix is to base output_path on a fixed, trusted root rather than the current working directory. A good choice is the directory containing triage_all_repos.py itself (i.e., the project/scripts directory), which is under the control of whoever deploys the script. We can then join that safe_root with the user-supplied args.write_md, normalize the result, and enforce that the normalized path starts with safe_root using os.path.commonpath. This preserves existing behavior for relative paths (they will still resolve under the script directory, just as they effectively do now under the working directory in normal usage), while safely rejecting absolute or traversing paths that would escape safe_root. Concretely:

  • Define safe_root using os.path.dirname(os.path.abspath(__file__)).
  • Replace base_dir = os.path.abspath(os.getcwd()) with safe_root = ... as above.
  • Build output_path with os.path.abspath(os.path.join(safe_root, args.write_md)) instead of using args.write_md directly.
  • Update the path check to compare against safe_root instead of base_dir.

All of these changes are localized to the main() function in scripts/triage_all_repos.py, and they do not require any new imports beyond the already-imported os.

Suggested changeset 1
scripts/triage_all_repos.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/scripts/triage_all_repos.py b/scripts/triage_all_repos.py
--- a/scripts/triage_all_repos.py
+++ b/scripts/triage_all_repos.py
@@ -161,10 +161,10 @@
     md = render_markdown_report(args.owner, scanned, skipped_no_analysis, per_repo_counts, top_alerts)
 
     # write report
-    base_dir = os.path.abspath(os.getcwd())
-    output_path = os.path.abspath(args.write_md)
-    if os.path.commonpath([base_dir, output_path]) != base_dir:
-        raise ValueError(f"Refusing to write report outside base directory: {output_path}")
+    safe_root = os.path.dirname(os.path.abspath(__file__))
+    output_path = os.path.abspath(os.path.join(safe_root, args.write_md))
+    if os.path.commonpath([safe_root, output_path]) != safe_root:
+        raise ValueError(f"Refusing to write report outside safe directory: {output_path}")
     os.makedirs(os.path.dirname(output_path), exist_ok=True)
     with open(output_path, "w", encoding="utf-8") as f:
         f.write(md)
EOF
@@ -161,10 +161,10 @@
md = render_markdown_report(args.owner, scanned, skipped_no_analysis, per_repo_counts, top_alerts)

# write report
base_dir = os.path.abspath(os.getcwd())
output_path = os.path.abspath(args.write_md)
if os.path.commonpath([base_dir, output_path]) != base_dir:
raise ValueError(f"Refusing to write report outside base directory: {output_path}")
safe_root = os.path.dirname(os.path.abspath(__file__))
output_path = os.path.abspath(os.path.join(safe_root, args.write_md))
if os.path.commonpath([safe_root, output_path]) != safe_root:
raise ValueError(f"Refusing to write report outside safe directory: {output_path}")
os.makedirs(os.path.dirname(output_path), exist_ok=True)
with open(output_path, "w", encoding="utf-8") as f:
f.write(md)
Copilot is powered by AI and may make mistakes. Always verify output.
with open(output_path, "w", encoding="utf-8") as f:

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.

Copilot Autofix

AI 4 months ago

In general, the fix is to ensure that any path derived from untrusted input is normalized and then checked to be within an allowed base directory before it is used in file operations. Normalization should handle .. segments and symlinks (os.path.realpath), and the containment check should be robust across platforms, avoiding exceptions from os.path.commonpath on different drives.

For this code, the best minimal fix is to:

  1. Normalize both the base directory and the output path with os.path.realpath (or at least normpath) after converting them to absolute paths.
  2. Use a defensive wrapper around os.path.commonpath so that if it raises ValueError (e.g., on Windows with different drives), we treat that as “outside the base directory” and raise a controlled ValueError with a clear message instead of proceeding to write.
  3. Keep the existing behavior that refuses to write outside the current working directory, preserving functionality and intent.

Concretely, in scripts/triage_all_repos.py within main():

  • Replace the current computation and check:
    base_dir = os.path.abspath(os.getcwd())
    output_path = os.path.abspath(args.write_md)
    if os.path.commonpath([base_dir, output_path]) != base_dir:
        raise ValueError(f"Refusing to write report outside base directory: {output_path}")

with code that:

  • Normalizes with os.path.realpath.
  • Handles ValueError from os.path.commonpath by rejecting the path.

No new imports are required because os is already imported. All changes remain confined to the shown snippet.

Suggested changeset 1
scripts/triage_all_repos.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/scripts/triage_all_repos.py b/scripts/triage_all_repos.py
--- a/scripts/triage_all_repos.py
+++ b/scripts/triage_all_repos.py
@@ -161,10 +161,15 @@
     md = render_markdown_report(args.owner, scanned, skipped_no_analysis, per_repo_counts, top_alerts)
 
     # write report
-    base_dir = os.path.abspath(os.getcwd())
-    output_path = os.path.abspath(args.write_md)
-    if os.path.commonpath([base_dir, output_path]) != base_dir:
+    base_dir = os.path.realpath(os.path.abspath(os.getcwd()))
+    output_path = os.path.realpath(os.path.abspath(args.write_md))
+    try:
+        common = os.path.commonpath([base_dir, output_path])
+    except ValueError:
+        # Paths on different drives or otherwise incomparable; treat as outside base_dir
         raise ValueError(f"Refusing to write report outside base directory: {output_path}")
+    if common != base_dir:
+        raise ValueError(f"Refusing to write report outside base directory: {output_path}")
     os.makedirs(os.path.dirname(output_path), exist_ok=True)
     with open(output_path, "w", encoding="utf-8") as f:
         f.write(md)
EOF
@@ -161,10 +161,15 @@
md = render_markdown_report(args.owner, scanned, skipped_no_analysis, per_repo_counts, top_alerts)

# write report
base_dir = os.path.abspath(os.getcwd())
output_path = os.path.abspath(args.write_md)
if os.path.commonpath([base_dir, output_path]) != base_dir:
base_dir = os.path.realpath(os.path.abspath(os.getcwd()))
output_path = os.path.realpath(os.path.abspath(args.write_md))
try:
common = os.path.commonpath([base_dir, output_path])
except ValueError:
# Paths on different drives or otherwise incomparable; treat as outside base_dir
raise ValueError(f"Refusing to write report outside base directory: {output_path}")
if common != base_dir:
raise ValueError(f"Refusing to write report outside base directory: {output_path}")
os.makedirs(os.path.dirname(output_path), exist_ok=True)
with open(output_path, "w", encoding="utf-8") as f:
f.write(md)
Copilot is powered by AI and may make mistakes. Always verify output.
f.write(md)

if args.update_issue:
issue_owner, issue_repo = args.issue_repo.split("/", 1)
upsert_issue(rest, issue_owner, issue_repo, args.issue_title, md, labels=["security", "code-scanning", "triage"])

print(f"Wrote: {args.write_md}")
print(f"Wrote: {output_path}")
if args.update_issue:
print(f"Updated issue: {args.issue_repo} -> {args.issue_title}")
return 0
Expand Down