We still get a fair bit of old school ASP .Net webforms code, which was cutting edge about twenty years ago, but remains a collection of bad choices that tried to make web programming feel like desktop programming. As an aside, what a reversed world we live in today, where everyone uses Electron and our desktop programming feels like web programming.
In any case, Thomas sends us some pretty ugly webforms code.
Me.Session("Albumid") = CType(e.Item.FindControl("lblAlbumID"), Label).Text
If Not Me.Session("Albumid") Is Nothing And Not Me.Session("Uid") Is Nothing Then
Me.strDelSQL = "Delete from tbl_Albumshare_Mast where Album_Id=" & Me.Session("Albumid") & " and Sharer_ID=" & Me.Session("Uid")
Me.objDB.ExecQry(Me.strDelSQL)
Me.Response.Redirect("frmmyalbumshare.aspx", False)
End If
Broadly, the goal of this code is to take an album ID from the user and delete an album from the database where the ID matches and the album is owned by this user. And everything is horrible here.
First, the use of e.Item.FindControl
to look up the input field is suspicious. That's usually what you do when you've dynamically generated controls, but I don't think that's happening here. Because we use FindControl
we have to cast it to Label
, and only then can we get the text property. That itself raises a question, because why are user inputs getting stored in a label?. That's not what labels are for. They're clearly doing some sort of hacky lookup/autocomplete field thing and using the label to actually store the input, but presenting the user with a different UI for picking it. Not an uncommon pattern in those days, but a hidden field would be a more typical way of doing it.
We then construct a SQL injection vulnerability and run it against the database.
Then, we use a redirect to send the user to a different page. Again, a common thing to do back then, but a thing I hate: using HTTP 300 codes to redirect users as a form of navigation is awkward and weird.
Of course, none of that is precisely the WTF here. The real WTF is the developer apparently hates local variables. strDelSQL
is a member variable- why do we need to store this in a class member? Especially because in the page lifecycle, we handle one request and then die; either this is unnecessary, or far, far worse, some other code is looking at this SQL query later. But far worse than a badly scoped variable is the overuse of session variables.
Thomas sends us this block as a representation, but the developer responsible just stored everything they could in a session variable. While in this example, "Uid" probably makes sense, "Albumid" definitely doesn't. And in other cases, basically every user form input gets stored in a session variable, and even worse: they're often used as loop variables. while Session("myloop") < 5
type stuff.
And finally, we already know I hate hungarian notation, but here we have a client side form holdover that pollutes the user experience: the URL they get redirected to has a hungarian frm
prefix. It's bad enough that you're using hungarian notation, but don't show that to your users! They really don't need to see everything.