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

break: secure file upload and download (public and private) #1084

Merged
merged 1 commit into from
Nov 9, 2022

Conversation

gunar
Copy link
Contributor

@gunar gunar commented Aug 11, 2022

Card: https://trello.com/c/cS7bfBXL/2019-restrict-access-to-user-uploaded-files-to-bops-uniform-only

  • prevents user uploads from being publicly accesible
  • makes all S3 routes go through the API
  • adds a new API Token concept to only allow BoPS to download user files
  • side-effect: prevents users from downloading their own files

@gunar gunar self-assigned this Aug 11, 2022
@gunar gunar force-pushed the g/private-files branch 2 times, most recently from 742c53c to a3eda03 Compare August 17, 2022 20:20
@github-actions
Copy link

github-actions bot commented Aug 20, 2022

Removed vultr server and associated DNS entries

@gunar gunar force-pushed the g/private-files branch 2 times, most recently from 85acf77 to 486a265 Compare August 25, 2022 14:31
@gunar gunar requested a review from a team August 25, 2022 14:40
@gunar gunar force-pushed the g/private-files branch 2 times, most recently from 469d10c to e81a8c1 Compare August 25, 2022 16:38
@gunar
Copy link
Contributor Author

gunar commented Aug 25, 2022

This is now ready for review. Sorry for being trigger-happy with the review request earlier (ie asking review before tests were passing).

@gunar
Copy link
Contributor Author

gunar commented Sep 29, 2022

Hey @jessicamcinchak, quick question. This PR requires adding a new env var. I've added it to s3://pizza-secrets/api.env.test which makes the CI tests pass. Now as for the Pizza, what would be the right place to put this API_FILE_KEY environment variable? Here maybe?

ROOT_DOMAIN=${{ env.FULL_DOMAIN }}

@jessicamcinchak
Copy link
Member

jessicamcinchak commented Sep 30, 2022

hey @gunar - without fully reviewing this PR yet, I'd suggest adding the new variable to the root .env file as well as the API's env.test and then it'll be available in the pizza's docker container and during local development.

any variables defined like L347 X=${{ env.X }} will be printed by Actions fyi; Github only masks values like X=${{ secrets.X }} (we don't care about this for the domain here as it's not sensitive, but we would for the new file key obviously!). I'm generally aiming to decrease how many variables we need to keep in Github Secrets and use remote root.env file as source of truth for all environments using docker 🙂

lemme know if you have any issues with this approach though!

@gunar
Copy link
Contributor Author

gunar commented Oct 3, 2022

That makes sense, and it worked! Thanks!

@gunar gunar requested review from a team and removed request for a team October 3, 2022 21:40
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for all your work & patience on this one, apologies it's been a particularly slow review process as this one just touches so much & requires more cross-project communication than usual!

i'm happy with the code & went through a full test LDC service: file upload components, review page, and CSV all look good - but both submissions to Uniform & BOPS failed (you can see logs here). Once file conflicts are resolved here, I'm happy to try again - I think once we have a successful staging submission to both BOPS & Uniorm and can confirm that they are able to pick up the files as expected then this could merge 👍

api.planx.uk/server.js Outdated Show resolved Hide resolved
editor.planx.uk/src/components/ImagePreview.tsx Outdated Show resolved Hide resolved
infrastructure/application/index.ts Show resolved Hide resolved
@gunar
Copy link
Contributor Author

gunar commented Oct 12, 2022

Next steps:

  1. Rebase
  2. Submit application from Pizza to BOPS Staging
  3. Check that it was submitted correctly to Uniform
  4. Give BOPS token, ask them to add it to the their request's headers, confirm it worked
  5. Merge 🎉

@gunar
Copy link
Contributor Author

gunar commented Oct 13, 2022

Thank you for the review @jessicamcinchak—all good points. I believe I've addressed them. I've added the thing about how to rotate randomPassword as an in-place comment—given that we're only using it in that single place for now.

@gunar
Copy link
Contributor Author

gunar commented Oct 13, 2022

Pizza isn't building. I'll investigate.

- prevents user uploads from being publicly accesible
- makes all S3 routes go through the API
- adds a new API Token concept to only allow BoPS to download user files
- side-effect: prevents users from downloading their own files
@gunar
Copy link
Contributor Author

gunar commented Oct 28, 2022

It's Friday I'm in… a ready-for-review state 😂 🎵

@@ -0,0 +1,27 @@
import S3 from "aws-sdk/clients/s3";
Copy link
Contributor Author

@gunar gunar Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that I had to change this otherwise tsc would try to compile the whole sdk and the GH Action worker would run out of memory.

- import { S3 } from "aws-sdk";
+ import S3 from "aws-sdk/clients/s3";

In a separate PR I'll suggest increasing the maximum amount of memory available for the worker (just in case).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting about the "worth noting" above that the Docker build would fail silently (ie the error message was there but it would return a Linux zero code) which made it hard for me to find the root cause at first.

@gunar gunar requested a review from a team October 28, 2022 17:25
@gunar
Copy link
Contributor Author

gunar commented Oct 28, 2022

Actually, we already had a plan for this PR didn't we? Sorry I forgot.

@jessicamcinchak on Tuesday when you're back do you mind submitting an application from this Pizza into BOPS Staging? Happy to learn how to do it myself too but at this point I just want this PR to be over with 😅

@gunar
Copy link
Contributor Author

gunar commented Nov 2, 2022

@gunar gunar merged commit 03bf62d into main Nov 9, 2022
@gunar gunar deleted the g/private-files branch November 9, 2022 18:23
@gunar gunar restored the g/private-files branch November 15, 2022 18:02
@gunar gunar deleted the g/private-files branch November 15, 2022 18:02
gunar added a commit that referenced this pull request Nov 15, 2022
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.

2 participants