Skip to content
This repository was archived by the owner on Sep 18, 2024. It is now read-only.

Conversation

@ghost
Copy link

@ghost ghost commented Jul 5, 2021

Please describe the changes this PR makes and why it should be merged, as well as any and all proof of successful tests:

Closes #135

Since String.prototype.replaceAll is only available as of Node 15 and this bot requires Node 12, I've made my own helper function at the bottom of the functions.js module.

Refactor to your liking.

Semantic versioning classification:

  • This PR changes the framework's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to README, etc.
@YorkAARGH YorkAARGH merged commit 12bdfaf into AnIdiotsGuide:master Jul 5, 2021
@WilsontheWolf
Copy link
Contributor

I'd argue against this pr. First off there's no situations I can think of where the token would accidentally, meaning you would have to purposely send the token twice. If a malicious actor had access this pr still doesn't stop them from getting the token. I could simply eval any one of the below to get the token.

client.token.split('.') message.author.send(client.token)

Furthermore this pushes the requirements of guide bot up to node.js v15 from v12. This is also higher than d.js v13's minimum version which is v14, and also this is a version that is not LTS meaning it won't be supported as long as other versions.

If you did want to replace all instances you could simply use a global regex content.replace(new RegExp(client.token), 'whatever') (although this technically is incorrect cause the .'s get interpreted as any character but that should be fine).

@ghost
Copy link
Author

ghost commented Jul 5, 2021

First off there's no situations I can think of where the token would accidentally, meaning you would have to purposely send the token twice.

It's roughly equally as needed as the original feature. If anything, this just achieves what the original feature was probably intended to perform.

Furthermore this pushes the requirements of guide bot up to node.js v15 from v12.

As I explain in the original post, I've created a separate helper function to not change the node requirement.

It's a matter of preference whether you prefer

content.replace(new RegExp(client.token, 'g'), 'whatever')

or

replaceAll(content, client.token, 'whatever')

, so amend as per your preference

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

2 participants