5
\$\begingroup\$

I was hoping for someone to review my current project, which was created in Swift and uses a PHP web service. I'm not worried about UI elements, as this is just a 'test' project, but I'm concerned about two things: using the best practices and security. I'm concerned that the SQL query is not safe, among other things.

user.swift

import Foundation

class User: NSObject {

    var firstName: String?
    var lastName: String?
    var username: String
    var password: String
    var email: String?

    var recievedJSON: NSMutableData = NSMutableData()
    var userData: [[String: String]]!

    var verified: Bool = false

    required init(username: String, password: String) {
        self.username = username
        self.password = password
    }

    init(firstName: String, lastName: String, username: String, password: String, email: String) {
        self.firstName = firstName
        self.lastName = lastName
        self.username = username
        self.password = password
        self.email = email
    }

    func attemptRegister() {
        var variables: [String] = ["firstname=" + self.firstName! + "&"]
        variables.append("lastname=" + self.lastName! + "&")
        variables.append("username=" + self.username + "&")
        variables.append("password=" + self.password + "&")
        variables.append("email=" + self.email!)
        request("https://codekaufman.com/register.php", variables: variables)
    }

    func attemptSignIn() {
        var variables: [String] = ["username=" + self.username + "&"]
        variables.append("password=" + self.password)
        request("https://codekaufman.com/login.php", variables: variables)
        println("Attempting sign-in...")
    }

    private func request(urlPath: String, variables: [String]?) {
        var url: NSURL = NSURL(string: urlPath)!
        var request: NSMutableURLRequest = NSMutableURLRequest(URL: url)

        if(variables != nil) {
            request.HTTPMethod = "POST"

            var bodyData: NSString = ""
            for item in variables! {
                bodyData = bodyData + NSString(string: item)
            }

            request.HTTPBody = bodyData.dataUsingEncoding(NSUTF8StringEncoding)
        }

        var connection: NSURLConnection = NSURLConnection(request: request, delegate: self, startImmediately: false)!
        connection.start()
        println("Connection started.")
    }

    func connection(connection: NSURLConnection!, didReceiveData data: NSData!){
        self.recievedJSON.appendData(data)
        println("Data recieved.")
    }

    func connectionDidFinishLoading(connection: NSURLConnection!) {
        userData = parseJSON(recievedJSON)

        if(userData != nil) {
            println("Data recieved:")
            println(userData[0])
        } else {
            println("No data recieved.")
        }

    }

    func parseJSON(inputData: NSData) -> [[String: String]]? {
        var error: NSError?
        var userData: [[String: String]]!
        userData = NSJSONSerialization.JSONObjectWithData(inputData, options: NSJSONReadingOptions.MutableContainers, error: &error) as? [[String: String]]

        if (userData != nil) {
            println("NSData had data, printing and returning.")
            println(userData)
            return userData
        } else {
            println("NSData empty, returning nil.")
            return nil
        }
    }

}

register.php

<?php

error_reporting(E_ALL);
ini_set('display errors', 1);

// Don't worry about this, I'm aware of the security issue. :)
$username = '';
$password = '';

try {
    $dbh = new PDO('mysql:host=localhost; dbname=codeggdj_users', $username, $password);
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $recievedUsername = $_POST['username'];
    $recievedPassword = password_hash($_POST['password'], PASSWORD_BCRYPT);
    $recievedFirstName = $_POST['firstname'];
    $recievedLastName = $_POST['lastname'];

    $sth = $dbh->prepare("INSERT INTO users (username, password, first_name, last_name) VALUES (?, ?, ?, ?)");
    $sth->execute([$recievedUsername, $recievedPassword, $recievedFirstName, $recievedLastName]);
} catch(PDOException $e) {
    echo $e->getMessage();
}

?>

login.php

<?php

error_reporting(E_ALL);
ini_set('display errors', 1);

// Don't worry about this, I'm aware of the security issue. :)
$username = '';
$password = '';

