Skip to content

Fix Path Traversal in validate_assignment via Path Normalization #1978

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

Merged
brichet merged 4 commits into from
Apr 17, 2026
+7 −3
Merged

Fix Path Traversal in validate_assignment via Path Normalization #1978

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb21dd4
Fix path traversal validate_assignment
yueyueL Jan 9, 2026
43c4041
Merge branch 'jupyter:main' into main
yueyueL Feb 27, 2026
a8c4774
refactor: use pathlib for path traversal fix
yueyueL Feb 27, 2026
49b4e74
Update nbgrader/server_extensions/validate_assignment/handlers.py
yueyueL Feb 27, 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
5 changes: 4 additions & 1 deletion nbgrader/server_extensions/validate_assignment/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ def load_config(self):
return app.config

def validate_notebook(self, path):
fullpath = os.path.join(self.root_dir, path)
fullpath = os.path.normpath(os.path.join(self.root_dir, path))
root_normalized = os.path.normpath(self.root_dir)
if not fullpath.startswith(root_normalized + os.sep):
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The normalization order may not properly handle all path traversal cases. When the user-supplied path contains absolute paths or path traversal sequences at the beginning, os.path.join(self.root_dir, path) might not behave as expected. If path starts with a slash (absolute path), os.path.join will discard self.root_dir and return just path, which could then bypass the security check after normalization.

Consider validating that the user-supplied path is relative and doesn't start with "/" or contain absolute path components before joining it with root_dir.

Suggested change
fullpath = os.path.normpath(os.path.join(self.root_dir, path))
root_normalized = os.path.normpath(self.root_dir)
if not fullpath.startswith(root_normalized + os.sep):
# Reject absolute paths outright
if os.path.isabs(path):
raise web.HTTPError(400, "Absolute paths are not allowed")
# Normalize the user-supplied path alone
normalized_path = os.path.normpath(path)
# Reject paths that attempt to traverse above the root directory
if normalized_path == os.pardir or normalized_path.startswith(os.pardir + os.sep):
raise web.HTTPError(403, "Access denied: path traversal outside allowed directory")
# Normalize root_dir and construct the full path safely
root_normalized = os.path.normpath(self.root_dir)
fullpath = os.path.normpath(os.path.join(root_normalized, normalized_path))
# Ensure the resulting path is still within the root directory
if os.path.commonpath([root_normalized, fullpath]) != root_normalized:

Copilot uses AI. Check for mistakes.
raise web.HTTPError(403, "Access denied: path outside allowed directory")
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

This security fix lacks test coverage for path traversal scenarios. The existing UI tests in validate_assignment.spec.ts only test normal validation success and failure cases, but don't verify that malicious paths like "../../../etc/passwd" or absolute paths are properly blocked. Consider adding security-focused tests that verify:

  1. Path traversal attempts using "../" are blocked with 403
  2. Absolute paths are rejected
  3. Valid relative paths within root_dir continue to work
  4. Edge cases like empty paths or "." are handled correctly

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The containment check has a critical security flaw. When fullpath equals root_normalized (i.e., when the path parameter is empty or "." or similar), the check fullpath.startswith(root_normalized + os.sep) will fail because the root directory itself doesn't end with a separator. This means accessing the root directory will be incorrectly blocked with a 403 error.

Additionally, this check is vulnerable to a prefix bypass. For example, if root_normalized is "/home/user/notebooks", a malicious path could be crafted to create a fullpath like "/home/user/notebooks_evil/file.ipynb", which would pass the check because it starts with the prefix "/home/user/notebooks" but isn't actually inside the directory.

The correct implementation should use os.path.commonpath or ensure proper path comparison. A safer approach would be:

  1. Check if os.path.commonpath([fullpath, root_normalized]) == root_normalized
  2. Or use fullpath.startswith(root_normalized + os.sep) or fullpath == root_normalized
Suggested change
if not fullpath.startswith(root_normalized + os.sep):
raise web.HTTPError(403, "Access denied: path outside allowed directory")
try:
common = os.path.commonpath([fullpath, root_normalized])
except ValueError:
# Incomparable paths (e.g. different drives on Windows): treat as outside.
raise web.HTTPError(403, "Access denied: path outside allowed directory")
if common != root_normalized:
raise web.HTTPError(403, "Access denied: path outside allowed directory")

Copilot uses AI. Check for mistakes.
try:
config = self.load_config()
Expand Down
Loading