Skip to content

Commit

Permalink
feat: Validate custom input traits for short answers of new submissions
Browse files Browse the repository at this point in the history 
* Also validate `extraSettings` especially the regex for custom short input

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
  • Loading branch information
@susnux
susnux committed Nov 22, 2023
1 parent d617a58 commit 76ccb29
Show file tree
Hide file tree
Showing 8 changed files with 349 additions and 12 deletions.
2 changes: 2 additions & 0 deletions docs/DataStructure.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,5 @@ Optional extra settings for some [Question Types](#question-types)
|--------------------|---------------|--------|--------|-------------|
| `allowOtherAnswer` | `multiple, multiple_unique` | Boolean | Allows the user to specify a custom answer |
| `shuffleOptions` | `dropdown, multiple, multiple_unique` | Boolean | The list of options should be shuffled |
| `validationType` | `short` | null, 'phone', 'email', 'regex', 'number' | Custom validation for checking a submission |
| `validationRegex` | `short` | string | if `validationType` is 'regex' this defines the regular expression to apply |
28 changes: 28 additions & 0 deletions lib/Constants.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,34 @@ class Constants {
self::ANSWER_TYPE_TIME => 'H:i'
];

/**
* !! Keep in sync with src/models/ValidationTypes.js !!
*/

// Allowed short input types
public const SHORT_INPUT_TYPES = [
'phone',
'email',
'regex',
'number'
];

// This are allowed extra settings
public const EXTRA_SETTINGS_DROPDOWN = [
'allowOtherAnswer',
'shuffleOptions',
];

public const EXTRA_SETTINGS_MULTIPLE = [
'allowOtherAnswer',
'shuffleOptions',
];

public const EXTRA_SETTINGS_SHORT = [
'validationType',
'validationRegex'
];

/**
* !! Keep in sync with src/mixins/ShareTypes.js !!
*/
Expand Down
4 changes: 4 additions & 0 deletions lib/Controller/ApiController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,10 @@ public function updateQuestion(int $id, array $keyValuePairs): DataResponse {
throw new OCSForbiddenException('Please use reorderQuestions() to change order');
}

if (key_exists('extraSettings', $keyValuePairs) && !$this->formsService->areExtraSettingsValid($keyValuePairs['extraSettings'], $question->getType())) {
throw new OCSBadRequestException('Invalid extraSettings, will not update.');
}

// Create QuestionEntity with given Params & Id.
$question = Question::fromParams($keyValuePairs);
$question->setId($id);
Expand Down
82 changes: 82 additions & 0 deletions lib/Service/FormsService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,4 +516,86 @@ public function setLastUpdatedTimestamp(int $formId): void {
$form->setLastUpdated(time());
$this->formMapper->update($form);
}

/*
* Validates the extraSettings
*
* @param array $extraSettings input extra settings
* @param string $questionType the question type
* @return bool if the settings are valid
*/
public function areExtraSettingsValid(array $extraSettings, string $questionType) {
if (count($extraSettings) === 0) {
return true;
}

// Ensure only allowed keys are set
switch ($questionType) {
case Constants::ANSWER_TYPE_DROPDOWN:
$allowed = Constants::EXTRA_SETTINGS_DROPDOWN;
break;
case Constants::ANSWER_TYPE_MULTIPLE:
case Constants::ANSWER_TYPE_MULTIPLEUNIQUE:
$allowed = Constants::EXTRA_SETTINGS_MULTIPLE;
break;
case Constants::ANSWER_TYPE_SHORT:
$allowed = Constants::EXTRA_SETTINGS_SHORT;
break;
default:
$allowed = [];
}
// Number of keys in extraSettings but not in allowed (but not the other way round)
$diff = array_diff(array_keys($extraSettings), $allowed);
if (count($diff) > 0) {
return false;
}

// Special handling of short input for validation
if ($questionType === Constants::ANSWER_TYPE_SHORT && isset($extraSettings['validationType'])) {
// Ensure input validation type is known
if (!in_array($extraSettings['validationType'], Constants::SHORT_INPUT_TYPES)) {
return false;
}

// For custom validation we need to sanitize the regex
if ($extraSettings['validationType'] === 'regex') {
// regex is required for "custom" input validation type
if (!isset($extraSettings['validationRegex'])) {
return false;
}

// regex option must be a string
if (!is_string($extraSettings['validationRegex'])) {
return false;
}

// empty regex matches every thing, this happens also when a new question is created
if (strlen($extraSettings['validationRegex']) === 0) {
return true;
}

// general pattern of a valid regex
$VALID_REGEX = '/^\/(.+)\/([smi]{0,3})$/';
// pattern to look for unescaped slashes
$REGEX_UNESCAPED_SLASHES = '/(?<=(^|[^\\\\]))(\\\\\\\\)*\\//';

$matches = [];
// only pattern with delimiters and supported modifiers (by PHP *and* JS)
if (@preg_match($VALID_REGEX, $extraSettings['validationRegex'], $matches) !== 1) {
return false;
}

// We use slashes as delimters, so unescaped slashes within the pattern are **not** allowed
if (@preg_match($REGEX_UNESCAPED_SLASHES, $matches[1]) === 1) {
return false;
}

// Try to compile the given pattern, `preg_match` will return false if the pattern is invalid
if (@preg_match($extraSettings['validationRegex'], 'some string') === false) {
return false;
}
}
}
return true;
}
}
49 changes: 47 additions & 2 deletions lib/Service/SubmissionService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;

