Can you find the bug in this code?
This is a real bug that I came across yesterday in some code I had written about a week before. I was a little surprised at the mechanics but it makes sense once you understand what is happening …
1: private void Foo()
2: {
3: try
4: {
5: AuthenticationInfo authenticationInfo = GetAuthenticationInfo(Request.QueryString["t"]);
6: Authenticator authenticator = new Authenticator(new LoginProvider());
7: AuthenticationStatus authenticationStatus = authenticator.Authenticate(authenticationInfo);
8: if (authenticationStatus.Authenticated)
9: {
10: IUser user =
11: BLUser.Load(authenticationInfo.UserName, authenticationInfo.OrganizationCode, authenticationInfo.DomainId);
12: user.SetNewSessionId();
13: AuthenticationTokenParser authenticationTokenParser = new AuthenticationTokenParser();
14: string authenticationToken = authenticationTokenParser.Create(user.UserName, user.OrganizationId, user.DomainId);
15: FormsAuthentication.SetAuthCookie(authenticationToken, true);
16: string redirectUrl = FormsAuthentication.GetRedirectUrl(authenticationToken, true);
17: if (redirectUrl == null || redirectUrl.Trim().Length == 0)
18: {
19: redirectUrl = “~/Home.aspx”;
20: }
21: Response.Redirect(redirectUrl, true);
22: }
23: Response.Redirect(“~/Home.aspx”);
24: }
25: catch
26: {
27: Response.Redirect(“~/Home.aspx”);
28: }
29: }
I will post the answer soon if no-one gets it. 
Jonathan Cogley is the CEO and founder of Thycotic Software, a .NET consulting company and ISV in Washington DC. Our product, Secret Server is a enterprise password manager system for teams to secure their passwords. Is your team still storing passwords in a text file?
Comments
14 Responses to “Can you find the bug in this code?”
Leave a Reply
I can’t see all of the code. It’s getting cut off on the right side.
I’m using Internet Explorer 7 on Windows XP SP2.
1: private void Foo()
2: {
3: try
4: {
5: AuthenticationInfo authenticationInfo = GetAuthenticationInfo(Request.QueryString["t"]);
6: Authenticator authenticator = new Authenticator(new LoginProvider());
7: AuthenticationStatus authenticationStatus = authenticator.Authenticate(authenticationInfo);
8: if (authenticationStatus.Authenticated)
9: {
10: IUser user =
11: BLUser.Load(authenticationInfo.UserName, authenticationInfo.OrganizationCode, authenticationInfo.DomainId);
12: user.SetNewSessionId();
13: AuthenticationTokenParser authenticationTokenParser = new AuthenticationTokenParser();
14: string authenticationToken = authenticationTokenParser.Create(user.UserName, user.OrganizationId, user.DomainId);
15: FormsAuthentication.SetAuthCookie(authenticationToken, true);
16: string redirectUrl = FormsAuthentication.GetRedirectUrl(authenticationToken, true);
17: if (redirectUrl == null || redirectUrl.Trim().Length == 0)
18: {
19: redirectUrl = “~/Home.aspx”;
20: }
21: Response.Redirect(redirectUrl, true);
22: }
23: Response.Redirect(”~/Home.aspx”);
24: }
25: catch
26: {
27: Response.Redirect(”~/Home.aspx”);
28: }
29: }
@drakiula: Thanks - seems to be something with the new blog theme causing the code to be cut off.
Wild guess, line 23 should have been inside else {}
Probally not what you mean, but “~/Home.aspx” can be found 3 times in the same method, which means it’s a 100% candidate for refactoring into a const and the “Response.Redirect” could be located under the closing bracket from the catch.
Response.Redirect will throw a ThreadAbortException causing the catch block to be executed.
The solution is to call the overload of Response.Redirect(string url, bool endresponse) with endresponse set to *false*. This prevents it from raising the ThreadAbortException. Keep in mind that this causes the entire page to execute.
We ran into this issue with ASP.net 1.0. I believe 1.1 is when the overload for Response.Redirect got introduced.
I don’t know your default filename settings, but I would refactor the code (from 17 to 23) as:
if (redirectUrl != null)
{
Response.Redirect(redirectUrl, true);
}
else
{
Response.Redirect(”~/Home.aspx”);
}
redirectUrl might be just an empty string (”") in order to redirect to the default page (perhaps Default.aspx).
I am not completely sure, probably the Response.Redirect(redirectUrl, true); is throwing an AbortedThread exception and it is being caught in the generic handler. In the end, the generic handler is redirecting the user to a completely different page (Home.aspx).
Regards,
Pablo.
I have to agree with rajbk. Redirecting forces the page to say, “Hey, I know you’re still doing something, but I am redirecting.” in the form of an exception. which should take you to the catch block and force you to home.aspx every time.
GerRedirectUrl issues an additional auth cookie.
If the code were arranged in three blocks: the 1st to authenticate, the 2nd to use that information to create a url, and the 3rd to redirect to that url.
Then, maybe, this issue could be avoided?
The threadabort exception has already been mentioned but something else I noticed is unless a redirectUrl is specified the user will be redirected to home.aspx. Assuming Home.aspx is a page that requires the user to be authenticated the user is now stuck in an infinite login -> redirect to login page loop.
Also is there any particular reason for not using String.IsNullorEmpty on line 17?
Missing Object null values check , the AuthenticationInformation may be null to due missing querystring …
Thanks to everyone for your responses!
I have posted the “fix” here:
http://weblogs.asp.net/jcogley/archive/2008/03/26/can-you-find-the-bug-in-this-code-fixed.aspx