fix: require auth for agent file downloads#3067
Open
wolfkill wants to merge 5 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the /v1/agent/files/download endpoint by aligning it with nearby agent/skill endpoints’ authentication dependency and restricting downloads to agent-created temp directories, with regression tests intended to prevent future path-boundary regressions.
Changes:
- Adds a user dependency (
Depends(get_user_from_headers)) to the agent file download route. - Removes
ROOT_PATHfrom the allowlist so downloads are restricted to/tmpandPILOT_PATH/tmp. - Adds tests intended to validate the auth dependency and allowlist/path-boundary behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py | Adds user dependency to the route and tightens allowlist by removing ROOT_PATH. |
| packages/dbgpt-app/src/dbgpt_app/tests/test_agent_file_download_api.py | Adds regression tests for dependency presence and allowed/denied download paths. |
Comments suppressed due to low confidence (1)
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py:3989
- The
file_pathparameter description says “Absolute path”, but the handler still accepts relative paths and resolves them againstROOT_PATH. SinceROOT_PATHhas been removed from the allowlist, relative paths will now typically resolve into a directory that is denied, which is confusing for API consumers. Either enforce absolute-only input (and drop the ROOT_PATH join) or update the description/logic so relative paths resolve into an allowed base directory.
async def download_agent_file(
file_path: str = Query(..., description="Absolute path to the file to download"),
user_token: UserRequest = Depends(get_user_from_headers),
):
"""Download a file created by agent tools (shell_interpreter, code_interpreter).
Only files under allowed directories (/tmp, PILOT_PATH/tmp/) can be downloaded.
This prevents arbitrary file access on the server.
"""
from fastapi import HTTPException
from fastapi.responses import FileResponse
from dbgpt.configs.model_config import PILOT_PATH, ROOT_PATH
# If path is not absolute, resolve relative to ROOT_PATH (sandbox working dir)
if not os.path.isabs(file_path):
file_path = os.path.join(ROOT_PATH, file_path)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+28
to
+40
| async def test_agent_file_download_rejects_project_root_file(tmp_path, monkeypatch): | ||
| from dbgpt.configs import model_config | ||
| from dbgpt_app.openapi.api_v1 import agentic_data_api | ||
|
|
||
| project_file = tmp_path / "docker-compose.yml" | ||
| project_file.write_text("services: {}\n", encoding="utf-8") | ||
| monkeypatch.setattr(model_config, "ROOT_PATH", str(tmp_path)) | ||
| monkeypatch.setattr(model_config, "PILOT_PATH", str(tmp_path / "pilot")) | ||
|
|
||
| with pytest.raises(HTTPException) as exc_info: | ||
| await agentic_data_api.download_agent_file(str(project_file)) | ||
|
|
||
| assert exc_info.value.status_code == 403 |
Comment on lines
+44
to
+58
| async def test_agent_file_download_allows_pilot_tmp_file(tmp_path, monkeypatch): | ||
| from dbgpt.configs import model_config | ||
| from dbgpt_app.openapi.api_v1 import agentic_data_api | ||
|
|
||
| pilot_tmp = tmp_path / "pilot" / "tmp" | ||
| pilot_tmp.mkdir(parents=True) | ||
| generated_file = pilot_tmp / "result.txt" | ||
| generated_file.write_text("ok\n", encoding="utf-8") | ||
| monkeypatch.setattr(model_config, "ROOT_PATH", str(tmp_path / "project")) | ||
| monkeypatch.setattr(model_config, "PILOT_PATH", str(tmp_path / "pilot")) | ||
|
|
||
| response = await agentic_data_api.download_agent_file(str(generated_file)) | ||
|
|
||
| assert isinstance(response, FileResponse) | ||
| assert response.path == str(generated_file) |
Comment on lines
3971
to
3975
| @router.get("/v1/agent/files/download") | ||
| async def download_agent_file( | ||
| file_path: str = Query(..., description="Absolute path to the file to download"), | ||
| user_token: UserRequest = Depends(get_user_from_headers), | ||
| ): |
Collaborator
|
Thanks for your fix. A note on removing |
wolfkill
force-pushed
the
fix/agent-file-download-auth
branch
from
10b4568 to
228b143
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/v1/agent/files/download/tmpandPILOT_PATH/tmpas the agent-created file directoriesFixes #3018
Tests