Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/force validation #603

Open
wants to merge 24 commits into
base: improve/variantCreationForm
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ebcea74
Add $_SESSION entries for validated variants
loeswerkman Jun 9, 2022
ad3e048
Check whether variants are validated using $_SESSION
loeswerkman Jun 9, 2022
6b6dc8f
Fix issue in SQL queries
loeswerkman Jun 9, 2022
e23d860
Add documentation
loeswerkman Jun 9, 2022
2421eef
Add variants as keys instead of values to $_SESSION
loeswerkman Jun 9, 2022
55ce178
Fix issue concerning use of placeholders in SQL
loeswerkman Jun 9, 2022
cbb4822
Clean up code
loeswerkman Jun 9, 2022
d176f46
Trim reference sequence off at the mapping step
loeswerkman Jun 9, 2022
f6a7401
Check the correct $_SESSION variable at isset()
loeswerkman Jun 9, 2022
1de6020
Stop emptying all fields at each call
loeswerkman Jun 9, 2022
c1ac867
Stop removing the onChange of empty values
loeswerkman Jun 9, 2022
a5fb253
Update message on 'empty' fields
loeswerkman Jun 9, 2022
be47eb6
Make $sFieldName safe using htmlspecialchars()
loeswerkman Jun 10, 2022
d8b5ae0
Remove unnecessary urldecode()s
loeswerkman Jun 10, 2022
5f41d21
Add empty() around literal empty call
loeswerkman Jun 10, 2022
8c879af
Remove unnecessary global call
loeswerkman Jun 10, 2022
1d7f4c9
Use SQL's LEFT() instead of PHP's substr()
loeswerkman Jun 10, 2022
59f09d3
Check whether all expected $_REQUESTs were set
loeswerkman Jun 10, 2022
8590c12
Use function to add to $_SESSION & remove refseq
loeswerkman Jun 10, 2022
5380861
Check input using empty() instead of isset()
loeswerkman Jun 10, 2022
bd4733c
Clear up fixme
loeswerkman Jun 10, 2022
ae3563a
Remove todo that is no longer relevant
loeswerkman Jun 10, 2022
2f7ccf2
Move global call to top of the function
loeswerkman Jun 10, 2022
2f82e26
Invert wrong checks of required input
loeswerkman Jun 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 26 additions & 36 deletions src/ajax/check_hgvs_dialogue.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
$aTranscripts = (empty($_REQUEST['transcripts'])? array() : explode('|', urldecode($_REQUEST['transcripts'])));

// Retrieving the name of the input field.
$sFieldName = urldecode($_REQUEST['fieldName']);
$sFieldName = htmlspecialchars(urldecode($_REQUEST['fieldName']));
loeswerkman marked this conversation as resolved.
Show resolved Hide resolved



Expand Down Expand Up @@ -82,38 +82,6 @@



