Skip to content

Commit ad35f5d

Browse files
Fix BOLA/IDOR: enforce process ownership on process and file APIs
Add verify_process_ownership helper and apply it to all /api/process/* endpoints and the /api/file/upload endpoint so authenticated callers can only access processes they own. Returns 404 (not 403) for missing or non-owned processes to avoid confirming existence. status/render_status previously had no auth at all; both now authenticate and check ownership. Adds ownership-enforcement tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 868bd5d commit ad35f5d

5 files changed

Lines changed: 267 additions & 1 deletion

File tree

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
from fastapi import HTTPException
2+
3+
from libs.base.typed_fastapi import TypedFastAPI
4+
from libs.models.entities import Process
5+
from libs.repositories.process_repository import ProcessRepository
6+
7+
8+
async def verify_process_ownership(
9+
app: TypedFastAPI, process_id: str, user_id: str
10+
) -> Process:
11+
"""Ensure the authenticated caller owns the requested process.
12+
13+
Fetches the Process entity and compares its stored ``user_id`` against the
14+
authenticated caller. A missing process and a process owned by a different
15+
user are treated identically and both raise a 404, so callers cannot probe
16+
for the existence of other users' processes.
17+
18+
Args:
19+
app: The FastAPI application carrying the dependency-injection context.
20+
process_id: The identifier of the process being accessed.
21+
user_id: The authenticated caller's user principal id.
22+
23+
Returns:
24+
The owned Process entity.
25+
26+
Raises:
27+
HTTPException: 404 if the process does not exist or is not owned by the
28+
authenticated caller.
29+
"""
30+
if not process_id:
31+
raise HTTPException(status_code=400, detail="Process ID is required")
32+
33+
if not user_id:
34+
raise HTTPException(status_code=401, detail="User not authenticated")
35+
36+
async with app.app_context.create_scope() as scope:
37+
process_repository = scope.get_service(ProcessRepository)
38+
process = await process_repository.get_async(process_id)
39+
40+
if not process or process.user_id != user_id:
41+
# Return 404 (not 403) so the response does not confirm the existence of
42+
# processes belonging to other users.
43+
raise HTTPException(status_code=404, detail="Process not found")
44+
45+
return process

src/backend-api/src/app/routers/router_files.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,12 @@ async def upload_file(
7171

7272
process_record = await processRepository.get_async(process_id)
7373

74+
# Verify the caller owns this process before uploading into it.
75+
# Return 404 (not 403) so the response does not confirm the
76+
# existence of processes belonging to other users.
77+
if not process_record or process_record.user_id != user_id:
78+
raise HTTPException(status_code=404, detail="Process not found")
79+
7480
file_id = str(uuid4())
7581
file_name = re.sub(r"[^\w.-]", "_", file.filename)
7682
blob_path = f"{process_id}/source/{file_name}"

src/backend-api/src/app/routers/router_process.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
from libs.models.entities import Process
1212
from libs.repositories.process_repository import ProcessRepository
1313
from libs.services.auth import get_authenticated_user
14+
from libs.services.authorization import verify_process_ownership
1415
from libs.services.interfaces import ILoggerService
1516
from libs.services.process_services import ProcessService
1617
from routers.models.files import FileInfo
@@ -83,6 +84,11 @@ async def status(process_id: str, request: Request):
8384
f"Process router status endpoint called for process_id: {process_id}"
8485
)
8586

87+
# Authenticate the caller and verify they own this process
88+
authenticated_user = get_authenticated_user(request)
89+
user_id = authenticated_user.user_principal_id
90+
await verify_process_ownership(app, process_id, user_id)
91+
8692
# loading business component for process
8793
processService = app.app_context.get_service(ProcessService)
8894

@@ -99,6 +105,11 @@ async def render_status(process_id: str, request: Request):
99105
f"Process router render status endpoint called for process_id: {process_id}"
100106
)
101107

108+
# Authenticate the caller and verify they own this process
109+
authenticated_user = get_authenticated_user(request)
110+
user_id = authenticated_user.user_principal_id
111+
await verify_process_ownership(app, process_id, user_id)
112+
102113
# loading business component for process
103114
processService = app.app_context.get_service(ProcessService)
104115

@@ -138,6 +149,9 @@ async def upload_files(
138149
if not user_id:
139150
raise HTTPException(status_code=401, detail="User not authenticated")
140151

152+
# Verify the caller owns this process before touching its blob storage
153+
await verify_process_ownership(app, process_id, user_id)
154+
141155
# Make uploaded files list
142156
uploaded_files: list[FileInfo] = []
143157

@@ -195,6 +209,8 @@ async def upload_files(
195209
response.headers["Location"] = f"/process/{process_id}/"
196210

197211
return result_response
212+
except HTTPException:
213+
raise
198214
except Exception as e:
199215
logger_service.log_error(f"Error in upload_files: {str(e)}")
200216
raise HTTPException(status_code=500, detail=f"Error uploading files: {str(e)}")
@@ -233,6 +249,9 @@ async def delete_file(
233249
if not user_id:
234250
raise HTTPException(status_code=401, detail="User not authenticated")
235251

252+
# Verify the caller owns this process before deleting any file
253+
await verify_process_ownership(app, process_id, user_id)
254+
236255
# Get process service
237256
processService = app.app_context.get_service(ProcessService)
238257

@@ -274,6 +293,8 @@ async def delete_file(
274293
status_code=404,
275294
detail=f"File '{file_name}' not found for process '{process_id}'",
276295
)
296+
except HTTPException:
297+
raise
277298
except Exception as e:
278299
logger_service.log_error(f"Error in delete_file: {str(e)}")
279300
raise HTTPException(status_code=500, detail=f"Error deleting file: {str(e)}")
@@ -309,6 +330,9 @@ async def delete_process(
309330
if not user_id:
310331
raise HTTPException(status_code=401, detail="User not authenticated")
311332

333+
# Verify the caller owns this process before deleting its files
334+
await verify_process_ownership(app, process_id, user_id)
335+
312336
# Get process service
313337
processService = app.app_context.get_service(ProcessService)
314338

@@ -333,6 +357,8 @@ async def delete_process(
333357

334358
return result_response
335359

360+
except HTTPException:
361+
raise
336362
except Exception as e:
337363
logger_service.log_error(f"Error in delete_process: {str(e)}")
338364
raise HTTPException(status_code=500, detail=f"Error deleting process: {str(e)}")
@@ -365,6 +391,9 @@ async def start_processing(
365391
if not user_id:
366392
raise HTTPException(status_code=401, detail="User not authenticated")
367393

394+
# Verify the caller owns this process before queueing it for processing
395+
await verify_process_ownership(app, process_id, user_id)
396+
368397
# Get process service
369398
processService = app.app_context.get_service(ProcessService)
370399

@@ -394,6 +423,8 @@ async def start_processing(
394423
"user_id": str(user_id),
395424
"status": "queued",
396425
}
426+
except HTTPException:
427+
raise
397428
except Exception as e:
398429
logger_service.log_error(f"Error in start_processing: {str(e)}")
399430
raise HTTPException(
@@ -425,6 +456,9 @@ async def download_process_files(
425456
if not user_id:
426457
raise HTTPException(status_code=401, detail="User not authenticated")
427458

459+
# Verify the caller owns this process before returning its files
460+
await verify_process_ownership(app, process_id, user_id)
461+
428462
# Get process service
429463
processService = app.app_context.get_service(ProcessService)
430464

@@ -492,6 +526,9 @@ async def get_process_summary(
492526
if not user_id:
493527
raise HTTPException(status_code=401, detail="User not authenticated")
494528

529+
# Verify the caller owns this process before returning its summary
530+
await verify_process_ownership(app, process_id, user_id)
531+
495532
# Get process service
496533
processService = app.app_context.get_service(ProcessService)
497534

@@ -549,6 +586,9 @@ async def get_file_content(
549586
if not user_id:
550587
raise HTTPException(status_code=401, detail="User not authenticated")
551588

589+
# Verify the caller owns this process before returning file content
590+
await verify_process_ownership(app, process_id, user_id)
591+
552592
# Get process service
553593
processService = app.app_context.get_service(ProcessService)
554594

@@ -607,6 +647,9 @@ async def cancel_process(
607647
if not user_id:
608648
raise HTTPException(status_code=401, detail="User not authenticated")
609649

650+
# Verify the caller owns this process before forwarding the kill request
651+
await verify_process_ownership(app, process_id, user_id)
652+
610653
# Get processor control URL from configuration
611654
config = app.app_context.configuration
612655
processor_url = config.processor_control_url or "http://processor:8080"
@@ -704,6 +747,9 @@ async def get_cancel_status(
704747
if not user_id:
705748
raise HTTPException(status_code=401, detail="User not authenticated")
706749

750+
# Verify the caller owns this process before reading its cancel status
751+
await verify_process_ownership(app, process_id, user_id)
752+
707753
# Get processor control URL from configuration
708754
config = app.app_context.configuration
709755
processor_url = config.processor_control_url or "http://processor:8080"

src/backend-api/src/tests/routers/test_router_files.py

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,12 @@ def _make_app(*, process_record=None, file_count=1, blob_helper=None):
2424
process_repo = MagicMock()
2525
process_repo.get_async = AsyncMock(
2626
return_value=process_record
27-
or SimpleNamespace(id="p-1", source_file_count=0, status="initialized")
27+
or SimpleNamespace(
28+
id="p-1",
29+
user_id="user-1",
30+
source_file_count=0,
31+
status="initialized",
32+
)
2833
)
2934
process_repo.update_async = AsyncMock(return_value=None)
3035

@@ -159,3 +164,38 @@ def test_returns_500_when_blob_upload_fails(self):
159164
headers=AUTH_HEADERS,
160165
)
161166
assert res.status_code == 500
167+
168+
def test_returns_404_when_caller_does_not_own_process(self):
169+
# Process exists but is owned by a different user than the caller.
170+
app, mocks = _make_app(
171+
process_record=SimpleNamespace(
172+
id="p-1",
173+
user_id="someone-else",
174+
source_file_count=0,
175+
status="initialized",
176+
)
177+
)
178+
client = TestClient(app)
179+
res = client.post(
180+
"/api/file/upload",
181+
files={"file": ("a.txt", b"x", "text/plain")},
182+
data={"process_id": VALID_PROCESS_ID},
183+
headers=AUTH_HEADERS,
184+
)
185+
assert res.status_code == 404
186+
mocks["blob_helper"].upload_blob.assert_not_awaited()
187+
mocks["process_repo"].update_async.assert_not_awaited()
188+
189+
def test_returns_404_when_process_missing(self):
190+
app, mocks = _make_app(process_record=False)
191+
# Force get_async to return None (no such process).
192+
mocks["process_repo"].get_async = AsyncMock(return_value=None)
193+
client = TestClient(app)
194+
res = client.post(
195+
"/api/file/upload",
196+
files={"file": ("a.txt", b"x", "text/plain")},
197+
data={"process_id": VALID_PROCESS_ID},
198+
headers=AUTH_HEADERS,
199+
)
200+
assert res.status_code == 404
201+
mocks["blob_helper"].upload_blob.assert_not_awaited()

0 commit comments

Comments
 (0)