use OCP\Mail\IMailer;
use Psr\Log\LoggerInterface;

class SubmissionService {
Expand Down Expand Up @@ -83,6 +83,9 @@ class SubmissionService {
/** @var IUser */
private $currentUser;

/** @var IMailer */
private $mailer;

public function __construct(FormMapper $formMapper,
QuestionMapper $questionMapper,
SubmissionMapper $submissionMapper,
Expand All @@ -92,7 +95,8 @@ public function __construct(FormMapper $formMapper,
IL10N $l10n,
LoggerInterface $logger,
IUserManager $userManager,
IUserSession $userSession) {
IUserSession $userSession,
IMailer $mailer) {
$this->formMapper = $formMapper;
$this->questionMapper = $questionMapper;
$this->submissionMapper = $submissionMapper;
Expand All @@ -102,6 +106,7 @@ public function __construct(FormMapper $formMapper,
$this->l10n = $l10n;
$this->logger = $logger;
$this->userManager = $userManager;
$this->mailer = $mailer;

$this->currentUser = $userSession->getUser();
}
Expand Down Expand Up @@ -342,6 +347,11 @@ public function validateSubmission(array $questions, array $answers): bool {
}
}
}

// Handle custom validation of short answers
if ($question['type'] === Constants::ANSWER_TYPE_SHORT && !$this->validateShortQuestion($question, $answers[$questionId][0])) {
return false;
}
}
}

Expand All @@ -367,4 +377,39 @@ private function validateDateTime(string $dateStr, string $format) {
$d = DateTime::createFromFormat($format, $dateStr);
return $d && $d->format($format) === $dateStr;
}

