Sails: SSL vs. Non SSL

[corbanb]

I am working on a sails 0.11 app and just ran into an issue with multi-from data. When the form is posted via non-SSL everything works fine. When its posted via SSL the post data shows up missing.

I should also note that IF an image is not attached the same POST works great either way on SSL and non-SSL.

Anyone have thoughts on how this could even be possible?

node: 0.10.33
sails: 0.11.0
skipper: 0.5.5

All running on Heroku and their SSL. I have narrowed it down to that but having a hard time validating locally with an SSL cert to be able to step into it.

[corbanb]

Doing further investigation now is showing req.body is empty when the image is attached to the multi-form post. No req.param's either.

[corbanb]

For demo purposes I created a demo app here:

https://github.com/corbanb/sails-demo-app/blob/master/api/controllers/FormController.js

http://sails-demo-app.herokuapp.com/form/index

• req.body & req.file('image') are parsed correctly

https://sails-demo-app.herokuapp.com/form/index

• req.body is empty
• req.file('image') is parsed correctly

Is this to be expected? Does skipper only work on non-SSL?

[corbanb]

@zolmeister @sgress454 - either of you seen this?

Saw you both might have context based on this.

[corbanb]

I am keeping notes here in hopes this can be cleared up. I spent more time today adding json reponses to the form submit here, this should make it easier to see what's coming back and what's not.

Also created a gist of the app's log output from skipper doing:

• http+text+file - HTTP with both works great
• https+text+file - HTTPS with both misses text params
• https+text - HTTPS text only works
• https+file - HTTPS file only works

Working more to try and find the issue but nothing is sticking out yet as to why https is causing this issues when mixing content.

[corbanb]

I have now tried to replace the bodyParser as the docs say in 3 ways with 0 luck.

the 3 options are commented out for now.
https://github.com/corbanb/sails-demo-app/blob/master/config/http.js#L72

[jurajpetrik]

I have this same issue! When uploading a file and putting

            <form action="https://myapp.herokuapp.com/imageUpload" method="post" enctype="multipart/form-data">
                <input type="hidden" name="userId" value="32"> 
                <input type="file" name="image">
            </form>

           req.param('userId');//undefined

I spent a lot of time trying to find out what the problem is because it works on my local machine (http) and fails on heroku(https)

The way I get around it now is add the text parameter as a part of the url query. So I rewrote the above form as

            <form action="https://myapp.herokuapp.com/imageUpload?userId=32" method="post" enctype="multipart/form-data">
                <input type="file" name="image">
            </form>

[sgress454]

@corbanb @jurajpetrik Skipper is certainly intended to work with both encrypted and unencrypted connections. I know these can be painful issues to debug, so thanks for keeping up with it. One guess would be that for SSL connections, the text parameters are being re-ordered so that they appear at the end of the stream instead of the beginning. Skipper only reads text parameters until it finds the first file in the stream; this is based on a big assumption about field order which has some evidence behind it, but if it doesn't hold for SSL connections, we'll have to revisit it.

[corbanb]

@sgress454 I spent hours looking at this and putting the params in both orders. It doesn't hold up either way sadly. I'm not sure its currently doable to POST mutliforms over SSL at all with skipper when files and text is present. If you pull the demo app I built and push to heroku or any app with SSL its easy to text the options. I have turned on DEBUG=skipper in my app.

If anyone wants full access to the heroku app or the repo let me know.

[corbanb]

Spent more time today working on some options here. I am now getting 'Unable to expose body parameter location in streaming upload!'. To me that says its stripping it due to a setting or how I am uploading files. Is this something I can change?

@sgress454 or @mikermcneil any ideas?

[sgress454]

Based on the full text of the warning, it really does appear that things are being re-ordered somehow. Maybe it's part of the browser implementation of SSL. Skipper is seeing a text param after it's already passed control back to userland code, which would only happen if the text param was seen _after_ a file was already processed. I would try capturing and logging the request at the bodyparser level to try and see _exactly_ what's being received.

[corbanb]

@sgress454 you are right its something to do with browsers. I am getting everything fine in safari 8. but chrome 41 fails on the ordering. Not sure how to fix this now.

[plato-cambrian]

I'm also seeing an empty req.body with post request from Chrome 43. My form parses fine in firefox 38.

[plato-cambrian]

@jurajpetrik thanks, i had success with your workaround of only posting file fields from the form, and sending the other data in the query string

[corbanb]

@plato-cambrian @jurajpetrik I wish that worked for me. sadly it will not due to the sensitive nature of our data on the post. We actually have moved from sails for the time being on the app and having success with the multiparty module combined with knox.

[albertkim]

I experience this issue with Heroku. I am uploading an image from an IOS client via AFNetworking, and req.body shows up empty. Strangely, it seems to work locally on my Mac.

[tjwebb]

