Skip to content

Constrain server data file paths #1423

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
fallintoplace wants to merge 1 commit into
base: main
Choose a base branch
from
+100 −26
Open

Constrain server data file paths #1423

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
74 changes: 74 additions & 0 deletions python/cuopt_server/cuopt_server/tests/test_file_path_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0

import os

import pytest
from fastapi import HTTPException

from cuopt_server import webserver
from cuopt_server.utils import settings


@pytest.fixture(autouse=True)
def restore_data_dir():
original_data_dir = settings.get_data_dir()
try:
yield
finally:
settings.set_data_dir(original_data_dir)


def test_validate_file_path_returns_file_if_relative_path_stays_in_data_dir(
tmp_path,
):
data_dir = tmp_path / "data"
data_dir.mkdir()
data_file = data_dir / "input.json"
data_file.write_text("{}", encoding="utf-8")
settings.set_data_dir(str(data_dir))

assert webserver.validate_file_path("input.json") == str(data_file)


def test_validate_file_path_rejects_absolute_path(tmp_path):
data_dir = tmp_path / "data"
data_dir.mkdir()
outside_file = tmp_path / "input.json"
outside_file.write_text("{}", encoding="utf-8")
settings.set_data_dir(str(data_dir))

with pytest.raises(HTTPException) as exc_info:
webserver.validate_file_path(str(outside_file))

assert exc_info.value.status_code == 400
assert "relative to CUOPT_DATA_DIR" in exc_info.value.detail


def test_validate_file_path_rejects_parent_directory_escape(tmp_path):
data_dir = tmp_path / "data"
data_dir.mkdir()
outside_file = tmp_path / "input.json"
outside_file.write_text("{}", encoding="utf-8")
settings.set_data_dir(str(data_dir))

with pytest.raises(HTTPException) as exc_info:
webserver.validate_file_path("../input.json")

assert exc_info.value.status_code == 400
assert "stay inside CUOPT_DATA_DIR" in exc_info.value.detail


def test_validate_file_path_rejects_symlink_escape(tmp_path):
data_dir = tmp_path / "data"
data_dir.mkdir()
outside_file = tmp_path / "input.json"
outside_file.write_text("{}", encoding="utf-8")
os.symlink(outside_file, data_dir / "linked-input.json")
settings.set_data_dir(str(data_dir))

with pytest.raises(HTTPException) as exc_info:
webserver.validate_file_path("linked-input.json")

assert exc_info.value.status_code == 400
assert "stay inside CUOPT_DATA_DIR" in exc_info.value.detail
Comment on lines +22 to +74

@coderabbitai coderabbitai Bot Jun 10, 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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Cover the remaining new validation branches.

This suite never exercises the CUOPT_DATA_DIR-unset branch or the new "must be a regular file" branch. A regression in either path would still leave this PR green even though both are part of the new validator behavior.

Suggested tests 
+def test_validate_file_path_rejects_when_data_dir_is_unset():
+    settings.set_data_dir("")
+
+    with pytest.raises(HTTPException) as exc_info:
+        webserver.validate_file_path("input.json")
+
+    assert exc_info.value.status_code == 400
+    assert "data directory not set" in exc_info.value.detail
+
+
+def test_validate_file_path_rejects_directory_inside_data_dir(tmp_path):
+    data_dir = tmp_path / "data"
+    nested_dir = data_dir / "subdir"
+    nested_dir.mkdir(parents=True)
+    settings.set_data_dir(str(data_dir))
+
+    with pytest.raises(HTTPException) as exc_info:
+        webserver.validate_file_path("subdir")
+
+    assert exc_info.value.status_code == 400
+    assert "specified data file does not exist" in exc_info.value.detail

