POP QUIZ: What’s wrong with this code – part 1

Here is another trivia question.  Comment with your answer and I will post the comments Friday along with the answer.

What is wrong with this code

Using Forms Authentication, here is Login.aspx.cs (updated the code slightly, assume the AuthenticateRequest function will check the user input to make sure it is valid entries):

using System;
using System.Data;
using System.Configuration;
using System.Collections;
using System.Web;
using System.Web.Security;
using System.Web.UI;
using System.Web.UI.WebControls;
using System.Web.UI.WebControls.WebParts;
using System.Web.UI.HtmlControls;

public partial class Login : System.Web.UI.Page
protected void Page_Load(object sender, EventArgs e)


void Logon_Click(object sender, EventArgs e)
string username = UserNameTextBox.Text;
string password = UserPassTextBox.Text;

if (AuthenticateRequest(username, password))

FormsAuthenticationTicket ticket = new FormsAuthenticationTicket(1,

// Encrypt the ticket.
string encTicket = FormsAuthentication.Encrypt(ticket);

// Create the cookie.

(username, false);
throw new System.Exception(
“Error authenticating the user”);

bool AuthenticateRequest(string username, string password)
// Do authentication here

So what is wrong here?

kick it on DotNetKicks.com

Comments (15)

  1. You’ve been kicked (a good thing) – Trackback from DotNetKicks.com

  2. jeff says:

    do you mean aside from not sanitizing the user input??

  3. isPersistent & userData are not declared?

    And why is this call:


                  (UserEmail.Text, Persist.Checked);

    using UserEmail.Text as username, when UserNameTextBox.Text was considered the username on the call to RedirectFromLoginPage()?

  4. Chris says:

    Throwing a new Exception() is silly if the authentication fails. A misuse of Exceptions. That’s the very first thing I notice.

    Second is FormsAuthentication.RedirectFromLoginPage

                  (UserEmail.Text, Persist.Checked). In this case, UserEmail is not validated.

    Third thing would be DateTime.Now.AddMinutes(30). That 30 should be a value retrieved from a web.config. At the very least, it should be a const.

  5. jonflanders says:

    My guess is that the call to FormsAuthentication.RedirectFromLoginPage overwrites the encrypted cookie with a non-encrypted cookie.

  6. rluanma says:

    Well, upon authentication failure throwing out an exception is a bad idea – especially if it’s unhandled. It will crash and the worker process will be restarted.

  7. Sean McLellan says:

    I’ll play.

    I’m assuming you’re looking for code than coding guidelines (lack of method comments, private in front of the methods, etc) assuming that there’s a page with a button and the event is attached and assuming that the end user is accepting cookies, and assuming that you’re not looking for cross-site problems (the page is using SSL or some other means to validate that the person who pressed the ‘login’ button is receiving the cookie) and that there’s easier ways of creating the ticket

    I’m going to go with two issues. 1) The code is setting a cookie and then redirecting away. The browser might not be nice and set the cookie on the 302 2) RedirectFromLoginPage sets its own cookie if CookiesSupported=true.

  8. Jack Ma says:

    Issue the authentication cookie two times…

  9. Jack Ma says:

    Issue the authentication cookie two times…

  10. André Cardoso says:

    The password is in clear text?

  11. Hey, the code was changed!

    As you can see from my previous comment, the code in one spot used to be:

    FormsAuthentication.RedirectFromLoginPage(UserEmail.Text, Persist.Checked);

    Now it says:

    FormsAuthentication.RedirectFromLoginPage(username, false);

    So what gives?  You change the code from a "QUIZ" without mentioning it was changed? Weak.

  12. Sorry, posted too quickly.

    Oh, now I see the update notice.  Very subtle.  I’d suggest bolding or italicizing when you change blog post content so it stands out.

  13. yeah, sorry about that.  I will be more careful before I post it next time.

  14. Rahul Soni says:

    Apart from the other things, I would like to add that this Class name might create problems in production.

    CS0030: Cannot convert type ‘ASP.login_aspx’ to ‘System.Web.UI.WebControls.Login’




  15. Very good point Rahul, depending on what version of .NET you are using, this could cause problems.