From 620c9b63aa61855cee81ac9c9ab55be6e370f69c Mon Sep 17 00:00:00 2001 From: gigosaurus Date: Tue, 26 Sep 2017 21:59:10 +0100 Subject: [PATCH 1/5] Fix signup validation. Emails, names and addresses are now much less strict, and more valid postcodes are allowed. --- components/signup.php | 6 +- components/signupajax.php | 4 +- lib/validation.php | 128 +++++++++----------------------------- 3 files changed, 34 insertions(+), 104 deletions(-) diff --git a/components/signup.php b/components/signup.php index c850da5..ddfda22 100644 --- a/components/signup.php +++ b/components/signup.php @@ -80,18 +80,18 @@ if (isset($_REQUEST['signupid']) && isset($_REQUEST['signuppw'])) { $errors['address'] = $error; } $fields['address'] = sanitizeAddress($_POST['address']); - if (!validRealName($_REQUEST['realname'], $override)) { + if (!validName($_REQUEST['realname'], $override)) { $valid = false; $errors['realname'] = $error; } $fields['realname'] = $_REQUEST['realname']; } else { - if (!(validRealName($_REQUEST['contact'], false) || $override)) { + if (!(validName($_REQUEST['contact'], false) || $override)) { $valid = false; $errors['contact'] = $error; } $fields['contact'] = $_REQUEST['contact']; - if (!validSocName($_REQUEST['realname'], $override)) { + if (!validName($_REQUEST['realname'], $override)) { $valid = false; $errors['realname'] = $error; } diff --git a/components/signupajax.php b/components/signupajax.php index 3e4bca4..2da8fa2 100644 --- a/components/signupajax.php +++ b/components/signupajax.php @@ -29,7 +29,7 @@ if (isset($_GET['key'])) { break; case "realname": $realname = $_GET['value']; - if (validRealName($realname, false)) { + if (validName($realname, false)) { echo "OK"; } else { echo $error; @@ -37,7 +37,7 @@ if (isset($_GET['key'])) { break; case "socname": $socname = $_GET['value']; - if (validSocName($socname, false)) { + if (validName($socname, false)) { echo "OK"; } else { echo $error; diff --git a/lib/validation.php b/lib/validation.php index 9994eae..69d6fbf 100644 --- a/lib/validation.php +++ b/lib/validation.php @@ -7,18 +7,22 @@ require_once("sanitization.php"); function validEmail($email) { global $error; - //split user and domain - list($user, $domain) = explode("@", $email); - // check for bad characters, and check for zero length user & domain - if (!preg_match("/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$/i", $email) or !$user or !$domain) { - $error = 'an invalid email address (syntax)'; + + // check for valid syntax + if (!filter_var($email, FILTER_VALIDATE_EMAIL)) { + $error = 'Invalid email address (syntax)'; return false; } + // Syntax OK + // domain consists of any character after a '@' and cannot contain '@' + // therefore any character after the last '@' is part of the domain + $domain = substr($email, strrpos($email, '@') + 1); + // Check for an mail server - elseif (!getmxrr($domain, $mx) or !gethostbyname($domain)) { - $error = "no mail servers listed for '$domain'"; + if (!getmxrr($domain, $mx) or !gethostbyname($domain)) { + $error = "No mail servers listed for '$domain'"; return false; } else { // Email address valid from technical point of view @@ -26,41 +30,6 @@ function validEmail($email) } } -// test whether a password is considered Strong Enough -// ideally we'd want to use cracklib or something here, but no RPM for the php bindings :-( -// dont use this, use weakPassword instead it uses cracklib -function strongPassword($pass) -{ - - // you call this a password? my cat could bruteforce this. - if (strlen($pass) < 6) { - return false; - } - -// start at 0, and increment for certain features - $score = 0; - - -// greater than 8 characters - if (strlen($pass) > 8) $score++; -// includes lowercase characters - if (preg_match("/[a-z]/", $pass)) $score++; -// includes uppercase characters - if (preg_match("/[A-Z]/", $pass)) $score++; -// includes digits - if (preg_match("/\d/", $pass)) $score++; -// includes "non-word" characters - if (preg_match("/\W/", $pass)) $score++; - -// I reckons if it has at least 3 of the above it should be... adequate -// better if it checked for dictionary words too though - if ($score > 3) { - return true; - } else { - return false; - } -} - # Use cracklib to check for weak passwords. # returns FALSE if the password is good i.e. not weak # otherwise returns a string saying why its weak @@ -112,7 +81,7 @@ function isAlias($username) return $ok; } -//check if a user with a sid already exsists +//check if a user with a sid already exists function sidUsed($sid) { $sucsDB = NewADOConnection('postgres8'); @@ -127,12 +96,12 @@ function sidUsed($sid) function validUsername($username) { global $error; - // check if uname is sytactically valid + // check if uname is syntactically valid $syntax = preg_match("/^[a-z][a-z0-9_]*$/", $username); if (!$syntax || (strlen($username) < 2)) { - $error = "Usernames must start with a letter, only contain lowercase letter, numbers 0-9 and underscores (_) and be at least 2 characters."; + $error = "Usernames must start with a letter, only contain lowercase letters, numbers 0-9 and underscores (_) and be at least 2 characters."; return false; - } // check if the username already exsists + } // check if the username already exists elseif (posix_getpwnam($username)) { $error = "Username already taken"; return false; @@ -171,7 +140,7 @@ function validSID($SID, $override) } } -function validRealName($realName, $override) +function validName($realName, $override) { global $error; if ($override) { @@ -182,56 +151,12 @@ function validRealName($realName, $override) return true; } } else { - //check for enough names for real name (we insist on at least 2 - if (count(explode(" ", $realName)) < 2) { - $error = "Too few names given, please give at least two."; - return false; - } //check for a sane realname, see comment below - elseif (!preg_match("/^([A-Z]([.]+ +[A-Z])*([\']+[A-Z])*[a-z]+[ -]*)+$/", $realName)) { - $error = "Name incorrectly formatted, email admin@sucs.org if this is an error."; - return false; - } /* - * This should force sane real names, with capitals for the first letter of each word, - * Whist alowing for complex names such as Robin M. O'Leary - * - * break down of regexp - * - * ( - * [A-Z] - start with a single capital - * ([.]+ +[A-Z])* - zero or more of, (at least one "." followed by at least one space then another single capital) //we dont expect people to have initals at the end of there names so this is alright - * ([\']+[A-Z])* - zero or more of, (at least one "'"s followed by a single capital letter) - * [a-z]+ - One or more lower case letters, this forces initals to be followed by a "." - *[ -]* - zero or more " "s or "-"s so double barreled names are supported - * ) - * - * In its current state - * Robin M. O'Leary is valid - * Robin M O'Leary is not - * Robin M. OLeary is Not - * Robin M. O'LeaRy is valid (though its not meant to be.. bad side effect of not requireing at least one space...) - * BUT... this alows for McSmith's... which is rather nice :)... and of course delibrate - * RObin M O'Leary is not - * - */ - else { - return true; - } - } -} -function validSocName($socname, $override) -{ - global $error; - if ($override) { - if ($socname == "") { - $error = "You MUST provide some sort of name"; - return false; - } else { - return true; - } - } else { - if (!preg_match('/^[A-Z1-9]/', $socname) || strlen($socname) < 2) { - $error = "Must start with a capital letter or a number and be more than 1 character"; + // names can legally be really weird so just check that it is at least 1 visible character + // followed by any number of non-control characters + $realName = trim($realName); + if (!preg_match("/^[[:graph:]][[:print:]]*$/", $realName)) { + $error = "Invalid name"; return false; } else { return true; @@ -243,9 +168,11 @@ function validAddress($address) { global $error; $address = sanitizeAddress($address); - $regex = "/^([A-Z0-9]([[:alnum:]]|[ .\/'-])*\n)+[A-Z0-9]([[:alnum:]]|[ .\/'-])*$/"; + + // check that they at least entered in something. Address doesn't need to be as strict when the postcode is. + $regex = "/^.{5,}+$/s"; if (!preg_match($regex, $address)) { - $error = "Please supply at least two valid lines of address."; + $error = "Please supply a valid address."; return false; } else { return true; @@ -255,7 +182,10 @@ function validAddress($address) function validPostcode($postcode) { $postcode = sanitizePostcode($postcode); - if (!preg_match('/^[A-Z]{1,2}[0-9]{1,2}[A-Z]{0,1} [0-9][A-Z]{2}$/', $postcode)) { + + // matches all postcodes following the valid format described in a 2012 government published document + $postcodeRegex = "/^([A-Z](([0-9][0-9]?)|([A-Z][0-9][0-9]?)|([A-Z]?[0-9][A-Z])) ?[0-9][ABD-HJLNP-UW-Z]{2})$/"; + if (!preg_match($postcodeRegex, $postcode)) { return false; } else { return $postcode; -- GitLab From 777965c6a5b8f1376f205d48cf62a73a93acfd6a Mon Sep 17 00:00:00 2001 From: Kit Manners Date: Wed, 27 Sep 2017 00:07:42 +0100 Subject: [PATCH 2/5] Fix typo in signup completion --- templates/signup.tpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/signup.tpl b/templates/signup.tpl index f96d25e..2b97a42 100644 --- a/templates/signup.tpl +++ b/templates/signup.tpl @@ -136,7 +136,7 @@ {if !$failed}

Welcome to SUCS

Signup is complete, please see below for your password, a copy has also been send to {$email}, we request you - change this immediatley. See our Getting Started page for some + change this immediately. See our Getting Started page for some ways you can start using your new SUCS account!

Username: {$username}
-- GitLab From fcfabbae140b7ba17f7f03a575e73c45b7cffe79 Mon Sep 17 00:00:00 2001 From: Kit Manners Date: Wed, 27 Sep 2017 01:13:07 +0100 Subject: [PATCH 3/5] Fix retrieving full name from campus ldap --- lib/validationData.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/validationData.php b/lib/validationData.php index 14c475b..06d16cb 100644 --- a/lib/validationData.php +++ b/lib/validationData.php @@ -7,7 +7,7 @@ function lookupSID($sid) $sr = ldap_search($ds, "ou=Active,ou=Resources,o=Swansea", "EDUPERSONTARGETEDID=" . $sid); $info = ldap_get_entries($ds, $sr); ldap_unbind($ds); - return ucwords(strtolower($info[0]['givenName'][0] . " " . $info[0]['sn'][0])); + return ucwords(strtolower($info[0]['givenname'][0] . " " . $info[0]['sn'][0])); } // lookup addresses from postcodes using the university's website -- GitLab From 86ccc6b7187951a49ec8875e755fa68174c6551f Mon Sep 17 00:00:00 2001 From: Kit Manners Date: Wed, 27 Sep 2017 01:20:53 +0100 Subject: [PATCH 4/5] Add missing space in error message --- lib/validation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/validation.php b/lib/validation.php index 69d6fbf..b981c9a 100644 --- a/lib/validation.php +++ b/lib/validation.php @@ -132,7 +132,7 @@ function validSID($SID, $override) $error = "A user with that student ID already exists, email admin@sucs.org if this is an error."; return false; } elseif (lookupSID($SID) == " ") { - $error = "Student not found, emailadmin@sucs.org if this is an error."; + $error = "Student not found, email admin@sucs.org if this is an error."; return false; } else { return true; -- GitLab From 4063d666a99a4f452b0036d409634fc7ccd826a5 Mon Sep 17 00:00:00 2001 From: gigosaurus Date: Wed, 27 Sep 2017 02:11:31 +0100 Subject: [PATCH 5/5] Remember the student number if they entered it in a previous form --- components/signup.php | 4 ++++ components/susignup.php | 2 ++ templates/signup.tpl | 2 +- templates/susignup.tpl | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/components/signup.php b/components/signup.php index ddfda22..27b3c79 100644 --- a/components/signup.php +++ b/components/signup.php @@ -50,6 +50,10 @@ if (isset($_REQUEST['signupid']) && isset($_REQUEST['signuppw'])) { // pass on the id and passwd and id the validation is overridable $smarty->assign("signupid", $signupid); $smarty->assign("signuppw", $signuppw); + // pass on the student id if it exists + if (isset($_REQUEST['signupsid'])) { + $smarty->assign("signupsid", $signupsid); + } $smarty->assign("overridable", $overridable); $smarty->assign("usertype", $row[type]); // if accepting the form diff --git a/components/susignup.php b/components/susignup.php index f33cdb4..0bfde55 100755 --- a/components/susignup.php +++ b/components/susignup.php @@ -51,6 +51,7 @@ if (!empty($_REQUEST['sid']) && !empty($_REQUEST['transactionID'])) { $mode = "form"; $smarty->assign("id", $signuptmpresult->fields["id"]); $smarty->assign("pass", $signuptmpresult->fields["password"]); + $smarty->assign("sid", $signuptmpresult->fields["sid"]); // else if they aren't in the SUCS DB, then bootstrap signup process } else if ($tmpresult->fields == false) { $mode = "form"; @@ -59,6 +60,7 @@ if (!empty($_REQUEST['sid']) && !empty($_REQUEST['transactionID'])) { $id = $iddata->fields['id']; $smarty->assign("id", $id); $smarty->assign("pass", $pass); + $smarty->assign("sid", $sid); } else { // they should never get here die("You'll see this if there has been a database error. Someone probably knows and is trying to fix it. Sorry."); diff --git a/templates/signup.tpl b/templates/signup.tpl index f96d25e..c144585 100644 --- a/templates/signup.tpl +++ b/templates/signup.tpl @@ -36,7 +36,7 @@

+ {if $mode=='re-form'}value='{$fields.studentid}'{elseif isset($signupsid)}value='{$signupsid}'{/if} />
{$errors.studentid}{else} style="color:green; diff --git a/templates/susignup.tpl b/templates/susignup.tpl index 322625f..abde068 100644 --- a/templates/susignup.tpl +++ b/templates/susignup.tpl @@ -39,6 +39,7 @@
+
{else} @@ -49,4 +50,4 @@ An error occured during signup, please email, with as much information as you can provide, admin@sucs.org for assistance. -{/if} \ No newline at end of file +{/if} -- GitLab