Add CRLF to encoded multipart form data #24670

glowe · 2019-11-06T02:14:10Z

Some (3) WPT tests were failing because they expected the body for a multipart form data response to end with a CRLF. So I updated encode_multipart_form_data to add the missing terminator.

Looking at the corresponding spec (https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart%2Fform-data-encoding-algorithm) and RFC (https://tools.ietf.org/html/rfc7578), I couldn't find anything mentioned about this detail.

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes

highfive · 2019-11-06T02:14:18Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

highfive · 2019-11-06T02:14:19Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlformelement.rs
@KiChjang: components/script/dom/htmlformelement.rs

jdm · 2019-11-12T19:35:45Z

Review ping @Manishearth.

Manishearth · 2019-11-12T19:40:48Z

Can we file a bug on the HTML spec about this? I'm wary of landing unspecced behavior without having a bug on file.

Manishearth · 2019-11-12T19:41:00Z

r=me once we have such a bug

Manishearth · 2019-11-26T06:35:53Z

@bors-servo r+

I'll file a bug once this lands

bors-servo · 2019-11-26T06:35:55Z

📌 Commit c1f473a has been approved by Manishearth

bors-servo · 2019-11-26T06:37:18Z

⌛ Testing commit c1f473a with merge 54b042b...


        Auto merge of #24670 - glowe:add-crlf-to-boundary, r=Manishearth

Add CRLF to encoded multipart form data  Some (3) WPT tests were failing because they expected the body for a multipart form data response to end with a CRLF. So I updated encode_multipart_form_data to add the missing terminator. Looking at the corresponding spec (https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart%2Fform-data-encoding-algorithm) and RFC (https://tools.ietf.org/html/rfc7578), I couldn't find anything mentioned about this detail. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes

bors-servo · 2019-11-26T07:22:04Z

💔 Test failed - status-taskcluster

CYBAI · 2019-11-26T07:47:34Z

  ▶ Unexpected subtest result in /_mozilla/mozilla/FileAPI/file-upload.html:
  │ FAIL [expected PASS] form submission of uploaded file
  │   → assert_equals: expected "OK" but got "FAIL: request body doesn't match: -----------------------------580133049509808152\nContent-Disposition: form-data; name=\"file-input\"; filename=\"upload.txt\"\ncontent-type: text/plain\n\nHello\n-----------------------------580133049509808152--+++++++-----------------------------580133049509808152\nContent-Disposition: form-data; name=\"file-input\"; filename=\"upload.txt\"\ncontent-type: text/plain\n\nHello\n-----------------------------580133049509808152--\n"
  │ 
  │ run_test/testframe.onload/<@http://web-platform.test:8000/_mozilla/mozilla/FileAPI/file-upload.html:16:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1984:25
  └ run_test/testframe.onload@http://web-platform.test:8000/_mozilla/mozilla/FileAPI/file-upload.html:14:7

glowe · 2019-11-27T05:20:07Z

As a sanity check, I'm trying to figure out a way to run this failed test in Firefox. I'll ask for help if I get stuck.

Manishearth · 2019-11-27T05:28:44Z

You can run stuff online at https://wpt.fyi

…

On Tue, Nov 26, 2019, 9:20 PM Graham Lowe ***@***.***> wrote: As a sanity check, I'm trying to figure out a way to run this failed test in Firefox. I'll ask for help if I get stuck. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24670?email_source=notifications&email_token=AAMK6SF7ZGZFNJISTQMJ2MTQVX7QRA5CNFSM4JJOP46KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFIKK2I#issuecomment-558933353>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMK6SG4F224FHTDWUX7JTLQVX7QRANCNFSM4JJOP46A> .

glowe · 2019-11-28T03:28:15Z

Ok, there's a bunch of issues I've discovered:

I wasn't able to use https://wpt.fyi because the test doesn't exist there–it's in the mozilla directory which are web-platform-tests that cannot be upstreamed. However, I was able to run the test manually by using the relevant instructions in the web-platform-tests README.md, adding an appropriate symlink, and using wpt serve. I tested against Safari, Chrome, and Firefox and the test failed for all 3 browsers. One of the discrepancies is that the browsers do include a trailing CRLF after the final boundary delimiter. However, there are other differences–I've pushed the changes in this commit so you can see:
62bac99
Chrome and Firefox (Safari fails for other reasons) pass the multipart form tests in response-consume.html (https://wpt.fyi/results/fetch/api/response/response-consume.html?label=experimental&label=master&aligned), because they send a trailing CRLF after the final boundary and the test expects it as shown here:

servo/tests/wpt/web-platform-tests/fetch/api/response/response-consume.html

Line 71 in 5f1681a

'--' + multipartBoundary + '--\r\n');

However, I read rfc2046 (https://tools.ietf.org/html/rfc2046#section-5.1.1) which is referenced by rfc7578 and the BNF would seem to indicate that the trailing CRLF should only be sent when there is an epilogue:

close-delimiter := delimiter "--"

dash-boundary := "--" boundary
                 ; boundary taken from the value of
                 ; boundary parameter of the
                 ; Content-Type field.

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

So it would seem that browsers don't strictly follow the rfc.

I'm happy to withdraw this PR or keep proceeding. Let me know what you think. Thanks!

Manishearth · 2019-11-28T14:59:19Z

tests/wpt/mozilla/tests/mozilla/FileAPI/resource/file-submission.py

@@ -20,8 +20,8 @@ def main(request, response):

    boundary = content_type[1].strip("boundary=")

-    body = "--" + boundary + "\r\nContent-Disposition: form-data; name=\"file-input\"; filename=\"upload.txt\""


Why does the filename go away?

I shouldn't have committed this change, but it's the version of the file that "passes" in Chrome, Safari, and Firefox. The reason the filename is empty, the contents are empty, and the Content-Type is application/octet-stream is because the test actually degrades to submitting a form with an empty file input in those browsers. This is because the test relies on a servo specific testing extension on the file input object called selectFiles, which doesn't work in the other browsers.

I'll replace this commit with the proper changes to have this test working in Servo in a few moments.

Ok. Dropped the commit that had the filename change. The only change in this file will be the addition of the \r\n now.

Manishearth

r=me , please squash!


        Add CRLF to encoded multipart form data

Some WPT tests were failing because they expected the body for a multipart form data response to end with a CRLF. So I updated encode_multipart_form_data to add the missing terminator.

glowe · 2019-11-30T00:10:49Z

r=me , please squash!

Squashed! Thanks!

jdm · 2019-11-30T00:23:05Z

@bors-servo r=Manishearth

bors-servo · 2019-11-30T00:23:07Z

📌 Commit 3d322f9 has been approved by Manishearth

bors-servo · 2019-11-30T00:36:25Z

⌛ Testing commit 3d322f9 with merge 0aec926...


        Auto merge of #24670 - glowe:add-crlf-to-boundary, r=Manishearth

Add CRLF to encoded multipart form data  Some (3) WPT tests were failing because they expected the body for a multipart form data response to end with a CRLF. So I updated encode_multipart_form_data to add the missing terminator. Looking at the corresponding spec (https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart%2Fform-data-encoding-algorithm) and RFC (https://tools.ietf.org/html/rfc7578), I couldn't find anything mentioned about this detail. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes

bors-servo · 2019-11-30T00:42:55Z

💔 Test failed - status-taskcluster

glowe · 2019-11-30T01:03:58Z

Failures appears to be unrelated to this change:

--- stderr
configure: WARNING: using cross tools not prefixed with host triplet
make[1]: warning: -jN forced in submake: disabling jobserver mode.
/root/.cargo/registry/src/github.com-1ecc6299db9ec823/servo-fontconfig-sys-4.0.7/missing: line 81: gperf: command not found
WARNING: 'gperf' is missing on your system.
         You might have modified some files without having the proper
         tools for further handling them.  Check the 'README' file, it
         often tells you about the needed prerequisites for installing
         this package.  You may also peek at any GNU archive site, in
         case some other package contains this missing 'gperf' program.
make[3]: *** [fcobjshash.h] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [all-recursive] Error 1
make[1]: *** [all] Error 2
make: *** [/repo/target/android/i686-linux-android/release/build/servo-fontconfig-sys-f1af55aa4d8a679d/out/libfontconfig.a] Error 2
thread 'main' panicked at 'assertion failed: Command::new("make").env("MAKEFLAGS",
                         env::var("CARGO_MAKEFLAGS").unwrap_or_default()).args(&["-R",
                                                                                 "-f",
                                                                                 "makefile.cargo"]).status().unwrap().success()', /root/.cargo/registry/src/github.com-1ecc6299db9ec823/servo-fontconfig-sys-4.0.7/build.rs:20:5
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:84
...
  18: std::panic::catch_unwind
             at src/libstd/panic.rs:395
  19: std::rt::lang_start_internal
             at src/libstd/rt.rs:47
  20: main
  21: __libc_start_main
  22: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
warning: build failed, waiting for other jobs to finish...
   Completed fontsan v0.4.0 custom-build (run) in 16.4s
   Completed to_shmem_derive v0.0.1 in 13.7s
   Completed webxr v0.0.1 custom-build in 4.2s
   Completed mozjs_sys v0.67.1 custom-build in 5.4s
   Completed serde_derive v1.0.103 in 57.5s
   Completed style_derive v0.0.1 in 21.7s
   Completed rust-webvr v0.16.1 custom-build in 11.1s
   Completed mozangle v0.2.7 custom-build (run) in 45.3s
error: build failed
[Warning] Could not generate notification! Optional Python module 'dbus' is not installed.
Build FAILED in 0:03:17

jdm · 2019-11-30T01:09:55Z

Yes, that's #24715. I'll retry once #24929 is done merging.

jdm · 2019-11-30T01:21:34Z

@bors-servo retry

bors-servo · 2019-11-30T01:21:40Z

⌛ Testing commit 3d322f9 with merge 3db4737...


        Auto merge of #24670 - glowe:add-crlf-to-boundary, r=Manishearth

Add CRLF to encoded multipart form data  Some (3) WPT tests were failing because they expected the body for a multipart form data response to end with a CRLF. So I updated encode_multipart_form_data to add the missing terminator. Looking at the corresponding spec (https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart%2Fform-data-encoding-algorithm) and RFC (https://tools.ietf.org/html/rfc7578), I couldn't find anything mentioned about this detail. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes

bors-servo · 2019-11-30T02:40:05Z

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing 3db4737 to master...

highfive assigned Manishearth Nov 6, 2019

highfive added the S-awaiting-review label Nov 6, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 26, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 26, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Nov 28, 2019

Manishearth reviewed Nov 28, 2019

View changes

glowe force-pushed the glowe:add-crlf-to-boundary branch from 62bac99 to b74ceb1 Nov 29, 2019

Manishearth approved these changes Nov 29, 2019

View changes

jdm added S-needs-squash and removed S-awaiting-review labels Nov 29, 2019

Add CRLF to encoded multipart form data

Loading status checks…

3d322f9

Some WPT tests were failing because they expected the body for a multipart form data response to end with a CRLF. So I updated encode_multipart_form_data to add the missing terminator.

glowe force-pushed the glowe:add-crlf-to-boundary branch from b74ceb1 to 3d322f9 Nov 30, 2019

highfive added the S-awaiting-review label Nov 30, 2019

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Nov 30, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 30, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 30, 2019

bors-servo merged commit 3d322f9 into servo:master Nov 30, 2019
1 of 2 checks passed

1 of 2 checks passed

Community-TC (pull_request) TaskGroup: failure
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 30, 2019

servo / servo

Add CRLF to encoded multipart form data #24670

Add CRLF to encoded multipart form data #24670

glowe commented Nov 6, 2019

highfive commented Nov 6, 2019

highfive commented Nov 6, 2019

jdm commented Nov 12, 2019

Manishearth commented Nov 12, 2019

Manishearth commented Nov 12, 2019

Manishearth commented Nov 26, 2019

bors-servo commented Nov 26, 2019

bors-servo commented Nov 26, 2019

bors-servo commented Nov 26, 2019

CYBAI commented Nov 26, 2019

glowe commented Nov 27, 2019

Manishearth commented Nov 27, 2019

glowe commented Nov 28, 2019 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth left a comment

glowe commented Nov 30, 2019

jdm commented Nov 30, 2019

bors-servo commented Nov 30, 2019

bors-servo commented Nov 30, 2019

bors-servo commented Nov 30, 2019

glowe commented Nov 30, 2019

jdm commented Nov 30, 2019

jdm commented Nov 30, 2019

bors-servo commented Nov 30, 2019

bors-servo commented Nov 30, 2019

		@@ -20,8 +20,8 @@ def main(request, response):

		boundary = content_type[1].strip("boundary=")

		body = "--" + boundary + "\r\nContent-Disposition: form-data; name=\"file-input\"; filename=\"upload.txt\""

servo / servo

Join GitHub today

Add CRLF to encoded multipart form data #24670

Add CRLF to encoded multipart form data #24670

Conversation

glowe commented Nov 6, 2019

highfive commented Nov 6, 2019

highfive commented Nov 6, 2019

jdm commented Nov 12, 2019

Manishearth commented Nov 12, 2019

Manishearth commented Nov 12, 2019

Manishearth commented Nov 26, 2019

bors-servo commented Nov 26, 2019

bors-servo commented Nov 26, 2019

bors-servo commented Nov 26, 2019

CYBAI commented Nov 26, 2019

glowe commented Nov 27, 2019

Manishearth commented Nov 27, 2019

glowe commented Nov 28, 2019 • edited

This comment has been minimized.

Manishearth Nov 28, 2019

This comment has been minimized.

glowe Nov 29, 2019 • edited

This comment has been minimized.

glowe Nov 29, 2019

Manishearth left a comment

glowe commented Nov 30, 2019

jdm commented Nov 30, 2019

bors-servo commented Nov 30, 2019

bors-servo commented Nov 30, 2019

bors-servo commented Nov 30, 2019

glowe commented Nov 30, 2019

jdm commented Nov 30, 2019

jdm commented Nov 30, 2019

bors-servo commented Nov 30, 2019

bors-servo commented Nov 30, 2019

glowe commented Nov 28, 2019 •

edited

glowe Nov 29, 2019 •

edited