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

Add support for chunked downloads #30

Closed

Conversation

mrhaddad
Copy link

Not an expert in SSRF attacks and not sure if this something you want ssrf_filter to support, but we need a secure way to read chunked responses. How does this look to you?

@coveralls
Copy link

coveralls commented Nov 16, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 34b8af5 on mrhaddad:add-support-for-chunked-downloads into f7cbd43 on arkadiyt:master.

@arkadiyt
Copy link
Owner

Thanks @mrhaddad - I will take a look at this PR this week

@mrhaddad
Copy link
Author

mrhaddad commented Dec 2, 2019

Thanks @mrhaddad - I will take a look at this PR this week

Hey @arkadiyt, do you think you'll be able to take a look at this PR any time soon? Thanks!

@arkadiyt arkadiyt force-pushed the add-support-for-chunked-downloads branch from 9a75850 to 11a1bdb Compare December 5, 2019 00:15
@arkadiyt
Copy link
Owner

arkadiyt commented Dec 5, 2019

@mrhaddad sorry on the delay here, it was partly holidays and partly a rabbit hole of getting tests to pass due to Webmock not supporting chunked responses.

I made a few updates here:

  • I changed the result of SsrfFilter.get/put/post/delete to always be a response object, I think it's less confusing than having a different return type depending on the input options (also returning an enumerator makes it so you can't check other response attributes, like the status code)
  • I moved some of the validation code so that it still does redirect checks even on chunked responses (otherwise your body proc could invoked even when it shouldn't be)

With the new changes the more recent versions of ruby pass but ruby-2.4 and lower fail, I'm not sure why yet

@mrhaddad mrhaddad force-pushed the add-support-for-chunked-downloads branch from 4966d7a to acd9ac5 Compare December 9, 2019 19:13
@mrhaddad
Copy link
Author

mrhaddad commented Dec 9, 2019

Thanks @arkadiyt. Changes look good. I had to amend to unset the forced hostname when calling the chunked response block as this block may make Net::HTTP calls as well. I also added changes to read the response body before returning to maintain parity with existing version. Haven't had a chance to look into the test failures on earlier versions of Ruby yet. Let me know what you think when you get a chance!

@maxjacobson
Copy link

👋 checking in on this one. I confess I also do not know what's going on with the test failures on the older versions of Ruby. I tried to reproduce it locally but could not.

@arkadiyt arkadiyt deleted the branch arkadiyt:master August 5, 2022 23:59
@arkadiyt arkadiyt closed this Aug 5, 2022
@maxjacobson
Copy link

👋🏻 hey there. I see this one was closed. Does that mean the feature won't be supported?

@arkadiyt arkadiyt mentioned this pull request Aug 28, 2022
@arkadiyt
Copy link
Owner

arkadiyt commented Aug 28, 2022

@maxjacobson @mrhaddad sorry on the long delay here - I've merged this on #51 and will release 1.1.0 shortly

@mrhaddad thanks very much for writing this clean, well-tested code

@maxjacobson
Copy link

@arkadiyt Amazing! We'll take a look. Much appreciated

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

Successfully merging this pull request may close these issues.

4 participants