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 `maxUploadFileSize` #256

Merged
merged 16 commits into from Sep 16, 2019

Conversation

@zone117x
Copy link
Member

commented Aug 6, 2019

Progress towards Production ready node.js HTTP server

Completes issues:

  • #161 GET /hub_info should advertise the hub's maximum file size
  • #187 Edit gaia hub drivers to check content-size on gaia hub writes

When a content-length header is provided (almost always the case) then it is checked against the hub's max configured size. If the size is too large, the http request is immediately rejected with a specific error for the client (blockstack.js) to be able to handle appropriately.

Additionally, while the a request payload is being streamed, the size is monitored and if the content-length is exceeded, the stream is immediately destroyed.

If a content-length header is not provided, then the max hub size limit is used instead. This is to allow for clients that do not necessarily know the content-length in advance. The fetch w3c spec specifies that a ReadableStream (which has an unknown length) should be allowed as the POST body payload. This is a feature we may want to use in the future for streaming encryption and network request.

Manual Testing

TODO: Memory load testing during large file writes to ensure there are no performance regressions.

Automated Testing

Near full test coverage. SuperAgent is a PITA and difficult to spoof malicious client behavior. Some error paths are not covered in the http code reporting layer, but covered in the error handling layer right beneath it.

Submission Checklist

  • Based on correct branch: feature submissions should be on develop, hotfixes should be on master

  • The code passes our eslint definitions, unit tests, and
    contains correct TypeFlow annotations.

  • Submission contains tests that cover any and all new functionality or code changes.

  • Submission documents any new features or endpoints, and describes how developers
    would be expected to interact with them.

  • Author has agreed to our contributor's agreement.

zone117x added 9 commits Aug 5, 2019
…hecks, and upload stream monitoring
…ing POST handlers (/revoke-all and /list-files)
* develop:
  Update changelog entry for `Access-Control-Max-Age` header
  Add test for CORs preflight Access-Control-Max-Age header.
  Set the CORs preflight header to be cached for 24 hours #254
@zone117x zone117x requested review from jcnelson and kantai Aug 6, 2019
@codecov

This comment has been minimized.

Copy link

commented Aug 6, 2019

Codecov Report

Merging #256 into develop will increase coverage by 0.34%.
The diff coverage is 89.79%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #256      +/-   ##
===========================================
+ Coverage    80.68%   81.03%   +0.34%     
===========================================
  Files           21       21              
  Lines         1553     1592      +39     
  Branches       271      278       +7     
===========================================
+ Hits          1253     1290      +37     
- Misses         225      226       +1     
- Partials        75       76       +1
Impacted Files Coverage Δ
hub/src/server/config.ts 88.11% <100%> (+0.11%) ⬆️
hub/src/server/errors.ts 100% <100%> (ø) ⬆️
hub/src/server/http.ts 73.07% <50%> (+0.42%) ⬆️
hub/src/server/utils.ts 92.85% <93.75%> (+0.12%) ⬆️
hub/src/server/server.ts 96% <95.65%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff1cd24...a51b537. Read the comment docs.

