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

limit queries #13

Merged
merged 5 commits into from
Jul 9, 2014
Merged

limit queries #13

merged 5 commits into from
Jul 9, 2014

Conversation

fanchyna
Copy link
Contributor

@fanchyna fanchyna commented Jul 7, 2014

limit the number of queries per ip per day, default is 3, configurable in web.xml, effective immediately (no need to restart tomcat).
files added:

  • queryexceeded.html
  • SimpleQueryLimitFilter.java
    files updated:
  • web.xml

@madian9
Copy link

madian9 commented Jul 7, 2014

What's the current value? 3 per day sounds low to me
On Jul 7, 2014, at 12:27 PM, Jian Wu wrote:

limit the number of queries per ip per day, default is 3, configurable in web.xml, effective immediately (no need to restart tomcat).
files added:

queryexceeded.html
SimpleQueryLimitFilter.java files updated:
web.xml
You can merge this Pull Request by running

git pull https://github.com/SeerLabs/CiteSeerX limit_request
Or view, comment on, or merge it at:

#13

Commit Summary

add solrlogo
File Changes

A web/citeseerx_webapp/images/solrlogo1.png (0)
Patch Links:

https://github.com/SeerLabs/CiteSeerX/pull/13.patch
https://github.com/SeerLabs/CiteSeerX/pull/13.diff

Reply to this email directly or view it on GitHub.

@fanchyna
Copy link
Contributor Author

fanchyna commented Jul 7, 2014

3 is the default, the web.xml can override it. I would match the download limit, so 2000.

@kylemarkwilliams
Copy link
Contributor

Agreed, 3 seems low as a default. Also, it seems like some files are missing from the commit (I only see the logo?)

@madian9
Copy link

madian9 commented Jul 7, 2014

Let's set default to 100 at least
On Jul 7, 2014, at 12:35 PM, Kyle Williams wrote:

Agreed, 3 seems low as a default. Also, it seems like some files are missing from the commit (I only see the logo?)


Reply to this email directly or view it on GitHub.

@fanchyna
Copy link
Contributor Author

fanchyna commented Jul 7, 2014

I forgot to run the commit command. Now you should see the files. 2000 is set in web.xml

@cleegiles
Copy link

How easy is this to change? We may need to reduce 2000.
On 7/7/14 1:40 PM, Jian Wu wrote:

I forgot to run the commit command. Now you should see the files. 2000 is set in web.xml


Reply to this email directly or view it on GitHub:
#13 (comment)

@fanchyna
Copy link
Contributor Author

fanchyna commented Jul 7, 2014

it is not implemented yet, i choose 2000 just to match the download limit. how about 1000?

@cleegiles
Copy link

Probably more reasonable.

Why did the attacks stop over the weekend? What happened?

On 7/7/14 3:01 PM, Jian Wu wrote:

it is not implemented yet, i choose 2000 just to match the download limit. how about 1000?


Reply to this email directly or view it on GitHub:
#13 (comment)

@fanchyna
Copy link
Contributor Author

fanchyna commented Jul 7, 2014

I am writing a new method to my tomcat_log_analyzer to separate the queries. This will tell us more about the attack.

@@ -0,0 +1,271 @@
/*
* Copyright 2007 Penn State University
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update this to 2014. Also, other comments that refer to downloads should refer to queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fullfilled

@fanchyna
Copy link
Contributor Author

fanchyna commented Jul 8, 2014

I've changed the variable naming and tested it on the staging machine. The limit specified in web.xml is changed to 1000 per day. I've submitted the updated version. Thanks.

@kylemarkwilliams
Copy link
Contributor

Besides my last comment on whether stopping the same query is really necessary, I think this is ready to merge.

@fanchyna
Copy link
Contributor Author

fanchyna commented Jul 9, 2014

I think the original designer was thinking of cases when some users or bots automatically send the same queries too fast and cause some "query spikes". Anyway, I will go with you this time.

@fanchyna
Copy link
Contributor Author

fanchyna commented Jul 9, 2014

I've commented out the block to check ipaddr+request. New code is tested on csxstaging01.

kylemarkwilliams added a commit that referenced this pull request Jul 9, 2014
Limits the number of queries per IP. Tested on staging.
@kylemarkwilliams kylemarkwilliams merged commit f5fcf92 into master Jul 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants