Hey Dave, sounds like a good presentation. A lot of the earlier common mistakes are probably there because we needed to attend a couple sessions like yours. In any case, I'm glad we've cleaned up a lot since then. That does make me think you should include instructions in the presentation for the correct action to take when you think you've discovered a security vulnerability.

Anyways, responding directly to your points, I did want to advise based on what I read in the code without being too touchy. I understand you're not crying wolf since you're examining code that has changed, but at the same time I think a statement like this - Most of the mistakes opened the system up to Cross-Site-Scripting. - is misleading given the things I'll note below.

Also, I'm not really sure what code you're looking at when you say 1.2 and 1.3. There hasn't been a 1.2 or 1.3 release yet, though we did clean up a lot of the issues you mentioned at various points in the alphas, betas, and maybe release candidates. Are you being intentionally vague for the purpose of users of the older versions? If so, no sweat, I just had to work a little harder to find the original code.

So... "technical review":

1. Agreed, but you might also check for validation elsewhere. For example, in that case arg(3) is being tested in hook_menu() with is_numeric(). This means you'd have to be able to make an XSS attack out of a number... which as far as I know isn't possible. I'm sure there are other examples where this was actually a security vulnerability.
2. This is the biggie... even when we knew to be conscious of this, it still slipped through. I think this goes along w/ the watch out for things you're too familiar with advice you had. A second set of eyes always helps. Also, one tip we picked up was to simply test all your forms w/ some HTML in there, like entering First name and seeing if it showed up in italics anywhere.
3. This is a good general rule, but examining the function in question will show that that variable was NULL... it may have been used to display debug info while testing, but as far as I can see we removed the debug code and simply forgot to remove this variable until a later revision. So... safe.
4. I can't even find that example, but it would be worth explaining when to use this function instead of just defaulting to check_plain() along with an example for Drupal core.
5. Agree with the point, but again the example is misleading. In this case, $arg1 is an order object, and we're not printing that... we're printing instead the formatted results of uc_payment_balance() which returns a numeric string w/ a currency code. The data being outputted is quite controlled and never affected by user input.
6. Agreed. Might be worth mentioning that even literal values in queries should use placeholders to be forward thinking. I'll point out here again that there was other validation in the menu item for this function, too, such that the $uid value passed to the function must match the current user's uid which will always be an integer. Still should use placeholders, though.
7. Guilty as charged... I actually don't get why the name isn't just passed in as an argument on that URL.

I'm not sure what else you should cover... the kinds of attacks you're on the lookout for are great. You might just make a mention that proper use of the Forms API will protect a site from XSS over forms and $_POSTed information... Also, I'd make it explicit that we're filtering user input when it gets displayed and not when it's being stored. i.e. you don't check_plain a string you're storing in the DB.

Hope that helps, and sorry if I sound overprotective. Just wanna make sure Ubercart gets blamed for actual faults. I think the main thing is just checking for other types of validation on data that's printed (i.e. type checking, restrictions on input, etc.). In most of the cases you pointed out, I still think the changes and compliance with general security standards were a very good thing, but they weren't the security holes that they were pointed out to be.

Lastly... thanks so much for taking the time to do this write up. I know it took a lot of time to compile, and I don't want to dissuade you with my comments. Let me know if you have any follow up questions and I'll be happy to answer.

Dave, Ryan,

I think this write up is great. IMO it should be published after some spelling corrections in Drupal Planet not only in Ubercart. Being able to point at our mistakes isn't something bad, quite the opposite it shows the maturity of the product/ developers.