if ($bRefSeqIsSupportedByVV) {
// We want to reset all values if a variant input was changed,
// because we want to keep all validated variants coherent.
// We only do this if the variants can be fully checked by
// VariantValidator, because we can only map those. For variants
// that we cannot map, we can also not automatically create a
// coherent set of variants, and must thus allow the user to
// create this set themselves.
print('
// Resetting all values.
if ($(\'#variantForm input[name*="VariantOn"]\').hasClass("accept")) {
// If any input in the form is of the class accept(ed), this means
// that these input fields were filled in after full mapping and
// validation of VariantValidator. If then, the script is called
// again, we want to RESET these values, since we do not want to
// risk having incoherent variants in the form simultaneously,
// especially not those we have mapped and validated ourselves.
// Resetting the transcript fields.
var oTranscriptFields = $(\'#variantForm input[name$="VariantOnTranscript/DNA"]\');
oTranscriptFields.val("").removeClass();
oTranscriptFields.siblings("img").attr({src: "gfx/trans.png"});
$(\'#variantForm input[name$="VariantOnTranscript/RNA"]\').val("").removeClass();
$(\'#variantForm input[name$="VariantOnTranscript/Protein"]\').val("").removeClass();

// Resetting the genomic fields.
var oGenomicVariants = $(\'#variantForm input[name^="VariantOnGenome/DNA"]\');
oGenomicVariants.val("").removeClass();
oGenomicVariants.siblings("img").attr({src: "gfx/trans.png"});
}
');
}



// Preparing the JS for the buttons.
Expand Down Expand Up @@ -223,6 +191,14 @@ function update_images_per_step($sStep, $sImage)
);


// Setting the SESSION array in which we will store all validated variants.
// This array will be used later on in the checkFields() function after
// the submission has been posted, to ensure that the variants were really
// actually validated.
if (!isset($_SESSION['VV']['validated_variants'])) {
$_SESSION['VV']['validated_variants'] = array();
}

// Check whether this variant is supported by LOVD.
$aVariant = lovd_getVariantInfo($_REQUEST['var'], false);
$bIsSupportedByLOVD = !isset($aVariant['errors']['ENOTSUPPORTED']);
Expand All @@ -233,11 +209,13 @@ function update_images_per_step($sStep, $sImage)

update_images_per_step('statusChecks', 'gfx/cross.png');
update_dialogue(
'<br>Your variant contains syntax which our HGVS check cannot recognise. ' .
'<br>Your variant contains syntax that our HGVS check cannot recognise. ' .
'Therefore, we cannot validate your variant nor map it to other reference sequences. ' .
'Please thoroughly validate your variant by hand.',
'oButtonOKCouldBeValid'
);
// Adding the variant to $_SESSION to mark it as validated.
$_SESSION['VV']['validated_variants'][($sType == 'VOG'? $sGenomeBuildID : $sReferenceSequence)][$_REQUEST['var']] = true;
loeswerkman marked this conversation as resolved.
Show resolved Hide resolved
exit;
}

Expand Down Expand Up @@ -304,6 +282,8 @@ function update_images_per_step($sStep, $sImage)
'Therefore, we cannot map your variant nor validate the positions.',
'oButtonOKCouldBeValid'
);
// Adding the variant to $_SESSION to mark it as validated.
$_SESSION['VV']['validated_variants'][($sType == 'VOG'? $sGenomeBuildID : $sReferenceSequence)][$_REQUEST['var']] = true;
exit;
}

Expand Down Expand Up @@ -412,6 +392,9 @@ function update_images_per_step($sStep, $sImage)
' Therefore, we could only check the syntax and not perform the mapping.',
'oButtonOKCouldBeValid'
);
// Adding the variant to $_SESSION to mark it as validated.
$_SESSION['VV']['validated_variants'][($sType == 'VOG'? $sGenomeBuildID : $sReferenceSequence)][
substr(strstr($_REQUEST['var'], ':'), 1)] = true;
exit;
}

Expand Down Expand Up @@ -530,6 +513,9 @@ function update_images_per_step($sStep, $sImage)
$(\'#variantForm input[name$="\' + sBaseOfFieldName + "RNA" + \'"]\').val("' . $aTranscriptData['RNA'] . '").attr("class", "accept");
$(\'#variantForm input[name$="\' + sBaseOfFieldName + "Protein" + \'"]\').val("' . $aTranscriptData['protein'] . '").attr("class", "accept");
}');

// Adding this variant to the array of validated variants to mark it as validated.
$_SESSION['VV']['validated_variants'][$sTranscript][$aTranscriptData['DNA']] = true;
}

// Returning the mapping for genomic variants.
Expand Down Expand Up @@ -572,9 +558,13 @@ function update_images_per_step($sStep, $sImage)
oGenomicField.val("' . $sMappedGenomicVariant . '").attr("class", "accept");
oGenomicField.siblings("img:first").attr({src: "gfx/check.png", title: "Validated"});
');

