From a7571aa305b53f0adc1a825032115151091d99e3 Mon Sep 17 00:00:00 2001 From: Nicky Gerritsen Date: Fri, 24 Nov 2023 11:05:53 +0100 Subject: [PATCH] Add support for validation for db config values. Add validation to all integer fields and to timelimit_overshoot. This fixes #2227. --- etc/db-config.yaml | 68 ++++++++++++++++++ .../Controller/API/GeneralInfoController.php | 19 ++++- .../src/Controller/Jury/ConfigController.php | 22 ++++-- webapp/src/Entity/Configuration.php | 2 +- webapp/src/Service/ConfigurationService.php | 70 ++++++++++++++++--- .../Constraints/TimelimitOvershoot.php | 16 +++++ .../TimelimitOvershootValidator.php | 26 +++++++ webapp/templates/jury/config.html.twig | 39 ++++++----- webapp/tests/Unit/BaseTestCase.php | 6 +- .../Controller/API/ConfigControllerTest.php | 16 +++++ .../Controller/Jury/ConfigControllerTest.php | 19 ++++- .../Unit/Service/ConfigurationServiceTest.php | 8 +++ 12 files changed, 273 insertions(+), 38 deletions(-) create mode 100644 webapp/src/Validator/Constraints/TimelimitOvershoot.php create mode 100644 webapp/src/Validator/Constraints/TimelimitOvershootValidator.php diff --git a/etc/db-config.yaml b/etc/db-config.yaml index f74df89c527..2644261b115 100644 --- a/etc/db-config.yaml +++ b/etc/db-config.yaml @@ -17,6 +17,9 @@ default_value: 20 public: true description: Penalty time in minutes per wrong submission (if eventually solved). + constraint: Range + constraint_arguments: + min: 0 - name: results_prio type: array_keyval default_value: @@ -47,56 +50,87 @@ default_value: 2097152 public: false description: Maximum memory usage (in kB) by submissions. This includes the shell which starts the compiled solution and also any interpreter like the Java VM, which takes away approx. 300MB! Can be overridden per problem. + constraint: Range + constraint_arguments: + min: 1 - name: output_limit type: int default_value: 8192 public: false description: Maximum output (in kB) submissions may generate. Any excessive output is truncated, so this should be greater than the maximum testdata output. Can be overridden per problem. + constraint: Range + constraint_arguments: + min: 1 - name: process_limit type: int default_value: 64 public: false description: Maximum number of processes that the submission is allowed to start (including shell and possibly interpreters). + constraint: Range + constraint_arguments: + min: 1 - name: sourcesize_limit type: int default_value: 256 public: true description: Maximum source code size (in kB) of a submission. + constraint: Range + constraint_arguments: + min: 1 - name: sourcefiles_limit type: int default_value: 100 public: true description: Maximum number of source files in one submission. Set to `1` to disable multi-file submissions. + constraint: Range + constraint_arguments: + min: 1 - name: script_timelimit type: int default_value: 30 public: false description: Maximum seconds available for compile/compare scripts. This is a safeguard against malicious code and buggy scripts, so a reasonable but large amount should do. + constraint: Range + constraint_arguments: + min: 1 - name: script_memory_limit type: int default_value: 2097152 public: false description: Maximum memory usage (in kB) by compile/compare scripts. This is a safeguard against malicious code and buggy script, so a reasonable but large amount should do. + constraint: Range + constraint_arguments: + min: 1 - name: script_filesize_limit type: int default_value: 2621440 public: false description: Maximum filesize (in kB) compile/compare scripts may write. Submission will fail with compiler-error when trying to write more, so this should be greater than any **intermediate or final** result written by compilers. + constraint: Range + constraint_arguments: + min: 1 - name: timelimit_overshoot type: string default_value: 1s|10% public: false description: Time that submissions are kept running beyond timelimit before being killed. Specify as `Xs` for X seconds, `Y%` as percentage, or a combination of both separated by one of `+|&` for the sum, maximum, or minimum of both. + constraint: TimelimitOvershoot - name: output_storage_limit type: int default_value: 50000 public: false description: Maximum size of error/system output stored in the database (in bytes); use `-1` to disable any limits. + constraint: Range + constraint_arguments: + min: 1 - name: output_display_limit type: int default_value: 2000 public: false description: Maximum size of run/diff/error/system output shown in the jury interface (in bytes); use `-1` to disable any limits. + constraint: Range + constraint_arguments: + min: 1 - name: lazy_eval_results type: int default_value: 1 @@ -106,21 +140,34 @@ 1: Lazy 2: Full judging 3: Only on request + constraint: Range + constraint_arguments: + min: 1 + max: 3 - name: judgehost_warning type: int default_value: 30 public: false description: Time in seconds after a judgehost last checked in before showing its status as `warning`. + constraint: Range + constraint_arguments: + min: 1 - name: judgehost_critical type: int default_value: 120 public: false description: Time in seconds after a judgehost last checked in before showing its status as `critical`. + constraint: Range + constraint_arguments: + min: 1 - name: diskspace_error type: int default_value: 1048576 public: false description: Minimum free disk space (in kB) on judgehosts before posting an internal error. + constraint: Range + constraint_arguments: + min: 0 - name: default_compare type: string default_value: compare @@ -210,6 +257,10 @@ 0: never 1: only on compilation error(s) 2: always + constraint: Range + constraint_arguments: + min: 0 + max: 2 - name: show_sample_output type: bool default_value: false @@ -240,6 +291,9 @@ default_value: 200 public: false description: Maximum width/height of a thumbnail for uploaded testcase images. + constraint: Range + constraint_arguments: + min: 0 - name: show_limits_on_team_page type: bool default_value: true @@ -259,6 +313,9 @@ default_value: 0 public: false description: Maximum width of team column on scoreboard. Leave `0` for no maximum. + constraint: Range + constraint_arguments: + min: 0 - name: show_public_stats type: bool default_value: true @@ -278,6 +335,10 @@ 0: Always 1: After login 2: After first submission + constraint: Range + constraint_arguments: + min: 0 + max: 2 - category: Authentication description: Options related to authentication. items: @@ -319,6 +380,10 @@ 0: all local 1: configuration data external 2: configuration and live data external + constraint: Range + constraint_arguments: + min: 0 + max: 2 docdescription: See :doc:`the chapter on running DOMjudge as a shadow system` for more information. - name: external_contest_sources_allow_untrusted_certificates type: bool @@ -349,6 +414,9 @@ default_value: 120 public: false description: Time in seconds after an external contest source reader last checked in before showing its status as `critical`. + constraint: Range + constraint_arguments: + min: 0 - name: adminer_enabled type: bool default_value: false diff --git a/webapp/src/Controller/API/GeneralInfoController.php b/webapp/src/Controller/API/GeneralInfoController.php index bcc75840487..0de157989db 100644 --- a/webapp/src/Controller/API/GeneralInfoController.php +++ b/webapp/src/Controller/API/GeneralInfoController.php @@ -248,6 +248,18 @@ public function getDatabaseConfigurationAction( description: 'The full configuration after change', content: new OA\JsonContent(type: 'object') )] + #[OA\Response( + response: 400, + description: 'An error occurred while saving the configuration', + content: new OA\JsonContent( + properties: [ + new OA\Property( + property: 'errors', + type: 'object' + ) + ] + ) + )] #[OA\RequestBody( required: true, content: [ @@ -255,9 +267,12 @@ public function getDatabaseConfigurationAction( new OA\MediaType(mediaType: 'application/json'), ] )] - public function updateConfigurationAction(Request $request): array + public function updateConfigurationAction(Request $request): JsonResponse|array { - $this->config->saveChanges($request->request->all(), $this->eventLogService, $this->dj); + $errors = $this->config->saveChanges($request->request->all(), $this->eventLogService, $this->dj); + if (!empty($errors)) { + return new JsonResponse(['errors' => $errors], 400); + } return $this->config->all(false); } diff --git a/webapp/src/Controller/Jury/ConfigController.php b/webapp/src/Controller/Jury/ConfigController.php index 461b6fda235..acedfa07793 100644 --- a/webapp/src/Controller/Jury/ConfigController.php +++ b/webapp/src/Controller/Jury/ConfigController.php @@ -43,9 +43,6 @@ public function indexAction(EventLogService $eventLogService, Request $request): ->getQuery() ->getResult(); if ($request->getMethod() == 'POST' && $request->request->has('save')) { - $this->addFlash('scoreboard_refresh', 'After changing specific ' . - 'settings, you might need to refresh the scoreboard.'); - $data = []; foreach ($request->request->all() as $key => $value) { if (str_starts_with($key, 'config_')) { @@ -65,8 +62,17 @@ public function indexAction(EventLogService $eventLogService, Request $request): } } } - $this->config->saveChanges($data, $eventLogService, $this->dj); - return $this->redirectToRoute('jury_config'); + $errors = $this->config->saveChanges($data, $eventLogService, $this->dj, $options); + + if (empty($errors)) { + $this->addFlash('scoreboard_refresh', 'After changing specific ' . + 'settings, you might need to refresh the scoreboard.'); + + return $this->redirectToRoute('jury_config'); + } else { + $this->addFlash('danger', 'Some errors occurred while saving configuration, ' . + 'please check the data you entered.'); + } } $categories = []; @@ -76,12 +82,16 @@ public function indexAction(EventLogService $eventLogService, Request $request): } } $allData = []; + $activeCategory = null; foreach ($categories as $category) { $data = []; foreach ($specs as $specName => $spec) { if ($spec['category'] !== $category) { continue; } + if (isset($errors[$specName]) && $activeCategory === null) { + $activeCategory = $category; + } $data[] = [ 'name' => $specName, 'type' => $spec['type'], @@ -103,6 +113,8 @@ public function indexAction(EventLogService $eventLogService, Request $request): } return $this->render('jury/config.html.twig', [ 'options' => $allData, + 'errors' => $errors ?? [], + 'activeCategory' => $activeCategory ?? 'Scoring', ]); } diff --git a/webapp/src/Entity/Configuration.php b/webapp/src/Entity/Configuration.php index 82713459fbe..dc41cbed879 100644 --- a/webapp/src/Entity/Configuration.php +++ b/webapp/src/Entity/Configuration.php @@ -27,7 +27,7 @@ class Configuration type: 'json', options: ['comment' => 'Content of the configuration variable (JSON encoded)']) ] - private mixed $value; + private mixed $value = null; public function getConfigid(): int { diff --git a/webapp/src/Service/ConfigurationService.php b/webapp/src/Service/ConfigurationService.php index eeeccf9d730..cd59df5c8e8 100644 --- a/webapp/src/Service/ConfigurationService.php +++ b/webapp/src/Service/ConfigurationService.php @@ -6,6 +6,7 @@ use App\Entity\Configuration; use App\Entity\Executable; use App\Entity\Judging; +use BadMethodCallException; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\NonUniqueResultException; use InvalidArgumentException; @@ -16,6 +17,8 @@ use Symfony\Component\Config\Resource\FileResource; use Symfony\Component\DependencyInjection\Attribute\Autoconfigure; use Symfony\Component\DependencyInjection\Attribute\Autowire; +use Symfony\Component\Validator\ConstraintViolationInterface; +use Symfony\Component\Validator\Validator\ValidatorInterface; #[Autoconfigure(public: true)] class ConfigurationService @@ -27,6 +30,7 @@ public function __construct( protected readonly LoggerInterface $logger, #[Autowire(service: 'config_cache_factory')] protected readonly ConfigCacheFactoryInterface $configCache, + protected readonly ValidatorInterface $validator, #[Autowire('%kernel.debug%')] protected readonly bool $debug, #[Autowire('%kernel.cache_dir%')] @@ -144,12 +148,15 @@ function (ConfigCacheInterface $cache) { * Save the changes from the given request. * * @throws NonUniqueResultException + * @param array|null $options + * @return array Error per item */ public function saveChanges( array $dataToSet, EventLogService $eventLog, - DOMJudgeService $dj - ): void { + DOMJudgeService $dj, + ?array &$options = null + ): array { $specs = $this->getConfigSpecification(); foreach ($specs as &$spec) { $spec = $this->addOptions($spec); @@ -157,12 +164,13 @@ public function saveChanges( unset($spec); /** @var Configuration[] $options */ - $options = $this->em->createQueryBuilder() + $options = $options ?? $this->em->createQueryBuilder() ->from(Configuration::class, 'c', 'c.name') ->select('c') ->getQuery() ->getResult(); + $errors = []; $logUnverifiedJudgings = false; foreach ($specs as $specName => $spec) { $oldValue = $spec['default_value']; @@ -174,6 +182,7 @@ public function saveChanges( $optionToSet = new Configuration(); $optionToSet->setName($specName); $optionIsNew = true; + $options[$specName] = $optionToSet; } if (!array_key_exists($specName, $dataToSet)) { if ($spec['type'] == 'bool') { @@ -190,6 +199,43 @@ public function saveChanges( } else { $val = $dataToSet[$specName]; } + + if (isset($spec['constraint'])) { + $constraintClasses = [ + sprintf('App\Validator\Constraints\%s', $spec['constraint']), + sprintf('Symfony\Component\Validator\Constraints\%s', $spec['constraint']), + ]; + $constraint = null; + foreach ($constraintClasses as $constraintClass) { + if (class_exists($constraintClass)) { + $constraintArguments = (array)($spec['constraint_arguments'] ?? []); + $constraint = new $constraintClass(...$constraintArguments); + break; + } + } + + if ($constraint === null) { + throw new BadMethodCallException( + sprintf( + 'Constraint classes %s do not exist', + implode(', ', $constraintClasses) + ) + ); + } + + $errorsForField = $this->validator->validate($val, $constraint); + if ($errorsForField->count()) { + $errors[$specName] = implode( + ', ', + array_map( + fn(ConstraintViolationInterface $violation + ) => $violation->getMessage(), + iterator_to_array($errorsForField) + ) + ); + } + } + if ($specName == 'verification_required' && $oldValue && !$val) { // If toggled off, we have to send events for all judgings @@ -239,22 +285,28 @@ public function saveChanges( [ $specName, $spec['type'] ] ); } - if ($optionToSet->getValue() != $oldValue) { - $valJson = $dj->jsonEncode($optionToSet->getValue()); - $dj->auditlog('configuration', $specName, 'updated', $valJson); - if ($optionIsNew) { - $this->em->persist($optionToSet); + if (!isset($errors[$specName])) { + if ($optionToSet->getValue() != $oldValue) { + $valJson = $dj->jsonEncode($optionToSet->getValue()); + $dj->auditlog('configuration', $specName, 'updated', $valJson); + if ($optionIsNew) { + $this->em->persist($optionToSet); + } } } } - $this->em->flush(); + if (empty($errors)) { + $this->em->flush(); + } if ($logUnverifiedJudgings) { $this->logUnverifiedJudgings($eventLog); } $this->dbConfigCache = null; + + return $errors; } /** diff --git a/webapp/src/Validator/Constraints/TimelimitOvershoot.php b/webapp/src/Validator/Constraints/TimelimitOvershoot.php new file mode 100644 index 00000000000..48637d8488c --- /dev/null +++ b/webapp/src/Validator/Constraints/TimelimitOvershoot.php @@ -0,0 +1,16 @@ +context->buildViolation($constraint->message)->addViolation(); + } + } +} diff --git a/webapp/templates/jury/config.html.twig b/webapp/templates/jury/config.html.twig index 262b718eb0f..adfaf9767e1 100644 --- a/webapp/templates/jury/config.html.twig +++ b/webapp/templates/jury/config.html.twig @@ -21,14 +21,14 @@
{% for category in options %} -
+
    @@ -49,7 +49,7 @@
    {% elseif option.type == 'int' %} {% if option.options is not null %} - {% else %} - {% elseif option.type == 'string' or option.type == 'enum' %} {% if option.options is not null %} - {% else %} - {% endif %} @@ -85,7 +85,7 @@ {% for key,val in option.value %}
    {% if option.key_options is not null %} - {% else %} - {% endif %} {% if option.value_options is not null %} - {% else %} -
    {% if option.key_options is not null %} - {% else %} - {% endif %} {% if option.value_options is not null %} - {% else %} - {% if option.options is not null %} -
    - @@ -191,6 +191,11 @@
    {% endif %} {% endif %} + {% if errors[option.name] is defined %} +
    + {{ errors[option.name] }} +
    + {% endif %}
    {{ option.description | markdown_to_html }}
    diff --git a/webapp/tests/Unit/BaseTestCase.php b/webapp/tests/Unit/BaseTestCase.php index d3f04262210..075e6fc7001 100644 --- a/webapp/tests/Unit/BaseTestCase.php +++ b/webapp/tests/Unit/BaseTestCase.php @@ -199,10 +199,10 @@ protected function withChangedConfiguration( $dataToSet = [$configKey => $configValue]; // Save the changes. - $config->saveChanges($dataToSet, $eventLog, $dj); + $errors = $config->saveChanges($dataToSet, $eventLog, $dj); - // Call the callback. - $callback(); + // Call the callback with the errors. + $callback($errors); } protected function verifyPageResponse( diff --git a/webapp/tests/Unit/Controller/API/ConfigControllerTest.php b/webapp/tests/Unit/Controller/API/ConfigControllerTest.php index 17f2cb843c7..032693b955f 100644 --- a/webapp/tests/Unit/Controller/API/ConfigControllerTest.php +++ b/webapp/tests/Unit/Controller/API/ConfigControllerTest.php @@ -101,6 +101,22 @@ public function testConfigChangeAPIVisible(): void static::assertEquals(0, $response['data_source']); } + /** + * Test that changing a config variable via the API to an invalid value produces the correct error. + */ + public function testConfigChangeAPIInvalid(): void + { + $response = $this->verifyApiJsonResponse('GET', $this->endpoint, 200, 'admin'); + + static::assertIsArray($response); + static::assertEquals(20, $response['penalty_time']); + + $proposedChange = ['penalty_time' => -1]; + $response = $this->verifyApiJsonResponse('PUT', $this->endpoint, 400, 'admin', $proposedChange); + + static::assertEquals(['errors' => ['penalty_time' => 'This value should be 0 or more.']], $response); + } + /** * Test that invalid data is not accepted. */ diff --git a/webapp/tests/Unit/Controller/Jury/ConfigControllerTest.php b/webapp/tests/Unit/Controller/Jury/ConfigControllerTest.php index 74919339f89..8f8bf3d6350 100644 --- a/webapp/tests/Unit/Controller/Jury/ConfigControllerTest.php +++ b/webapp/tests/Unit/Controller/Jury/ConfigControllerTest.php @@ -45,11 +45,28 @@ public function testConfigSettingsPresent(): void public function testChangedPenaltyTime(): void { $this->withChangedConfiguration('penalty_time', "30", - function () { + function ($errors) { + static::assertEmpty($errors); $this->verifyPageResponse('GET', '/jury/config', 200); $crawler = $this->getCurrentCrawler(); $minutes = $crawler->filter('input#config_penalty_time')->extract(['value']); static::assertEquals("30", $minutes[0]); }); } + + /** + * Test that an invalid penalty time produces an error + */ + public function testChangedPenaltyTimeInvalid(): void + { + $this->withChangedConfiguration('penalty_time', "-1", + function ($errors) { + static::assertEquals(['penalty_time' => 'This value should be 0 or more.'], $errors); + $this->verifyPageResponse('GET', '/jury/config', 200); + $crawler = $this->getCurrentCrawler(); + $minutes = $crawler->filter('input#config_penalty_time')->extract(['value']); + // test that it is still 20, i.e. it didn't change + static::assertEquals("20", $minutes[0]); + }); + } } diff --git a/webapp/tests/Unit/Service/ConfigurationServiceTest.php b/webapp/tests/Unit/Service/ConfigurationServiceTest.php index 6ba67d587e0..77a543c06ba 100644 --- a/webapp/tests/Unit/Service/ConfigurationServiceTest.php +++ b/webapp/tests/Unit/Service/ConfigurationServiceTest.php @@ -14,6 +14,7 @@ use Psr\Log\LoggerInterface; use ReflectionException; use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase; +use Symfony\Component\Validator\Validator\ValidatorInterface; use Symfony\Component\Yaml\Yaml; class ConfigurationServiceTest extends KernelTestCase @@ -28,6 +29,11 @@ class ConfigurationServiceTest extends KernelTestCase */ private ObjectRepository&MockObject $configRepository; + /** + * @var (ValidatorInterface&MockObject)|null + */ + private ?ValidatorInterface $validator; + private InvocationMocker $emGetRepositoryExpects; /** @@ -44,6 +50,7 @@ protected function setUp(): void $this->em = $this->createMock(EntityManagerInterface::class); $this->configRepository = $this->createMock(ObjectRepository::class); + $this->validator = $this->createMock(ValidatorInterface::class); $this->emGetRepositoryExpects = $this->em->expects(self::any()) ->method('getRepository') ->willReturnMap([[Configuration::class, $this->configRepository]]); @@ -51,6 +58,7 @@ protected function setUp(): void $this->config = new ConfigurationService( $this->em, $this->logger, self::getContainer()->get('config_cache_factory'), + $this->validator, self::getContainer()->getParameter('kernel.debug'), self::getContainer()->getParameter('kernel.cache_dir'), self::getContainer()->getParameter('domjudge.etcdir')