Initial commit of ratelimit add-on

[double16]

Limits request rate to prevent overloading the target or being blocked.

[double16]

If anyone has icon skills that would be helpful. I do not have those skills.

[kingthorin]

To address the DCO requirement you'll need to sign-off the commit(s):

• https://github.com/zaproxy/zaproxy/blob/main/CONTRIBUTING.md#developer-certificate-of-origin
• https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff

[double16]

IP addresses are not handled correctly.

[kingthorin]

Good catch!

[kingthorin]

Finally got to testing this today. One item of note the "Domain" option can be a bit ummm dangerous or over zealous in the case of theregister.co.uk ➡️ co.uk or testfire.net ➡️ net.

I noticed that duplicate rules can be added, I didn't test how that pans out functionally/performance wise but there should probably at least be a simple check to prevent users from making the exact same rule multiple times. I understand with regex they can still do the wrong thing, but...

I was also thinking that adding the icon to menu items might be a good move:

diff --git a/addOns/ratelimit/src/main/java/org/zaproxy/addon/ratelimit/ExtensionRateLimit.java b/addOns/ratelimit/src/main/java/org/zaproxy/addon/ratelimit/ExtensionRateLimit.java
index b078ad250..acd46977e 100644
--- a/addOns/ratelimit/src/main/java/org/zaproxy/addon/ratelimit/ExtensionRateLimit.java
+++ b/addOns/ratelimit/src/main/java/org/zaproxy/addon/ratelimit/ExtensionRateLimit.java
@@ -142,7 +142,7 @@ public class ExtensionRateLimit extends ExtensionAdaptor implements HttpSenderLi
     private ZapMenuItem getRateLimitMenuItem() {
         if (rateLimitMenuItem == null) {
             rateLimitMenuItem = new ZapMenuItem("ratelimit.topmenu.tools.title");
-
+            rateLimitMenuItem.setIcon(ICON);
             rateLimitMenuItem.addActionListener(
                     e ->
                             Control.getSingleton()
@@ -202,6 +202,7 @@ public class ExtensionRateLimit extends ExtensionAdaptor implements HttpSenderLi
             popupMenu =
                     new PopupMenuHttpMessageContainer(
                             Constant.messages.getString("ratelimit.context.title"));
+            popupMenu.setIcon(ICON);
             popupMenu.add(
                     new RateLimitHostMenu(
                             this,

Lastly I think it might be a good move to add a toolbar to the panel/tab with a button to open the rule configuration.

I'll try to finish reading through the code later today.

[double16]

testfire.net ➡️ net should not have happened, did you experience that? I added a test for it.

The worst a duplicate rule would do is limit the rate more than desired. The code already considers the case of a host matching multiple rules. The rule with the slowest limit is chosen. The list of rules is ordered and traversed in that order so exact duplicates would produce stable results. I did add checks to not add duplicates from the context menu.

[kingthorin]

I'll retry with the add-on prior to your fix and double check.

[kingthorin]

You're right testfire.net didn't chop to .net it stayed whole.

[kingthorin]

Good catch.

[thc202]

Some comments:

• IMHO this does not belong in its own add-on (Network add-on seems to be a better place).
• Using a HTTP Sender listener is not a good implementation, there are no ordering guarantees which would/could break the expected behaviour.
• This duplicates existing functionality (would be good to know what are the long term plans).

[double16]

Can you point me to an alternative to HTTP Sender listener?

This isn't very duplicative, most features do not support rate limiting. When asking for long term plans I assume you're asking the team, and not myself since this is my first PR.

I can move to the Network add-on, but I'll wait until the other issues are answered. I don't want to spend a lot more time on a PR that isn't going to be accepted.

[kingthorin]

I've spoken briefly with some of the team about this. I'll try to answer:

• It would be great to move it to the network add-on. (Probably in BaseHttpSender, wrapping sendImpl.)
• It isn't very duplicative, we agree on that front.
• As for the "long term" bit that wasn't for the team, that was more asking if you're interested/able to address the few places that it does duplicate (in separate PR(s)).

[kingthorin]

Could you also rebase this against up-to-date upstream/main at some point as well? Java 8 CI is no longer done, the project is now targeting 11.

[double16]

Sounds good. I can start moving to the network add-on next week. I'm interested and able to replace the current rate limits with this one.

[kingthorin]

Thanks. I believe the other places are active scan (in the core) and the fuzzer (in this repo).

[thc202]

Note that the feature in the Fuzzer applies to all messages that can be fuzzed (e.g. WebSocket), not just HTTP.

[double16]

For the API class, merge the RateLimitAPI class into NetworkApi or keep separate?

[double16]

There is an internal.ui package. Do you want all of the rate limit UI to be prefixed with "RateLimit", or is it better to create a internal.ui.ratelimit package? FWIW, I prefer internal.ui.ratelimit.

[thc202]

IMO merge the APIs. Re the package, I don't have a preference as long as it's added under the internal package.

[double16]

When I use gradlew addOns:network:copyZapAddOn, the new ratelimit i18n messages aren't found. In the .zap file I see them in Messages.properties. Is there a cache I need to clear?

[thc202]

The keys should have the network prefix.

[kingthorin]

You should rebase instead of merging. It'll make your life easier.

[thc202]

You can use gradlew :aO:network:iZAO.

[double16]

Ideas for passing the RateLimitTracker from the ExtensionNetwork into BaseHttpSender? Adding a field to the context doesn't seem to work out because some of the context code is in zaproxy.

[thc202]

Feel free to add a setter to CloseableHttpSenderImpl, add a no-op tracker, and set it by default (to not require setting up the tracker for tests).

[double16]

How do I add the RateLimitOptionsPanel without using LegacyOptionsPanel ? It doesn't make sense to use it because rate limit is new.

[kingthorin]

You could flatten this. In the end the only thing that matters to the greater project is that it is/was added :wink:

[double16]

You could flatten this. In the end the only thing that matters to the greater project is that it is/was added 😉

I don't know how to do that efficiently. I'm assuming a squash merge, so it'll all end up as a single commit into main.

[kingthorin]

I can do it for you and provide some instructions how to then update your local so you don't clobber anything. Sound okay?

[double16]

I can do it for you and provide some instructions how to then update your local so you don't clobber anything. Sound okay?

I'd appreciate that!