// Adding the variant to the array of validated variants to mark it as validated.
$_SESSION['VV']['validated_variants'][$sGBID][$sMappedGenomicVariant] = true;
}


// TODO: Check whether the below code does not conflict with the $_SESSION enforce method.
loeswerkman marked this conversation as resolved.
Show resolved Hide resolved
// Fill and deactivate all fields that remained empty.
// Some fields might not have been filled after the mapping. Some
// unknown issues have occurred here, most likely concerning the
Expand All @@ -593,8 +583,8 @@ function update_images_per_step($sStep, $sImage)
} else {
$(this).val((sName.endsWith("DNA")? "c" : (sName.endsWith("RNA")? "r" : "p")) + ".?");
}
$(this).off("change").attr("class", "warn");
$(this).siblings("img:first").attr({src: "gfx/check_orange.png", title: "Variant mapping failed! The HGVS check of this field is turned off to allow changes to be made freely. To turn the checks back on, refresh the page."});
$(this).attr("class", "warn");
$(this).siblings("img:first").attr({src: "gfx/check_orange.png", title: "Variant mapping failed! Please fill this field manually."});
}
});
');
Expand Down
49 changes: 36 additions & 13 deletions src/class/object_genome_variants.php
Original file line number Diff line number Diff line change
Expand Up @@ -256,21 +256,44 @@ function checkFields ($aData, $zData = false, $aOptions = array())

foreach ($aData as $sField => $sVariant) {
if (strpos($sField, 'DNA') !== false) {
// We want to check the input of all DNA fields: are these variant
// descriptions indeed cleanly formatted? And if our check seems
// to fail, is this perhaps because it holds syntax that we do
// not support? If we DO support the syntax but the variant does
// not seem to be HGVS, we will send an error.
if (lovd_isHGVS($sVariant)) {
// The variant looks good!
continue;
// We want to check the input of all DNA fields: if their description passed
// all our HGVS checks, it will have been added to $_SESSION.

// Before we can check $_SESSION, we need to retrieve the key to which it
// should have been added, which is the GB ID for a VOG and the reference
// sequence for a VOT.
global $_DB;
loeswerkman marked this conversation as resolved.
Show resolved Hide resolved
if (strpos($sField, 'Transcript') !== false) {
// VOT.
$sReference = $_DB->query(
'SELECT id_ncbi FROM ' . TABLE_TRANSCRIPTS . ' WHERE id = ?',
// Get the transcript ID from the field name.
array(strstr($sField, '_', true))
)->fetchColumn();
} else {
// VOG.
$sReference = $_DB->query(
'SELECT id FROM ' . TABLE_GENOME_BUILDS . ' WHERE column_suffix = ?',
// Get the column suffix from the field name.
array((
!substr($sField, strlen('VariantOnGenome/DNA'))?
'' :
substr($sField, strlen('VariantOnGenome/DNA/'))
))
)->fetchColumn();
}
$aVariantInfo = lovd_getVariantInfo($sVariant, false);
if (array_diff(array_keys($aVariantInfo['errors']), array('ENOTSUPPORTED')) // Are there any errors other than ENOTSUPPORTED?
|| array_diff(array_keys($aVariantInfo['warnings']), array('WNOTSUPPORTED'))) { // Are there any warnings other than WNOTSUPPORTED?
// There are problems that are not caused by lack of syntax support.
lovd_errorAdd($sField, 'The variant ' . $sVariant . ' did not pass our checks. Please take another look and try again.');
// We have the key, so now let's see if the variant has been added.

if (isset($_SESSION['VV']['validated_variants'][$sReference][$sVariant])) {
// The variant in this field has been validated!
continue;
}

// This field's variant was not fully validated! It should
// not be sent to the database. Let's add an error.
lovd_errorAdd($sField,
'The variant ' . $sVariant . ' did not pass our checks. Please take another look and try again.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable $sVariant isn't cleaned here, so shouldn't be sent out to the browser without cleaning (XSS vulnerability).

);
}
}

Expand Down