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

fix: collections not being initialized without User-Agent header #3645

Merged
merged 4 commits into from
May 10, 2024

Conversation

azurit
Copy link
Member

@azurit azurit commented Apr 3, 2024

ip and global collections are not initialized for requests without a User-Agent header. This may allow malicious users to bypass rules using these collections.

Fixes #3642.

@azurit azurit changed the title Fix for collections not being initialized in some cases fix: fix for collections not being initialized in some cases Apr 3, 2024
@fzipi fzipi changed the title fix: fix for collections not being initialized in some cases fix: collections not being initialized without User-Agent header Apr 9, 2024
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write tests for this change?

I mean, for both cases. With and without user-agent.

@azurit
Copy link
Member Author

azurit commented Apr 9, 2024

@fzipi Sure but is there a way how can i know if collections were or were not initialized? How to write a test for it?

@fzipi
Copy link
Member

fzipi commented Apr 21, 2024

Don't know what to say here. @dune73 do you have a clue on how to test this?

@dune73
Copy link
Member

dune73 commented Apr 23, 2024

Testing this with ftw is complicated.

If you want to test it, you would need an additional rule that checks for a variable in the collection. If you do not want to do that, then you add a plugin with an effect based on the collection. Like the anti-dos plugin and then you run enough requests to trigger (and kill all subsequent testing).

Honestly, I think we can add a remark here and forget about the testing for the time being, it's not worth the hassle.

fzipi
fzipi previously approved these changes May 9, 2024
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can add a comment to the rule then better. If not, I'll just approve.

@azurit
Copy link
Member Author

azurit commented May 9, 2024

Sure but what comment?

@dune73
Copy link
Member

dune73 commented May 10, 2024

Proposal:

The creation of the IP and the GLOBAL collection is not being tested as
of this writing due to limits in ftw and our testing setup. 
Proper testing would involve the checking of a variable in the said collections.

@azurit
Copy link
Member Author

azurit commented May 10, 2024

Done!

@fzipi fzipi added this pull request to the merge queue May 10, 2024
Merged via the queue into coreruleset:main with commit 88c3a77 May 10, 2024
4 checks passed
@azurit azurit deleted the IPcolFix branch May 17, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialization of collection does not work
3 participants