Skip to content

refactor: improve path handling in config, node steps, and build#2015

Open
niStee wants to merge 24 commits into
topgrade-rs:mainfrom
niStee:security/consolidated-codeql-fixes
Open

refactor: improve path handling in config, node steps, and build#2015
niStee wants to merge 24 commits into
topgrade-rs:mainfrom
niStee:security/consolidated-codeql-fixes

Conversation

@niStee
Copy link
Copy Markdown
Contributor

@niStee niStee commented May 13, 2026

What does this PR do

Improves code quality and readability by refactoring path handling across several modules to be more idiomatic.

Note: This PR originally started as an attempt to resolve CodeQL path traversal alerts using AI autofixes. After maintainer review, we determined those alerts were false positives given the threat model of a local CLI tool. The overly defensive security checks and suppressions were removed, and the PR was repurposed to focus purely on structural code health.

Key Changes:

  • src/config.rs: Flattened the directory traversal logic to reduce unnecessary rightward drift and cached entry.path() to avoid redundant calls inside the loop.
  • src/steps/node.rs: Refactored to use more idiomatic PathBuf handling.
  • build.rs: Cleaned up the OUT_DIR path parsing logic to use safe, owned PathBuf representations.
  • Removed redundant path component validation checks and tests that were unnecessarily testing the Rust standard library.

Standards checklist

  • The PR title is descriptive
  • I have read CONTRIBUTING.md
  • Optional: I have tested the code myself, with the relevant tools installed. If yes, add Topgrade's output of the relevant steps.

AI involvement

Initial overly defensive fixes were generated by GitHub Copilot. The final implementation was refined manually based on maintainer feedback to focus on standard Rust refactoring rather than security suppressions.

niStee and others added 12 commits May 11, 2026 14:58
… in path expression

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
… in path expression

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
… in path expression

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
… in path expression

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
… in path expression

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@niStee niStee changed the title Security: Consolidate CodeQL autofixes for multiple alerts fix: Security Consolidate CodeQL autofixes for multiple alerts May 13, 2026
@niStee
Copy link
Copy Markdown
Contributor Author

niStee commented May 13, 2026

There is another one:

https://github.com/niStee/topgrade/blob/b8ca6373816b1581392755bd4f641cde76542e7a/src/self_renamer.rs#L18

Uncontrolled data used in path expression.
This path depends on a user-provided value:

let exe_path = current_exe()?;

Copy link
Copy Markdown
Member

@GideonBear GideonBear left a comment

Choose a reason for hiding this comment

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

Not going to fully review yet. Can you explain individually for each case why it's dangerous?

Comment thread src/steps/node.rs
Comment thread src/self_renamer.rs Outdated
Comment thread build.rs Outdated
Comment thread src/config.rs Outdated
@niStee niStee marked this pull request as draft May 15, 2026 17:44
@niStee niStee changed the title fix: Security Consolidate CodeQL autofixes for multiple alerts chore(security): resolve CodeQL path traversal alerts via suppressions and idiomatic path handling May 15, 2026
@niStee
Copy link
Copy Markdown
Contributor Author

niStee commented May 15, 2026

Hi @GideonBear, thank you so much for the detailed review! You were right to point out that the initial commits were heavily influenced by Copilot autofixes and ended up being overly defensive. I had a fundamental misunderstanding of the threat model for a local CLI tool and of how certain standard library functions behave.

I really appreciate you highlighting these issues, particularly with regard to build.rs. Thanks to your feedback, I have learnt a lot more about Cargo's environment variables and trust model.

I have completely refactored the pull request (PR) to address the CodeQL alerts properly and remove the unnecessary precautions.

The PR is now out of draft mode. Please let me know if you think the new approach looks good!

@niStee niStee marked this pull request as ready for review May 16, 2026 16:00
Comment thread src/config.rs Outdated
Comment thread src/config.rs Outdated
@niStee niStee changed the title chore(security): resolve CodeQL path traversal alerts via suppressions and idiomatic path handling refactor: improve path handling in config, node steps, and build May 16, 2026
@niStee niStee requested a review from GideonBear May 16, 2026 23:45
@uwuclxdy uwuclxdy requested a review from Copilot May 17, 2026 21:55

This comment was marked as resolved.

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.

4 participants