try {
    $dbh = new PDO('mysql:host=localhost; dbname=codeggdj_users', $username, $password);
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $recievedUsername = $_POST['username'];
    $recievedPassword = $_POST['password'];

    $sth = $dbh->prepare('SELECT password FROM users WHERE username = ?');
    $sth->execute([$recievedUsername]);

    if($sth->rowCount()) {
       $row = $sth->fetch(PDO::FETCH_OBJ);

       if(password_verify($recievedPassword, $row->password)) {
           $sth = $dbh->prepare('SELECT id, username, first_name, last_name FROM users WHERE username = ?');
           $sth->execute([$recievedUsername]);

           echo json_encode($row = $sth->fetchAll(PDO::FETCH_ASSOC));
       } else {
           echo 'Incorrect Password';
       }
    } else {
       echo 'Incorrect Username';
    }

} catch(PDOException $e) {
    echo $e->getMessage();
}

?>
\$\endgroup\$
3
\$\begingroup\$

I'm concerned that the SQL query is not safe, among other things.

Well, you're right to be concerned. I don't know much about PHP (basically nothing), so I'll let someone who does know about PHP comment about that half of your code.

But from the Swift end, we're passing the password as plaintext. We really shouldn't be doing this. When we send passwords over any sort of networking request, the password should be encrypted in some way. MD5 is a common approach. I'm not a safety expert and couldn't say one way or the other whether or not MD5 is still appropriate or not, but anything is better than passing completely unencrypted passwords.


func attemptRegister() {
    var variables: [String] = ["firstname=" + self.firstName! + "&"]
    variables.append("lastname=" + self.lastName! + "&")
    variables.append("username=" + self.username + "&")
    variables.append("password=" + self.password + "&")
    variables.append("email=" + self.email!)
    request("https://codekaufman.com/register.php", variables: variables)
}

This method will crash every time it's call and one of your optional properties is nil.

If these properties truly are optional, then we need to optionally unwrap them:

if let email = self.email {
    variables.append("email=" + email)
}

And moreover, appending the & to the end of each of these variables here seems really contrived.

Moreover, why do we need to build the array here and pass it into another method... when the array is built of nothing more than instance variables that the other method has access to?

To be honest, if this is me, I think I completely rethink and restructure this class. You call this class User, but it's nothing more than a utility for signing in or registering with your service. How usable is this object after the user has either signed in or registered? And should we REALLY be storing a password for as long as this object is kept around?

Realistically, our username/password (and in the case of registering, the other information) should be passed to a function, even if it's a class function of the user class. As soon as we send the information to the web service, we should forget about it within the app.

And then, assuming a successful response from the web service, our User class should be instantiated with the information about this user that our app actually needs to use. A userid? Display name, profile picture, etc? These are the things our User class should keep as instance variables--the information that our application needs AFTER the user has successfully signed in or registered. And we definitely should not be hanging on to information we need for signing in or registering (except any of this information that is also information that our app needs to function).


This bit of code by the way is a little redundant:

if (userData != nil) {
    println("NSData had data, printing and returning.")
    println(userData)
    return userData
} else {
    println("NSData empty, returning nil.")
    return nil
}

The only reason to actually split this into an if/else is because you want to print this stuff out for diagnostic reasons. But realistically, in the final version of your app, you won't want to print this stuff out... and as such, the if/else could be eliminated.

As such, I'd rewrite this section as:

#if DEBUG
    if (userData != nil) {
        println("NSData had data, printing and returning.")
        println(userData)
    } else {
        println("NSData empty, returning nil.")
    }
#endif

return userData

We don't need to return a literal nil when userData is nil. If userData is nil, then the statement return userData will already return nil for us!

\$\endgroup\$
3
  • \$\begingroup\$ Oh, also, please note - I am using HTTPS with SSL, is the encryption still needed? \$\endgroup\$
    – Jeffrey
    Dec 25 '14 at 1:59
  • \$\begingroup\$ I'd ask on Information Security, because as I said, I'm not an expert. But my instinct says yes, it's still important. Particularly so given that you're writing an app that people use on mobile devices and people tend to connect their mobile device to absolutely any available WiFi hotspot. \$\endgroup\$
    – nhgrif
    Dec 25 '14 at 2:09
  • \$\begingroup\$ @jkaufman And here's a StackOverflow answer for MD5ing a string in Swift. \$\endgroup\$
    – nhgrif
    Dec 25 '14 at 2:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.