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.

Sign up for GitHub

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

Jump to bottom

Audit API for parameters that can heavily de-optimize requests #1070

Closed
Tracked by #1069
Mr0grog opened this issue Jan 26, 2023 · 0 comments · Fixed by #1084
Closed
Tracked by #1069

Audit API for parameters that can heavily de-optimize requests #1070

Mr0grog opened this issue Jan 26, 2023 · 0 comments · Fixed by #1084

Comments

@Mr0grog
Copy link
Member

Mr0grog commented Jan 26, 2023 •
edited
Loading

We want the API to be publicly readable, and there are a lot of parameters or options that can cause it to have unacceptable performance (e.g. causing full table scans on large tables, causing N+1 queries) if publicly accessible. We need to audit all the controllers and models and either:

  • Remove non-essential ones
  • Restrict access to essential ones
This was referenced Jan 26, 2023
Closed
Closed
Mr0grog added a commit that referenced this issue Jan 27, 2023
@Mr0grog
Ensure permissions are correctly enforced
935976c 
It turns out we were lazy about a lot of permissions in the API since we had a small set of users and nobody with `view` permissions didn't also have `annotate`. Now that we are enabling public access, that's a problem! This makes sure we're checking appropriate permissions in all the API controllers and actions.

Found as part of auditing API access and options in #1070.
@Mr0grog Mr0grog mentioned this issue Jan 27, 2023
Merged
Mr0grog added a commit that referenced this issue Jan 27, 2023
@Mr0grog
Ensure permissions are correctly enforced (#1072)
613c428 
It turns out we were lazy about a lot of permissions in the API since we had a small set of users and nobody with `view` permissions didn't also have `annotate`. Now that we are enabling public access, that's a problem! This makes sure we're checking appropriate permissions in all the API controllers and actions.

This also does a little work to differentiate requests with no credentials and invalid credentials, which is important now that we are enabling public view access (no credentials is OK, invalid credentials is obviously an error, but needs an API-style response rather than the default authentication error handling). Devise doesn't provide anything that differentiates those, so we have to drop down to check some Warden data.

Found as part of auditing API access and options in #1070.
Mr0grog added a commit that referenced this issue Feb 3, 2023
@Mr0grog
Block public access to some /versions parameters
9487e7d 
Some parameters for the various `/versions` collections cause expensive queries, so they are disallowed for public, non-logged-in usage.

Solves part of #1070.
@Mr0grog Mr0grog mentioned this issue Feb 3, 2023
Merged
@Mr0grog Mr0grog linked a pull request Feb 6, 2023 that will close this issue
Block public access to some expensive parameters #1084
Merged
Mr0grog added a commit that referenced this issue Feb 7, 2023
@Mr0grog
Block public access to some expensive parameters (#1084)
e02fd14 
Some query parameters cause expensive queries, so they are now disallowed for public, non-logged-in usage. If you try to use them without being logged in, you’ll get a 403 response with a message about what parameters for that controller require logging in.

If we need to extend this to other places, we can use the new `BlockedParamsConcern` on a controller.

Fixes #1070.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Block public access to some expensive parameters edgi-govdata-archiving/web-monitoring-db
1 participant
@Mr0grog