MultipartParser does not like http://www.uploadify.com/

[bmoelk]

uploadify is a swf object that doesn't put the last CRLF chars after the LAST_BOUNDARY. This causes problems for the MultipartParser implementation because it won't end the stream correctly.

Here's the fix I hacked in:

      } else if (flags & F.LAST_BOUNDARY) {
        if (c == HYPHEN) {
          index++;
          if ((i+1) == len) {
            callback('partEnd');
            callback('end');
            state = S.END;
            break;
          };
        } else {
          index = 0;
        }

So, this checks if the buffer is exhausted and you've read in the last two HYPENs of the LAST_BOUNDARY. If that's the case, then ends the stream. Seems to work ok for uploadify.

[gf3]

Uploadify is a nasty mess of code. Yuck.

[felixge]

Hey bmoelk. Your fix does not work, because it assumes that you can do a lookahead on a data stream which you can't. Of course the odds will make your solution appear right most of the time here, but it's not a proper fix.

That being said, you should really report a bug with uploadify, they are violating the spec.

If you don't want to go through that hassle, I would probably accept a patch for this that doesn't rely on lookahead.

[bmoelk]

The only assumption I see is if there are additional buffers and the data after the LAST_BOUNDARY marker begins with two hyphens, this code will end the stream prematurely. It's not very likely, but I agree it is possible. Is that the assumption you're talking about?

I believe an option that would work is to pass through bytesReceived/bytesExpected to the multipart form parser and then check to see if the total bytes add up in addition to the check I've patched in above (double hyphens after the LAST_BOUNDARY).

I don't have a problem telling uploadify they are violating the spec, but I'm sure they aren't the only ones who will be. The other multi-part form parsers that we've used, do not choke on this so IMO it's not favorable to formidable to take a "we follow the specs" hardline. We just want stuff to work! :)

[felixge]

I'm definitely willing to support broken clients, it's the real world. This is just the first broken client that was reported and I'm short on time, that's why I'm not fixing it myself right away. If it was a browser, I'd fix this instantly.

I think the proper solution is to add another state called: QUIRKY_END

Then in the parsers end() function check if the state is QUIRKY_END || END.

Let me know if you can provide a patch like that, otherwise I'll do it myself at some point this or next week.

[bmoelk]

Ok, no worries or rush, my patch is "good enough" for me right now. I'm quite short on time myself, so it's not likely that I'll have time to work through a proper patch before the end of this month.

I appreciate your attention and responsiveness. Thanks!

[bmoelk]

I was about to post a bug on the uploadify forum, so I did some digging in regards to the spec (I think this is the right one):

http://www.ietf.org/rfc/rfc2388.txt

which references this spec:

http://www.ietf.org/rfc/rfc2046.txt

The relevant bits I think are on page 21:

 multipart-body := [preamble CRLF]
                   dash-boundary transport-padding CRLF
                   body-part *encapsulation
                   close-delimiter transport-padding
                   [CRLF epilogue]

In this case, it appears the CRLF end is an optional element? Am I reading the spec correctly?

[ncr]

Just got bitten by this, this time from YUI Flash uploader. Trying to wrap my head around the parser code. Should I add the before-mentioned QUIRKY_END state given the fact that CRLF epilogue is optional? Any other guidance?

[felixge]

Yes, I think bmoelk is right about the spec. I just didn't have time to work on this yet.

If you want to look into it just make sure that all existing tests continue passing, and the new behavior of making the CRLF optional has a test as well.

[ncr]

I started with the failing test: ncr@7d5d8c9 If this path is worng, stop me before I put more time into this ;)

Update: the link is dead, I've recreated the fork.

[felixge]

Looks good! Unfortunately I don't have individual tests for the state transitions of the parser (like I do for node-mysql), so this is the best way to start this : ).

Also see my comment on that commit.

[ncr]

I've recreated my fork and just removed the "\r\n" from the existing test case as you suggested in the comments (now gone) under that "failing test" commit. I think I've managed to make the parser ignore everything after last boundary's two hyphens.

Here's the change: ncr@dd668b3

Should I create a separate pull request? Or more importantly - is this change any good? :)

[felixge]

Hi, the patch looks great! I applied it and pushed it out as part of v0.9.10:

v0.9.9...v0.9.10

Thank you so much for your help!