Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a path traversal check to get_static_doc to prevent reading discovery documents outside of the designated directory, and includes a test case to verify this behavior. The review feedback points out that using os.path.basename is platform-dependent and suggests explicitly checking for both forward and backward slashes to ensure robust protection across all operating systems, along with adding a corresponding test case with backslashes.
| if os.path.basename(doc_name) != doc_name: | ||
| return content |
There was a problem hiding this comment.
Using os.path.basename to detect path separators is platform-dependent. On Unix-like systems, os.path.basename does not recognize backslashes (\) as directory separators, whereas on Windows it does. To ensure robust, platform-independent protection against directory traversal attacks, we should explicitly reject both forward slashes (/) and backslashes (\) in the document name, regardless of the operating system the code is running on.
| if os.path.basename(doc_name) != doc_name: | |
| return content | |
| if "/" in doc_name or "\\" in doc_name or os.path.basename(doc_name) != doc_name: | |
| return content |
There was a problem hiding this comment.
The platform dependence is intentional here. os.path is ntpath on Windows, where basename already treats \, /, drive prefixes (C:zoo) and UNC roots as separators, so ..\..\zoo.v1.json gets rejected. On POSIX a backslash is just a filename character, so open() would look for a file literally named ..\..\zoo.v1.json inside DISCOVERY_DOC_DIR and can't leave the directory. Rejecting \ explicitly wouldn't close any extra path, so I've kept the check as is.
| f.write(read_datafile("zoo.json")) | ||
| outside = os.path.join(tmpdir, "zoo") | ||
| # Both an absolute path and one relative to the bundled documents. | ||
| for name in (outside, os.path.relpath(outside, DISCOVERY_DOC_DIR)): |
There was a problem hiding this comment.
To ensure that the platform-independent path traversal protection works correctly across all environments, we should explicitly test a path containing backslashes (\) even when running on Unix-like systems.
| for name in (outside, os.path.relpath(outside, DISCOVERY_DOC_DIR)): | |
| for name in (outside, os.path.relpath(outside, DISCOVERY_DOC_DIR), "..\\..\\zoo"): |
There was a problem hiding this comment.
On Linux/macOS that case would pass trivially, since ..\..\zoo is a plain file name there and never resolves to the planted file, so it wouldn't really exercise anything. On Windows the existing relpath case is already built with backslashes, so that separator gets covered when the suite runs there.
get_static_docjoins{serviceName}.{version}.jsonontoDISCOVERY_DOC_DIRwithout checking where the result lands, so with static discovery (the default forbuild()) a name such as../../../../tmp/evil, an absolute path, or a UNC path on Windows loads a discovery document from outside the bundled set. That document suppliesrootUrl, so the returned client sends its developer key and credentials to whatever host the file names; the HTTP discovery path is not affected because uritemplate percent-encodes the same values. Names that are not a bare file name are now treated as unknown, which surfaces as the existingUnknownApiNameOrVersion.