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

Implement additional filtering for Sentry #347

Closed
matthew-white opened this issue Mar 25, 2021 · 8 comments · Fixed by #375
Closed

Implement additional filtering for Sentry #347

matthew-white opened this issue Mar 25, 2021 · 8 comments · Fixed by #375

Comments

@matthew-white
Copy link
Member

matthew-white commented Mar 25, 2021

We currently filter many Sentry events. This issue is to implement additional filtering. Here are some ideas for that filtering:

  1. Filter out the request body for additional endpoints
    • /sessions (POST)
    • /users (POST)
    • /users/:id (PATCH)
    • /users/reset/initiate
    • /projects/:id/key
    • /config/backups/initiate
    • /backup (at least the POST version)
    • Any others?
  2. Filter the request for the submission download endpoints. Filtering out the request body would work for the GET endpoints, but the POST endpoints use query parameters, not the request body.
  3. Filter out the q query parameter from /users
  4. Filter out tokens from URLs (@lognaturel mentioned that this would be a good idea)
    • App users
    • Form drafts
    • Public links (the st query parameter)
  5. Filter out the CSRF token from the request body
  6. Filter out the Authorization header. I think Sentry filters it out server-side, but recommends not sending it in the first place. I think it would be easy to remove?
@issa-tseng
Copy link
Contributor

yeah the reason i've been shy about this is bc it's really really hard to test this code AND it's like the stuff that runs IN the failsafe so i don't want to write a lot of stuff that could fail. but it does seem like we have to do this.

@lognaturel
Copy link
Member

lognaturel commented Mar 30, 2021

The __enketo_meta_deviceid cookie would be best to filter out.

Add /users/reset/initiate to endpoints to filter out request body for.

@matthew-white
Copy link
Member Author

matthew-white commented Mar 31, 2021

The __enketo_meta_deviceid cookie would be best to filter out.

Ah I'm remembering that Backend already strips out cookie values, passing only cookie names to Sentry. So I think no change is needed for this cookie. But is this value passed in any other way? Like using the deviceID query parameter?

Add /users/reset/initiate to endpoints to filter out request body for.

We filter out the request body for /users/reset/verify, but not /users/reset/initiate. Is the information passed to /users/reset/initiate also considered sensitive?

@lognaturel
Copy link
Member

lognaturel commented Mar 31, 2021

When did the additional stripping get added? https://sentry.io/organizations/getodk/issues/2291374869 (needs credentials) is an example that illustrates both I mentioned. This specific one is a POST on /users. Others with /users/reset/initiate include the body as well.

Is the information passed to /users/reset/initiate also considered sensitive?

Yes.

@matthew-white
Copy link
Member Author

When did the additional stripping get added?

The stripping was added in 8f16ad8, which went out with v1.1.0. We won't see the stripping though from servers using earlier versions of Central. (On another note, it'd be amazing to somehow get the Central version into Sentry, but obviously that's out of scope for this issue.)

Is the information passed to /users/reset/initiate also considered sensitive?

Yes.

Sounds good. Based on that, I've added /users/reset/initiate, POST /users, PATCH /users/:id, and POST /sessions to the description above. I've also added that we should filter out the q query parameter from GET /users.

@lognaturel
Copy link
Member

went out with v1.1.0

Right, it's not unlikely they just haven't updated.

@lognaturel
Copy link
Member

The stripping was added in 8f16ad8, which went out with v1.1.0.

We have an example of a hosted server that we know is running v1.1 still sending the full __enketo_meta_deviceid and __csrf values.

@matthew-white
Copy link
Member Author

Interesting, that's definitely unexpected. 🤔 I think we should look into that as part of this ticket.

ktuite added a commit that referenced this issue Jul 21, 2021
More Sentry filtering: remove draft from token from URL

Unit tests for sentry filtering functionality

Sentry filtering: preserve some query params, always clear request body

Better organization and comments

Removed duplicate test url

Code and tests to sanitize enketo st query parameter

Remove x-action-notes header

Added GET + projects/formList to sensitive endpoints

appease linter with const sanitizedEvent
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 a pull request may close this issue.

3 participants