Skip to content

Commit 5658600

Browse files
LukeTowersLukeTowers
committed
Ensure FileUpload formwidget only ever looks at related file records
1 parent fd74844 commit 5658600

3 files changed

Lines changed: 300 additions & 20 deletions

File tree

‎formwidgets/FileUpload.php‎

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,20 @@
1-
<?php namespace Backend\FormWidgets;
2-
3-
use Db;
4-
use Input;
5-
use Event;
6-
use Request;
7-
use Response;
8-
use Validator;
9-
use Backend\Widgets\Form;
1+
<?php
2+
3+
namespace Backend\FormWidgets;
4+
105
use Backend\Classes\FormField;
116
use Backend\Classes\FormWidgetBase;
12-
use Winter\Storm\Filesystem\Definitions as FileDefinitions;
13-
use ApplicationException;
14-
use ValidationException;
7+
use Backend\Widgets\Form;
158
use Exception;
9+
use Illuminate\Support\Facades\Response;
10+
use System\Models\File;
11+
use Winter\Storm\Exception\ApplicationException;
12+
use Winter\Storm\Exception\ValidationException;
13+
use Winter\Storm\Filesystem\Definitions as FileDefinitions;
14+
use Winter\Storm\Support\Facades\DB;
15+
use Winter\Storm\Support\Facades\Event;
16+
use Winter\Storm\Support\Facades\Input;
17+
use Winter\Storm\Support\Facades\Validator;
1618

1719
/**
1820
* File upload field
@@ -98,7 +100,7 @@ class FileUpload extends FormWidgetBase
98100
protected $defaultAlias = 'fileupload';
99101

100102
/**
101-
* @var Backend\Widgets\Form The embedded form for modifying the properties of the selected file
103+
* @var Form The embedded form for modifying the properties of the selected file
102104
*/
103105
protected $configFormWidget;
104106

