Add SSRF filter for HTTPX#23480
Conversation
da5859b to
e34c8c9
Compare
e34c8c9 to
e71ed4c
Compare
| expect(response).to have_http_status(:not_found) | ||
| expect(Query.exists?(target_query.id)).to be(true) | ||
| it "does not filter requests to public IP addresses" do | ||
| result = httpx.get(public_endpoint) |
There was a problem hiding this comment.
@klaustopher FYI: Added some tests as requested. Sadly we can't properly test this through webmock, so I am making real requests here.
If this becomes a source of test flicker we'll have to do something about it (I don't yet know what).
There was a problem hiding this comment.
We can add another test, where we put 127.0.0.1 in the allowlist. This should be done with the with_setting helper, That's the difference to the base gem that we allow the allowlist
There was a problem hiding this comment.
Working on it. Even though neither with_settings nor with_config seem to do the trick. Little excerpt from with_config:
[1] pry(#<RSpec::ExampleGroups::OpenProject::Httpx::SSRFFiltering::WhenLocalIPAddressesAreAllowed>)> OpenProject::Configuration[:ssrf_protection_ip_allowlist]
=> "127.0.0.1,::1"
[2] pry(#<RSpec::ExampleGroups::OpenProject::Httpx::SSRFFiltering::WhenLocalIPAddressesAreAllowed>)> OpenProject::Configuration.ssrf_protection_ip_allowlist
=> []So I am going to mock it like in spec/lib/open_project/ssrf_protection_spec.rb using allow(OpenProject::Configuration).to receive(:ssrf_protection_ip_allowlist)
e71ed4c to
c6bac07
Compare
Filtering in front of HTTPX calls is less secure, because it's vulnerable to DNS rebinding. In addition to that it's also duplicate work, because all affected callsites would have to make sure to "remember" SSRF filtering. This SSRF filter is inspired by the original HTTPX SSRF Filter, but using our custom IP address matcher that allows to configure safe IP addresses or ranges.
c6bac07 to
294611c
Compare
Filtering in front of HTTPX calls is less secure, because it's vulnerable to DNS rebinding. In addition to that it's also duplicate work, because all affected callsites would have to make sure to "remember" SSRF filtering.
This SSRF filter is inspired by the original HTTPX SSRF Filter, but using our custom IP address matcher that allows to configure safe IP addresses or ranges.
Ticket
none
Notes
I left the existing checks for
safe_ip?where they are, because they tend to add more meaningful error messages to the corresponding form than leaving them out.Without those the corresponding forms would be left alone with a request error from trying to run
httpx.get, which ranges from "error message is now less helpful" to "error case wasn't considered at all so far".It's worth noting that the HTTPX maintainer seems open to the idea of supporting additional requirements regarding SSRF directly in the gem. This PR here might be replaced with a native alternative, once one becomes available. See https://gitlab.com/os85/httpx/-/work_items/384