Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multipart/form-data upload forms #1

Closed
simonw opened this issue Mar 1, 2020 · 8 comments
Closed

Support multipart/form-data upload forms #1

simonw opened this issue Mar 1, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@simonw
Copy link
Owner

simonw commented Mar 1, 2020

This is tricky because I'd like to avoid consuming the entire upload body into memory just to check the token. Ideally this would work with streaming the data somehow.

@simonw
Copy link
Owner Author

simonw commented Jul 6, 2020

This is now a blocker for simonw/datasette-upload-csvs#11

@simonw
Copy link
Owner Author

simonw commented Jul 6, 2020

I think the way to do this is to consume the body up to finding the csrftoken field, and also to throw an error if we get to a file attachment BEFORE seeing that field. Then we can set up a new receive function that replays what we've seen already from memory.

It will be up to template authors to ensure the csrftoken is sent first - they will see the error in development if they get this wrong.

@simonw
Copy link
Owner Author

simonw commented Jul 6, 2020

I'll ship this as 1.0.

@simonw
Copy link
Owner Author

simonw commented Jul 6, 2020

I'd already decided on that approach in the comments:

asgi-csrf/asgi_csrf.py

Lines 136 to 139 in 245d8ba

elif content_type == b"multipart/form-data":
# Consume non-file items until we see a csrftoken
# If we see a file item first, it's an error
assert False, "multipart/form-data is not yet supported"

@simonw
Copy link
Owner Author

simonw commented Jul 6, 2020

I'll need to add a dependency on python-multipart: https://github.com/andrew-d/python-multipart

@simonw
Copy link
Owner Author

simonw commented Jul 6, 2020

From careful reading of https://github.com/andrew-d/python-multipart/blob/b3e7b45d2e2e90281421fb5c00a0b7f4b402d02c/multipart/multipart.py I think I know how to do this. Here's where a FileParser is constructed there:

https://github.com/andrew-d/python-multipart/blob/b3e7b45d2e2e90281421fb5c00a0b7f4b402d02c/multipart/multipart.py#L1851-L1857

    # Instantiate a form parser.
    form_parser = FormParser(content_type,
                             on_field,
                             on_file,
                             boundary=boundary,
                             file_name=file_name,
                             config=config)

But that class takes an optional FileClass argument:

    :param FileClass: The class to use for uploaded files.  Defaults to
                      :class:`File`, but you can provide your own class if you
                      wish to customize behaviour.  The class will be
                      instantiated as FileClass(file_name, field_name), and it
                      must provide the folllowing functions::
                          file_instance.write(data)
                          file_instance.finalize()
                          file_instance.close()

This is only written to the moment a file upload chunk starts to be processed.

So... I can pass in my own implementation of FileClass which simply raises an error when .write() is called on it. I can use this to catch the case where a file upload starts to be processed before a csrftoken form field has been encountered.

@simonw
Copy link
Owner Author

simonw commented Aug 15, 2020

I'm going to ship this as an alpha release so I can test it.

simonw added a commit that referenced this issue Aug 15, 2020
simonw added a commit that referenced this issue Aug 15, 2020
@simonw
Copy link
Owner Author

simonw commented Aug 15, 2020

I'm going for 100% test coverage to ensure this is well tested: #13

@simonw simonw closed this as completed Aug 15, 2020
simonw added a commit that referenced this issue Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant