AccountCreationUserBucket.js
- uses a global 'MW' object. is this safe?
- I know there was talk of some centralized mediawiki JS object, just wanted to put it somewhere. Is there a place that's more appropriate?
- allActive is run for everyone in any bucket... make sure this is only run when needed, it could affect other pages
- It's run for everyone not in an "none" bucket
- 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)
- Yeah, that's the core of the study =)
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
- I made this whole system more sensible and readible now
- 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.
- I think this is more sensible and readible as well now
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. :)
- done
- 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)
- I'm pushing everything through a centralized validation function now
- extraction of the <a href>'s URL from the 'link' parameter is very fragile, and seems to assume that the link will always be in the form '<a href="(url)">'. 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)
- I based this code around the code that's doing the generation and it's pretty insanely brittle. I'll work on fixing this up soon.
- 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. :)
- 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
- validating =)
- again should probably validate the campaign param, though it's safeish here i think
CustomUserloginTemplate
- 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
CustomUsercreateTemplate
- duplicates code from CustomUserLoginTemplate -- could probably merge some of those functions. no biggie now, but beware of getting out of sync during later maintenance
- Would like to do this, but not sure of a good way while still allowing polymorphism to do its thing
This post was posted by Peachey88, but signed as Brion VIBBER.