/**
* Validate short question answers if special validation types are set
*/
private function validateShortQuestion(array $question, string $data): bool {
if (!isset($question['extraSettings']) || !isset($question['extraSettings']['validationType'])) {
// No type defined, so fallback to 'text' => no special handling
return true;
}

switch ($question['extraSettings']['validationType']) {
case 'email':
return $this->mailer->validateMailAddress($data);
case 'number':
return is_numeric($data);
case 'phone':
// some special characters are used (depending on the locale)
$sanitized = str_replace([' ', '(', ')', '.', '/', '-', 'x'], '', $data);
// allow leading + for international numbers
if (str_starts_with($sanitized, '+')) {
$sanitized = substr($sanitized, 1);
}
return preg_match('/^[0-9]{3,}$/', $sanitized) === 1;
case 'regex':
// empty regex matches everything
if (!isset($question['extraSettings']['validationRegex'])) {
return true;
}
return preg_match($question['extraSettings']['validationRegex'], $data) === 1;
default:
$this->logger->error('Invalid input type for question detected, please report this issue!', ['validationType' => $question['extraSettings']['validationType']]);
// The result is just a non-validated text on the results, but not a fully declined submission. So no need to throw but simply return false here.
return false;
}
}
}
12 changes: 6 additions & 6 deletions src/components/Questions/QuestionShort.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@
:type="validationObject.inputType"
@input="onInput"
@keydown.enter.exact.prevent="onKeydownEnter">
<NcActions v-if="edit"
<NcActions v-if="!readOnly"
:id="validationTypeMenuId"
:aria-label="t('forms', 'Input types')"
:aria-label="t('forms', 'Input types (currently: {type})', { type: validationObject.label })"
:container="`#${validationTypeMenuId}`"
:open.sync="isValidationTypeMenuOpen"
class="validation-type-menu__toggle"
Expand Down Expand Up @@ -100,12 +100,12 @@ export default {
data() {
return {
validationTypes,
typeMenuOpen: false,
isValidationTypeMenuOpen: false,
}
},
computed: {
submissionInputPlaceholder() {
if (this.edit) {
if (!this.readOnly) {
return this.validationObject.createPlaceholder || this.answerType.createPlaceholder
}
return this.validationObject.submitPlaceholder || this.answerType.submitPlaceholder
Expand Down Expand Up @@ -169,7 +169,7 @@ export default {
this.onExtraSettingsChange({ validationType, validationRegex: this.validationRegex })
} else {
// For all other types except regex we close the menu (for regex we keep it open to allow entering a regex)
this.typeMenuOpen = false
this.isValidationTypeMenuOpen = false
this.onExtraSettingsChange({ validationType: validationType === 'text' ? undefined : validationType })
}
},
Expand Down Expand Up @@ -210,7 +210,7 @@ export default {
*/
onSubmitRegex(event) {
if (this.onInputRegex(event)) {
this.typeMenuOpen = false
this.isValidationTypeMenuOpen = false
}
},
},
Expand Down
121 changes: 121 additions & 0 deletions tests/Unit/Service/FormsServiceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1353,4 +1353,125 @@ public function testNotifyNewSubmission($shares, $shareNotifications) {

$formsService->notifyNewSubmission($form, $submitter);
}

/**
* @dataProvider dataAreExtraSettingsValid
*
* @param array $extraSettings input settings
* @param string $questionType question type
* @param bool $expected expected return value
*
*/
public function testAreExtraSettingsValid(array $extraSettings, string $questionType, bool $expected) {
$this->assertEquals($expected, $this->formsService->areExtraSettingsValid($extraSettings, $questionType));
}

public function dataAreExtraSettingsValid() {
return [
'empty-extra-settings' => [
'extraSettings' => [],
'questionType' => Constants::ANSWER_TYPE_LONG,
'expected' => true
],
'invalid key' => [
'extraSettings' => [
'some' => 'value'
],
'questionType' => Constants::ANSWER_TYPE_LONG,
'expected' => false
],
'valid key' => [
'extraSettings' => [
'shuffleOptions' => true
],
'questionType' => Constants::ANSWER_TYPE_MULTIPLE,
'expected' => true
],
'invalid subtype' => [
'extraSettings' => [
'validationType' => 'iban',
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'expected' => false
],
'valid-custom-regex' => [
'extraSettings' => [
'validationType' => 'regex',
'validationRegex' => '/^[a-z]{3,}/m'
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'expected' => true
],
'valid-custom-empty-regex' => [
'extraSettings' => [
'validationType' => 'regex',
'validationRegex' => ''
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'expected' => true
],
'invalid-custom-regex-modifier' => [
'extraSettings' => [
'validationType' => 'regex',
'validationRegex' => '/^[a-z]{3,}/gm'
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'expected' => false
],
'invalid-custom-regex' => [
'extraSettings' => [
'validationType' => 'regex',
'validationRegex' => '['
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'rval' => false
],
'invalid-custom-regex-delimiters' => [
'extraSettings' => [
'validationType' => 'regex',
'validationRegex' => '/1/2/'
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'rval' => false
],
'invalid-custom-regex-pattern' => [
'extraSettings' => [
'validationType' => 'regex',
'validationRegex' => '/'.'[/'
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'rval' => false
],
'invalid-custom-regex-type' => [
'extraSettings' => [
'validationType' => 'regex',
'validationRegex' => 112
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'rval' => false
],
'invalid-custom-missing-regex' => [
'extraSettings' => [
'validationType' => 'regex',
],
'questionType' => Constants::ANSWER_TYPE_SHORT,
'rval' => false
],
'valid-dropdown-settings' => [
'extraSettings' => [
'shuffleOptions' => false,
],
'questionType' => Constants::ANSWER_TYPE_DROPDOWN,
'expected' => true
],
'invalid-dropdown-settings' => [
'extraSettings' => [
'shuffleOptions' => true,
'someInvalidOption' => true
],
'questionType' => Constants::ANSWER_TYPE_DROPDOWN,
'expected' => false
]
];
}
}
Loading

0 comments on commit 76ccb29

Please sign in to comment.