Recent Posts

Tags

News

  • Sentient

    My services as a speaker, consultant, trainer and developer are available through Sentient.

    Sentient is a leading provider of software development, consultancy and training services on the Microsoft platform.

    Please feel free to contact me if you have a question about something posted in my blog.

Community

Email Notifications

Links

Archives

Sentient thoughts about .NET

Jonathan Greensted's .NET weblog

FirstContact - Part 1

Untitled Page This blog is the first in a series where I will explorer the best way to write a very simple piece of business logic using the current crop Microsoft technologies starting with C# and ASP.net.

The business process I have selected is what I call "First Contact".  This is the point of initial interaction between a prospect and a business.

I will be looking at various flavours of "First Contact" in this series starting with a simple "Contact Us" web page.

The rest of this blog demonstrates the most simples and most flawed use of ASP.net to provide an example how badly things can go wrong!

The example consists of the following elements (source files included at the end):

  • contactus.htm - a web page containing the contact form to be completed.
  • addcontact.aspx - an aspx web page which handles adding the contact details to the Contact database and notifies the customer service department about the enquiry.

This example implements the required business logic and hence is perfectly valid in that regard however if your business is running code like this you should be very, very worried.  The challenge is to know if you are running good code or bad code like this.

In the next blog in this series I will highlight the issues I see with this example and refactor it to illustrate how it can be improved.   

What issues can you see in this example?  Please add them to the comments for this blog.

contactus.htm

<html>

<body>

    <formmethod=getaction="addcontact.aspx">

        <imgsrc="generiCoLogo.gif">

        <br>

        <table>

            <tr>

                <td>

                    Name

                </td>

                <td>

                    <inputname="name"type="text">

                </td>

                </tr>

            <tr>

                <td>

                    Email

                </td>

                <td>

                    <inputname="email"type="text">

                </td>

            </tr>

            <tr>

                <td>

                    Phone

                </td>

                <td>

                    <inputname="phone"type="text">

                </td>

                </tr>

            <tr>

                <td>

                    Message

                </td>

                <td>

                    <textareaname="message"></textarea>

                </td>

            </tr>

        </table>

        <inputid="Send"type="submit"value="Send">

    </form>

</body>

</html>


addcontact.aspx

<%@ Page Language="C#" %>

<html>

<body>

<%

    if (Request["name"] == "" ||

        Request["email"] == "" ||

        Request["phone"] == "" ||

        Request["message"] == "")

    {

        Response.Write("<h1>Oops!</h1>Please enter name, email, phone and message.<br><br>Click <a href=\"contactus.htm\">here</a> to go back.");

    }

    else

    {

        string message = string.Format("Name: {0}\nEmail: {1}\nPhone: {2}\nMessage: {3}",

            Request["name"], Request["email"], Request["phone"], Request["message"]);

 

        System.Web.Mail.SmtpMail.Send("nobody@sentient.co.uk","customerservice@sentient.co.uk","FirstContact", message);

 

        string sql = string.Format("INSERT INTO Contacts(name, email, phone, message) VALUES ('{0}','{1}','{2}','{3}')",

            Request["name"], Request["email"], Request["phone"], Request["message"]);

 

        System.Data.SqlClient.SqlConnection connection = new System.Data.SqlClient.SqlConnection("Server=localhost;Database=FirstContact;User ID=sa;Password=");

        System.Data.SqlClient.SqlCommand command = new System.Data.SqlClient.SqlCommand(sql, connection);

 

        command.Connection.Open();

 

        if (command.ExecuteNonQuery() > 0)

        {

            Response.Write("<h1>Thank-you</h1>One of our customer service representatives will be in touch with you shortly.");

        }

        else

        {

            Response.Write("<h1>Sorry!</h1>There was a problem please <a href=\"contactus.htm\">try again</a>.");

        }

 

        connection.Close();

    }   

%>

</body>

</html>

Comments

jonathangreensted said:

Hmm, well off hand the first three things to fix would be the SQL injection vulnerability, the lack of exception handling and connection management around the database operation, and then there is this whole matter of mixing old-school form POSTs and ASP.NET! :)
# November 15, 2004 1:33 AM

jonathangreensted said:

Thanks Scott, that was quick!

All valid points but I think there are more issues here.

Anyone else care to comment while I'm off to read Scotts blog!
# November 15, 2004 9:03 AM

jonathangreensted said:

Well you are using inline code instead of code behind so the connection string is visible. Plus it's also connecting using the SA account which is bad, but even worse it's using the default password of nothing!
# November 15, 2004 11:51 AM

jonathangreensted said:

* xhtml compliance
* localization
* Requests should check for null
* Visible pwd
* validation of posted data formats
* hardcoded conn str
* blah dee blah .... few more, but said enough

Oh, erm, i just hate inline code.

Nice to see you blogging.

Steven.
# November 16, 2004 9:03 PM

jonathangreensted said:

Nice work Nick & Steven but there are still more issues.

Can you find them?
# November 17, 2004 8:53 AM

jonathangreensted said:

Its late and im struggling...

No mail server specified.

Didnt I read that table names should not be plural - Contact not Contacts

Picky but ExecuteNonQuery=1 not >0

form method should be post - or am i mad

no <HEAD> tags but think thats been said

no length checking against fields - but think that comes under validation

When they miss an entry the form will reset all the blank fields (this is ASP ? where are the runat=server)

Thats it for tonight, back to some coding...
Gary

# November 27, 2004 12:20 AM

jonathangreensted said:

Hi JJ,

ok it's early but here it goes.

using sql insert statement instead of stored proc is not good.

you haven't used 'using 'to get some namespace qualifiers so you do not need to fully qualify all the references, i think you can use them with inline code-behind.

whack a using around the db access so it clears up once you have finished.

you need some spaces in your html :)

to be honest the db access should be broken out to a new custom class thast you can easily re-use so you don't need to repeat all the connection crud.

even if you do move the conn string to the config file use integrated security as you have no password in clear text anywhere then and don't use sa anyway, especially with no password.

sorry if i've repeated anybody elses pointers

Happy christmas!


# December 24, 2004 7:11 AM

TrackBack said:

^_~
# April 16, 2005 3:18 AM

TrackBack said:

^_~,pretty good!csharpsseeoo
# May 17, 2005 9:58 PM

TrackBack said:

FirstContact - Part 1ooeess
# July 17, 2005 2:46 AM

TrackBack said:

FirstContact - Part 1ooeess
# July 31, 2005 10:27 PM
Leave a Comment

(required) 

(required) 

(optional)

(required)