As per coding guidelines, **/*test*.{cpp,py}: "Add test coverage for edge cases (empty, infeasible, unbounded, degenerate) when adding new code paths" and python/**/tests/**: "Regression coverage for fixed bugs".

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cuopt_server/cuopt_server/tests/test_file_path_validation.py` around
lines 22 - 74, Add tests in test_file_path_validation.py to cover the two
missing branches in webserver.validate_file_path: (1) the CUOPT_DATA_DIR-unset
path by clearing/unsetting settings.set_data_dir (or not calling it) and calling
validate_file_path to assert it raises the expected HTTPException/status and
message about CUOPT_DATA_DIR being unset; and (2) the "must be a regular file"
path by creating a non-regular target inside the data dir (e.g., a subdirectory
or FIFO) and calling validate_file_path on that name to assert it raises
HTTPException with the "regular file" (or similar) detail. Use the existing test
helpers and exception assertions (pytest.raises and exc_info.value.detail) and
reference validate_file_path and settings.set_data_dir so the new tests exercise
those branches.

Source: Coding guidelines

52 changes: 26 additions & 26 deletions python/cuopt_server/cuopt_server/webserver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,36 +190,36 @@ def get_output_name(resultdir, CUOPT_DATA_FILE, CUOPT_RESULT_FILE):


# Validate if given data file and file path exists
def validate_file_path(CUOPT_DATA_FILE):
def validate_file_path(cuopt_data_file):
ddir = settings.get_data_dir()
try:
file_path = os.path.join(ddir, CUOPT_DATA_FILE)
if not ddir:
logging.error("cuopt data directory not set!")
# If no datadir was set but the path is relative,
# this can't work
if not CUOPT_DATA_FILE.startswith("/"):
raise ValueError(
f"cuopt server was started without data directory "
f"defined but local path {CUOPT_DATA_FILE} "
"was specified"
)
if not os.path.exists(file_path):
logging.error(f"File path '{file_path}' doesn't exist")
msg = f"Specified path '{file_path}' does not exist"
if CUOPT_DATA_FILE.startswith("/"):
dir = os.path.dirname(CUOPT_DATA_FILE)
if not os.path.isdir(dir):
msg += f". Absolute path '{dir}' does not exist"
msg += ". Perhaps you did not intend to "
"specify an absolute path?"
raise ValueError(msg)
except Exception as e:
if not ddir:
logging.error("cuopt data directory not set!")
raise HTTPException(
status_code=400,
detail="cuopt data directory not set",
)

if os.path.isabs(cuopt_data_file):
raise HTTPException(
status_code=400,
detail="cuopt-data-file must be relative to CUOPT_DATA_DIR",
)

root = os.path.realpath(ddir)
file_path = os.path.realpath(os.path.join(root, cuopt_data_file))
if os.path.commonpath([root, file_path]) != root:
raise HTTPException(
status_code=400,
detail="cuopt-data-file must stay inside CUOPT_DATA_DIR",
)

if not os.path.isfile(file_path):
logging.error(f"File path '{file_path}' doesn't exist")
raise HTTPException(
status_code=400,
detail="unable to read "
"optimization data from file %s, %s" % (file_path, str(e)),
detail=f"specified data file does not exist: {cuopt_data_file}",
Comment on lines +216 to +220

@coderabbitai coderabbitai Bot Jun 10, 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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid logging the resolved server path for missing files.

Line 217 logs file_path, which includes the fully resolved CUOPT_DATA_DIR path for a user-controlled miss. That leaks internal filesystem layout into server logs. Log cuopt_data_file or a generic message instead.

Suggested change 
-        logging.error(f"File path '{file_path}' doesn't exist")
+        logging.error(
+            "Requested cuopt data file '%s' does not exist inside CUOPT_DATA_DIR",
+            cuopt_data_file,
+        )

As per coding guidelines, python/cuopt_server/**: "No credential or internal-path leakage in error responses or logs".

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cuopt_server/cuopt_server/webserver.py` around lines 216 - 220, The
log call leaks the resolved internal filesystem path via file_path; update the
error logging in webserver.py so it does not include file_path (use
cuopt_data_file or a generic message instead) and ensure any HTTPException
detail or other logs never expose the resolved server path; locate the check
using file_path and cuopt_data_file and replace the logging.error(f"File path
'{file_path}'...") with a safe message that references only cuopt_data_file or a
non-path generic string.

Source: Coding guidelines

)

return file_path


Expand Down