CFML photo upload script

[TechBoyJK]

Hi Guys,

I'm writing a simple classified ad site and while I have an image upload function that works, I'm curious if there is any way I can make it better.

The only thing I really want to add is a "upload progress bar", a javascript to check the file size before uploading (so that i can deny 20MB files, etc), and additional support for more file types.

When the photo is uploaded, a record is created in the database. The record includes the following info

photo_id (int-identity-pk)
classified_id (int-fk)
public_url (varchar-255)
private_url (varchar-255)
photo_file (varchar-50-null)
caption (nvchar-16-null)
created (datetime-null)
created_ip (varchar-15-null)
last_modified (datetime-null)
last_modified_ip (varchar-15-null)
photo_position (smallint-2)

The columns are pretty self explanatory. A unique record id for the photo, the classified ad id, the public URL that webpages use to call the photo, the private URL where the photo is located on the server (aka c:/myphotos/), the photo's file name, caption, datetime
info, and the position of the photo which determines the order in which photos are displayed.

This seems to work well and I'm happy with how I'm collecting the photo data.

Since there needs to be small, medium, and large versions of the photo (thumbnail, main ad pic, large view), what I've done is keep on file the core file name of the photo, ex. photo1.jpg. WHen the photo is processed, I have a script that runs that takes the original, resizes it to one of the template sizes, and prefixes the filename with a tag to show what size the photo is.

original - photo1.jpg
small - s_photo1.jpg
medium - m_photo1.jpg
large - l_photo1.jpg

This way the code for when the ad is displayed is hardcoded to include the prefix, but not the original file name

ex - #public_url#/s_#photo_file#
ex - #public_url#/m_#photo_file#

There are two pieces of code that make up most of this process. The form processing part where CFFILE is used to upload the photo, etc. In this piece of code is a CFINCLUDE that takes the photo and resizes it. Can someone look at this code and see if there are any red flags? It seems to work fine and is pretty quick.

The first block of code actually has code for 20 photos (and I plan to rewrite it into a loop instead of hard coded out), but I just included the code for pics 1 and 2.

The 2nd block of code is a snippet from the photo reprocessing script.. Since the code for resizing the different image sizes is essentially the same, I just included the code for resizing to "medium"


-----------------------------------------------------------------------

<CFPARAM NAME="local_photo_file" default="">
<CFPARAM NAME="local_photo_caption" default="">
<CFPARAM NAME="local_created" default="">
<CFPARAM NAME="local_created_ip" default="">
<CFPARAM NAME="local_photo_position" default="">

<CFIF (isDefined("FORM.photo_file_1") AND FORM.photo_file_1 NEQ "")>

<CFSET local_photo_position = '1'>
<CFSET local_photo_caption = '#FORM.photo_caption_1#'>

<CFFILE

action="upload"
destination="c:\openbd\webapps\company_com\imagehost\classifieds\#main_category#\#CID#"
accept="image/jpg, image/jpeg, image/pjpeg, image/gif"
nameconflict="makeunique"
filefield="FORM.photo_file_1"

>

<CFINCLUDE TEMPLATE="#new_photo#">

<CFLOCK SCOPE="application" TIMEOUT="1" TYPE="readonly">

<CFQUERY NAME="primary_thumbnail_info" datasource="classifieds">

UPDATE #tbl_classifieds#

SET thumbnail_url = '#new_photo_server#/imagehost/classifieds/#MAIN_CATEGORY#/#CID#',
thumbnail_file = '#local_photo_file#'

WHERE classified_id = #CID#
;

</CFQUERY>

</CFLOCK>

</CFIF>

<CFIF (isDefined("FORM.photo_file_2") AND FORM.photo_file_2 NEQ "")>

<CFSET local_photo_position = '2'>
<CFSET local_photo_caption = '#FORM.photo_caption_2#'>

<CFFILE

