shahondin1624/Mitgliederverwaltung
Public Access
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 16 Wiki Activity

security: enforce CSRF protection on POST/DELETE export endpoints (Closes #171)

Browse Source
This commit was merged in pull request #187.
This commit is contained in:
shahondin1624
2026-04-10 18:50:25 +02:00
parent af006d7250
commit 58968331eb
4 changed files with 132 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/Controller/DsgvoController.php
-3
View File
@@ -11,7 +11,6 @@ use OCA\Mitgliederverwaltung\Service\ValidationException;
use OCP\AppFramework\ApiController;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
use OCP\AppFramework\Http\DataDownloadResponse;
use OCP\AppFramework\Http\JSONResponse;
use OCP\IRequest;
@@ -58,7 +57,6 @@ class DsgvoController extends ApiController {
* POST /api/v1/members/{id}/dsgvo-export
* Body: { "password": "..." }
*/
#[NoCSRFRequired]
public function export(int $id): DataDownloadResponse|JSONResponse {
try {
$this->assertAdmin();
@@ -119,7 +117,6 @@ class DsgvoController extends ApiController {
* DELETE /api/v1/members/{id}/dsgvo-delete
* Body: { "confirm": true }
*/
#[NoCSRFRequired]
public function hardDelete(int $id): JSONResponse {
try {
$this->assertAdmin();
lib/Controller/ExportController.php
-5
View File
@@ -155,7 +155,6 @@ class ExportController extends ApiController {
* POST /api/v1/export/members/encrypted
* Body: { "password": "...", "status": "aktiv" }
*/
#[NoCSRFRequired]
public function membersEncrypted(): DataDownloadResponse|JSONResponse {
try {
$password = $this->request->getParam('password');
@@ -204,7 +203,6 @@ class ExportController extends ApiController {
* POST /api/v1/export/fees/encrypted
* Body: { "password": "...", "year": 2026 }
*/
#[NoCSRFRequired]
public function feesEncrypted(): DataDownloadResponse|JSONResponse {
try {
$password = $this->request->getParam('password');
@@ -260,7 +258,6 @@ class ExportController extends ApiController {
* POST /api/v1/export/birthdays/encrypted
* Body: { "password": "...", "month": 4 }
*/
#[NoCSRFRequired]
public function birthdaysEncrypted(): DataDownloadResponse|JSONResponse {
try {
$password = $this->request->getParam('password');
@@ -354,7 +351,6 @@ class ExportController extends ApiController {
*
* Requires explicit confirmation because sensitive data is decrypted.
*/
#[NoCSRFRequired]
public function bundleSensitive(): DataDownloadResponse|JSONResponse {
try {
$confirm = $this->request->getParam('confirm');
@@ -448,7 +444,6 @@ class ExportController extends ApiController {
* POST /api/v1/export/banking/encrypted
* Body: { "password": "..." }
*/
#[NoCSRFRequired]
public function bankingEncrypted(): DataDownloadResponse|JSONResponse {
try {
// Permission check: banking visibility required
lib/Controller/ReportController.php
-1
View File
@@ -163,7 +163,6 @@ class ReportController extends ApiController {
* POST /api/v1/reports/{type}/encrypted
* Body: { "password": "...", ...params }
*/
#[NoCSRFRequired]
public function encrypted(string $type): DataDownloadResponse|JSONResponse {
try {
$password = $this->request->getParam('password');
tests/Unit/CsrfProtectionTest.php
+132
View File
@@ -0,0 +1,132 @@
<?php
declare(strict_types=1);
namespace OCA\Mitgliederverwaltung\Tests\Unit;
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
use PHPUnit\Framework\TestCase;
use ReflectionClass;
/**
* Verify that POST/DELETE endpoints do NOT have #[NoCSRFRequired].
*
* GET endpoints may legitimately skip CSRF checks (standard Nextcloud practice).
* POST/DELETE endpoints must have CSRF protection enabled (no #[NoCSRFRequired]).
*
* Part of Issue #171.
*/
class CsrfProtectionTest extends TestCase {
/**
* Map of controller class => method names that are POST/DELETE and
* MUST NOT have #[NoCSRFRequired].
*/
private function getPostDeleteEndpoints(): array {
return [
// ExportController — encrypted exports use POST
\OCA\Mitgliederverwaltung\Controller\ExportController::class => [
'membersEncrypted',
'feesEncrypted',
'birthdaysEncrypted',
'bundleSensitive',
'bankingEncrypted',
],
// ReportController — encrypted report uses POST
\OCA\Mitgliederverwaltung\Controller\ReportController::class => [
'encrypted',
],
// DsgvoController — export (POST) and hardDelete (DELETE)
\OCA\Mitgliederverwaltung\Controller\DsgvoController::class => [
'export',
'hardDelete',
],
];
}
/**
* Map of controller class => method names that are GET and
* MAY have #[NoCSRFRequired] (read-only endpoints).
*/
private function getGetEndpoints(): array {
return [
\OCA\Mitgliederverwaltung\Controller\ExportController::class => [
'members',
'fees',
'birthdays',
'bundle',
'entityTypes',
'entity',
],
\OCA\Mitgliederverwaltung\Controller\ReportController::class => [
'index',
'preview',
'pdf',
],
];
}
/**
* @dataProvider postDeleteEndpointsProvider
*/
public function testPostDeleteEndpointsRequireCsrf(string $controllerClass, string $method): void {
$rc = new ReflectionClass($controllerClass);
$rm = $rc->getMethod($method);
$attrs = $rm->getAttributes(NoCSRFRequired::class);
$this->assertEmpty(
$attrs,
sprintf(
'%s::%s is a POST/DELETE endpoint but has #[NoCSRFRequired] — '
. 'this bypasses CSRF protection on a state-changing endpoint.',
$controllerClass,
$method
)
);
}
/**
* @dataProvider getEndpointsProvider
*/
public function testGetEndpointsAllowNoCsrf(string $controllerClass, string $method): void {
$rc = new ReflectionClass($controllerClass);
$rm = $rc->getMethod($method);
$attrs = $rm->getAttributes(NoCSRFRequired::class);
$this->assertNotEmpty(
$attrs,
sprintf(
'%s::%s is a GET (read-only) endpoint and should have #[NoCSRFRequired] '
. 'for compatibility with Nextcloud API calls.',
$controllerClass,
$method
)
);
}
public static function postDeleteEndpointsProvider(): array {
$data = [];
$endpoints = (new self('dummy'))->getPostDeleteEndpoints();
foreach ($endpoints as $class => $methods) {
foreach ($methods as $method) {
$shortClass = (new ReflectionClass($class))->getShortName();
$data["$shortClass::$method"] = [$class, $method];
}
}
return $data;
}
public static function getEndpointsProvider(): array {
$data = [];
$endpoints = (new self('dummy'))->getGetEndpoints();
foreach ($endpoints as $class => $methods) {
foreach ($methods as $method) {
$shortClass = (new ReflectionClass($class))->getShortName();
$data["$shortClass::$method"] = [$class, $method];
}
}
return $data;
}
}