mirror of
https://github.com/basecamp/once-campfire.git
synced 2026-10-10 16:50:08 +09:00
Don't let an http_proxy environment variable bypass the pinned address in link previews
Opengraph::Fetch connects to the IP it checked, not to a fresh lookup of the host. With http_proxy set, Net::HTTP would instead send the hostname to the proxy, which looks it up again, so the connection could end up somewhere other than the checked address. Pass nil as the proxy address, as web push delivery already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
committed by
Rosa Gutierrez
parent
b52b244f38
commit
a7e9ea53dd
@@ -23,11 +23,14 @@ class Opengraph::Fetch
|
||||
end
|
||||
|
||||
private
|
||||
# The nil proxy address ignores http_proxy: a proxy would look the host up again, and the connection
|
||||
# would no longer go to the checked IP.
|
||||
#
|
||||
# The timeouts bound each operation. A host can still send its headers or body a byte at a time,
|
||||
# in as many reads as it likes: UnfurlLinksController puts the whole unfurl under one deadline.
|
||||
def request(url, request_class, ip:)
|
||||
MAX_REDIRECTS.times do
|
||||
Net::HTTP.start(url.host, url.port, ipaddr: ip, use_ssl: url.scheme == "https", **timeouts) do |http|
|
||||
Net::HTTP.start(url.host, url.port, nil, ipaddr: ip, use_ssl: url.scheme == "https", **timeouts) do |http|
|
||||
http.request request_class.new(url) do |response|
|
||||
if response.is_a?(Net::HTTPRedirection)
|
||||
url, ip = resolve_redirect(response["location"])
|
||||
|
||||
@@ -74,6 +74,23 @@ class Opengraph::FetchTest < ActiveSupport::TestCase
|
||||
end
|
||||
end
|
||||
|
||||
test "#fetch_document connects to the resolved IP even when a proxy is configured" do
|
||||
url = URI.parse("http://www.example.com/")
|
||||
saved = ENV.slice("http_proxy", "HTTP_PROXY")
|
||||
%w[ http_proxy HTTP_PROXY ].each { |k| ENV[k] = "http://proxy.internal:3128" }
|
||||
|
||||
WebMock.disable_net_connect! allow: [ url.host ]
|
||||
TCPSocket.expects(:open).with { |*args, **| args.first == "proxy.internal" }.never
|
||||
TCPSocket.expects(:open).with { |*args, **| args.first == "1.2.3.4" && args[1] == 80 }.throws(:not_proxied)
|
||||
|
||||
assert_throws :not_proxied do
|
||||
@fetch.fetch_document(url, ip: "1.2.3.4")
|
||||
end
|
||||
ensure
|
||||
%w[ http_proxy HTTP_PROXY ].each { |k| ENV.delete(k) }
|
||||
saved.each { |k, v| ENV[k] = v }
|
||||
end
|
||||
|
||||
test "#fetch_document is empty following redirects that never finish" do
|
||||
WebMock.stub_request(:get, "https://www.example.com/")
|
||||
.to_return(status: 302, headers: { location: "https://www.example.com/" })
|
||||
|
||||
Reference in New Issue
Block a user