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

wip #112

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

wip #112

wants to merge 7 commits into from

Conversation

KnorpelSenf
Copy link
Contributor

Closes #111.

@KnorpelSenf
Copy link
Contributor Author

not tested at all, does not even compile yet

@kingmesal
Copy link

I'm very interested in dumping axios in favor of the built-in fetch. I run a lot of things in cloudflare workers and this would be quite a helpful change.

Any update on when this might be ready?

@KnorpelSenf
Copy link
Contributor Author

I'm ready to continue with this, I was just hoping for pointers regarding #111 (comment) before I go ahead. Shipping a custom implementation of multipart/form-data clearly gives us the best library, but I didn't feel like deciding alone whether or not we want to accept that in this library.

@josiasmontag
Copy link
Contributor

Sorry for the delay. We should go with a custom multipart/form-data implementation then as @KnorpelSenf suggested.

I think we need to keep the optional size parameter in the upload() method though. If one passes a custom stream with unknown length, there is no way to calculate a correct Content-Length header.

@KnorpelSenf
Copy link
Contributor Author

KnorpelSenf commented May 2, 2024

Sorry for the delay. We should go with a custom multipart/form-data implementation then as @KnorpelSenf suggested.

Alright, I will commence my work on this in the coming time, but my time for OSS is a bit limited right now, so please don't expect an immediate implementation :)

I think we need to keep the optional size parameter in the upload() method though. If one passes a custom stream with unknown length, there is no way to calculate a correct Content-Length header.

What if somebody passes a read stream, or an AsyncIterator<Uint8Array>? For example, if people download a file from one server via fetch and then obtain the file stream from the response body, there is no way of knowing the file size in advance. Is the content-length header strictly required?

@josiasmontag
Copy link
Contributor

What if somebody passes a read stream, or an AsyncIterator<Uint8Array>? For example, if people download a file from one server via fetch and then obtain the file stream from the response body, there is no way of knowing the file size in advance. Is the content-length header strictly required?

Unfortunately, S3 strictly requires the Content-Length header for uploads. Therefore, I would like to automatically detect the file size if possible (file streams) and otherwise use a size parameter for custom streams.

In the mentioned example the server should return a Content-Length header which could be passed as size parameter to the upload() method.

@KnorpelSenf
Copy link
Contributor Author

What if somebody passes a read stream, or an AsyncIterator<Uint8Array>? For example, if people download a file from one server via fetch and then obtain the file stream from the response body, there is no way of knowing the file size in advance. Is the content-length header strictly required?

Unfortunately, S3 strictly requires the Content-Length header for uploads. Therefore, I would like to automatically detect the file size if possible (file streams) and otherwise use a size parameter for custom streams.

Alright!

In the mentioned example the server should return a Content-Length header which could be passed as size parameter to the upload() method.

Yeah I'll add this to the doc string

@dncnbuck
Copy link

dncnbuck commented May 9, 2024

Hi @KnorpelSenf - I see you've mentioned you won't be able to dedicate much time to this PR - but regardless I'm going to ask! Do you have a rough timeframe here?

@KnorpelSenf
Copy link
Contributor Author

Yep, I'm gonna give a talk at https://events.geekle.us/typescript24/ on Tuesday so there are a number of things to prepare until then. The rest of that week will be spent catching up with life in general, and the week after that I'm mostly free and looking forward to getting back to this (but no promises, life can be surprising and I don't want to set myself deadlines)

@KnorpelSenf
Copy link
Contributor Author

If you feel like contributing, you can check out the base implementation of what I'll do here in this file: https://github.com/grammyjs/grammY/blob/main/src/core/payload.ts

@KnorpelSenf
Copy link
Contributor Author

I will get back to this after my summer vacation, likely in September. Sorry for the delays, there were a few other important tasks on my agenda :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moderate Severity: Axios CSRF Vulnerability in Versions 0.8.1 to 1.5.1
4 participants