Skip to content

GET /api/rooms/export returns 200 with an empty error.csv instead of 403 #42

Description

@Rikdekker

Steps to reproduce

  1. Log in as a user who is not a Nextcloud administrator.
  2. 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.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions