Skip to content

Commit 3e25ffa

Browse files
authored
Merge pull request #3277 from nextcloud/backport/3276/stable5.2
[stable5.2] fix: cap pagination limit for submissions endpoint
2 parents 6ca889d + acc1c83 commit 3e25ffa

File tree

3 files changed

+112
-0
lines changed

3 files changed

+112
-0
lines changed

lib/Constants.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,11 @@ class Constants {
234234
*/
235235
public const QUESTION_EXTRASETTINGS_OTHER_PREFIX = 'system-other-answer:';
236236

237+
/**
238+
* Maximum number of submissions returned per paginated API request.
239+
*/
240+
public const SUBMISSIONS_LIMIT_MAX = 1000;
241+
237242
public const SUPPORTED_EXPORT_FORMATS = [
238243
'csv' => 'text/csv',
239244
'ods' => 'application/vnd.oasis.opendocument.spreadsheet',

lib/Controller/ApiController.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1180,6 +1180,11 @@ public function getSubmissions(int $formId, ?string $query = null, ?int $limit =
11801180
return new DataDownloadResponse($submissionsData, $fileName, Constants::SUPPORTED_EXPORT_FORMATS[$fileFormat]);
11811181
}
11821182

1183+
// Cap user-supplied page size to avoid unbounded resource use
1184+
if ($limit !== null) {
1185+
$limit = max(1, min($limit, Constants::SUBMISSIONS_LIMIT_MAX));
1186+
}
1187+
11831188
// Load submissions and currently active questions
11841189
if ($canSeeAllSubmissions) {
11851190
$submissions = $this->submissionService->getSubmissions($formId, null, $query, $limit, $offset);

tests/Unit/Controller/ApiControllerTest.php

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,108 @@ public function testGetSubmissions(array $submissions, array $questions, array $
297297
$this->assertEquals(new DataResponse($expected), $this->apiController->getSubmissions(1));
298298
}
299299

300+
public function testGetSubmissions_limitIsCapped(): void {
301+
$form = new Form();
302+
$form->setId(1);
303+
$form->setOwnerId('otherUser');
304+
305+
$this->formsService->expects($this->once())
306+
->method('getFormIfAllowed')
307+
->with(1, Constants::PERMISSION_RESULTS)
308+
->willReturn($form);
309+
310+
$this->formsService->expects($this->once())
311+
->method('getPermissions')
312+
->with($form)
313+
->willReturn([Constants::PERMISSION_RESULTS]);
314+
315+
// An overly large limit must be clamped to SUBMISSIONS_LIMIT_MAX
316+
$this->submissionService->expects($this->once())
317+
->method('getSubmissions')
318+
->with(1, null, null, Constants::SUBMISSIONS_LIMIT_MAX, 0)
319+
->willReturn([]);
320+
321+
$this->submissionMapper->expects($this->once())
322+
->method('countSubmissions')
323+
->with(1)
324+
->willReturn(0);
325+
326+
$this->formsService->expects($this->once())
327+
->method('getQuestions')
328+
->with(1)
329+
->willReturn([]);
330+
331+
$this->apiController->getSubmissions(1, limit: 100000);
332+
}
333+
334+
public function testGetSubmissions_limitFloorIsOne(): void {
335+
$form = new Form();
336+
$form->setId(1);
337+
$form->setOwnerId('otherUser');
338+
339+
$this->formsService->expects($this->once())
340+
->method('getFormIfAllowed')
341+
->with(1, Constants::PERMISSION_RESULTS)
342+
->willReturn($form);
343+
344+
$this->formsService->expects($this->once())
345+
->method('getPermissions')
346+
->with($form)
347+
->willReturn([Constants::PERMISSION_RESULTS]);
348+
349+
// A non-positive limit must be floored to 1
350+
$this->submissionService->expects($this->once())
351+
->method('getSubmissions')
352+
->with(1, null, null, 1, 0)
353+
->willReturn([]);
354+
355+
$this->submissionMapper->expects($this->once())
356+
->method('countSubmissions')
357+
->with(1)
358+
->willReturn(0);
359+
360+
$this->formsService->expects($this->once())
361+
->method('getQuestions')
362+
->with(1)
363+
->willReturn([]);
364+
365+
$this->apiController->getSubmissions(1, limit: 0);
366+
}
367+
368+
public function testGetSubmissions_nullLimitStaysNull(): void {
369+
$form = new Form();
370+
$form->setId(1);
371+
$form->setOwnerId('otherUser');
372+
373+
$this->formsService->expects($this->once())
374+
->method('getFormIfAllowed')
375+
->with(1, Constants::PERMISSION_RESULTS)
376+
->willReturn($form);
377+
378+
$this->formsService->expects($this->once())
379+
->method('getPermissions')
380+
->with($form)
381+
->willReturn([Constants::PERMISSION_RESULTS]);
382+
383+
// null limit is preserved so the summary view can load all submissions
384+
$this->submissionService->expects($this->once())
385+
->method('getSubmissions')
386+
->with(1, null, null, null, 0)
387+
->willReturn([]);
388+
389+
$this->submissionMapper->expects($this->once())
390+
->method('countSubmissions')
391+
->with(1)
392+
->willReturn(0);
393+
394+
$this->formsService->expects($this->once())
395+
->method('getQuestions')
396+
->with(1)
397+
->willReturn([]);
398+
399+
$this->apiController->getSubmissions(1);
400+
}
401+
300402
public function testExportSubmissions_invalidForm() {
301403
$this->formsService->expects($this->once())
302404
->method('getFormIfAllowed')

0 commit comments

Comments
 (0)