Improve validation of Bot Tokens #1206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Try to decode the user id from the supplied bot token as a way of validating the token. If this should fail, indicate that the token is invalid.
…ton/Discord.Net into improve-tokenutil-validation
Emzi0767
suggested changes
Dec 1, 2018
| { | ||
| // decode the first segment as base64 | ||
| var v = Convert.FromBase64String(segments[0]); | ||
| BitConverter.ToUInt64(v, 0); |
There was a problem hiding this comment.
You misinterpreted what I said, again. The base64 contents result in a string representation of the ulong, i.e. it decodes to a string.
BitConverter is for converting binary representations to respective types (so if it decoded to 8 bytes, that'd be the ulong you'd need to convert).
What you actually need to do is something akin to Encoding.ASCII.GetString(v) and run the result of that through ulong.Parse() or ulong.TryParse().
There was a problem hiding this comment.
After checking against one of my own tokens (id = 428477944009195520), I was able to fix this result. The examples from Discord docs looked correct, despite following the wrong procedure. Fixed now.
foxbot
approved these changes
Dec 2, 2018
brankin3
added a commit
to brankin3/Discord.Net
that referenced
this pull request
Dec 5, 2018
* fix: Fixed CommandExecuted firing twice for failed RuntimeResults (discord-net#1192) * Fixed CommandExecuted firing twice for failed RuntimeResults * Changed to just checking the result type * Amendments * Resolve Issue discord-net#1188 (Allow Users to specify position when creating a new channel) (discord-net#1196) * Added ability to specify position when creating a channel * Adjusted categories to include guildproperties and allow specifying position when creating channel categories * fixed unimplemented methods (for CreateCategoryChannelAsync) and added appropriate documentation * feature: add Format.Url, Format.EscapeUrl Format.Url formats a URL into a markdown `[]()` masked URL. Format.EscapeUrl formats a URL into a Discord `<>` escaped URL. * feature: add extensions for bulk reactions this is not api-level bulk reactions (!) * fix: Update ChannelCreateAuditLogData.cs (discord-net#1195) * feature: Implement Dispose for types which have disposable data (discord-net#1171) * Initial set of dispose implementations Not handled yet: - Discord.Net.Websocket/Entities/SocketGuild - Discord.Net.Tests * Refactor DiscordSocketClient init into ctor This way we remove an IDisposableAnalyzer warning for not disposing the client when we set the client variable. * Dispose of clients when disposing sharded client * Finish implementing IDisposable where appropriate I opted to use NoWarn in the Tests project as it wasn't really necessary considering that our tests only run once * Tweak samples after feedback * fix: Added Msg.Content Null Check For Prefixes (discord-net#1200) * Added Msg.Content Null Check * Minor Change * Grouped Params In If Statement * Minor Change * api: [brk] Move Invites-related Methods from IGuildChannel to INestedChannel (discord-net#1172) * Move invites-related methods from IGuildChannel to INestedChannel * Add missing implementation ...because I somehow forgot it the first time around * fix: Solves AudioClient Lockup On Disconnect (discord-net#1203) * Solves Audio Disconnect Lockup * Execute Disconnected Event Before Logger & State * fix: Solves UdpClient "ObjectDisposedException" (discord-net#1202) * Solves "ObjectDisposedException" * Corrected Spelling Error * Fixed Spelling * test: bump dependency versions * fix: Update minimum Bot Token length to 58 char (discord-net#1204) * Update the minimum bot token length to 58 char - Updates the minimum length of a bot token to be 58 characters. An older 58 char bot token was found by Moiph - Makes this value an internal const instead of a magic number * update the TokenUtils tests for 58 char min * fix: Fix after message remaining in MessageUpdated if message author wasn't in the payload (discord-net#1209) * Fix leaving updated message as null when Discords doesn't include a message author in MESSAGE_UPDATE * Name! That! Parameter! * fix: Improve validation of Bot Tokens (discord-net#1206) * improve bot token validation by trying to decode user id from token Try to decode the user id from the supplied bot token as a way of validating the token. If this should fail, indicate that the token is invalid. * Update the tokenutils tests to pass the new validation checks * Add test case for CheckBotTokenValidity method * lint: clean up whitespace * Add check for null or whitespace string, lint whitespace * fix userid conversion * Add hint to user to check that token is not an oauth client secret * Catch exception that can be thrown by GetString * Refactor token conversion logic into it's own testable method * feature: Allow setting custom error messages for preconditions (discord-net#1124) * Rebase and use in all in-box preconditions * Silly git.... * Add configurable 'NotAGuild' message * Respond to feedback * docs: add example of custom error message to sample * feature: add DiscordSocketRestClient (discord-net#1198) * feature: add DiscordSocketRestClient this resolves discord-net#803. Users can access a DiscordSocketRestClient from the new `DiscordSocketClient.Rest` property. DiscordSocketRestClient is a wrapper over DiscordRestClient with certain state-modifying methods, such as Login/Logout disabled, to prevent users from breaking the client state. DiscordSocketRestClient uses the same API client as the DiscordSocketClient, allowing for shared ratelimiting - meaning users can now force HTTP requests without needing to wory about running into 429s. * fix: disallow users from bypassing shadowed login
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per feedback from @Emzi0767 received right before #1204 was merged, this PR improves the methods used to validate Bot Tokens.
In addition to checking that tokens satisfy a minimum length requirement, this method will also try to decode the user id from the first part of the Bot Token. Test cases have been updated as well.
Exception messages have been updated to include a hint to check if the supplied Bot Token is an OAuth client secret.