Repository: google/go-safeweb (go)

Issue:
Implement accessor methods for getting a file passed in a multipart request
A POST, PATCH or PUT request with the header `Content-Type:multipart/form-data` allows users to send files as part of the request. This is currently not implemented in  the `safehttp.MultipartForm` type. We need to investigate the behaviour of the current API, provided by `net/http`, and provide accessor methods for form files in the `safehttp` API

Pull request:
Extended the MultipartForm type to allow accessing file uploads
Fixes #32, #38

Extended the `MultipartForm` to support retrieving file uploads from incoming requests. This will prevent the user from directly accessing the multipart form but will still allow access to the accessor methods in `multipart` used for opening files.

Fixed the issue that was causing a multipart form to be capped at 32 MB (my misunderstanding of documentation).

This branch  is rebased on top of `grenfeldt-splitting-form-tests` branch  (#125 ).

Patch:
diff --git a/safehttp/form.go b/safehttp/form.go
index c67cb03..80fc1ad 100644
--- a/safehttp/form.go
+++ b/safehttp/form.go
@@ -16,6 +16,7 @@ package safehttp
 
 import (
 	"fmt"
+	"mime/multipart"
 	"strconv"
 )
 
@@ -195,9 +196,27 @@ func (f *Form) Err() error {
 	return f.err
 }
 
-// MultipartForm extends the Form structure to define a POST, PATCH or PUT
-// request that has Content-Type: multipart/form-data.
+// MultipartForm extends a parsed multipart form, part of the body of a
+// PATCH, POST or PUT request. A multipart form can include both form values and
+// file uploads, stored either in memory or on disk.
 type MultipartForm struct {
 	Form
-	// TODO(mihalimara22): add support for form files in the MultipartForm
+	mf *multipart.Form
+}
+
+// File returns the file parts associated with form key param or a nil
+// slice, if none. These can be then opened individually by calling
+// FileHeader.Open.
+func (f *MultipartForm) File(param string) []*multipart.FileHeader {
+	fh, ok := f.mf.File[param]
+	if !ok {
+		return nil
+	}
+	return fh
+}
+
+// RemoveFiles removes any temporary files associated with a Form and returns
+// the first error that occured, if any.
+func (f *MultipartForm) RemoveFiles() error {
+	return f.mf.RemoveAll()
 }
diff --git a/safehttp/incoming_request.go b/safehttp/incoming_request.go
index 0533ef9..ac8930a 100644
--- a/safehttp/incoming_request.go
+++ b/safehttp/incoming_request.go
@@ -51,10 +51,10 @@ func (r *IncomingRequest) Method() string {
 
 // PostForm parses the form parameters provided in the body of a POST, PATCH or
 // PUT request that does not have Content-Type: multipart/form-data. It returns
-// the parsed form parameters as a Form object, if no error occurred. If a parsing
-// error occurs it will return it, together with a nil Form. Unless we expect the
-// header Content-Type: multipart/form-data in a POST request, this method should
-// always be used for forms in POST requests.
+// the parsed form parameters as a Form object. If a parsing
+// error occurs it will return it, together with a nil Form. Unless we expect
+// the header Content-Type: multipart/form-data in a POST request, this method
+// should  always be used for forms in POST requests.
 func (r *IncomingRequest) PostForm() (*Form, error) {
 	var err error
 	r.postParseOnce.Do(func() {
@@ -79,19 +79,15 @@ func (r *IncomingRequest) PostForm() (*Form, error) {
 // MultipartForm parses the form parameters provided in the body of a POST,
 // PATCH or PUT request that has Content-Type set to multipart/form-data. It
 // returns a MultipartForm object containing the parsed form parameters and
-// files, if no error occurred, or the parsing error together with a nil
-// MultipartForm otherwise. When a form file is passed as part of a request,
-// maxMemory determines the upper limit of how much of the file can be stored in
-// main memory. If the file is bigger than maxMemory, capped at 32 MB, the
-// remaining part is going to  be stored on disk. This method should  only be
-// used when the user expects a POST request with the Content-Type: multipart/form-data header.
+// file uploads (if any) or the parsing error together with a nil MultipartForm
+// otherwise.
+//
+// If the parsed request body is larger than maxMemory, up to maxMemory bytes
+// will be stored in main memory, with the rest stored on disk in temporary
+// files.
 func (r *IncomingRequest) MultipartForm(maxMemory int64) (*MultipartForm, error) {
 	var err error
 	r.multipartParseOnce.Do(func() {
-		// Ensures no more than 32 MB are stored in memory when a form file is
-		// passed as part of the request. If this is bigger than 32 MB, the rest
-		// will be stored on disk.
-		const defaultMaxMemory = 32 << 20
 		if m := r.req.Method; m != MethodPost && m != MethodPatch && m != MethodPut {
 			err = fmt.Errorf("got request method %s, want POST/PATCH/PUT", m)
 			return
@@ -101,10 +97,6 @@ func (r *IncomingRequest) MultipartForm(maxMemory int64) (*MultipartForm, error)
 			err = fmt.Errorf("invalid method called for Content-Type: %s", ct)
 			return
 		}
-		if maxMemory < 0 || maxMemory > defaultMaxMemory {
-			maxMemory = defaultMaxMemory
-		}
-
 		err = r.req.ParseMultipartForm(maxMemory)
 	})
 	if err != nil {
@@ -113,7 +105,9 @@ func (r *IncomingRequest) MultipartForm(maxMemory int64) (*MultipartForm, error)
 	return &MultipartForm{
 			Form: Form{
 				values: r.req.MultipartForm.Value,
-			}},
+			},
+			mf: r.req.MultipartForm,
+		},
 		nil
 }
 


Test patch:
diff --git a/safehttp/form_test.go b/safehttp/form_test.go
index 323dcb3..c119359 100644
--- a/safehttp/form_test.go
+++ b/safehttp/form_test.go
@@ -16,6 +16,7 @@ package safehttp
 
 import (
 	"math"
+	"mime/multipart"
 	"strconv"
 	"testing"
 
@@ -501,3 +502,54 @@ func TestFormClearSliceUnknownType(t *testing.T) {
 		t.Error("clearSlice(&x) got: nil want: error")
 	}
 }
+
+func TestMultipartFormValidFile(t *testing.T) {
+	fh := &multipart.FileHeader{Filename: "bar"}
+	f := &MultipartForm{mf: &multipart.Form{
+		File: map[string][]*multipart.FileHeader{
+			"foo": {fh},
+		},
+	}}
+	fhs := f.File("foo")
+	if fhs == nil {
+		t.Errorf(`m.File("foo"): got nil, want file headers`)
+	}
+	if diff := cmp.Diff(fh, fhs[0], cmp.AllowUnexported(multipart.FileHeader{})); diff != "" {
+		t.Errorf("file headers mismatch (-want +got):\n%s", diff)
+	}
+}
+
+func TestMultipartFormValidFileAndVals(t *testing.T) {
+	fh := &multipart.FileHeader{Filename: "bar"}
+	f := &MultipartForm{
+		Form: Form{values: map[string][]string{"number": {"1"}}},
+		mf: &multipart.Form{
+			File: map[string][]*multipart.FileHeader{
+				"foo": {fh},
+			},
+		},
+	}
+
+	if want, got := int64(1), f.Int64("number", 0); want != got {
+		t.Errorf(`f.Int64("number"): got %d, want %d`, got, want)
+	}
+	if err := f.Err(); err != nil {
+		t.Errorf(`f.Err(): got err %v`, err)
+	}
+
+	fhs := f.File("foo")
+	if fhs == nil {
+		t.Errorf(`m.File("foo"): got nil, want file headers`)
+	}
+	if diff := cmp.Diff(fh, fhs[0], cmp.AllowUnexported(multipart.FileHeader{})); diff != "" {
+		t.Errorf("file headers mismatch (-want +got):\n%s", diff)
+	}
+}
+
+func TestMultipartFormMissingFile(t *testing.T) {
+	f := &MultipartForm{mf: &multipart.Form{}}
+	fhs := f.File("x")
+	if fhs != nil {
+		t.Errorf(`m.File("x"): got file headers, want nil`)
+	}
+}
diff --git a/safehttp/incoming_request_test.go b/safehttp/incoming_request_test.go
index 61886b4..91ba9e0 100644
--- a/safehttp/incoming_request_test.go
+++ b/safehttp/incoming_request_test.go
@@ -357,3 +357,97 @@ func TestIncomingRequestInvalidMultipartForm(t *testing.T) {
 		})
 	}
 }
+
+func TestIncomingRequestMultipartFileUpload(t *testing.T) {
+	body := "--123\r\n" +
+		"Content-Disposition: form-data; name=\"file\"; filename=\"myfile\"\r\n" +
+		"\r\n" +
+		"file content\r\n" +
+		"--123--\r\n"
+	r := safehttptest.NewRequest(safehttp.MethodPost, "/", strings.NewReader(body))
+	r.Header.Set("Content-Type", `multipart/form-data; boundary="123"`)
+
+	f, err := r.MultipartForm(1024)
+	if err != nil {
+		t.Errorf("r.MultipartForm(1024): got err %v", err)
+	}
+
+	fhs := f.File("file")
+	if fhs == nil {
+		t.Error(`f.File("file"): got nil, want file header`)
+	}
+	defer f.RemoveFiles()
+
+	file, err := fhs[0].Open()
+	content := make([]byte, 12)
+	file.Read(content)
+	if want, got := "file content", string(content); want != got {
+		t.Errorf("file.Read(content): got %s, want %s", got, want)
+	}
+}
+
+func TestIncomingRequestMultipartFormAndFileUpload(t *testing.T) {
+	body := "--123\r\n" +
+		"Content-Disposition: form-data; name=\"key\"\r\n" +
+		"\r\n" +
+		"12\r\n" +
+		"--123\r\n" +
+		"Content-Disposition: form-data; name=\"file\"; filename=\"myfile\"\r\n" +
+		"\r\n" +
+		"file content\r\n" +
+		"--123--\r\n"
+	r := safehttptest.NewRequest(safehttp.MethodPost, "/", strings.NewReader(body))
+	r.Header.Set("Content-Type", `multipart/form-data; boundary="123"`)
+
+	f, err := r.MultipartForm(1024)
+	if err != nil {
+		t.Errorf("r.MultipartForm(1024): got err %v", err)
+	}
+
+	if want, got := int64(12), f.Int64("key", 0); want != got {
+		t.Errorf(`f.Int64("key", 0): got %d, want %d`, got, want)
+	}
+	if err := f.Err(); err != nil {
+		t.Errorf("f.Err(): got err %v", err)
+	}
+
+	fhs := f.File("file")
+	if fhs == nil {
+		t.Error(`f.File("file"): got nil, want file header`)
+	}
+	defer f.RemoveFiles()
+
+	file, err := fhs[0].Open()
+	content := make([]byte, 12)
+	file.Read(content)
+	if want, got := "file content", string(content); want != got {
+		t.Errorf("file.Read(content): got %s, want %s", got, want)
+	}
+}
+
+func TestIncomingRequestFileUploadMissingContent(t *testing.T) {
+	body := "--123\r\n" +
+		"Content-Disposition: form-data; name=\"file\"; filename=\"myfile\"\r\n" +
+		"\r\n" +
+		"--123--\r\n"
+	r := safehttptest.NewRequest(safehttp.MethodPost, "/", strings.NewReader(body))
+	r.Header.Set("Content-Type", `multipart/form-data; boundary="123"`)
+
+	f, err := r.MultipartForm(1024)
+	if err != nil {
+		t.Errorf("r.MultipartForm(1024): got err %v", err)
+	}
+
+	fhs := f.File("file")
+	if fhs == nil {
+		t.Error(`f.File("file"): got nil, want file header`)
+	}
+	defer f.RemoveFiles()
+
+	file, err := fhs[0].Open()
+	content := make([]byte, 0)
+	file.Read(content)
+	if want, got := "", string(content); want != got {
+		t.Errorf("file.Read(content): got %s, want %s", got, want)
+	}
+}