Skip to content

Conversation

@kharkevich
Copy link
Member

BREAKING CHANGE: rework api structure, move some validation to FastAPI side


# If it's a real file and exists, serve it
if requested_path.is_file():
return FileResponse(str(requested_path))

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI 2 months ago

To fix the issue, we need to ensure that filename cannot be used to traverse outside the intended UI directory (ui_dir_path). The best way in this context is to normalize the full candidate path (using os.path.normpath or .resolve()), and then verify that the resulting absolute path starts with the expected root directory (ui_dir_path). Additionally, do not serve files if they are symlinks pointing outside this directory. The recommended approach is to check str(requested_path).startswith(str(ui_dir_path)) after normalizing the path. Update the route handler for /[{filename:path}] (lines 58–73): after constructing requested_path, add a check that validates the normalized absolute path starts with ui_dir_path, denying access otherwise. Edit only this function in mlflow_oidc_auth/routers/ui.py.

You will need to import os (for os.path.commonpath for a more robust check), if not already present, and possibly use string representations to avoid ambiguity.

Suggested changeset 1
mlflow_oidc_auth/routers/ui.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/mlflow_oidc_auth/routers/ui.py b/mlflow_oidc_auth/routers/ui.py
--- a/mlflow_oidc_auth/routers/ui.py
+++ b/mlflow_oidc_auth/routers/ui.py
@@ -5,6 +5,7 @@
 """
 
 from pathlib import Path
+import os
 
 from fastapi import APIRouter, Depends, HTTPException, Request
 from fastapi.responses import FileResponse, JSONResponse, RedirectResponse
@@ -64,11 +65,21 @@
     For SPA routes (including auth with parameters), serve index.html.
     """
     ui_dir_path, index_file = _get_ui_directory()
+    # Construct the requested path and normalize
     requested_path = (ui_dir_path / filename).resolve()
+    # Verify that requested_path is inside ui_dir_path
+    ui_dir_abs = ui_dir_path.resolve()
+    try:
+        # This handles case-insensitivity and symlinks
+        common_prefix = os.path.commonpath([str(requested_path), str(ui_dir_abs)])
+        is_inside = common_prefix == str(ui_dir_abs)
+    except ValueError:
+        is_inside = False
 
-    if requested_path.is_relative_to(ui_dir_path) and requested_path.is_file():
+    if is_inside and requested_path.is_file():
         return FileResponse(str(requested_path))
 
+    # Otherwise, serve SPA index.html (does not leak filesystem info)
     return FileResponse(str(index_file))
 
 
EOF
@@ -5,6 +5,7 @@
"""

from pathlib import Path
import os

from fastapi import APIRouter, Depends, HTTPException, Request
from fastapi.responses import FileResponse, JSONResponse, RedirectResponse
@@ -64,11 +65,21 @@
For SPA routes (including auth with parameters), serve index.html.
"""
ui_dir_path, index_file = _get_ui_directory()
# Construct the requested path and normalize
requested_path = (ui_dir_path / filename).resolve()
# Verify that requested_path is inside ui_dir_path
ui_dir_abs = ui_dir_path.resolve()
try:
# This handles case-insensitivity and symlinks
common_prefix = os.path.commonpath([str(requested_path), str(ui_dir_abs)])
is_inside = common_prefix == str(ui_dir_abs)
except ValueError:
is_inside = False

if requested_path.is_relative_to(ui_dir_path) and requested_path.is_file():
if is_inside and requested_path.is_file():
return FileResponse(str(requested_path))

# Otherwise, serve SPA index.html (does not leak filesystem info)
return FileResponse(str(index_file))


Copilot is powered by AI and may make mistakes. Always verify output.
For SPA routes (including auth with parameters), serve index.html.
"""
ui_dir_path, index_file = _get_ui_directory()
requested_path = (ui_dir_path / filename).resolve()

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI 2 months ago

The optimal way to fix this error is to sanitize and validate the user-controlled filename path parameter before using it. To do this:

  • Normalize the incoming filename before joining with the directory, using os.path.normpath (for strings) or Path(filename).name (for single files).
  • Reject absolute paths and prevent attempts to escape from the UI directory.
  • Construct the requested path by joining the base path and normalized filename, and resolving the final path.
  • Explicitly check the input for absolute paths and .. that escape UI directory, and raise an HTTP error (rather than serving index.html) if validation fails, to avoid silent failures and to surface security problems.
  • Use is_relative_to as an additional check after resolution.

You should make these changes in the serve_spa method, at line 59 and onward. You will also need to import (or use) os.path if you want to use its utilities, but since the project already uses pathlib, you can entirely stick with Path.


Suggested changeset 1
mlflow_oidc_auth/routers/ui.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/mlflow_oidc_auth/routers/ui.py b/mlflow_oidc_auth/routers/ui.py
--- a/mlflow_oidc_auth/routers/ui.py
+++ b/mlflow_oidc_auth/routers/ui.py
@@ -64,8 +64,15 @@
     For SPA routes (including auth with parameters), serve index.html.
     """
     ui_dir_path, index_file = _get_ui_directory()
