Skip to content

Python: support os.path.basename as a sanitizer in py/path-injection #21719

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
Copilot wants to merge 3 commits into
base: main
Choose a base branch
from
+38 −0
Open

Python: support os.path.basename as a sanitizer in py/path-injection #21719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dcbdd43
Initial plan
Copilot Apr 16, 2026
3a512ae
Add os.path.basename as a sanitizer for py/path-injection
Copilot Apr 16, 2026
3229b90
Apply suggestion: use exists(var | range | formula) idiom in OsPathBa…
Copilot Apr 16, 2026
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
21 changes: 21 additions & 0 deletions python/ql/lib/semmle/python/security/dataflow/PathInjectionCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
*/

private import python
private import semmle.python.dataflow.new.DataFlow

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
semmle.python.ApiGraphs
Loading
.
private import semmle.python.Concepts
private import semmle.python.dataflow.new.RemoteFlowSources
private import semmle.python.dataflow.new.BarrierGuards
private import semmle.python.ApiGraphs

/**
* Provides default sources, and sinks for detecting
Expand Down Expand Up @@ -105,4 +106,24 @@
class SanitizerFromModel extends Sanitizer {
SanitizerFromModel() { ModelOutput::barrierNode(this, "path-injection") }
}

/**
* A call to `os.path.basename`, considered as a sanitizer for path injection.
*
* `os.path.basename` returns the final component of a path, stripping any
* leading directory components. This prevents path traversal attacks since
* the result cannot contain directory separators or relative path components.
Comment on lines +114 to +115
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

This doc comment overstates the guarantees of os.path.basename: it does strip leading directory components, but the result can still be a relative path component such as ".." or ".", so it does not inherently prevent traversal when the value is later joined/resolved. Please adjust the wording to avoid claiming it cannot contain relative path components, and (if it remains a sanitizer) document the limitation/assumptions explicitly.

Suggested change
* leading directory components. This prevents path traversal attacks since
* the result cannot contain directory separators or relative path components.
* leading directory components. However, the result may still be a relative
* path component such as `"."` or `".."`, so this does not by itself prevent
* path traversal if the value is later joined or resolved as a path. This
* modeling therefore assumes the result is used only as a leaf name.

Copilot uses AI. Check for mistakes.
* See https://docs.python.org/3/library/os.path.html#os.path.basename
*/
private class OsPathBasenameCall extends Sanitizer, DataFlow::CallCfgNode {
OsPathBasenameCall() {
exists(API::Node osPathModule |
osPathModule = API::moduleImport("os").getMember("path")
or
osPathModule = API::moduleImport(["posixpath", "ntpath", "genericpath"])
|
this = osPathModule.getMember("basename").getACall()
)
}
}
}
17 changes: 17 additions & 0 deletions python/ql/test/query-tests/Security/CWE-022-PathInjection/path_injection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,3 +150,20 @@ def safe_set_of_files():
if filename in SAFE_FILES:
path = os.path.join(STATIC_DIR, filename)
f = open(path) # $ SPURIOUS: Alert


@app.route("/basename-sanitizer")
def basename_sanitizer():
filename = request.args.get('filename', '')
# Secure mitigation pattern: os.path.basename strips all directory components,
# preventing path traversal attacks.
path = os.path.join(STATIC_DIR, os.path.basename(filename))
f = open(path) # $ result=OK


@app.route("/basename-no-join")
def basename_no_join():
filename = request.args.get('filename', '')
# basename alone also prevents directory traversal
path = os.path.basename(filename)
f = open(path) # $ result=OK
Comment on lines +158 to +169
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

The new test cases (and accompanying comments) treat os.path.basename as a complete mitigation for path traversal. However, basename can return relative path components like ".." or "." (for example, os.path.basename("..") == ".." and os.path.basename("a/..") == ".."), and os.path.join(STATIC_DIR, "..") can still escape STATIC_DIR once resolved by the OS. Consider changing these cases back to $ Alert, or only marking them result=OK when combined with an additional check that rejects ./.. (or when a realpath/abspath+startswith containment check is applied).

Suggested change
# Secure mitigation pattern: os.path.basename strips all directory components,
# preventing path traversal attacks.
path = os.path.join(STATIC_DIR, os.path.basename(filename))
f = open(path) # $ result=OK
@app.route("/basename-no-join")
def basename_no_join():
filename = request.args.get('filename', '')
# basename alone also prevents directory traversal
path = os.path.basename(filename)
f = open(path) # $ result=OK
# `os.path.basename` alone is not a complete mitigation because it can return
# `.` or `..`, which may still escape `STATIC_DIR` after the join is resolved.
path = os.path.join(STATIC_DIR, os.path.basename(filename))
f = open(path) # $ Alert
@app.route("/basename-no-join")
def basename_no_join():
filename = request.args.get('filename', '')
# `os.path.basename` alone does not make user-controlled paths safe.
path = os.path.basename(filename)
f = open(path) # $ Alert

Copilot uses AI. Check for mistakes.
Loading