The way I get around it now is add the text parameter as a part of the url query. So I rewrote the above form as

How does this fix the issue, exactly?

[sgress454]

How does this fix the issue, exactly?

The issue is that text params that should be in req.body are getting stripped because they're being re-ordered in the request, and skipper doesn't look at text params that come _after_ file parts. By putting your text params in the URL, they bypass req.body entirely, and they should still show up in req.params.

[tjwebb]

Can anyone offer some insight on a generic solution to this? It sounds like a fragile situation that I think we'd like to harden a bit.

[russellsantos]

From what I understand, this is a consequence of two things:

1) Skipper assumes that post parameters are given in the request stream
before the file data. This is done by skipper so that it can actually
process the files as you are receiving the response, rather than copying it
first to disk like the other file upload middleware.
2) Chrome does not ensure that the post parameters sent in the request
stream before the file data when the data is sent via HTTPS.

We can solve this two ways:

1) Ask chrome to include the post parameters before the file data
(unlikely, as I don't think this is in the HTTP/S spec).
2) Add an option to skipper to buffer the file data on to disk and
completely read the request stream. This will allow us to read the post
parameters. Ideally this configuration option should not be a global
option, rather be something we can assign to each controller route. This
has the consequence of negating what I consider to be a pretty cool feature
of skipper.

Russell Santos
Software Consultant
Java/J2EE, Web, Mobile (iOS & Android)

On Fri, Jul 3, 2015 at 4:55 AM, Travis Webb [email protected]
wrote:

Can anyone offer some insight on a generic solution to this? It sounds
like a fragile situation that I think we'd like to harden a bit.

—
Reply to this email directly or view it on GitHub
https://github.com/balderdashy/skipper/issues/71#issuecomment-118165690.

[jetsonjohn]

I have the same issue for inputs declared after file input type. The text input type values is empty in the request

[jetsonjohn]

Is there any work around for this. I got several file type input in a form and it not in a particular order. It will be nice if we have a solution for this issue without the help of jQuery upload.

[sailsbot]

Thanks for posting, @corbanb. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

