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

Add backend file size limit #2658

Merged
merged 6 commits into from Sep 12, 2019

Conversation

@mkrump
Copy link
Contributor

commented Sep 6, 2019

Description

This PR adds back a backend file size check / limit for uploads. User uploads are limited to 25MB, while generated files are limited to 100MB.

In #2546 the backend file size check was removed. The 25MB limit was intended to apply to individual document uploads, however it was blocking office users from generating pdfs of the combined documents when the pdf size exceeded 25MB. But removing this check meant that the file size limit was only being enforced by the frontend and that generated files could be of unlimited size.

Reviewer Notes

The frontend will block any uploads greater than 25MB, so to manually test the backend limit you can adjust the generated file size limit on line 44 in personally_procured_move_attachments.go and then on the office side try to generate a pdf Create payment paperwork -> Download All Attachments, or alternatively you can upload 5 or so close to 25MB files (enough so that the combined pdf exceeds 100MB), then generate the pdf.

The response should be an error indicating that the file is too large (status code 413) and the server logs should contain a similar message.

Setup

make server_test

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@mkrump mkrump changed the title Add filesize limit to parameter to uploader Add backend file size limit Sep 6, 2019

@mkrump mkrump added the roci label Sep 6, 2019

@mkrump mkrump marked this pull request as ready for review Sep 9, 2019

@codecov

This comment has been minimized.

Copy link

commented Sep 9, 2019

Codecov Report

Merging #2658 into master will decrease coverage by 0.2%.
The diff coverage is 50%.

@@           Coverage Diff            @@
##           master   #2658     +/-   ##
========================================
- Coverage    56.3%   56.2%   -0.1%     
========================================
  Files         271     272      +1     
  Lines       12393   12472     +79     
========================================
+ Hits         6971    7002     +31     
- Misses       4725    4771     +46     
- Partials      697     699      +2
Impacted Files Coverage Δ
pkg/services/invoice/store_invoice.go 0% <0%> (ø) ⬆️
...nternalapi/personally_procured_move_attachments.go 39.2% <11.2%> (-7%) ⬇️
pkg/handlers/internalapi/uploads.go 62.2% <46.7%> (-5.8%) ⬇️
pkg/uploader/uploader.go 58.2% <86.7%> (+9.6%) ⬆️
pkg/services/accesscode/fetch_access_code.go
pkg/services/accesscode/claim_access_code.go 100% <0%> (ø)
pkg/notifications/move_canceled.go 0% <0%> (ø)
pkg/uploader/allowed_file_types.go 100% <0%> (+22.3%) ⬆️
pkg/services/accesscode/validate_access_code.go 100% <0%> (+25%) ⬆️
@@ -41,7 +41,7 @@ func (h CreatePersonallyProcuredMoveAttachmentsHandler) Handle(params ppmop.Crea
}

// Init our tools
loader := uploader.NewUploader(h.DB(), logger, h.FileStorer())
loader := uploader.NewUploader(h.DB(), logger, h.FileStorer(), 100*uploader.MB)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Sep 9, 2019

Contributor

Can we make the limit something we pass in with a command line flag? Then we'd be able to modify with environment variables on the fly without having to do a code push.

This comment has been minimized.

Copy link
@mkrump

mkrump Sep 9, 2019

Author Contributor

I don't think that we really parameterize any of the other handlers that way. I don't necessarily object to that, but seems like probably want to restructure the handler s.t. can inject the uploader.

How ofter are we really going to dynamically change the upload limit? Seems rare enough that a code change would be ok.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Sep 9, 2019

Contributor

My honest opinion is that if its a parameter then it should always be set by a command line flag or env var. I could see us wanting to restrict upload size in an emergency and its an easy lever to pull if its already set up for it.

At the very least I think we ought to set the two values as constants somewhere so we only have one place to change the code.

And it does feel like we ought to be building the uploader first and injecting it into the handler OR have a method on the hander that creates the uploader based on parameters we've passed into the handler struct.

This comment has been minimized.

Copy link
@mkrump

mkrump Sep 9, 2019

Author Contributor

At the very least I think we ought to set the two values as constants somewhere so we only have one place to change the code.

I think that it's actually better that each collaborator defines the file limit rather than relying on some constants specified globally. Im my mind, if I want to look for the max file size that an endpoint takes it makes sense that i'd find that in the endpoint or somewhere in the neighborhood of it.