@@ -175,14 +177,21 @@ protected function prepareVars()
175177
/**
176178
* Get the file record for this request, returns false if none available
177179
*
178-
* @return System\Models\File|false
180+
* @return File|false
179181
*/
180182
protected function getFileRecord()
181183
{
182184
$record = false;
183185

184186
if (!empty(post('file_id'))) {
185-
$record = $this->getRelationModel()->find(post('file_id')) ?: false;
187+
// Scope the lookup to this widget's own relation (including any files
188+
// bound via the current deferred-binding session) so that an
189+
// attacker-controlled file_id cannot reference an arbitrary
190+
// System\Models\File record belonging to another model. See
191+
// GHSA-3277-h8g9-qj5f.
192+
$record = $this->getRelationObject()
193+
->withDeferred($this->sessionKey)
194+
->find(post('file_id')) ?: false;
186195
}
187196

188197
return $record;
@@ -351,11 +360,25 @@ public function onRemoveAttachment(): void
351360
public function onSortAttachments(): void
352361
{
353362
if ($sortData = post('sortOrder')) {
363+
// Only reorder files that actually belong to this widget's relation
364+
// (including the current deferred-binding session), never arbitrary
365+
// System\Models\File rows referenced by a posted id. See
366+
// GHSA-3277-h8g9-qj5f.
367+
$keyName = $this->getRelationModel()->getKeyName();
368+
$validIds = $this->getRelationObject()
369+
->withDeferred($this->sessionKey)
370+
->pluck($keyName)
371+
->all();
372+
373+
$sortData = array_intersect_key($sortData, array_flip($validIds));
374+
if (empty($sortData)) {
375+
return;
376+
}
377+
354378
$ids = array_keys($sortData);
355379
$orders = array_values($sortData);
356380

357-
$fileModel = $this->getRelationModel();
358-
$fileModel->setSortableOrder($ids, $orders);
381+
$this->getRelationModel()->setSortableOrder($ids, $orders);
359382
}
360383
}
361384

‎tests/formwidgets/FileUploadScopingTest.php‎

Lines changed: 255 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,255 @@
1+
<?php
2+
3+
namespace Backend\Tests\FormWidgets;
4+
5+
use Backend\Classes\Controller;
6+
use Backend\Classes\FormField;
7+
use Backend\FormWidgets\FileUpload;
8+
use Database\Tester\Models\User as TesterUser;
9+
use System\Models\File as FileModel;
10+
use System\Tests\Bootstrap\PluginTestCase;
11+
use Winter\Storm\Database\Model;
12+
use Winter\Storm\Exception\ApplicationException;
13+
14+
/**
15+
* Ensures FileUpload::getFileRecord() only ever resolves a posted `file_id` that
16+
* belongs to the widget's own relation (including the current deferred-binding
17+
* session), and never an arbitrary System\Models\File row from another record.
18+
*
19+
* Regression test for GHSA-3277-h8g9-qj5f (IDOR via attacker-controlled file_id).
20+
*/
21+
class FileUploadScopingTest extends PluginTestCase
22+
{
23+
protected string $imagePath;
24+
25+
public function setUp(): void
26+
{
27+
parent::setUp();
28+
29+
$this->imagePath = base_path(
30+
'modules/system/tests/fixtures/plugins/database/tester/assets/images/avatar.png'
31+
);
32+
}
33+
34+
/**
35+
* The legitimate case: a file already attached to this record (parent saved)
36+
* must still resolve, otherwise editing existing attachments breaks.
37+
*/
38+
public function testResolvesOwnAttachedFile(): void
39+
{
40+
$user = $this->makeUser();
41+
$file = $user->avatar()->create(['data' => $this->imagePath]);
42+
$user->reloadRelations();
43+
44+
$widget = $this->makeWidget($user);
45+
46+
$this->postFileId($file->id);
47+
$this->assertSameFile($file, $widget->exposedGetFileRecord());
48+
}
49+
50+
/**
51+
* The legitimate upload-in-progress case: a freshly uploaded file that is only
52+
* bound to the parent via the deferred-binding session (parent not yet saved)
53+
* must still resolve. This guards against a fix that scopes to the relation but
54+
* forgets ->withDeferred($sessionKey), which would break the upload workflow.
55+
*/
56+
public function testResolvesOwnDeferredFile(): void
57+
{
58+
$sessionKey = 'fileupload-scoping-deferred';
59+
60+
$user = $this->makeUser();
61+
62+
$file = new FileModel;
63+
$file->data = $this->imagePath;
64+
$file->save();
65+
$user->avatar()->add($file, $sessionKey);
66+
67+
$widget = $this->makeWidget($user, $sessionKey);
68+
69+
$this->postFileId($file->id);
70+
$this->assertSameFile($file, $widget->exposedGetFileRecord());
71+
}
72+
73+
/**
74+
* The security property: a file_id belonging to a DIFFERENT record must never
75+
* resolve, even though it is a valid row in the global system_files table.
76+
*/
77+
public function testDoesNotResolveAnotherRecordsFile(): void
78+
{
79+
$owner = $this->makeUser();
80+
$attacker = $this->makeUser();
81+
82+
$victimFile = $owner->avatar()->create(['data' => $this->imagePath]);
83+
$owner->reloadRelations();
84+
85+
// Widget is bound to the attacker's record, but posts the victim's file id.
86+
$widget = $this->makeWidget($attacker);
87+
$this->postFileId($victimFile->id);
88+
89+
$this->assertFalse(
90+
$widget->exposedGetFileRecord(),
91+
'A file_id from another record must not resolve through this widget.'
92+
);
93+
}
94+
95+
/**
96+
* A non-existent / empty file_id must resolve to false, not throw.
97+
*/
98+
public function testDoesNotResolveMissingOrEmptyFileId(): void
99+
{
100+
$widget = $this->makeWidget($this->makeUser());
101+
102+
$this->postFileId(999999);
103+
$this->assertFalse($widget->exposedGetFileRecord());
104+
105+
$this->postFileId(null);
106+
$this->assertFalse($widget->exposedGetFileRecord());
107+
}
108+
109+
/**
110+
* End-to-end assertion mirroring the advisory PoC at the widget level:
111+
* saving the attachment config while posting another record's file_id must
112+
* NOT mutate that file's metadata.
113+
*/
114+
public function testSaveConfigCannotMutateAnotherRecordsFile(): void
115+
{
116+
$owner = $this->makeUser();
117+
$attacker = $this->makeUser();
118+
119+
$victimFile = $owner->avatar()->create([
120+
'data' => $this->imagePath,
121+
'title' => 'original title',
122+
'description' => 'original description',
123+
]);
124+
$owner->reloadRelations();
125+
126+
// Mirror a real AJAX request: the POST payload (including file_id) is
127+
// present before the widget is constructed for the request.
128+
$this->postData([
129+
'file_id' => $victimFile->id,
130+
'avatar' => [
131+
'title' => 'hijacked title',
132+
'description' => 'hijacked description',
133+
],
134+
]);
135+
136+
$widget = $this->makeWidget($attacker);
137+
138+
// Either the handler rejects the out-of-scope file, or it silently no-ops;
139+
// either way the victim's metadata must be untouched.
140+
try {
141+
$widget->onSaveAttachmentConfig();
142+
} catch (ApplicationException $ex) {
143+
// Acceptable: handler refused to find the out-of-scope file.
144+
}
145+
146+
$victimFile = FileModel::find($victimFile->id);
147+
$this->assertSame('original title', $victimFile->title);
148+
$this->assertSame('original description', $victimFile->description);
149+
}
150+
151+
/**
152+
* The legitimate case: reordering files that belong to this record's own
153+
* relation must update their sort_order.
154+
*/
155+
public function testSortReordersOwnFiles(): void
156+
{
157+
$user = $this->makeUser();
158+
$first = $user->photos()->create(['data' => $this->imagePath]);
159+
$second = $user->photos()->create(['data' => $this->imagePath]);
160+
$user->reloadRelations();
161+
162+
$widget = $this->makeWidget($user, null, 'photos');
163+
164+
// Swap their order.
165+
$this->postData(['sortOrder' => [
166+
$first->id => 20,
167+
$second->id => 10,
168+
]]);
169+
$widget->onSortAttachments();
170+
171+
$this->assertEquals(20, FileModel::find($first->id)->sort_order);
172+
$this->assertEquals(10, FileModel::find($second->id)->sort_order);
173+
}
174+
175+
/**
176+
* The security property: onSortAttachments must not write sort_order to a
177+
* file that belongs to a different record, even with a valid file id.
178+
*/
179+
public function testSortIgnoresAnotherRecordsFile(): void
180+
{
181+
$owner = $this->makeUser();
182+
$attacker = $this->makeUser();
183+
184+
$victimFile = $owner->photos()->create(['data' => $this->imagePath]);
185+
$owner->reloadRelations();
186+
$originalOrder = FileModel::find($victimFile->id)->sort_order;
187+
188+
// Attacker drives their own photos widget but posts the victim's file id.
189+
$widget = $this->makeWidget($attacker, null, 'photos');
190+
$this->postData(['sortOrder' => [
191+
$victimFile->id => $originalOrder + 9999,
192+
]]);
193+
$widget->onSortAttachments();
194+
195+
$this->assertEquals(
196+
$originalOrder,
197+
FileModel::find($victimFile->id)->sort_order,
198+
'Sort order of another record\'s file must not change.'
199+
);
200+
}
201+
202+
//
203+
// Helpers
204+
//
205+
206+
protected function makeUser(): TesterUser
207+
{
208+
$user = new TesterUser;
209+
$user->name = 'Test User';
210+
$user->email = uniqid('user', true) . '@test.com';
211+
$user->save();
212+
213+
return $user;
214+
}
215+
216+
protected function makeWidget(Model $model, ?string $sessionKey = null, string $relation = 'avatar'): FileUploadTestable
217+
{
218+
$formField = new FormField($relation, ucfirst($relation));
219+
$formField->valueFrom = $relation;
220+
221+
return new FileUploadTestable(new Controller, $formField, [
222+
'model' => $model,
223+
'sessionKey' => $sessionKey,
224+
]);
225+
}
226+
227+
protected function postFileId($id): void
228+
{
229+
$this->postData(['file_id' => $id]);
230+
}
231+
232+
protected function postData(array $data): void
233+
{
234+
request()->setMethod('POST');
235+
request()->request->replace($data);
236+
}
237+
238+
protected function assertSameFile($expected, $actual): void
239+
{
240+
$this->assertInstanceOf(FileModel::class, $actual);
241+
$this->assertEquals($expected->id, $actual->id);
242+
}
243+
}
244+
245+
/**
246+
* Exposes the protected getFileRecord() so the scoping boundary can be asserted
247+
* directly without rendering partials.
248+
*/
249+
class FileUploadTestable extends FileUpload
250+
{
251+
public function exposedGetFileRecord()
252+
{
253+
return $this->getFileRecord();
254+
}
255+
}

‎traits/FormModelWidget.php‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
1-
<?php namespace Backend\Traits;
1+
<?php
2+
3+
namespace Backend\Traits;
24

3-
use Lang;
4-
use ApplicationException;
55
use Exception;
6+
use Illuminate\Support\Facades\Lang;
67
use Winter\Storm\Database\Model;
78
use Winter\Storm\Database\Relations\Relation;
9+
use Winter\Storm\Exception\ApplicationException;
810

911
/**
1012
* Form Model Widget Trait

0 commit comments

Comments
 (0)