hub/src/server/server.ts Outdated Show resolved Hide resolved
@zone117x zone117x changed the title Implement `maxUploadFileSize` [WIP] Implement `maxUploadFileSize` Aug 6, 2019
zone117x added 2 commits Aug 7, 2019
…mory and the GC overhead
@zone117x zone117x changed the title [WIP] Implement `maxUploadFileSize` Implement `maxUploadFileSize` Aug 7, 2019
const readURL = await this.driver.performWrite(writeCommand)
// Create a PassThrough stream to monitor streaming chunk sizes.
const { monitoredStream, pipelinePromise } = monitorStreamProgress(stream, totalBytes => {
if (totalBytes > this.maxFileUploadSizeBytes) {

This comment has been minimized.

Copy link
@kantai

kantai Aug 7, 2019

Member

This should check against the Content-Length header -- if the uploader is streaming more than the claimed Content-Length, we should close and error.

This comment has been minimized.

Copy link
@zone117x

zone117x Aug 7, 2019

Author Member

Good catch, fixed.

…configured max size
@kantai
kantai approved these changes Aug 7, 2019
Copy link
Member

left a comment

Looks great to me!

@zone117x zone117x referenced this pull request Aug 7, 2019
@moxiegirl moxiegirl referenced this pull request Aug 7, 2019
@zone117x zone117x added this to the DX Q3 Sprint 1 milestone Aug 9, 2019
@zone117x zone117x self-assigned this Aug 9, 2019
@jeffdomke

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Sounds like next steps are @jcnelson finishes review then @timstackblock will help on the memory load testing.

const isLengthFinite = Number.isFinite(contentLengthBytes)

if (!isLengthFinite) {
const errMsg = `A valid 'Content-Length' header must be passed. Received header "${contentLengthHeader}"`

This comment has been minimized.

Copy link
@jcnelson

jcnelson Aug 23, 2019

Member

I think we should mention somewhere in the README that a Content-Length header is necessary -- there's no support for chunked encoding.

This comment has been minimized.

Copy link
@zone117x

zone117x Sep 9, 2019

Author Member

Per my comment #256 (comment)
I removed the requirement for providing a content-length header. After extensive testing, this is a frustrating header to provide given CORs whitelist contraints. Additionally, it excludes future uploading features that do not necessarily know the content length in advance.

}

export function bytesToMegabytes(bytes: number, decimals = 2) {
return Number.parseFloat((bytes / 1024 / 1024).toFixed(decimals))

This comment has been minimized.

Copy link
@jcnelson

jcnelson Aug 23, 2019

Member

Not a bug, but I'd prefer bytes / (1024 * 1024) -- I think / is always left-binding, but this would play it safe.

This comment has been minimized.

Copy link
@zone117x

zone117x Sep 9, 2019

Author Member

Fixed

Copy link
Member

left a comment

Code looks great! Just a small recommended update to the README, but it can be done outside of this PR.

@zone117x

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

Thanks for reviewing @jcnelson, the comment about chunked-encoding led me down a productive rabbit hole, and found some potential HTTP content-length header issues related to CORs. There are also some implications for potential future file upload streaming methods.

I may remove the requirement for the header -- pending additional research and testing.

@zone117x zone117x self-assigned this Aug 27, 2019
@timstackblock

This comment has been minimized.

Copy link

commented Aug 30, 2019

Confirmed the hub and reader is successfully running. Overall, the PR respects upper file limits but doesn’t respect lower limits.

3 sample files are needed:
file1 is less than 100KB,
file2 is greater than 100KB but less than upperlimit (20MB) default and
file3 is greater than upperlimit.

File1 upload (Successful):
• File upload was successful despite being less than the lower limit of 100KB.

File2 Upload(Successful):
• File upload was successful given it was within the range of acceptable file size limit.

File3 Upload(Unsuccessful):
• File size exceeds upper size limit.
• Response: "message":"Max file upload size is 20 megabytes. Rejected Content-Length of 109.4165 megabytes

@timstackblock timstackblock self-requested a review Aug 30, 2019
zone117x added 3 commits Sep 9, 2019
* develop:
  Add the native build packages
  Improve docker image size via npm pruning and selective file copying
  deleting docker folder
…vided overwise check against configured max defautl.
@zone117x

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

I removed the requirement for providing a content-length header. The behavior of this header cannot always be controlled. It is a forbidden header -- regular js web clients cannot set the header programmatically. The value sent by the browser is influenced by negotiated content encoding and compression. Additionally, it excludes future uploading features that do not necessarily know the content length in advance.

If the header is provided by a normal-behaving and non-streaming client, then it results in nice errors immediately provided to the client. If the header is not provided, then the error is provided to the client after the hub-configured limit has been exceeded.

However, the possibility of the header not being provided is a situation in which I think we should reasonably expect and allow for.

@jcnelson @kantai can you re-review? Thanks.

@zone117x zone117x requested a review from kantai Sep 10, 2019
* develop:
  Simplify `decodeTokenForPayload` #263 (comment)
  Update README with info related to replicated hub instances
  Remove TODO comment about legcay auth token capabilities
@kantai
kantai approved these changes Sep 16, 2019
Copy link
Member

left a comment

Looks great to me, @zone117x !

@yknl yknl modified the milestones: DX Q3 Sprint 2, DX Q3 Sprint 3 Sep 16, 2019
@zone117x zone117x merged commit ef07718 into develop Sep 16, 2019
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 81.03% (+0.34%) compared to ff1cd24
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.