Skip to content

fix(security): CodeQL remediation — workflow perms, path handling, exception narrowing - #88

Merged
tonythethompson merged 5 commits into
masterfrom
claude/security-remediation-to-main-348b80
Jul 17, 2026
Merged

fix(security): CodeQL remediation — workflow perms, path handling, exception narrowing#88
tonythethompson merged 5 commits into
masterfrom
claude/security-remediation-to-main-348b80

Conversation

@tonythethompson

@tonythethompson tonythethompson commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • Default GitHub Actions workflow permissions to contents: read; job-level overrides retained where write access is needed (release-extension.yml)
  • Fix unsafe Path.Combine usage flagged by CodeQL — switch to Path.Join and add rooted/relative branching for the one call site driven by an environment variable (AgentDebugLog)
  • Narrow overly broad exception handlers to the concrete exception types each call site can actually throw, instead of catching Exception

Test plan

  • Code review via /code-review — 0 critical/high/medium findings, 3 low informational notes, recommendation: APPROVE
  • dotnet test (not run in this session — recommend running in CI before merge)

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com


Summary by cubic

Tightened security by defaulting GitHub Actions to read-only, hardened path handling, and narrowed exception filters to address CodeQL findings. No functional changes; improves safety, log path resolution, and fixes a test catch-syntax regression.

  • Bug Fixes
    • GitHub Actions: set permissions: contents: read in release-extension.yml; jobs opt into broader scopes when needed.
    • Paths: replaced Path.Combine with Path.Join; in AgentDebugLog guard QUICKSHELL_WORKSPACE_ROOT (rooted vs. relative) to prevent writes outside the workspace; use Path.Join for LocalAppData log path.
    • Errors: replaced broad catches with targeted filters (IOException, UnauthorizedAccessException, JsonException); added ArgumentException handling for invalid paths in classification and layout analyzer.
    • Tests: fixed AtomicFileWriterTests.Dispose to use catch (Exception ex) when (...) and keep the exception referenced.

Written for commit 316cb71. Summary will update on new commits.

Review in cubic

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @tonythethompson, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 15 files

You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Architecture diagram
sequenceDiagram
    participant Runner as GitHub Actions Runner
    participant Workflow as release-extension.yml
    participant Repo as Repository
    participant User as End User
    participant FS as File System
    participant Env as Environment
    participant AppConfig as App Configuration
    participant Detectors as Classification Detectors
    participant Classifier as Project Classification
    participant Layout as Layout Analyzer
    participant Encoding as CommandIdEncoding

    Note over Runner,Repo: GitHub Actions Workflow Permissions

    Runner->>Workflow: Trigger workflow
    Workflow->>Workflow: CHANGED: Set permissions: contents: read
    alt Job requires write access
        Workflow->>Repo: Job-level override: write permissions
    else Default
        Workflow->>Repo: contents: read only
    end
    Repo-->>Runner: Access granted at scope level

    Note over User,FS: Path Handling — AgentDebugLog

    User->>Env: Set QUICKSHELL_WORKSPACE_ROOT
    Env-->>AppConfig: Read environment variable
    AppConfig->>AppConfig: CHANGED: Check if path is rooted
    alt Rooted path
        AppConfig->>FS: Path.Join(workspaceRoot, "debug-a49e01.log")
    else Relative path
        AppConfig->>FS: Path.Join(Environment.CurrentDirectory, workspaceRoot, "debug-a49e01.log")
    end
    AppConfig->>FS: Path.Join(LocalAppData, "QuickShell", "debug-a49e01.log")

    Note over Detectors,Layout: Exception Narrowing — Classification Pipeline

    Detectors->>FS: Check for project files (Path.Join)
    FS-->>Detectors: File/Directory existence
    alt IOException or UnauthorizedAccessException or JsonException
        Detectors->>Detectors: CHANGED: Catch specific exception types only
        Detectors-->>Classifier: Return null / empty
    else Success
        Detectors-->>Classifier: Detection result
    end

    Classifier->>FS: Read package.json, Cargo.toml, etc.
    FS-->>Classifier: File contents
    alt IOException or UnauthorizedAccessException or JsonException
        Classifier->>Classifier: CHANGED: Catch specific exception types
        Classifier-->>Layout: Degraded classification
    else Success
        Classifier-->>Layout: Classification result
    end

    Layout->>FS: Check layout signals (.git, .vscode, etc.)
    FS-->>Layout: Directory/File existence
    alt IOException or UnauthorizedAccessException
        Layout->>Layout: CHANGED: Catch specific exception types
        Layout-->>Detectors: Empty layout
    else Success
        Layout-->>Detectors: Full layout
    end

    Note over Encoding: Exception Narrowing — Path Encoding

    Encoding->>FS: Path.GetFullPath(directory)
    alt ArgumentException or IOException or NotSupportedException
        Encoding->>Encoding: CHANGED: Catch specific exception types
        Encoding-->>Classifier: Use trimmed input
    else Success
        Encoding-->>Classifier: Normalized path
    end

    Note over User,FS: Test Teardown — Best Effort Cleanup

    User->>FS: Delete temp directory
    alt IOException or UnauthorizedAccessException
        FS-->>User: CHANGED: Swallow specific exception types
    else Success
        FS-->>User: Cleanup complete
    end
Loading

Shadow auto-approve: would auto-approve. Three security hardening changes (workflow perms, path safety, exception narrowing) that preserve behavior and reduce risk. Diffs show focused, mechanical updates with no new exposure or tradeoffs requiring human judgment.

Re-trigger cubic

…ediation-to-main-348b80

# Conflicts:
#	QuickShell.Core.Tests/ProjectAnalysisAccessorTests.cs
#	QuickShell.Core.Tests/ProjectAnalysisServiceTests.cs
#	QuickShell.Core.Tests/QuickShellCompositionRootTests.cs
#	QuickShell.Core.Tests/ShortcutLayoutEnvelopeTests.cs
#	QuickShell.Core.Tests/ShortcutRepositoryWorkspacesChangedTests.cs
#	QuickShell.Core/Classification/Detectors/CompanionAppDetector.cs
#	QuickShell.Core/Classification/Detectors/DevServerDetector.cs
#	QuickShell.Core/Classification/ProjectClassificationBuilder.cs
#	QuickShell.Core/Classification/ProjectClassificationContributor.cs
#	QuickShell.Core/Classification/ProjectLayoutAnalyzer.cs
#	QuickShell.Core/Services/CommandIdEncoding.cs
#	QuickShell/Services/AgentDebugLog.cs
AtomicFileWriterTests.Dispose used C# pattern-matching syntax
(`catch (A or B)`) that isn't valid without `catch (Exception ex) when (...)`.
@tonythethompson
tonythethompson merged commit 56e4672 into master Jul 17, 2026
7 checks passed
@tonythethompson
tonythethompson deleted the claude/security-remediation-to-main-348b80 branch July 17, 2026 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant