Skip to content
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.

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

Add CodeQL recommendation against Path.Combine #18865

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

carldybdahl-microsoft
Copy link

The docs for Path.Combine warns:

Important

This method assumes that the first argument is an absolute path and that the following argument or arguments are relative paths. If this is not the case, and particularly if any subsequent arguments are strings input by the user, call the Join or TryJoin method instead.

This commit adds a corresponding CodeQL query to recommend against Path.Combine.

@Copilot Copilot bot review requested due to automatic review settings February 26, 2025 09:20
@carldybdahl-microsoft carldybdahl-microsoft requested a review from a team as a code owner February 26, 2025 09:20

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

csharp/ql/test/query-tests/Bad Practices/Path Combine/PathCombine.cs:3

  • [nitpick] The class name 'EmptyCatchBlock' does not convey its purpose related to Path.Combine testing. Consider renaming it to 'PathCombineTest' for better clarity.
class EmptyCatchBlock
Copy link
Contributor

github-actions bot commented Feb 26, 2025

QHelp previews:

csharp/ql/src/Bad Practices/PathCombine.qhelp

Call to System.IO.Path.Combine

Path.Combine may silently drop its earlier arguments if its later arguments are absolute paths. E.g. Path.Combine("C:\\Users\\Me\\Documents", "C:\\Program Files\\") == "C:\\Program Files".

Recommendation

Use Path.Join instead.

References

@@ -0,0 +1 @@
Bad Practices/PathCombine.ql

Check warning

Code scanning / CodeQL

Query test without inline test expectations Warning test

Query test does not use inline test expectations.
@michaelnebel
Copy link
Contributor

michaelnebel commented Feb 26, 2025

I will start a DCA run to see how this impacts the security and quality suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants