Stop a typed-in storage quota from 500ing when a client is created

Filling the "Storage quota (MB)" field on the new-client form raised a TypeError and the request died with a 500. Leaving it blank worked, which is why it reached a release: that path goes through `null ?? 0`, and the 0 is an int. The `integer` validation rule checks that a value looks like an integer. It does not convert it. `$request->validate()` returns the raw input, so the form field arrives as the string "2048" -- and the create form types that field as a string in React, so it is a string even over JSON. Both controllers declare strict_types, so handing it to `ClientAccounts::create()`'s `int $storageQuotaMb` is a TypeError. Fixed on both surfaces that call create(): the staff screen and /api/v1/clients. The API twin had the same defect, reachable by sending the quota as a quoted JSON value or a form-encoded body -- its own create test only ever sent a JSON number. Two more call sites had the same shape and are cast too, though nothing sends them a string today: the share-link download cap and a comment's reply_to. Both are safe only because a frontend file happens to call Number() first, which is a fact about that file rather than anything the signature guarantees. The null in each is preserved rather than collapsed to 0 -- "no cap" is not a cap of zero. `storage_quota_mb` is also cast on User and Invitation. The column is an unsignedInteger and both docblocks already promise int; it is read straight into provision()'s typed parameter when an invitation is redeemed, and which type a driver hands back is not something that call site should depend on. Found on the new files-test rehearsal instance, on its first real use, against the same build the whole fleet is running.
2026-09-20 18:43:20 +00:00 · 2026-09-12 21:16:49 -03:00
parent f06a3c7ab3
commit 7c7ba7cd53
8 changed files with 75 additions and 4 deletions
@@ -175,6 +175,13 @@ class User extends Authenticatable implements HasLocalePreference
            'ldap_synced_at' => 'datetime',
            'active' => 'boolean',
            'account_requested' => 'boolean',
+            // The column is an unsignedInteger and the docblock above
+            // already promises int. Saying so here is what makes that true
+            // for a reader as well: it is passed straight into typed
+            // signatures (ClientAccounts::create, ClientProvisioning::
+            // provision), and whether a driver hands back 2048 or "2048"
+            // is not something those call sites should depend on.
+            'storage_quota_mb' => 'integer',
            'erase_after' => 'datetime',
            'email_verified_at' => 'datetime',
            'password' => 'hashed',
@@ -144,7 +144,12 @@ class ClientsController extends Controller
            name: $validated['name'],
            email: $validated['email'],
            password: $validated['password'],
-            storageQuotaMb: $validated['storage_quota_mb'] ?? 0,
+            // As on the staff screen, and for the same reason: the
+            // `integer` rule accepts a numeric string and does not convert
+            // it. A JSON number arrives as an int and was fine; a
+            // form-encoded body or a quoted JSON value is a string, and
+            // this file is strict_types.
+            storageQuotaMb: (int) ($validated['storage_quota_mb'] ?? 0),
            welcome: false,
        );

@@ -158,7 +158,13 @@ class ClientsController extends Controller
            name: $validated['name'],
            email: $validated['email'],
            password: $validated['password'],
-            storageQuotaMb: $validated['storage_quota_mb'] ?? 0,
+            // Cast, because `integer` validates without converting:
+            // $request->validate() hands back the raw input, so a form
+            // field arrives as the string "2048" and this file is
+            // strict_types. Filling the quota in was a 500; leaving it
+            // blank went through null ?? 0 as an int, which is why it
+            // survived to the fleet.
+            storageQuotaMb: (int) ($validated['storage_quota_mb'] ?? 0),
            welcome: false,
        );

@@ -62,6 +62,9 @@ class Invitation extends Model
    {
        return [
            'expires_at' => 'datetime',
+            // Read straight into provision()'s `int $storageQuotaMb` when
+            // the invitation is redeemed -- see the same cast on User.
+            'storage_quota_mb' => 'integer',
        ];
    }

@@ -61,7 +61,9 @@ class FileCommentsController extends Controller
            $viewer,
            CommentVisibility::from($validated['visibility']),
            $validated['body'],
