search

bion33 / Storm

Storm is an extension for military gameplay on the NationStates website. It makes frequently used actions more convenient by assigning hotkeys to them. Please open an issue if you'd like to see a feature added or if something doesn't work as it should.
GNU General Public License v3.0
1 stars 0 forks source link

Needs a mechanism to impose the rate limit #7

Closed Eluvatar closed 2 years ago

Eluvatar commented 2 years ago

https://github.com/bion33/Storm/blob/581298a180fbe025d2a25674a7aad48fc9ab3619/scripts/background.js#L218

This script has been attributed as the source of hundreds of thousands of requests against nationstates.net per hour. Many of these requests look like this in our logs:

127.0.0.1 - - [18/Jan/2022:08:57:56 -0800] "GET /cgi-bin/api.cgi?nation=undefined&q=endorsements HTTP/1.1" 429 982 "-" "Application: Storm (https://github.com/Krypton-Nova/Storm); User: a_nation_with_a_warning_now"

(IP address and nation name in user agent redacted)

Please add a mechanism to impose the rate limit.

Aptenodyte commented 2 years ago

I played around with Storm for a bit and I believe I've found what's causing it to generate excessive requests. To reproduce this behavior, use the cross endorse shortcut on a nation, and then switch to another nation and use the cross endorse shortcut multiple times on this other nation.

Your script guards against spamming the cross endorse button with the check here. However, after you use the cross endorse shortcut on another nation, workerDone gets set to true here. It never gets set back to false, meaning that subsequent checks making sure that the endorsedWorker is not already running will pass. Because of that, the endorsedWorker function will be run multiple times, creating multiple setIntervals, and causing the script to send multiple requests simultaneously when it should only be sending one at a time.

In addition, the variable check is incremented here, but never reset. If the endorsedWorker has already made more API calls than the target nation has endorsements, check will go out of bounds, and the script will continue to issue API requests for "undefined" forever.

Fixing this shouldn't be too difficult though, I think all you'd have to do is add workerDone = false to the top of the endorsedWorker function to make sure it resets properly after being run again, and also add check = 0 in this if block.

Eluvatar commented 2 years ago

Thank you for that insightful analysis! It matches what we're seeing in the logs and makes sense from my limited understanding of this code.

bion33 commented 2 years ago

Thanks for the detailed issue. I'm looking into it.

bion33 commented 2 years ago

I'll post it here too just in case:

I've applied the fixes suggested, and confirmed that the sequence in which I reproduced the issue could not reproduce it again with these fixes.

I've added a version to the user agent so that it is obvious when a user uses the outdated addon. This may be the case in Chrome, as the addon is not on the store and is updated at the user's own discretion. Text is show in the popup menu indicating if there is an update. I've now added a notification so Chrome users in particular will be more actively notified of updates in the future.

bion33 commented 2 years ago

If the problem persists for Storm version >=4.5, feel free to reopen this issue :)