Also, these uploaders do things like limit the file types allowed, and it seems like it would get unwieldy if we start having command line parameters, for each endpoint using an uploader and all of the parameters.

This comment has been minimized.

Copy link
@mkrump

mkrump Sep 9, 2019

Author Contributor

And it does feel like we ought to be building the uploader first and injecting it into the handler OR have a method on the hander that creates the uploader based on parameters we've passed into the handler struct.

It wouldn't take much to do this, but the other dependencies aren't being injected (a paperwork.NewGenerator is instantiated inside the handler for example), so was just trying to keep things consistent.

This comment has been minimized.

Copy link
@mkrump

mkrump Sep 10, 2019

Author Contributor

so i added a limit in the constructor in 40fb9c1 and fa5e754. Is that better or worse?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Sep 10, 2019

Contributor

I guess that we might be talking past each other. Yes we can change the limit via a deploy but I also want the acceptance tests to fail if we use values above a limit (ie binary shouldn't even start, service should never stabilize in ECS). If we don't have a test that fails in the acceptance tests then we won't be able to have a startup failure either. The pkg/cli package is the current pattern for doing this.

The other issue here is that this dovetails into the anti-virus work I'm doing. So if the binary can start up with the ability to upload files I can't scan with anti-virus then that's a security hole in our system. So if we merge this as is its ok but its also not complete. I'll end up having to come back and do it myself anyway and I'd rather just do it in this PR.

This comment has been minimized.

Copy link
@mkrump

mkrump Sep 10, 2019

Author Contributor

can we all get together tomorrow to figure out what we want to do here? I'm inclined to merge as is after adding the filesize limits to the constructor, but @chrisgilmerproj and @jim want to make sure that we are all on the same page, before doing so (or get us there somehow 😄 ).

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Sep 10, 2019

Contributor

You can definitely merge as it meets the criteria of "limit upload size" but it doesn't really answer the criteria of "prevent app from starting up if upload size above X". I think the second one is more important than the first but I understand if the story is only trying to do the first.

I'm happy to get together and figure out a path forward on this.

This comment has been minimized.

Copy link
@mkrump

mkrump Sep 11, 2019

Author Contributor

cool. i think that i'm going to merge this one in then (if you can approve or dismiss your request for changes).

@chrisgilmerproj
Copy link
Contributor

left a comment

I would love to use command line flags for this limit. Also, it looks like we have a 25mb and a 100mb limit. Is there a difference? If so can we have two flags?

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

I would love to use command line flags for this limit. Also, it looks like we have a 25mb and a 100mb limit. Is there a difference? If so can we have two flags?

The requirement was that service members, office users, etc be limited to 25MB (to be in sync w/ what is already enforced on the frontend), but the generated documents would have a wider window, since they are going to be the combination of multiple up to 25MB documents.

@jim
jim approved these changes Sep 10, 2019
Copy link
Contributor

left a comment

🚛

MB = 1000 * 1000
)

func (b ByteSize) AsInt64() int64 {

This comment has been minimized.

Copy link
@jim

jim Sep 10, 2019

Contributor

(nit) I think it would be more idiomatic to call this method Int64.

This comment has been minimized.

Copy link
@mkrump

mkrump Sep 10, 2019

Author Contributor

nits are fine. updated af3d5f0

@@ -50,14 +52,17 @@ type Uploader struct {
}

// NewUploader creates and returns a new uploader
func NewUploader(db *pop.Connection, logger Logger, storer storage.FileStorer, fileSizeLimit ByteSize) *Uploader {
func NewUploader(db *pop.Connection, logger Logger, storer storage.FileStorer, fileSizeLimit ByteSize) (*Uploader, error) {
if fileSizeLimit > 350*MB {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Sep 10, 2019

Contributor

It would be pretty easy to just call cli.GetInt("FileSizeLimit") here and get the value from the CLI. It's not perfect but would honestly do the trick. Then we can have an acceptance test that validates that we're not over that limit so that the binary won't start up.

@chrisgilmerproj
Copy link
Contributor

left a comment

Approving as we're opening a new story to handle adding acceptance tests for the upload limit(s).

@mkrump mkrump merged commit 8ac25df into master Sep 12, 2019

15 of 16 checks passed

codecov/patch/go 50% of diff hit (target 56%)
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/project/go 55.9% (-0.1%) compared to 8cd66b0
Details

@mkrump mkrump deleted the mk-167951106-backend-filesize-limits branch Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.