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

only save basename of filename for a reply on API requests #3919

Merged
merged 2 commits into from Nov 2, 2018

Conversation

Projects
None yet
2 participants
@heartsucker
Copy link
Contributor

heartsucker commented Nov 2, 2018

Status

Ready for review

Description of Changes

Fixes #3918

Replies created via the API are currently being saved in the DB with a filename as /var/lib/securedrop/data/$filesystem_id/$filename. This fixes that.

Testing

  • make dev
  • Send reply via API
  • In the container sudo sqlite3 /var/lib/securedrop/db.sqlite 'select filename from replies'
  • See that /var/lib/... is not in any of the returned rows

Deployment

We may need a db migration to ensure that instances that had rows inserted in this manner are cleared otherwise they may broken in way that requires admin intervention. This could be done with a one-way alembic migration. The reviewer should consider this case before approving this PR.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

@heartsucker heartsucker requested a review from redshiftzero as a code owner Nov 2, 2018

@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Nov 2, 2018

This should go in a point release if we make another release in the 0.10.x series (else it will go to instances in 0.11.0 release at the latest). In my opinion, we should only perform a migration if we get reports of people hitting this issue. This is such that we avoid unnecessary migrations.

heartsucker added some commits Nov 2, 2018

@redshiftzero redshiftzero force-pushed the reply-basename branch from 64a3505 to a812b22 Nov 2, 2018

@redshiftzero
Copy link
Member

redshiftzero left a comment

good catch, 👍

@redshiftzero redshiftzero merged commit 71aa8d7 into develop Nov 2, 2018

5 checks passed

ci/circleci: admin-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: staging-test-with-rebase Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details
ci/circleci: updater-gui-tests Your tests passed on CircleCI!
Details

@redshiftzero redshiftzero deleted the reply-basename branch Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment