carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks
https://github.com/carrierwaveuploader/carrierwave
8.78k stars 1.66k forks source link

Carrierwave download of external url #2695

Closed janz93 closed 12 months ago

janz93 commented 1 year ago

Problem

Hello everyone 👋

I am facing errors from Webmock and VCR where specs fail with an unhandled web request error. e.g. UnhandledHTTPRequestError or NetConnectNotAllowedError. During my investigation I noticed that those unhandled web requests are caused by the download! method. This method gets called if an external_url is present. Which is here the case.

Question

My goal is to have an offline test suite, therefore, I was a bit surprised about this behavior, since I set enable_processing to false. I solved the offline approach by mocking all requests:

  1. Prevent IP resolving -> Overwrite the SsrfFilter::DEFAULT_RESOLVER https://github.com/arkadiyt/ssrf_filter/issues/33#issuecomment-1488800717
  2. Mock the external_url URI request -> webmock/vcr

Currently, I am unsure if this is the best approach or if carrierwave should handle this automatically depending on the enable_processing setting. Especially since setting enable_processing to false causes an early exit anyway.

rajyan commented 1 year ago

I'm not sure with the best practices of testing carrierwave too, but I'm currently disabling ssrf_filter by configuring skip_ssrf_protection? with a custom downloader only for test https://github.com/carrierwaveuploader/carrierwave/blob/8815592942f6c768d432dc6398a441f0c4f6118b/lib/carrierwave/downloader/base.rb#L96

and stubbing the external_url

Maybe some kind of configuration like https://github.com/carrierwaveuploader/carrierwave/pull/2575/files seems reasonable?

Do you have any opinions? @mshibuya

These are related issues

https://github.com/arkadiyt/ssrf_filter/issues/33 https://github.com/arkadiyt/ssrf_filter/issues/59 https://github.com/carrierwaveuploader/carrierwave/issues/2573