Skip to content

fix: reject path separators in get_static_doc names - #2834

Open
farkhalit wants to merge 1 commit into
googleapis:mainfrom
farkhalit:static-doc-path-check
Open

farkhalit wants to merge 1 commit into
googleapis:mainfrom
farkhalit:static-doc-path-check

Conversation

@farkhalit

Copy link
Copy Markdown

get_static_doc joins {serviceName}.{version}.json onto DISCOVERY_DOC_DIR without checking where the result lands, so with static discovery (the default for build()) 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 supplies rootUrl, 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 existing UnknownApiNameOrVersion.

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@farkhalit
farkhalit requested a review from a team as a code owner October 5, 2026 19:24
@product-auto-label product-auto-label Bot added the size: s Pull request size is small. label Oct 5, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment on lines +73 to +74
if os.path.basename(doc_name) != doc_name:
return content

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-medium medium

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.

Suggested change
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

Comment thread tests/test_discovery.py
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)):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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.

Suggested change
for name in (outside, os.path.relpath(outside, DISCOVERY_DOC_DIR)):
for name in (outside, os.path.relpath(outside, DISCOVERY_DOC_DIR), "..\\..\\zoo"):

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant