That Fine Code

So, I was tasked with changing a simple ColdFusion application. The application was written by a former co-worker who was also a “ColdFusion developer”.

The application has an HTML form in a login.cfm page that submits to a login_action.cfm page.

Now, take a look at this login_action.cfm, what am I supposed to do? Go hang myself? The following is an exact copy/paste: 

  <CFQUERY NAME="IsValidLogin" DATASOURCE="#dsn#">
  SELECT Count(*) AS LoginMatch 
  FROM users
  WHERE username = '#FORM.Login#'
  AND Password = '#FORM.Pwd#'
  </CFQUERY>

  <CFIF IsValidLogin.LoginMatch IS "0">
    <CFLOCATION URL="Loginagain.cfm">
  <CFELSEIF #FORM.Pwd# eq "change">
  <CFQUERY NAME="qryFName" DATASOURCE="#dsn#" DBTYPE="ODBC">
  SELECT first_name, username, responsibility
  From users
  Where username = '#FORM.Login#'
  </CFQUERY>    
    <CFSET Session.AdminName = #qryFName.first_name#>
    <CFSET Session.login = #qryFName.username#>
    <CFSET Session.resp = #qryFName.responsibility#>
    <CFLOCATION URL="change_password.cfm">
  <CFELSE>
  <CFQUERY NAME="qryFName" DATASOURCE="#dsn#" DBTYPE="ODBC">
  SELECT first_name, username, responsibility
  From users
  Where username = '#FORM.Login#'
  </CFQUERY>
    <CFSET Session.AdminName = #qryFName.first_name#>
    <CFSET Session.login = #qryFName.username#>
    <CFSET Session.resp = #qryFName.responsibility#>
    <CFSET Session.LoggedIn = "1">
    <CFLOCATION URL="../home.cfm">
  </cfif>

WTF!

By quickly scanning the code above, you notice that:

  • The queries do not use cfqueryparam. SQL Injection anyone?
  • Did I say queries? Why do we need three queries in the first place.
  • This “ColdFusion developer” likes to repeat not only queries, but also other blocks of code. Notice the repeated session assignments.
  • Notice the pound signs in the cfset tags. Obviously, this “ColdFusion developer” was a newcomer to ColdFusion (not to mention to database concepts) when he wrote the above code.
  • I did not list it here, but Loginagain.cfm is exactly the same as login.cfm with the exception of a “you’ve entered an invalid username or password” message.

I could not help but to re-write the above like this:

  <cfquery name="qLogin" datasource="#dsn#">
      SELECT *
      FROM users
      WHERE username = 
         <cfqueryparam cfsqltype="cf_sql_varchar" maxlength="8" value="#form.login#">
      AND Password =
         <cfqueryparam cfsqltype="cf_sql_varchar" maxlength="20" value="#form.pwd#">
  </cfquery>

  <cfif not qLogin.recordcount>
      <cflocation url="login.cfm?u=invalid" addtoken="no">
  <cfelse>
      <cfset session.AdminName = qLogin.first_name>
      <cfset session.login = qLogin.username>
      <cfset session.resp = qLogin.responsibility>
  </cfif>

  <cfif form.pwd eq "change">
      <cflocation url="change_password.cfm" addtoken="no">
  <cfelse>
      <cfset session.loggedIn = 1>
      <cflocation url="../home.cfm" addtoken="no">
  </cfif>

As I continue going through this simple application, I’m sure that I will be re-writing most of it :(

Once someone said: There is a huge difference between writing code and writing good code. I totally believe that.

Possibly related:

Tagged | Post a Comment