Skip to content

fix: reject path separators in get_static_doc names #2834

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
farkhalit wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: reject path separators in get_static_doc names #2834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 5 additions & 0 deletions googleapiclient/discovery_cache/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ def get_static_doc(serviceName, version):
content = None
doc_name = "{}.{}.json".format(serviceName, version)

# serviceName and version identify an API, they are not paths. Only look
# for the document directly inside of DISCOVERY_DOC_DIR.
if os.path.basename(doc_name) != doc_name:
return content
Comment on lines +73 to +74

@gemini-code-assist gemini-code-assist Bot Oct 5, 2026

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

@farkhalit farkhalit Oct 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.


try:
with open(os.path.join(DISCOVERY_DOC_DIR, doc_name), "r") as f:
content = f.read()
Expand Down
20 changes: 19 additions & 1 deletion tests/test_discovery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import pickle
import re
import sys
import tempfile
import unittest
from unittest import mock
import urllib
Expand Down Expand Up @@ -83,7 +84,7 @@
build_from_document,
key2param,
)
from googleapiclient.discovery_cache import DISCOVERY_DOC_MAX_AGE
from googleapiclient.discovery_cache import DISCOVERY_DOC_DIR, DISCOVERY_DOC_MAX_AGE
from googleapiclient.discovery_cache.base import Cache
from googleapiclient.errors import (
HttpError,
Expand Down Expand Up @@ -1499,6 +1500,23 @@ def test_unknown_api_when_static_discovery_true(self):
with self.assertRaises(UnknownApiNameOrVersion):
build("doesnotexist", "v3", cache_discovery=False, static_discovery=True)

def test_static_discovery_does_not_read_outside_of_documents_dir(self):
http = HttpMockSequence([({"status": "400"}, "")])
with tempfile.TemporaryDirectory() as tmpdir:
with open(os.path.join(tmpdir, "zoo.v1.json"), "w") as f:
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)):

@gemini-code-assist gemini-code-assist Bot Oct 5, 2026

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"):

@farkhalit farkhalit Oct 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

with self.assertRaises(UnknownApiNameOrVersion):
build(
name,
"v1",
http=http,
cache_discovery=False,
static_discovery=True,
)


class DictCache(Cache):
def __init__(self):
Expand Down
Loading