-            $this->replyTarget($viewer, $file, $validated['reply_to'] ?? null),
+            // Cast for the reason ShareLinksController gives: `integer`
+            // does not convert, and replyTarget() takes a strict ?int.
+            $this->replyTarget($viewer, $file, isset($validated['reply_to']) ? (int) $validated['reply_to'] : null),
        );

        return response()->json($this->payload($viewer, $file), 201);
@@ -89,7 +89,18 @@ class ShareLinksController extends Controller
            file: $file,
            creator: $user,
            expiresAt: $user->can('set_file_expiration_date') ? $expiresAt : null,
-            maxDownloads: $user->can('limit_downloads') ? $validated['max_downloads'] ?? null : null,
+            // Cast, and null kept as null rather than falling through a
+            // bare (int) that would turn "no cap" into a cap of zero. The
+            // `integer` rule validates a numeric string without converting
+            // it, and this file is strict_types, so an uncast "5" is a
+            // TypeError against `?int $maxDownloads`. Nothing sends one
+            // today only because files/edit.tsx calls Number() first --
+            // which is a fact about a frontend file, not a guarantee this
+            // signature has. It cost a 500 on the client form, where the
+            // same field was typed as a string.
+            maxDownloads: $user->can('limit_downloads') && ($validated['max_downloads'] ?? null) !== null
+                ? (int) $validated['max_downloads']
+                : null,
            token: $validated['token'] ?? null,
        );

@@ -157,6 +157,22 @@ test('creating a client still records every field', function () {
        ->where('user_id', $client->id)->where('client_custom_field_id', $optIn->id)->value('value'))->toBe('0');
 });

+test('a client can be created with the quota sent as a string', function () {
+    // Same defect as the staff screen: the `integer` rule accepts "2048"
+    // and hands it on unconverted, into a strict_types call expecting an
+    // int. A JSON number was always fine, which is why the API's own
+    // create test did not see it -- so this sends the quoted form a
+    // form-encoded caller or a cautious JSON serialiser would.
+    $this->withToken($this->token)->postJson('/api/v1/clients', [
+        'name' => 'Acme Ltd',
+        'email' => 'string-quota@acme.test',
+        'password' => 'a-sufficiently-long-password',
+        'storage_quota_mb' => '2048',
+    ])->assertStatus(201);
+
+    expect(User::query()->where('email', 'string-quota@acme.test')->sole()->storage_quota_mb)->toBe(2048);
+});
+
 test('a client can be created', function () {
    $this->withToken($this->token)->postJson('/api/v1/clients', [
        'name' => 'Acme Ltd',
@@ -120,6 +120,27 @@ test('the settings form still edits the stored setting, never the floor', functi
    );
 });

+test('creating a client with a quota typed into the form stores it', function () {
+    // The 500 this exists for. `integer` validates a numeric string and
+    // does not convert it, so the form's "2048" reached
+    // ClientAccounts::create()'s `int $storageQuotaMb` under strict_types
+    // and raised a TypeError. Blank was fine -- null ?? 0 is an int -- so
+    // every existing test here went through the one branch that worked,
+    // and it shipped to the whole fleet on 2.4.1.
+    //
+    // post() and not postJson(): a form body is strings, which is the
+    // condition. A JSON number would pass on the unfixed code.
+    $this->actingAs($this->admin)->post('/clients', [
+        'name' => 'Quota Ltd',
+        'email' => 'typed-quota@example.test',
+        'password' => 'a-sufficiently-long-password',
+        'password_confirmation' => 'a-sufficiently-long-password',
+        'storage_quota_mb' => '2048',
+    ])->assertRedirect()->assertSessionDoesntHaveErrors();
+
+    expect(User::query()->where('email', 'typed-quota@example.test')->sole()->storage_quota_mb)->toBe(2048);
+});
+
 test('clearing the storage quota field to blank on the edit form resets it to inherit the site default', function () {
    app(Settings::class)->set(Setting::DefaultClientStorageQuotaMb, 150);
    $client = User::factory()->client()->create(['storage_quota_mb' => 100]);