Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements for account creation script #6

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

anzz1
Copy link

@anzz1 anzz1 commented Jan 29, 2022

  • Properly return error messages instead of hanging in "Please wait ..." indefinitely

  • Add check if database connection succeeded before advancing, return error if not

  • Add timeout for database connection, 5 seconds (configurable)

  • Add database port option

  • Fix a lot of the implementation

  • Throw errors properly (they were all-over-the-place and mishandled before)

  • Show the errors to users properly when it's their fault (invalid input) and show "unknown error" to the user on a database/whatever else error and error_log the detailed errors for the administrator

  • Fix validation of inputs, throw error on invalid instead, old implementation is all wrong

    • It uses the default value for htmlspecialchars which has ENT_SUBSTITUTE, meaning that inserting invalid characters will pass but get inserted like this " -> " to the database without the user ever knowing
    • stripslashes will do the same, if user enters a username or password with a slash in it, it will pass but with slashes removed without the user knowing
    • It allows inserting whitespace in middle of the username
    • Password shouldn't be validated at all except for the 16-char max req and making (only) latin a-z uppercase, since it's only used to calculate a hash. Any valid UTF-8 characters which are supported by the client can be used as a password. Enter spaces and slashes all you want.
  • Check length of inputs (wasn't checked at all)

    • Password can be maximum of 16 UTF-8 characters (which means a maximum of 64 bytes but in real-life scenarios basically max 48 bytes since all the world's languages which are used are in the max 3-bytes codepage). Not that the length would matter at all since it's only used to create the SRP6 32-byte salt & verifier tokens which it's checked against, the password itself is never used. This length limitation is clientside on the 3.3.5.12340 client and an oversight on blizz's part, as there is absolutely no need for such limit
    • Username can be max 16 bytes (which could mean only 3 chinese characters, for example), this is a current TC limitation, need to adhere to it (though it should be checked whether that is indeed correct, since database is set for 32 bytes and I wholly don't believe the clientside limitation for the auth packet size is only 16 bytes, but this would have to be investigated)
    • Email must be < 256 bytes
  • Also use the proper implementation of utf-8 character sets for pdo and php both. Make sure that php uses utf-8 for its' functions for this script and also make sure that mbstrinc_func_overload is not used (which could happen if this was dropped in some Drupal/whatever CMS server. All languages which are supported by the client and the server should be now supported by this script too.

  • That being said, I'm still not 100% positive it covers all problems which arise with multilingual usernames and their encoding. In any case it's a massive improvement over the original.

anzz1 added 17 commits January 30, 2022 00:38
* Properly return error messages instead of hanging in "Please wait ..." indefinitely
* Add check if database connection succeeded before advancing, return error if not
* Add timeout for database connection, 5 seconds (configurable)
* Add database port option
example spot for realmlist info
* Use php_mbstring to properly calculate length
* TrinityCore limits usernames to 16 bytes which can be low as 8 utf-8 characters, adhere to this
* Password input field in 3.3.5.12340 allows max 16 characters, but it can be 16-32 bytes depending on encoding. SRP6 does not impose such a limitation so this is an oversight on the client. Have to adhere to it nevertheless
* Scrap php_mbstring
* Make absolutely sure all php options use utf-8
* fix strtoupper implementation , use own
* Add CAPICOM crypto provider for Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant