Extension talk:CustomUserSignup/LQT Archive 1

Quickie review notes
AccountCreationUserBucket.js


 * uses a global 'MW' object. is this safe?
 * allActive is run for everyone in any bucket... make sure this is only run when needed, it could affect other pages
 * wpSave and wpPreview buttons don't appears to be on the login page. Are these leftovers, or are we tracking all of their saves and previews from here on? (if so then that's prolly just fine)

CustomUserSIgnup.php
 * the hook for BeforePageDisplay should always be set, and execute conditionally based on a config variable -- or else simply left, since we're the only user and we can require ClickTracking :D
 * code relies on ext.UserBuckets being set up in $wgResourceModules first; this will break if ClickTracking.php ends up getting included after CustomUserSignup. It might be safer to set this from a function called via $wgExtensionFunctions -- this will run after all LocalSettings.php stuff is done.

CustomUserSignup.hooks.php
 * userCreateForm
 * use the instanceof operator instead of get_class and comparing the resulting string; this will be safer if, say, some other extension is also messing with you. :)
 * security: $campaign is not validated before being dropped into URLs; while some of these may be escaped safety, it could definitely still inject parameters into the form submission. Looks like direct HTML injection is a possibility on the 'link' parameter (this string is already HTML escaped, so interpolation is raw HTML)
 * extraction of the 's URL from the 'link' parameter is very fragile, and seems to assume that the link will always be in the form ''. This will break if a 'class', 'title', etc is added. A regex can do the same check more flexibly, or you could run it through a DomDocument (loadHTML etc) and pull out that way (eww scary)
 * since the link param overall gets rewritten, the regexes adding the campaign parameter to that URL can probably be replaced by just appending it down in the code that's manipulating the whole 'link' text.
 * welcomeScreen
 * again should probably validate the campaign param, though it's safeish here i think

CustomUserloginTemplate CustomUsercreateTemplate
 * constructor does a preg_match against 'campaign' and uses the resulting matches array, without checking if it actually had a match. Double-check this is ok, and if it is, then it probably should be bundled into a function and used on all these things to validate :D
 * msgWikiCustom -- the check for plaintext is a bit funky but should do for now
 * duplicates code from CustomUserLoginTemplate -- could probably merge some of those functions. no biggie now, but beware of getting out of sync during later maintenance

-- brion 23:33, 18 April 2011 (UTC)