ssrf_filter '1.1.0' introducing a breaking change

[ovidiuanca]

Since ssrf_filter updated its version from 1.0.8 to 1.1.0, there is an error being raised:

def filename_from_uri
  CGI.unescape(File.basename(uri.path))
end

# bash
undefined method `path' for nil:NilClass

CGI.unescape(File.basename(uri.path))

To give more information, the uri object above is constructed within the following code snippet:

response = SsrfFilter.get(uri, headers: headers) do |req|
  request = req
end
response.uri = request.uri

request.uri now comes as nil from the SsrfFilter.get method.

I am not sure whether this issue belongs in here, or if it should be posted to ssrf_filter.

[courtsimas]

👀

[eliastre100]

If it might be of any help, as I had the same error, and while searching for the orgin, it seems that the response is now given to the block instead of the request

https://github.com/arkadiyt/ssrf_filter/pull/51/files#diff-95bca3ab492cb05975c93fdbbd14a18288350481b2f06307f2699fdd3fdb17d6L192

[dboiii]

I got an error too. It's working when downgrade version 1.1.1 to 1.0.8

[vadim-zverugo]

I faced with this issue too after upgrading the ssrf_filter from v1.0 to v1.1. Fortunetely, it was caught by the tests and I fixed them by adding Content-Disposition: 'attachment; filename="sample.png"' HTTP header to the testing requests.

But another more critical issue brought up. When trying to upload a file using the remote URL (e.g. model.remote_{field}_url=) the following error ocurrs: NoMethodError: undefined method `closed?' for nil:NilClass

I didn't have a time for debugging so just downgraded the ssrf_filter lib back to v1.0.x.

[giangth94]

@ovidiuanca thanks for raising this, I recently upgraded Rails to 5.2 and encountered the same issue.

My team detected the issue when testing upload function using remote URL, and we received this exception

NoMethodError
undefined method `closed?' for nil:NilClass

# net/http/response.rb in stream_check at line 333

def stream_check
  raise IOError, 'attempt to read body out of block' if @socket.closed?
end

Had to revert ssrf_filter back to 1.0.8

[BrianHawley]

I patched the request_proc issue and it had the same stream_check error.

The problem is that SsrfFilter.get returns the response before reading the body, then closes the connection; it expects you to read the response body when the response is passed to the block. RemoteFile then attempts to read the body after the connection is closed. The solution would be, at least when using ssrf_filter 1.1.x, to use this code:

response = SsrfFilter.get(uri, headers: headers, request_proc: ->(req) { request = req }, &:body)

I'll adjust my PR accordingly.

Update: This doesn't make sense. Net::HTTPResponse reading_body calls body, so the stream_check error can only happen if it doesn't get that far. The NoMethodError is caused by Net::HTTPResponse's bad nil checking, which also has issues elsewhere in the class. What I don't get is why it's happening in the ssrf_filter 1.1.1 version, but not in the 1.0.8 version.

I'm going to revert the latest change in my PR. It doesn't hurt, but it doesn't help.

[guillec]

This was also causing issues for me while testing downloading remote files. I was stubbing the response to fetching a remote file and uri.path was throwing error since uri was nil.

Downgrading to 1.0.8 fixed the issue for me.