Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ Lines marked **Upgrade step** need you to act after updating.
- A webhook answer that isn't valid UTF-8 text is now saved and shows on the submissions page, with each unreadable character as "?". Before, it wasn't saved and the webhook was called up to 5 times. With `QUEUE_CONNECTION=sync`, the submission itself failed with a server error. (#284)
- Opening a question in the editor no longer saves its answer settings, so "Saving…" does not show up for nothing. They save only when you change a setting. (#288)
- When a save fails, the red bar with Retry no longer makes the editor taller than your window. The page stays as tall as the window, so your last question and the end screen stay in reach. (#290)
- Long answers are now accepted up to what the database can hold (about 36,000 plain letters, fewer with accents, emoji or non-Latin script), and a longer one is refused with a clear message and nothing is saved. Before, the limit was lower (about 30,000 plain letters) and the refusal was a technical message. (#293)

### Security

Expand Down
5 changes: 2 additions & 3 deletions app/Http/Controllers/Api/FormSubmitController.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use App\Http\Controllers\Controller;
use App\Models\Form;
use App\Models\FormSession;
use App\Rules\MaxSize;
use App\Rules\AnswerFitsColumn;
use Illuminate\Http\Request;
use Knuckles\Scribe\Attributes\BodyParam;
use Knuckles\Scribe\Attributes\Group;
Expand Down Expand Up @@ -41,8 +41,7 @@ public function __invoke(Request $request, Form $form)
'token' => 'required|string',
'is_uploading' => 'boolean',
'payload' => 'array|nullable|max:'.$form->formBlocks()->count(),
// an answer is stored encrypted in a TEXT column (64 KB), encryption almost doubles it
'payload.*' => [new MaxSize(30000)],
'payload.*' => [new AnswerFitsColumn],
]);

// a preview stores and sends nothing, see CreateFormSessionController
Expand Down
10 changes: 9 additions & 1 deletion app/Models/FormSessionResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ class FormSessionResponse extends Model
{
use HasFactory;

// the `value` column is a TEXT
public const VALUE_MAX_BYTES = 65535;

protected $guarded = [];

protected $casts = [
Expand Down Expand Up @@ -50,7 +53,12 @@ public function formSessionUploads()

public function setValueAttribute($new)
{
$this->attributes['value'] = config('app.debug') ? json_encode($new) : encrypt($new);
$this->attributes['value'] = self::toStored($new);
}

public static function toStored($value): string
{
return config('app.debug') ? json_encode($value) : encrypt($value);
}

public function getValueAttribute()
Expand Down
32 changes: 32 additions & 0 deletions app/Rules/AnswerFitsColumn.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace App\Rules;

use App\Models\FormSessionResponse;
use Closure;
use Illuminate\Contracts\Validation\ValidationRule;

class AnswerFitsColumn implements ValidationRule
{
public function validate(string $attribute, mixed $value, Closure $fail): void
{
if ($this->tooLong($value)) {
$fail('This answer is too long. Please shorten it and send it again.');
}
}

// walks lists at any depth, the way FormBlock::submit does
private function tooLong(mixed $value): bool
{
if (! is_array($value)) {
return false;
}

if (has_string_keys($value)) {
// measured the way it is stored: encrypted, or as JSON in debug mode
return strlen(FormSessionResponse::toStored($value['payload'] ?? null)) > FormSessionResponse::VALUE_MAX_BYTES;
}

return collect($value)->contains(fn ($answer) => $this->tooLong($answer));
}
Comment on lines +11 to +31

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A list inside a list skips this check and gets a 500 again. FormBlock::submit walks lists at any depth (app/Models/FormBlock.php:211), but this rule looks only one level deep. For [[{actionId, payload: 70,000 × "a"}]], $answer['payload'] is null, so the check passes and MariaDB throws SQLSTATE 22001. On main, the 30,000 cap measured the whole value, so the same request got a 422. Only a hand-made API call sends this, and nothing is saved, but it is the 500 this PR removes.

Walk the answer the same way the save does:

Suggested change
public function validate(string $attribute, mixed $value, Closure $fail): void
{
// a block sends one answer, or a list of answers (checkbox, consent)
$answers = is_array($value) && ! has_string_keys($value) ? $value : [$value];
foreach ($answers as $answer) {
// measured the way it is stored: encrypted, or as JSON in debug mode
if (is_array($answer) && strlen(FormSessionResponse::toStored($answer['payload'] ?? null)) > FormSessionResponse::VALUE_MAX_BYTES) {
$fail('This answer is too long. Please shorten it and send it again.');
return;
}
}
}
public function validate(string $attribute, mixed $value, Closure $fail): void
{
if ($this->tooLong($value)) {
$fail('This answer is too long. Please shorten it and send it again.');
}
}
// walks the answer the way FormBlock::submit stores it: a list holds more answers (checkbox, consent), at any depth
private function tooLong(mixed $value): bool
{
if (! is_array($value)) {
return false;
}
if (! has_string_keys($value)) {
return collect($value)->contains(fn ($answer) => $this->tooLong($answer));
}
// measured the way it is stored: encrypted, or as JSON in debug mode
return strlen(FormSessionResponse::toStored($value['payload'] ?? null)) > FormSessionResponse::VALUE_MAX_BYTES;
}

With this, that request gets the 422. FormSessionTest.php plus my checks (checkbox lists, numbers, null, a 33,000-byte answer) pass: 38 tests on MariaDB, debug on and off.

}
58 changes: 54 additions & 4 deletions tests/Feature/FormSessionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -338,23 +338,73 @@
'many short parameters' => [collect(range(1, 600))->mapWithKeys(fn ($i) => ["p$i" => 'x'])->all()],
]);

test('an answer over the size cap is rejected and not stored', function ($template) {
test('an answer at the size cap is stored, one character over is rejected with a clear message', function ($template, $char) {
$form = Form::factory()->create();
$form->applyTemplate($template);
$session = FormSession::factory()->create(['form_id' => $form->id]);
$block = $form->formBlocks[0];

// the longest answer of this character that still fits the TEXT column once stored
$fits = fn (int $count) => strlen(FormSessionResponse::toStored(str_repeat($char, $count))) <= 65535;
[$low, $high] = [1, 65535];
while ($low < $high) {
$middle = intdiv($low + $high + 1, 2);
$fits($middle) ? $low = $middle : $high = $middle - 1;
}
$atCap = str_repeat($char, $low);

expect($fits($low + 1))->toBeFalse();

$this->json('POST', route('api.public.forms.submit', ['form' => $form->uuid]), [
'token' => $session->token,
'payload' => $block->getSubmitPayload(str_repeat('a', 30000)),
])->assertStatus(422)->assertJsonValidationErrors('payload.'.$block->uuid);
'payload' => $block->getSubmitPayload($atCap.$char),
])
->assertStatus(422)
->assertJsonValidationErrors('payload.'.$block->uuid)
->assertJsonPath('message', 'This answer is too long. Please shorten it and send it again.');

expect(FormSessionResponse::count())->toBe(0);

$this->json('POST', route('api.public.forms.submit', ['form' => $form->uuid]), [
'token' => $session->token,
'payload' => $block->getSubmitPayload(str_repeat('a', 29000)),
'payload' => $block->getSubmitPayload($atCap),
])->assertOk();

expect(FormSessionResponse::count())->toBe(1);
})->with('templates')->with(['one byte per character' => 'a', 'two bytes per character' => 'ä', 'four bytes per character' => '😀']);

test('one answer over the size cap saves none of the other answers', function ($template) {
$form = Form::factory()->create();
$form->applyTemplate($template);
$session = FormSession::factory()->create(['form_id' => $form->id]);
[$first, , , $last] = $form->formBlocks;

$this->json('POST', route('api.public.forms.submit', ['form' => $form->uuid]), [
'token' => $session->token,
'payload' => [
...$first->getSubmitPayload('froggy@mailfrog.com'),
...$last->getSubmitPayload(str_repeat('a', 65535)),
],
])
->assertStatus(422)
->assertJsonPath('message', 'This answer is too long. Please shorten it and send it again.');

expect(FormSessionResponse::count())->toBe(0);
})->with('templates');

test('a too-long answer inside nested lists is rejected and not stored', function ($template) {
$form = Form::factory()->create();
$form->applyTemplate($template);
$session = FormSession::factory()->create(['form_id' => $form->id]);
$block = $form->formBlocks[0];
$answer = $block->getSubmitPayload(str_repeat('a', 70000))[$block->uuid];

$this->json('POST', route('api.public.forms.submit', ['form' => $form->uuid]), [
'token' => $session->token,
'payload' => [$block->uuid => [[$answer]]],
])->assertStatus(422)->assertJsonValidationErrors('payload.'.$block->uuid);

expect(FormSessionResponse::count())->toBe(0);
})->with('templates');

test('more answers than the form has blocks are rejected', function ($template) {
Expand Down
Loading