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

Prevent deep pagination #535

Merged
merged 2 commits into from
Apr 23, 2019
Merged

Prevent deep pagination #535

merged 2 commits into from
Apr 23, 2019

Conversation

thatandromeda
Copy link
Contributor

Ready for merge?

NO

Requires stakeholder approval; conversations in progress.

What does this PR do?

This prevents users from requesting search result pages past page 2000 (which just causes an elasticsearch error). It also prevents them from going too far into the search results unless they're authenticated or using the API, because it's computationally more intensive for us to serve those, and casual users are unlikely to page very far in.

Helpful background context (if appropriate)

How can a reviewer manually see the effects of these changes?

Try visiting a search page with page=10, page=11, page=11 while you are logged in, and page=2001.

Your localhost elasticsearch config may throw errors before page 2001, but production supports through 2000.

What are the relevant tickets?

n/a

Screenshots (if appropriate)

Todo:

  • Tests
  • Documentation
  • Stakeholder approval

Requires Database Migrations?

NO

Includes new or updated dependencies?

NO

Sometimes bots try to hit us by cycling through values of the page
parameter. This results in lots of errors in our logs (due to
elasticsearch limits on the number of results returned). Also, deeper
pages are much more resource-intensive for us to render.

Therefore, let's simply prevent people from accessing impossible pages,
and restrict non-logged-in users.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 914

  • 9 of 10 (90.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 83.707%

Changes Missing Coverage Covered Lines Changed/Added Lines %
app/controllers/search_controller.rb 9 10 90.0%
Totals Coverage Status
Change from base Build 903: 0.1%
Covered Lines: 1793
Relevant Lines: 2142

💛 - Coveralls

@thatandromeda thatandromeda merged commit b992c02 into dev Apr 23, 2019
@thatandromeda thatandromeda deleted the prevent_deep_pagination branch April 23, 2019 20:36
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.

None yet

3 participants