action="upload"
destination="c:\openbd\webapps\details_at\imagehost\classifieds\#main_category#\#CID#"
accept="image/jpg, image/jpeg, image/pjpeg, image/gif"
nameconflict="makeunique"
filefield="FORM.photo_file_2"

>

<CFINCLUDE TEMPLATE="#new_photo#">

</CFIF>

-----------------------------------------------------------------------
#New PHOTO#

<CFIMAGE action="info" srcfile="#cffile.serverdirectory#/#local_photo_file#">

<CFIF (cfimage.width LT 200) and NOT FileExists("#cffile.serverdirectory#/m_#local_photo_file#")>

<CFSET Size = "200">
<CFSET Percentage = Evaluate(Size / cfimage.width)>
<CFSET Percentage = Round(Percentage * 100)>

<CFIMAGE
srcfile="#cffile.serverdirectory#/#local_photo_file#"
destfile="#cffile.serverdirectory#/m_#local_photo_file#"
action="edit"
width="#Percentage#%"
height="#Percentage#%"
uridirectory="NO"
>

</CFIF>

 

[MrChad]

It's been a while since I looked at ColdFusion code, but 2 questions come to mind.

1. Why are you not using <CFQUERYPARAM> tags? Your code is vulnerable to SQL injections.

2. Why do you have an application scope lock around your update query?

 

[TechBoyJK]

Originally posted by: MrChad
It's been a while since I looked at ColdFusion code, but 2 questions come to mind.

1. Why are you not using <CFQUERYPARAM> tags? Your code is vulnerable to SQL injections.

2. Why do you have an application scope lock around your update query?

Ahhh.. I had an application variable in that query but removed it.. Never removed the scope around that.

I'll look into CFQUERYPARAM now.. I had been just trying to get everything to "work" so I could demo the site, and then planned to sit down and go through the code and secure it once the core code was in place.

Thanks!

 

[MrChad]

Originally posted by: TechBoyJK

Originally posted by: MrChad
It's been a while since I looked at ColdFusion code, but 2 questions come to mind.

1. Why are you not using <CFQUERYPARAM> tags? Your code is vulnerable to SQL injections.

2. Why do you have an application scope lock around your update query?

Ahhh.. I had an application variable in that query but removed it.. Never removed the scope around that.

I'll look into CFQUERYPARAM now.. I had been just trying to get everything to "work" so I could demo the site, and then planned to sit down and go through the code and secure it once the core code was in place.

Thanks!

Even if you were using an application variable, I would do something like this

<cflock scope="application" type="readonly">
....<cfset local_var = "#APPLICATION.app_var#">
</cflock>

<cfquery>
...some sql using #local_var#
</cfquery>

Copy the application variable into your local scope then use it. This way you don't have the lock remaining open until your SQL finishes executing.

 

[TechBoyJK]

Originally posted by: MrChad

Originally posted by: TechBoyJK

Originally posted by: MrChad
It's been a while since I looked at ColdFusion code, but 2 questions come to mind.

1. Why are you not using <CFQUERYPARAM> tags? Your code is vulnerable to SQL injections.

2. Why do you have an application scope lock around your update query?

Ahhh.. I had an application variable in that query but removed it.. Never removed the scope around that.

I'll look into CFQUERYPARAM now.. I had been just trying to get everything to "work" so I could demo the site, and then planned to sit down and go through the code and secure it once the core code was in place.

Thanks!

Even if you were using an application variable, I would do something like this

<cflock scope="application" type="readonly">
....<cfset local_var = "#APPLICATION.app_var#">
</cflock>

<cfquery>
...some sql using #local_var#
</cfquery>

Copy the application variable into your local scope then use it. This way you don't have the lock remaining open until your SQL finishes executing.

I've actually done that in most of the code. For application variables that are used throughout the site on almost every page, I actually just set that in the application.cfm file, so that I can always just refer to the var as a local one. I don't have to keep declaring the app variable as a local variable for every script..

Other than that, any other red flags about the code? It does what it's supposed to.