GET /api/rooms/export returns 200 with an empty error.csv instead of 403
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 12
- Forks
- 1
- PR merge metrics
- No merged PRs in 30d
Description
Steps to reproduce
- Log in as a user who is not a Nextcloud administrator.
- Request
GET /apps/roomvox/api/rooms/export.
Expected behaviour
403 Forbidden, consistent with every other admin-only endpoint in the app.
Actual behaviour
200 OK with a CSV download named error.csv and an empty body.
Cause
https://github.com/nextcloud/RoomVox/blob/main/lib/Controller/RoomApiController.php#L539-L543
$userId = $this->getCurrentUserId();
if ($userId === null || !$this->groupManager->isAdmin($userId)) {
return new DataDownloadResponse('', 'error.csv', 'text/csv');
}
The permission check is correct — the response to a failed check is not. It
signals the refusal through a filename instead of a status code.
Why this matters
No data leaks: the body is empty, so this is a correctness and consistency
issue rather than a security hole.
What it does break is anything that checks the status code, which is the normal
way to handle an HTTP error. A script that pipes this endpoint into a file gets
a zero-byte CSV and a success status, so a permissions problem looks like "there
are no rooms". The same applies to a non-admin hitting a stale browser tab: an
empty file downloads and nothing explains why.
It is also inconsistent within the same controller — import and
importPreview next to it both return a proper 403 with
{"error": "Admin access required"}.
Suggested fix
Return a real error response, as the neighbouring endpoints do. Note the return
type is DataDownloadResponse, so the signature has to widen (Response, or a
JSONResponse union) to return a 403:
if ($userId === null || !$this->groupManager->isAdmin($userId)) {
return new JSONResponse(['error' => 'Admin access required'], Http::STATUS_FORBIDDEN);
}
Worth checking at the same time: GET /api/rooms/sample-csv
(RoomApiController.php:555) performs no check of its own at all. It is still
admin-gated, because Nextcloud requires an admin session for any controller
method not marked #[NoAdminRequired], so this is not a hole either — but the
protection is implicit where every endpoint around it is explicit.
Notes
Found while auditing docs/architecture/api-reference.md against the code. The
current behaviour is now documented so the reference is accurate, and should be
updated again once this is fixed.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in lib/Controller/RoomApiController.php around lines 539-543 and compare the export response with the neighbouring import and importPreview endpoints. Verify the non-admin and unauthenticated cases return a 403 with the stated JSON error rather than an empty CSV, then check docs/architecture/api-reference.md for the behavior that needs updating.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- api, authorization, backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100