nextcloud / nextcloud/RoomVox

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

Open Beginner friendly
#42 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
PHP
Stars
12
Forks
1
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.