-    requested_path = (ui_dir_path / filename).resolve()
+    # Normalize the filename to prevent path traversal
+    safe_filename = Path(filename)
 
+    # Reject absolute paths and attempts to escape UI directory
+    if safe_filename.is_absolute() or any(part == ".." for part in safe_filename.parts):
+        raise HTTPException(status_code=400, detail="Invalid filename/path.")
+
+    requested_path = (ui_dir_path / safe_filename).resolve()
+
     if requested_path.is_relative_to(ui_dir_path) and requested_path.is_file():
         return FileResponse(str(requested_path))
 
EOF
@@ -64,8 +64,15 @@
For SPA routes (including auth with parameters), serve index.html.
"""
ui_dir_path, index_file = _get_ui_directory()
requested_path = (ui_dir_path / filename).resolve()
# Normalize the filename to prevent path traversal
safe_filename = Path(filename)

# Reject absolute paths and attempts to escape UI directory
if safe_filename.is_absolute() or any(part == ".." for part in safe_filename.parts):
raise HTTPException(status_code=400, detail="Invalid filename/path.")

requested_path = (ui_dir_path / safe_filename).resolve()

if requested_path.is_relative_to(ui_dir_path) and requested_path.is_file():
return FileResponse(str(requested_path))

Copilot is powered by AI and may make mistakes. Always verify output.
ui_dir_path, index_file = _get_ui_directory()
requested_path = (ui_dir_path / filename).resolve()

if requested_path.is_relative_to(ui_dir_path) and requested_path.is_file():

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI 2 months ago

To comprehensively prevent path traversal and ensure the file served by the route is within the intended ui_dir_path, normalize the constructed file path (using .resolve()), and then perform a robust check that the resolved path is actually a descendant of ui_dir_path. For broad compatibility and reduced risk of edge case bypass, use a try/except block with requested_path.relative_to(ui_dir_path) to ensure the path is within the root, or, alternatively, compare string representations to verify the root prefix. If the requested file does not reside within ui_dir_path, serve the index.html as a fallback as before (or optionally raise an error).

Only the function at line 59–73 (serve_spa) needs to be updated. No new imports are needed, but a compatible and robust path check must be implemented to replace is_relative_to (for portability and clarity).


Suggested changeset 1
mlflow_oidc_auth/routers/ui.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/mlflow_oidc_auth/routers/ui.py b/mlflow_oidc_auth/routers/ui.py
--- a/mlflow_oidc_auth/routers/ui.py
+++ b/mlflow_oidc_auth/routers/ui.py
@@ -66,8 +66,14 @@
     ui_dir_path, index_file = _get_ui_directory()
     requested_path = (ui_dir_path / filename).resolve()
 
-    if requested_path.is_relative_to(ui_dir_path) and requested_path.is_file():
-        return FileResponse(str(requested_path))
+    try:
+        # Ensure requested_path is strictly within ui_dir_path
+        requested_path.relative_to(ui_dir_path)
+        if requested_path.is_file():
+            return FileResponse(str(requested_path))
+    except ValueError:
+        # Path is outside the allowed directory
+        pass
 
     return FileResponse(str(index_file))
 
EOF
@@ -66,8 +66,14 @@
ui_dir_path, index_file = _get_ui_directory()
requested_path = (ui_dir_path / filename).resolve()

if requested_path.is_relative_to(ui_dir_path) and requested_path.is_file():
return FileResponse(str(requested_path))
try:
# Ensure requested_path is strictly within ui_dir_path
requested_path.relative_to(ui_dir_path)
if requested_path.is_file():
return FileResponse(str(requested_path))
except ValueError:
# Path is outside the allowed directory
pass

return FileResponse(str(index_file))

Copilot is powered by AI and may make mistakes. Always verify output.
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
70.4% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)
B Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants