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

DELETE methods not allowed by apache journalist interface config #3977

Closed
redshiftzero opened this issue Dec 6, 2018 · 2 comments
Closed
Labels

Comments

@redshiftzero
Copy link
Contributor

Description

This is the issue causing freedomofpress/securedrop-client#173

Steps to Reproduce

  1. Provision staging VMs
  2. Get a valid API token [ref]
  3. Add a source via the source interface
  4. Attempt to remove a star from a source (this is an idempotent operation so one should not need to first add the star to hit the endpoint):
curl -X DELETE -H "Content-Type: application/json" -H "Authorization: Token mytokenhere" --proxy socks5h://127.0.0.1:9150 mystagingserver.onion/api/v1/sources/my-source-uuid-here/remove_star

Expected Behavior

{"message":"Star removed"}

Actual Behavior

A 403 occurs, and we get redirected to the login page.

This redirect-for-403 behavior only happens for the API in staging because Apache is sending 403s to /notfound, causing the request.path to be /notfound (instead of something that begins with thisismeonion.onion/api/blahhh), meaning that code execution will continue until the redirect here.

Comments

The cause is the 403, and this is happening because of the LimitExcept directive we are using in the Apache configs. These allow only GET POST and HEAD requests.

To prove this to oneself, you can edit /etc/apache2/sites-enabled/journalist.conf and add DELETE as an allowed request method.

This is similar to bugs #3772 and #3877. In light of this we should:

  1. Revisit Review and annotate Apache configurations #1775 and,
  2. Incorporate some API endpoint testing which would have detected all three bugs into at least the pre-release QA process (these could eventually be in CI as part of the external test client described in [functional testing] Support tor-browser-selenium tests against app-staging in CI #3661)
@redshiftzero redshiftzero added this to the Long Term Product Backlog milestone Dec 6, 2018
@eloquence eloquence removed this from the Long Term Product Backlog milestone Dec 7, 2018
@redshiftzero
Copy link
Contributor Author

We discussed this bug today in the engineering meeting and decided that the way to resolve is to modify the journalist apache config, and request admins that want to use the journalist API to run securedrop-admin install. Otherwise we'll need to do another brittle apache config migration in the postinst of securedrop-app-code and given that the changes here are more extensive than other migrations we've made, we should avoid it if possible.

This does mean that we'll need to be extremely careful when making any other Apache config changes in the future (until we have a better story regarding minimizing config drift across instances - insert obligatory ansible pull reference #3136 which is looking like a pretty good way forward in the medium term imho).

@redshiftzero
Copy link
Contributor Author

Also, when we resolve this via updating the journalist apache config template, we should also resolve #3877 at that time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants