Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
681303003:
Add HTTPMultipartBuilder and its test. (Closed)
|
Created:
6 years, 1 month ago by Robert Sesek Modified:
6 years, 1 month ago Reviewers:
Mark Mentovai CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Project:
crashpad Visibility:
Public. |
DescriptionAdd HTTPMultipartBuilder and its test.
BUG=https://crbug.com/415544
R=mark@chromium.org
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/9db5d6f77370518edba19de874a8dc15b1fe21fa
Patch Set 1 : #Patch Set 2 : #
Total comments: 21
Patch Set 3 : Address comments #
Total comments: 26
Patch Set 4 : #Patch Set 5 : Assert safe MIME types #
Total comments: 1
Messages
Total messages: 11 (2 generated)
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/681303003/diff/40001/util/net/http_body_test_... File util/net/http_body_test_util.h (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_body_test_... util/net/http_body_test_util.h:28: //! \brief Reads a HTTPBodyStream to a string. If an error occurs, adds a an HTTPBodyStream? Line 36 too. (at least for how Americans would normally pronounce this.) https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... File util/net/http_multipart_builder.cc (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.cc:51: streams.push_back(new StringHTTPBodyStream(field)); It’s a little scuzzy for all of these new objects to be unowned until they’re given to the CompositeHTTPBodyStream. An early return would cause leakiness. But I see that it’d be difficult to correct this, so maybe just put a comment (at the top?) pointing out this hazard. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.cc:61: return scoped_ptr<HTTPBodyStream>(new CompositeHTTPBodyStream(streams)); You need a terminating boundary too, which should be the normal boundary with an extra “--” at its end. RFC 2046 §5.1.1. http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2 https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... File util/net/http_multipart_builder.h (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:22: #include <map> C++ system headers should precede project-local headers. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:42: void SetFormData(const std::string& key, const std::string& value); |key| in this method and [the key, see below] in SetFileAttachment() should share a namespace. HTTP doesn’t require it, but HTTP doesn‘t actually require unique keys at all. You’ve decided to ensure unique keys in your implementation, which is a perfectly reasonable thing to do, so I think you should also ensure uniqueness among |key| and [the key used by SetFileAttachment()]. This should be documented (since you’ve gone to the trouble of documenting |key| uniqueness, which is good). The implementation itself is unaware that the namespace is shared, and will actually output two parts with identical names. I don’t think that this is wise. You may be able to do this pretty easily by tracking everything in a single std::map<std::string, HTTPBodyStream*> instead of having two std::map<std::string, …>s. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:45: //! multipart data, available at `name` of \a upload_file_name. I think this should be a 3-argument call: (const std::string& key, const std::string& upload_file_name, const base::FilePath& path). |upload_file_name| should appear at filename in the HTTP part. See RFC 2388 §4.4 and blink/Source/platform/network/FormDataBuilder.cpp blink::FormDataBuilder::addFilenameToMultiPartHeader(). You could do this with a 2-argument form by using path.value() to infer filename, but I don’t think that’s so hot. It’s reasonable to want to “lie” about the filename to the server, because what’s on disk might be some temporary meaningless name. (In addition, it’s reasonable to require upload_file_name to just be a basename, or to take the basename of whatever’s passed in. Full paths aren’t really appropriate.) |key| is distinct from |upload_file_name|. In the HTTP part, you’ll get name="key" and filename="upload_file_name". Optionally, you could also allow a content_type parameter here, or you could ignore it and always write Content-Type: application/octet-stream (RFC 2388 §4.1). https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:63: static std::string GenerateBoundaryString(); Is there even a reason to expose these private static methods in the class at all? They can just be anonymous-namespace functions in http_multipart_builder.cc. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... File util/net/http_multipart_builder_test.cc (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder_test.cc:33: last_line = i + 2; You should also ++i again since you don’t want to look at the '\n' again if you’ve reached this. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder_test.cc:36: return lines; This should signal somehow if the string didn’t end in \r\n. You could ADD_FAILURE() or return std::string() or both. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder_test.cc:69: EXPECT_GT(boundary.length(), 1u); EXPECT_GE, the standard allows a boundary of one character (although it’d be pretty stupid for a non- context-sensitive boundary to be that short). Same elsewhere.
https://codereview.chromium.org/681303003/diff/40001/util/net/http_body_test_... File util/net/http_body_test_util.h (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_body_test_... util/net/http_body_test_util.h:28: //! \brief Reads a HTTPBodyStream to a string. If an error occurs, adds a On 2014/10/28 22:34:28, Mark Mentovai wrote: > an HTTPBodyStream? Line 36 too. > > (at least for how Americans would normally pronounce this.) H here is not a vowel nor unvocalized, so "a" is correct here. HaytchTTP. http://editingandwritingservices.com/a-or-an-before-words-beginning-with-h/ https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... File util/net/http_multipart_builder.cc (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.cc:51: streams.push_back(new StringHTTPBodyStream(field)); On 2014/10/28 22:34:28, Mark Mentovai wrote: > It’s a little scuzzy for all of these new objects to be unowned until they’re > given to the CompositeHTTPBodyStream. An early return would cause leakiness. But > I see that it’d be difficult to correct this, so maybe just put a comment (at > the top?) pointing out this hazard. Done. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.cc:61: return scoped_ptr<HTTPBodyStream>(new CompositeHTTPBodyStream(streams)); On 2014/10/28 22:34:28, Mark Mentovai wrote: > You need a terminating boundary too, which should be the normal boundary with an > extra “--” at its end. > > RFC 2046 §5.1.1. > http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2 I was originally going to handle this in Transport along with setting the Content-Type (which also requires the boundary), but after writing the test for transport, it's easier to do it here. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... File util/net/http_multipart_builder.h (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:22: #include <map> On 2014/10/28 22:34:28, Mark Mentovai wrote: > C++ system headers should precede project-local headers. Done. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:42: void SetFormData(const std::string& key, const std::string& value); On 2014/10/28 22:34:28, Mark Mentovai wrote: > |key| in this method and [the key, see below] in SetFileAttachment() should > share a namespace. HTTP doesn’t require it, but HTTP doesn‘t actually require > unique keys at all. You’ve decided to ensure unique keys in your implementation, > which is a perfectly reasonable thing to do, so I think you should also ensure > uniqueness among |key| and [the key used by SetFileAttachment()]. > > This should be documented (since you’ve gone to the trouble of documenting |key| > uniqueness, which is good). > > The implementation itself is unaware that the namespace is shared, and will > actually output two parts with identical names. I don’t think that this is wise. Right, I was playing both sides of this coin by enforcing uniqueness in each of the individual keyspaces, while relying on HTTP to not care if there is a collision between the two. > You may be able to do this pretty easily by tracking everything in a single > std::map<std::string, HTTPBodyStream*> instead of having two > std::map<std::string, …>s. This class cannot own HTTPBodyStream objects because they are passed to the CompositeHTTPBodyStream, which takes ownership. I'd either have to add a copy ctor to HTTPBodyStream, or have GetBodyStream() also clear all state on this object. I do now enforce uniqueness in both keyspaces, though. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:45: //! multipart data, available at `name` of \a upload_file_name. On 2014/10/28 22:34:28, Mark Mentovai wrote: > I think this should be a 3-argument call: (const std::string& key, const > std::string& upload_file_name, const base::FilePath& path). > > |upload_file_name| should appear at filename in the HTTP part. See RFC 2388 §4.4 > and blink/Source/platform/network/FormDataBuilder.cpp > blink::FormDataBuilder::addFilenameToMultiPartHeader(). You could do this with a > 2-argument form by using path.value() to infer filename, but I don’t think > that’s so hot. It’s reasonable to want to “lie” about the filename to the > server, because what’s on disk might be some temporary meaningless name. (In > addition, it’s reasonable to require upload_file_name to just be a basename, or > to take the basename of whatever’s passed in. Full paths aren’t really > appropriate.) > > |key| is distinct from |upload_file_name|. In the HTTP part, you’ll get > name="key" and filename="upload_file_name". > > Optionally, you could also allow a content_type parameter here, or you could > ignore it and always write Content-Type: application/octet-stream (RFC 2388 > §4.1). Done. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder.h:63: static std::string GenerateBoundaryString(); On 2014/10/28 22:34:28, Mark Mentovai wrote: > Is there even a reason to expose these private static methods in the class at > all? They can just be anonymous-namespace functions in > http_multipart_builder.cc. No, moved to anon. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... File util/net/http_multipart_builder_test.cc (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder_test.cc:33: last_line = i + 2; On 2014/10/28 22:34:28, Mark Mentovai wrote: > You should also ++i again since you don’t want to look at the '\n' again if > you’ve reached this. Done. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder_test.cc:36: return lines; On 2014/10/28 22:34:28, Mark Mentovai wrote: > This should signal somehow if the string didn’t end in \r\n. You could > ADD_FAILURE() or return std::string() or both. I don't think it's an error to not end in CRLF. The goal here is to split, so instead I return the remainder as the final item. https://codereview.chromium.org/681303003/diff/40001/util/net/http_multipart_... util/net/http_multipart_builder_test.cc:69: EXPECT_GT(boundary.length(), 1u); On 2014/10/28 22:34:28, Mark Mentovai wrote: > EXPECT_GE, the standard allows a boundary of one character (although it’d be > pretty stupid for a non- > context-sensitive boundary to be that short). > > Same elsewhere. Done.
https://codereview.chromium.org/681303003/diff/40001/util/net/http_body_test_... File util/net/http_body_test_util.h (right): https://codereview.chromium.org/681303003/diff/40001/util/net/http_body_test_... util/net/http_body_test_util.h:28: //! \brief Reads a HTTPBodyStream to a string. If an error occurs, adds a Robert Sesek wrote: > On 2014/10/28 22:34:28, Mark Mentovai wrote: > > an HTTPBodyStream? Line 36 too. > > > > (at least for how Americans would normally pronounce this.) > > H here is not a vowel nor unvocalized, so "a" is correct here. HaytchTTP. > > http://editingandwritingservices.com/a-or-an-before-words-beginning-with-h/ Heh. Not once have I heard you pronounce this as haytch. Aitch with a long A, sure. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... File util/net/http_multipart_builder.cc (right): https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:82: const char kBoundaryCRLF[] = "\r\n\r\n"; Can you move this above or below the function definitions so that it stands out on its own a little bit more? Actually, it’s only used in GetBodyStream(), so maybe you can just move it to the top of that method. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:119: if (attachment.content_type.empty()) Since you need the if anyway, I’d write this as if (content_type.empty()) attachment.content_type = "application/octet-stream"; else attachment.content_type = content_type; or perhaps with a ternary, which is the most space-savingest option of all attachment.content_type = content_type.empty() ? "application/octet-stream" : content_type; or consider giving FileAttachment a constructor so you can just write FileAttachment attachment(upload_file_name, content_type, path); https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:136: field += "\r\n"; Since you have kBoundaryCRLF for a pair of CRLFs, maybe you want a const char[] for a single CRLF too. You use it three times in this function. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:144: attachment.filename.c_str()); Encode me with EncodeFieldName(). Blink does this too. Maybe rename EncodeFieldName() to reflect that it’s more generic than just field names. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:146: attachment.content_type.c_str(), kBoundaryCRLF); Not checking Content-Type for bad characters gives me the heebie-jeebies. Could you add a CHECK that makes sure that the Content-Type isn’t fishy? I guess I’d base fishiness on the definition of “token” from RFC 2045 §5.1, except that we’d also include “/” in the set of accepted characters for obvious reasons. You could use find_first_not_of() to look for bad characters. I’d place the CHECK here, in SetFileAttachment(), or in the FileAttachment constructor (if you add one). https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:153: streams.push_back(new StringHTTPBodyStream(boundary() + "--")); No leading “--” here too? I think it needs it. RFC 2046 §5.1.1 (“The boundary delimiter line following the last body part…”) http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2 https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:153: streams.push_back(new StringHTTPBodyStream(boundary() + "--")); I would also end this with a CRLF even if no standard strictly requires it. blink::FormDataBuilder::addBoundaryToMultiPartHeader() does this too. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... File util/net/http_multipart_builder.h (right): https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:52: //! \param[in] path The path of the files whose contents will be uploaded. file, singular. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:54: //! If this is empty, "application/octet-stream" will be used. `"application/octet-stream"` https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:67: std::string boundary() const { return boundary_; } Either make this getter private, or just omit it altogether, because the only uses are in the class itself and it has direct access to boundary_. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:78: void EraseKey(const std::string& key); It still seems like we should be able to just use a single map, but we can run with this for now. OPTIONAL: I’m just thinking about how to do it with a single map. It could look like class FormFieldPart { virtual void AddToHTTPBodyStream(std::vector<HTTPBodyStream*>* streams) = 0; }; class StringFormFieldPart : public FormFieldPart { void AddToHTTPBodyStream(std::vector<HTTPBodyStream*>* streams) override { // make a stream and add it } }; class FileUploadFormFieldPart : public FormFieldPart { void AddToHTTPBodyStream(std::vector<HTTPBodyStream*>* streams) override { // make streams and add them } }; Then your one and only map in this class would be std::map<std::string, FormFieldPart*>. One map, and simpler semantics in this class. As an alternative to AddToHTTPBodyStream(), you could define the interface as scoped_ptr<HTTPBodyStream> GetHTTPBodyStream(); StringFormFieldPart would return a StringHTTPBodyStream. FileUploadFormFieldPart would return a CompositeHTTPBodyStream. HTTPMultipartBuilder::GetBodyStream() would return a CompositeHTTPBodyStream that contains both StringHTTPBodyStream and CompositeHTTPBodyStream objects. That’s kind of cool, actually. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... File util/net/http_multipart_builder_test.cc (right): https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder_test.cc:92: // with "--". See the comment in the other file. I think that final boundary is both preceded and followed by “--”.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... File util/net/http_multipart_builder.cc (right): https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:82: const char kBoundaryCRLF[] = "\r\n\r\n"; On 2014/10/29 21:29:13, Mark Mentovai wrote: > Can you move this above or below the function definitions so that it stands out > on its own a little bit more? > > Actually, it’s only used in GetBodyStream(), so maybe you can just move it to > the top of that method. Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:119: if (attachment.content_type.empty()) On 2014/10/29 21:29:14, Mark Mentovai wrote: > Since you need the if anyway, I’d write this as > > if (content_type.empty()) > attachment.content_type = "application/octet-stream"; > else > attachment.content_type = content_type; > > or perhaps with a ternary, which is the most space-savingest option of all > > attachment.content_type = > content_type.empty() ? "application/octet-stream" : content_type; > > or consider giving FileAttachment a constructor so you can just write > > FileAttachment attachment(upload_file_name, content_type, path); Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:119: if (attachment.content_type.empty()) On 2014/10/29 21:29:14, Mark Mentovai wrote: > Since you need the if anyway, I’d write this as > > if (content_type.empty()) > attachment.content_type = "application/octet-stream"; > else > attachment.content_type = content_type; > > or perhaps with a ternary, which is the most space-savingest option of all > > attachment.content_type = > content_type.empty() ? "application/octet-stream" : content_type; > > or consider giving FileAttachment a constructor so you can just write > > FileAttachment attachment(upload_file_name, content_type, path); Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:136: field += "\r\n"; On 2014/10/29 21:29:14, Mark Mentovai wrote: > Since you have kBoundaryCRLF for a pair of CRLFs, maybe you want a const char[] > for a single CRLF too. You use it three times in this function. Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:144: attachment.filename.c_str()); On 2014/10/29 21:29:13, Mark Mentovai wrote: > Encode me with EncodeFieldName(). Blink does this too. Maybe rename > EncodeFieldName() to reflect that it’s more generic than just field names. Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:146: attachment.content_type.c_str(), kBoundaryCRLF); On 2014/10/29 21:29:14, Mark Mentovai wrote: > Not checking Content-Type for bad characters gives me the heebie-jeebies. > > Could you add a CHECK that makes sure that the Content-Type isn’t fishy? I guess > I’d base fishiness on the definition of “token” from RFC 2045 §5.1, except that > we’d also include “/” in the set of accepted characters for obvious reasons. You > could use find_first_not_of() to look for bad characters. > > I’d place the CHECK here, in SetFileAttachment(), or in the FileAttachment > constructor (if you add one). I don't see the point of checking for "fishyness". The field is either valid or invalid, and just checking for invalid characters seems like a half-baked solution. For example, if you used any one of the "tspecials", then this validation would also need to ensure that it's properly quoted. You could also specify allays/ibf-x which would be totally invalid but not "fishy". Fully validating Content-Type seems pointless, since it's really up to the server to decide what to accept. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:153: streams.push_back(new StringHTTPBodyStream(boundary() + "--")); On 2014/10/29 21:29:13, Mark Mentovai wrote: > I would also end this with a CRLF even if no standard strictly requires it. > blink::FormDataBuilder::addBoundaryToMultiPartHeader() does this too. Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:153: streams.push_back(new StringHTTPBodyStream(boundary() + "--")); On 2014/10/29 21:29:14, Mark Mentovai wrote: > No leading “--” here too? I think it needs it. > > RFC 2046 §5.1.1 (“The boundary delimiter line following the last body part…”) > http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2 Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... File util/net/http_multipart_builder.h (right): https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:52: //! \param[in] path The path of the files whose contents will be uploaded. On 2014/10/29 21:29:14, Mark Mentovai wrote: > file, singular. Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:54: //! If this is empty, "application/octet-stream" will be used. On 2014/10/29 21:29:14, Mark Mentovai wrote: > `"application/octet-stream"` Done. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:67: std::string boundary() const { return boundary_; } On 2014/10/29 21:29:14, Mark Mentovai wrote: > Either make this getter private, or just omit it altogether, because the only > uses are in the class itself and it has direct access to boundary_. Per my response on the last round, this will be necessary for setting the Content-Type header. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.h:78: void EraseKey(const std::string& key); On 2014/10/29 21:29:14, Mark Mentovai wrote: > It still seems like we should be able to just use a single map, but we can run > with this for now. > > OPTIONAL: I’m just thinking about how to do it with a single map. It could look > like > > class FormFieldPart { > virtual void AddToHTTPBodyStream(std::vector<HTTPBodyStream*>* streams) = 0; > }; > class StringFormFieldPart : public FormFieldPart { > void AddToHTTPBodyStream(std::vector<HTTPBodyStream*>* streams) override { > // make a stream and add it > } > }; > class FileUploadFormFieldPart : public FormFieldPart { > void AddToHTTPBodyStream(std::vector<HTTPBodyStream*>* streams) override { > // make streams and add them > } > }; > > Then your one and only map in this class would be std::map<std::string, > FormFieldPart*>. One map, and simpler semantics in this class. > > As an alternative to AddToHTTPBodyStream(), you could define the interface as > > scoped_ptr<HTTPBodyStream> GetHTTPBodyStream(); > > StringFormFieldPart would return a StringHTTPBodyStream. FileUploadFormFieldPart > would return a CompositeHTTPBodyStream. HTTPMultipartBuilder::GetBodyStream() > would return a CompositeHTTPBodyStream that contains both StringHTTPBodyStream > and CompositeHTTPBodyStream objects. That’s kind of cool, actually. This screams over-abstracted.
LGTM, I do feel strongly about this CHECK. https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... File util/net/http_multipart_builder.cc (right): https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:146: attachment.content_type.c_str(), kBoundaryCRLF); Robert Sesek wrote: > I don't see the point of checking for "fishyness". My concern is something setting Content-Type to something like "application/pdf\r\n\r\n%PDFBlahBlahBlahExploitCommonPDFParserVulnerabilityBlahHereIsSomeTrailingGarbage" which, if anyone ever uses this code along with some OS-level extension-to-MIME type mapper, would be totally plausible and outside of our control. I think we should detect this with a strict CHECK. allays/ibf-x would be valid if we checked this as I proposed—we’d want all of the legal characters for “token” plus “/”. I agree that it’s the server’s responsibility to decide what to do with this. But I don’t agree that we should leave ourselves open to the possibility of being hijacked and taking us out of control of what the server actually sees as the intended “part.”
https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... File util/net/http_multipart_builder.cc (right): https://codereview.chromium.org/681303003/diff/60001/util/net/http_multipart_... util/net/http_multipart_builder.cc:146: attachment.content_type.c_str(), kBoundaryCRLF); On 2014/10/29 22:46:55, Mark Mentovai wrote: > Robert Sesek wrote: > > I don't see the point of checking for "fishyness". > > My concern is something setting Content-Type to something like > > "application/pdf\r\n\r\n%PDFBlahBlahBlahExploitCommonPDFParserVulnerabilityBlahHereIsSomeTrailingGarbage" > > which, if anyone ever uses this code along with some OS-level extension-to-MIME > type mapper, would be totally plausible and outside of our control. > > I think we should detect this with a strict CHECK. > > allays/ibf-x would be valid if we checked this as I proposed—we’d want all of > the legal characters for “token” plus “/”. I agree that it’s the server’s > responsibility to decide what to do with this. But I don’t agree that we should > leave ourselves open to the possibility of being hijacked and taking us out of > control of what the server actually sees as the intended “part.” Per offline discussion, limiting to [a-zA-Z0-9-_./]
Message was sent while issue was closed.
Committed patchset #5 (id:120001) manually as 9db5d6f77370518edba19de874a8dc15b1fe21fa (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/681303003/diff/120001/util/net/http_multipart... File util/net/http_multipart_builder.cc (right): https://codereview.chromium.org/681303003/diff/120001/util/net/http_multipart... util/net/http_multipart_builder.cc:107: c == '-'); You need '+' also. application/xml+xhtml, for example. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