• review our contribution guide to make sure this submission meets our criteria (only _verified bugs_ with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
• create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

[corbanb]

@sailsbot what? Its been closed cause no one was active on it? I think this means someone needs to fix it. Not close it. :frowning:

[Bartjezzz]

Yup. I'm calling in as well. Same issue here!

• [email protected]
• [email protected]
• Form data includes 4 params + jpeg.
• Posts via iOS AFNetworking 2.0 / NSURLConnection tested

Both work on local machine in http, but switching to SSL on Heroku shows:

2015-09-25T14:21:56.864173+00:00 app[web.2]: debug  : sessionID: undefined
2015-09-25T14:21:56.864351+00:00 app[web.2]: debug  :  feedback: undefined
2015-09-25T14:21:56.864391+00:00 app[web.2]: debug  :  metaData: undefined

Funny enough. Restarting the sails instance sometimes resolves this issue. Any Sails peeps working on this?

[sgress454]

@Bartjezzz I imagine it works sometimes because the field order in the request is non-deterministic; sometimes they come in first, sometimes they don't?

@russellsantos You are correct in that a "buffering-to-disk" option would solve this, but it wouldn't just negate a pretty cool feature of Skipper--it would negate pretty much the reason Skipper was created, which was to avoid the glaring issue in the default body parser that would let a large file upload cripple a server by filling up all the disk space (or memory, depending on where you were buffering to). I'm not saying your idea is a bad one--rather, I'm saying that if we can't fix this issue in Skipper without resorting to buffering, it's probably easier to just not use Skipper in affected apps, and use the Express body parser instead.

It seems awful to have to wait for an entire file to upload just to get access to a couple of measly bytes at the end representing a text field. Hopefully there's something we're all missing.

[Bartjezzz]

So we've completely redone the code. It's hideous but should work.

1. Do a clean POST with all the params
2. Create second POST with only the jpeg as image/jpeg and add 1 or 2 queryParams (not in actual body).

In HTTP it is working. In HTTPS it loses the actual image data. No idea what's going on. Whats the bare minimum to making this work? No query params at all?

req.file('screenshot').upload(function (err, uploadedFiles)
        {
            if (err) return res.send(500, err);
            //return res.send(200, uploadedFiles);
            log.debug("Upload file count: " + uploadedFiles.length);
            log.debug("Upload file name: " + uploadedFiles[0].filename);
        });

[Bartjezzz]

Update on our end. We completely removed Skipper from parsing anything related to attachments for now.

in HTTP.js we overwrite bodyparsing

bodyParser:function(req,res,next)
      {
          if (arguments.length <3)
          {
             return;
          }

          if (req.path === '/path/to/screenshot/upload')
          {
            next();
          }
          else
          {
              skipper(req,res,next);
          }
      },

and in code we're using multiparty to solve the issue

// our own body parser
        var form = new multiparty.Form();

        form.parse(req, function(err, fields, files)
        {
            //do things with files
        });

[suyash515]

@Bartjezzz Correct me if I'm wrong but if we replace the bodyparser in http.js, then this is used only once when the application is lifted and as such all of these conditions are executed only once and not per route.

[Bartjezzz]

@suyash515 Not sure if that is correct. We created a body parser middleware and placed in the http tick. so it should run every request

[suyash515]

@Bartjezzz You are absolutely right! It does work as you mentioned. The first time I tested it was not working - maybe did something wrong. Thanks a lot for this solution.

[sailsbot]

Thanks for posting, @corbanb. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

• review our contribution guide to make sure this submission meets our criteria (only _verified bugs_ with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
• create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

[sgress454]

Just an update on this, I am still unable to replicate this locally. I've tried using a simple Sails app with self-signed certificate, and also tried it through an Nginx proxy (side note--if you want to install Nginx with SSL support on OS X, _use Homebrew_, or risk losing hours of your life). The next step would be to try in Heroku, to replicate @corbanb's findings (hopefully using their Docker container rather than continuously redeploying Heroku)...

[corbanb]

As a reminder I have left comments above on how to test and what the logs look like:

https://github.com/balderdashy/skipper/issues/71#issuecomment-78316205 - sails app (might need to be updated now)

https://github.com/balderdashy/skipper/issues/71#issuecomment-88589779 - log output

[sailsbot]

@corbanb,@jurajpetrik,@sgress454,@plato-cambrian,@albertkim,@tjwebb,@russellsantos,@jetsonjohn,@sailsbot,@Bartjezzz,@suyash515: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

[sgress454]

The saga continues. After renewed interest in maxWaitTimeBeforePassingControlToApp, it seems like this might actually be the culprit, rather than a re-ordering of fields as we've been assuming. For an explanation of why that delay is in there, see this comment. We're currently thinking through alternatives (suffice it to say, it's not as simple as it seems), but in the meantime it seems like just making that delay configurable might solve a lot of these issues. If you run your app with DEBUG=skipper sails lift you'll be able to see if you're getting bit by the timeout.

[sailsbot]

@corbanb,@jurajpetrik,@sgress454,@plato-cambrian,@albertkim,@tjwebb,@russellsantos,@jetsonjohn,@sailsbot,@Bartjezzz,@suyash515: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

[jurajpetrik]

this is very much still an issue, you useless bot.

[sailsbot]

@jurajpetrik 😢 Don't hate me. I didn't ask to be programmed with knives for hands.

[dbeast86]

Am running into the same issue. Any new solutions or updates? For now, will try separate HTTP post requests as mentioned above.

[sgress454]

@corbanb,@jurajpetrik,@plato-cambrian,@albertkim,@tjwebb,@russellsantos,@jetsonjohn,@sailsbot,@Bartjezzz,@suyash515:

Following the path of our earlier discovery, we've made the maxWaitTimeBeforePassingControlToApp delay longer (500ms instead of 50ms) and also made it configurable.

Just to reiterate what @mikermcneil says in his explanation of the timeout, maxWaitTimeBeforePassingControlToApp exists to handle cases where an error is thrown within the multipart body parser, causing it to _never close the form_ so that the server hangs until the Node request timeout (2 minutes, by default!) fires. It will never delay normal requests from processing -- as soon as a file is encountered, or the full request is received, Skipper passes control back to userland code. However, in certain cases the timeout is too short and it fires before either of those conditions are met, causing the issue seen in this thread.

So, hopefully installing Skipper v0.6 will resolve this issue. If anyone has the time to test it out, please post your results here so we can verify the fix!

[corbanb]

If someone wants to fork my demo app it might be pretty easy. I don't have time for this right now.

https://github.com/balderdashy/skipper/issues/71#issuecomment-88631718

[sailsbot]

@corbanb,@jurajpetrik,@sgress454,@plato-cambrian,@albertkim,@tjwebb,@russellsantos,@jetsonjohn,@sailsbot,@Bartjezzz,@suyash515: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

[kevinluvian]

@sgress454 increase maxWaitTimeBeforePassingControlToApp worked, at least for me

[Ggleny]

@kevinluvian where you set maxWaitTimeBeforePassingControlToApp ?

[jblazevicClover]

i just uncomment "bodyParser: require('skipper')" in config/http.js and do in terminal npm install skipper.

[kayoderock]

Please, I am having this same issue with sails 1.0.2 - Is there any solution to the issue now? Thanks.

[corbanb]

@sailsbot should of never closed this. @balderdashy should close source this project until its fixed. pretty shitty of them.

[Ren14]

Hola ! Tengo el mismo problema actualmente. Podrían abrir nuevamente el problema ?