Fix Path Traversal in validate_assignment via Path Normalization

[yueyueL]

Description

This PR fixes a path traversal vulnerability in nbgrader/server_extensions/validate_assignment/handlers.py.

Following the merge of #1984 (Python ≥3.10), the fix has been updated to use pathlib as suggested.

Changes

• Replaced os.path.normpath approach with pathlib.Path for path resolution.
• Used Path.resolve() to handle ../ sequences and symlinks.
• Used Path.is_relative_to() to ensure the path stays within root_dir.
• Requests outside the root directory now raise a 403 Access denied error.

Testing

• Valid notebook paths within root_dir work correctly.
• Malicious paths (e.g., ../../../etc/passwd) are blocked with a 403 response.
• Updated tests to reflect the new pathlib-based implementation.

[github-actions]

👈 Launch a Binder on branch yueyueL/nbgrader/main

[Copilot]

Pull request overview

This PR attempts to fix a path traversal vulnerability in the validate_assignment server extension by adding path normalization and containment checks to prevent unauthorized file access via malicious paths containing ../ sequences.

Key Changes

• Added os.path.normpath() to normalize the joined path and root directory
• Implemented a prefix-based containment check to ensure files remain within the root directory
• Requests attempting path traversal now raise HTTP 403 errors

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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]

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]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Consider adding validation to handle edge cases where the user-supplied path might be None or empty. While these cases may be prevented by the calling code (lines 98-102), defensive programming would suggest checking for these conditions before path operations to provide clearer error messages. For example:

if not path:
    raise web.HTTPError(400, "Path parameter is required")

[Copilot]

The path traversal security fix should have dedicated unit tests to verify that malicious paths (like ../../../etc/passwd, absolute paths, or paths with null bytes) are properly rejected. While UI tests exist for this handler, they don't appear to test these security scenarios. Consider adding test cases that verify:

1. Normal paths within root_dir work correctly
2. Paths with ../ sequences attempting to escape root_dir are blocked with 403
3. Absolute paths are handled correctly
4. Edge cases like empty paths, paths with null bytes, or Windows drive letters (if applicable) are handled safely

Suggested change

	root = Path(self.root_dir).resolve()
	target = (root / path).resolve()
	if not target.is_relative_to(root):
	raise web.HTTPError(403, "Access denied: path outside allowed directory")
	fullpath = str(target)
	# Basic validation of the incoming path
	if not isinstance(path, str):
	raise web.HTTPError(400, "Invalid path type")
	if not path:
	raise web.HTTPError(400, "Missing path")
	if "\x00" in path:
	# Explicitly reject paths containing null bytes
	raise web.HTTPError(400, "Invalid path")
	root = Path(self.root_dir).resolve()
	try:
	target = (root / path).resolve()
	except (TypeError, ValueError):
	# Catch invalid or malformed paths before they reach the validator
	raise web.HTTPError(400, "Invalid path")
	root_str = os.fspath(root)
	target_str = os.fspath(target)
	try:
	common = os.path.commonpath([root_str, target_str])
	except ValueError:
	# On Windows, commonpath raises ValueError for different drives
	raise web.HTTPError(403, "Access denied: path outside allowed directory")
	if common != root_str:
	raise web.HTTPError(403, "Access denied: path outside allowed directory")
	fullpath = target_str