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

[proposal] make req actually emit warnings per documented behavior #191

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thesaxonedone
Copy link

@thesaxonedone thesaxonedone commented Jun 17, 2019

The comments in multipart.js (where I have made my edits) indicate that warnings get emitted on the req object so that controllers can handle them. This doesn't actually happen, so I changed the code to do so.

Moreover, I think this behavior should happen in a production environment so that it can be handled if necessary, so I made that change as well. We can make the logging restricted to development, but the events should be emitted.

This allows the issue specified in balderdashy/sails#4748 to be handled in the controller, as the event that otherwise would just log a message saying

 An upstream (STREAM_NAME) timed out before it was plugged into a receiver. It was still unused after waiting 2000ms. You can configure this timeout by changing the `maxTimeToBuffer` option.

will now fire on the req object so that it can be appropriately handled as opposed to being swallowed by the multipart.js code as it currently stands

While you may read balderdashy/sails#4748 for more details, the thrust of the issue is that if a multipart-form is submitted to a route with extraneous and unexpected parameters, you will get the above log message, but the form submission will still upload in its entirety. As an example, if your controller expects a multipart-form with parameter1 and parameter2, and the client instead submits the form with parameter1, parameter2, and an extraneous parameter3, and moreover parameter3 is 4GB in size, the controller will sit there receiving the traffic (and blackhole it, thankfully), even though theoretically the controller could respond with a 400 or kill the connection once it sees that parameter3 has not been hooked up to an upstream, since at that point we know the request is malformed. Making the 'warning' event fire on the req object (which is what this PR does) allows us to do this.

This allows some measure of defense against network clogging DOS attacks. Currently the only way to address this is with passing

{limit: 'XXmb'}

as an option to the skipper constructor, which basically provides a global limit to a request size, but this affects the URLEncodedBodyParser and JSONBodyParser as well, which is not desired.

@sailsbot
Copy link
Collaborator

Hi @thesaxonedone! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

@sailsbot
Copy link
Collaborator

Hi @thesaxonedone! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

3 similar comments
@sailsbot
Copy link
Collaborator

Hi @thesaxonedone! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

@sailsbot
Copy link
Collaborator

Hi @thesaxonedone! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

@sailsbot
Copy link
Collaborator

Hi @thesaxonedone! It looks like the title of your pull request doesn’t quite match our guidelines yet. Would you please edit your pull request's title so that it begins with [proposal], [patch], [fixes #<issue number>], [implements #<other PR number>], or [misc]? Once you've edited it, I'll take another look!

@thesaxonedone thesaxonedone changed the title make req actually emit warnings per documented behavior [proposal] make req actually emit warnings per documented behavior Jun 17, 2019
@sailsbot
Copy link
Collaborator

Thanks for submitting this pull request, @thesaxonedone! We'll look at it ASAP.

In the mean time, here are some ways you can help speed things along:

  • discuss this pull request with other contributors and get their feedback. (Reactions and comments can help us make better decisions, anticipate compatibility problems, and prevent bugs.)
  • ask another JavaScript developer to review the files changed in this pull request. (Peer reviews definitely don't guarantee perfection, but they help catch mistakes and enourage collaborative thinking. Code reviews are so useful that some open source projects require a minimum number of reviews before even considering a merge!)
  • if appropriate, ask your business to sponsor your pull request. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
  • make sure you've answered the "why?" (Before we can review and merge a pull request, we feel it is important to fully understand the use case: the human reason these changes are important for you, your team, or your organization.)

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

@thesaxonedone
Copy link
Author

any update on this?

@thesaxonedone
Copy link
Author

Is this repository even maintained?

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

Successfully merging this pull request may close these issues.

2 participants