From fb019d1712359bfb09ec8af8de68dd37a750f332 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Thu, 16 Mar 2023 14:04:46 -0600 Subject: [PATCH] SECURITY: Multiple commits for Version bump 3.1.0.beta3 (#20707) * SECURITY: Fix XSS in full name composer reply We are using htmlSafe when rendering the name field so we need to escape any html being passed in. * SECURITY: Monkey-patch web-push gem to use safer HTTP client `FinalDestination::HTTP` is our patch of `Net::HTTP` which defend us against SSRF and DNS rebinding attacks. * SECURITY: SSRF protection bypass with IPv4-mapped IPv6 addresses As part of this commit, we've also expanded our list of private IP ranges based on https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml and https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml * SECURITY: XSS on chat excerpts Non-markdown tags weren't being escaped in chat excerpts. This could be triggered by editing a chat message containing a tag (self XSS), or by replying to a chat message with a tag (XSS). Co-authored-by: Jan Cernik * FIX: Escaped mentions in chat excerpts Mentions are now displayed as using the non-cooked message which fixes the problem. This is not ideal. I think we might want to rework how these excerpts are created and rendered in the near future. Co-authored-by: Jan Cernik * SECURITY: Add FinalDestination::FastImage that's SSRF safe --------- Co-authored-by: Alan Guo Xiang Tan Co-authored-by: Jan Cernik Co-authored-by: Ted Johansson --- .../discourse/app/components/choose-topic.hbs | 2 +- .../app/components/composer-action-title.js | 5 +- .../discourse/app/helpers/replace-emoji.js | 4 +- .../app/templates/modal/insert-hyperlink.hbs | 2 +- .../discourse/app/templates/review-topics.hbs | 2 +- .../tests/acceptance/composer-actions-test.js | 6 +- .../discourse/tests/fixtures/topic.js | 2 +- .../integration/helpers/replace-emoji-test.js | 29 ++++++++ .../addon/templates/components/topic-row.hbs | 2 +- lib/cooked_processor_mixin.rb | 2 +- lib/final_destination/fast_image.rb | 23 ++++++ lib/final_destination/ssrf_detector.rb | 58 +++++++++++---- lib/freedom_patches/web_push_request.rb | 27 +++++++ plugins/chat/app/models/chat_message.rb | 2 +- .../components/chat-channel-about-view.hbs | 2 +- .../components/chat-channel-card.hbs | 4 +- .../components/chat-channel-title.hbs | 2 +- plugins/chat/spec/models/chat_message_spec.rb | 18 ----- plugins/chat/spec/system/chat_channel_spec.rb | 28 ++++++++ spec/lib/final_destination/fast_image_spec.rb | 70 +++++++++++++++++++ .../final_destination/ssrf_detector_spec.rb | 16 ++++- spec/lib/freedom_patches/web_push_spec.rb | 60 ++++++++++++++++ spec/services/search_indexer_spec.rb | 2 +- 23 files changed, 315 insertions(+), 53 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js create mode 100644 lib/final_destination/fast_image.rb create mode 100644 lib/freedom_patches/web_push_request.rb create mode 100644 spec/lib/final_destination/fast_image_spec.rb create mode 100644 spec/lib/freedom_patches/web_push_spec.rb diff --git a/app/assets/javascripts/discourse/app/components/choose-topic.hbs b/app/assets/javascripts/discourse/app/components/choose-topic.hbs index f9bf06d223..15e4285039 100644 --- a/app/assets/javascripts/discourse/app/components/choose-topic.hbs +++ b/app/assets/javascripts/discourse/app/components/choose-topic.hbs @@ -30,7 +30,7 @@ /> - {{replace-emoji t.fancy_title}} + {{replace-emoji t.title}} {{bound-category-link diff --git a/app/assets/javascripts/discourse/app/components/composer-action-title.js b/app/assets/javascripts/discourse/app/components/composer-action-title.js index b4c35e6c9e..5e863b0995 100644 --- a/app/assets/javascripts/discourse/app/components/composer-action-title.js +++ b/app/assets/javascripts/discourse/app/components/composer-action-title.js @@ -12,6 +12,7 @@ import { alias } from "@ember/object/computed"; import discourseComputed from "discourse-common/utils/decorators"; import { iconHTML } from "discourse-common/lib/icon-library"; import { htmlSafe } from "@ember/template"; +import { escape } from "pretty-text/sanitizer"; const TITLES = { [PRIVATE_MESSAGE]: "topic.private_message", @@ -84,7 +85,9 @@ export default Component.extend({ }, _formatReplyToUserPost(avatar, link) { - const htmlLink = `${link.anchor}`; + const htmlLink = `${escape( + link.anchor + )}`; return htmlSafe(`${avatar}${htmlLink}`); }, }); diff --git a/app/assets/javascripts/discourse/app/helpers/replace-emoji.js b/app/assets/javascripts/discourse/app/helpers/replace-emoji.js index 3109cf2900..81d78a3a1f 100644 --- a/app/assets/javascripts/discourse/app/helpers/replace-emoji.js +++ b/app/assets/javascripts/discourse/app/helpers/replace-emoji.js @@ -1,7 +1,9 @@ import { emojiUnescape } from "discourse/lib/text"; -import { htmlSafe } from "@ember/template"; +import { htmlSafe, isHTMLSafe } from "@ember/template"; import { registerUnbound } from "discourse-common/lib/helpers"; +import { escapeExpression } from "discourse/lib/utilities"; registerUnbound("replace-emoji", (text, options) => { + text = isHTMLSafe(text) ? text.toString() : escapeExpression(text); return htmlSafe(emojiUnescape(text, options)); }); diff --git a/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs b/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs index affbf5d1b6..a69187ed8f 100644 --- a/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs +++ b/app/assets/javascripts/discourse/app/templates/modal/insert-hyperlink.hbs @@ -21,7 +21,7 @@ data-title={{result.fancy_title}} > - {{replace-emoji result.fancy_title}} + {{replace-emoji result.title}}
{{#if result.category.parentCategory}} {{category-link result.category.parentCategory}} diff --git a/app/assets/javascripts/discourse/app/templates/review-topics.hbs b/app/assets/javascripts/discourse/app/templates/review-topics.hbs index 286e391e0a..5bed189751 100644 --- a/app/assets/javascripts/discourse/app/templates/review-topics.hbs +++ b/app/assets/javascripts/discourse/app/templates/review-topics.hbs @@ -16,7 +16,7 @@ href={{rt.relative_url}} rel="noopener noreferrer" target="_blank" - >{{replace-emoji rt.fancy_title}} + >{{replace-emoji rt.title}}
diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-actions-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-actions-test.js index 35804af709..dcd0adea59 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-actions-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-actions-test.js @@ -518,11 +518,11 @@ acceptance("Prioritize Full Name", function (needs) { test("Reply to post use full name", async function (assert) { await visit("/t/short-topic-with-two-posts/54079"); - await click("article#post_2 button.reply"); + await click("article#post_3 button.reply"); assert.strictEqual( - query(".action-title .user-link").innerText.trim(), - "james, john, the third" + query(".action-title .user-link").innerHTML.trim(), + "<h1>Tim Stone</h1>" ); }); diff --git a/app/assets/javascripts/discourse/tests/fixtures/topic.js b/app/assets/javascripts/discourse/tests/fixtures/topic.js index fa37c17c5c..bbefba6ead 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/topic.js +++ b/app/assets/javascripts/discourse/tests/fixtures/topic.js @@ -6497,7 +6497,7 @@ export default { }, { id: 419, - name: "Tim Stone", + name: "

Tim Stone

", username: "tms", avatar_template: "/letter_avatar_proxy/v4/letter/t/3be4f8/{size}.png", uploaded_avatar_id: 40181, diff --git a/app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js b/app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js new file mode 100644 index 0000000000..2ce8e73be3 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/integration/helpers/replace-emoji-test.js @@ -0,0 +1,29 @@ +import { module, test } from "qunit"; +import { setupRenderingTest } from "discourse/tests/helpers/component-test"; +import { render } from "@ember/test-helpers"; +import { hbs } from "ember-cli-htmlbars"; + +module("Integration | Helper | replace-emoji", function (hooks) { + setupRenderingTest(hooks); + + test("it replaces the emoji", async function (assert) { + await render(hbs`{{replace-emoji "some text :heart:"}}`); + + assert.dom(`span`).includesText("some text"); + assert.dom(`.emoji[title="heart"]`).exists(); + }); + + test("it escapes the text", async function (assert) { + await render( + hbs`{{replace-emoji ""}}` + ); + + assert.dom(`span`).hasText(""); + }); + + test("it renders html-safe text", async function (assert) { + await render(hbs`{{replace-emoji (html-safe "safe text")}}`); + + assert.dom(`span`).hasText("safe text"); + }); +}); diff --git a/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs b/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs index d9b2074909..fc832e4c64 100644 --- a/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs +++ b/app/assets/javascripts/select-kit/addon/templates/components/topic-row.hbs @@ -1,5 +1,5 @@ -
{{replace-emoji this.item.fancy_title}}
+
{{replace-emoji this.item.title}}
{{bound-category-link this.item.category diff --git a/lib/cooked_processor_mixin.rb b/lib/cooked_processor_mixin.rb index 4947a9a02e..1ad4d74853 100644 --- a/lib/cooked_processor_mixin.rb +++ b/lib/cooked_processor_mixin.rb @@ -193,7 +193,7 @@ module CookedProcessorMixin if upload && upload.width && upload.width > 0 @size_cache[url] = [upload.width, upload.height] else - @size_cache[url] = FastImage.size(absolute_url) + @size_cache[url] = FinalDestination::FastImage.size(absolute_url) end rescue Zlib::BufError, URI::Error, OpenSSL::SSL::SSLError # FastImage.size raises BufError for some gifs, leave it. diff --git a/lib/final_destination/fast_image.rb b/lib/final_destination/fast_image.rb new file mode 100644 index 0000000000..615c7848db --- /dev/null +++ b/lib/final_destination/fast_image.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class FinalDestination::FastImage < ::FastImage + def initialize(url, options = {}) + uri = URI(normalized_url(url)) + options.merge!(http_header: { "Host" => uri.hostname }) + uri.hostname = resolved_ip(uri) + + super(uri.to_s, options) + rescue FinalDestination::SSRFDetector::DisallowedIpError, SocketError, Timeout::Error + super("") + end + + private + + def resolved_ip(uri) + FinalDestination::SSRFDetector.lookup_and_filter_ips(uri.hostname).first + end + + def normalized_url(uri) + UrlHelper.normalized_encode(uri) + end +end diff --git a/lib/final_destination/ssrf_detector.rb b/lib/final_destination/ssrf_detector.rb index d9953044de..7a3bf1f036 100644 --- a/lib/final_destination/ssrf_detector.rb +++ b/lib/final_destination/ssrf_detector.rb @@ -7,18 +7,47 @@ class FinalDestination class LookupFailedError < SocketError end - def self.standard_private_ranges - @private_ranges ||= [ - IPAddr.new("0.0.0.0/8"), - IPAddr.new("127.0.0.1"), - IPAddr.new("172.16.0.0/12"), - IPAddr.new("192.168.0.0/16"), - IPAddr.new("10.0.0.0/8"), - IPAddr.new("::1"), - IPAddr.new("fc00::/7"), - IPAddr.new("fe80::/10"), - ] - end + # This is a list of private IPv4 IP ranges that are not allowed to be globally reachable as given by + # https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml. + PRIVATE_IPV4_RANGES = [ + IPAddr.new("0.0.0.0/8"), + IPAddr.new("10.0.0.0/8"), + IPAddr.new("100.64.0.0/10"), + IPAddr.new("127.0.0.0/8"), + IPAddr.new("169.254.0.0/16"), + IPAddr.new("172.16.0.0/12"), + IPAddr.new("192.0.0.0/24"), + IPAddr.new("192.0.0.0/29"), + IPAddr.new("192.0.0.8/32"), + IPAddr.new("192.0.0.170/32"), + IPAddr.new("192.0.0.171/32"), + IPAddr.new("192.0.2.0/24"), + IPAddr.new("192.168.0.0/16"), + IPAddr.new("192.175.48.0/24"), + IPAddr.new("198.18.0.0/15"), + IPAddr.new("198.51.100.0/24"), + IPAddr.new("203.0.113.0/24"), + IPAddr.new("240.0.0.0/4"), + IPAddr.new("255.255.255.255/32"), + ] + + # This is a list of private IPv6 IP ranges that are not allowed to be globally reachable as given by + # https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml. + # + # ::ffff:0:0/96 is excluded from the list because it is used for IPv4-mapped IPv6 addresses which is something we want to allow. + PRIVATE_IPV6_RANGES = [ + IPAddr.new("::1/128"), + IPAddr.new("::/128"), + IPAddr.new("64:ff9b:1::/48"), + IPAddr.new("100::/64"), + IPAddr.new("2001::/23"), + IPAddr.new("2001:2::/48"), + IPAddr.new("2001:db8::/32"), + IPAddr.new("fc00::/7"), + IPAddr.new("fe80::/10"), + ] + + PRIVATE_IP_RANGES = PRIVATE_IPV4_RANGES + PRIVATE_IPV6_RANGES def self.blocked_ip_blocks SiteSetting @@ -54,10 +83,9 @@ class FinalDestination def self.ip_allowed?(ip) ip = ip.is_a?(IPAddr) ? ip : IPAddr.new(ip) + ip = ip.native - if ip_in_ranges?(ip, blocked_ip_blocks) || ip_in_ranges?(ip, standard_private_ranges) - return false - end + return false if ip_in_ranges?(ip, blocked_ip_blocks) || ip_in_ranges?(ip, PRIVATE_IP_RANGES) true end diff --git a/lib/freedom_patches/web_push_request.rb b/lib/freedom_patches/web_push_request.rb new file mode 100644 index 0000000000..ee6c23ce47 --- /dev/null +++ b/lib/freedom_patches/web_push_request.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +# This is a patch to avoid the direct use of `Net::HTTP` in the `webpush` gem and instead rely on `FinalDestination::HTTP` +# which protects us from DNS rebinding attacks as well as server side forgery requests. +# +# This patch is considered temporary until we can decide on a longer term solution. In the meantime, we need to patch +# the SSRF vulnerability being exposed by this gem. +module WebPushPatch + def perform + http = FinalDestination::HTTP.new(uri.host, uri.port, *proxy_options) + http.use_ssl = true + http.ssl_timeout = @options[:ssl_timeout] unless @options[:ssl_timeout].nil? + http.open_timeout = @options[:open_timeout] unless @options[:open_timeout].nil? + http.read_timeout = @options[:read_timeout] unless @options[:read_timeout].nil? + + req = FinalDestination::HTTP::Post.new(uri.request_uri, headers) + req.body = body + + resp = http.request(req) + verify_response(resp) + + resp + end +end + +klass = defined?(WebPush) ? WebPush : Webpush +klass::Request.prepend(WebPushPatch) diff --git a/plugins/chat/app/models/chat_message.rb b/plugins/chat/app/models/chat_message.rb index 13039d6421..be14d09e49 100644 --- a/plugins/chat/app/models/chat_message.rb +++ b/plugins/chat/app/models/chat_message.rb @@ -90,7 +90,7 @@ class ChatMessage < ActiveRecord::Base return uploads.first.original_filename if cooked.blank? && uploads.present? # this may return blank for some complex things like quotes, that is acceptable - PrettyText.excerpt(cooked, max_length, { text_entities: true }) + PrettyText.excerpt(message, max_length, { text_entities: true }) end def cooked_for_excerpt diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs index 77f999c9b6..29b3fa73c0 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-about-view.hbs @@ -31,7 +31,7 @@
- {{replace-emoji this.channel.escapedTitle}} + {{replace-emoji this.channel.title}}
{{this.channel.slug}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs index a7c4b39bb4..9b4d077b09 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-card.hbs @@ -15,7 +15,7 @@ class="chat-channel-card__name-container" > - {{replace-emoji @channel.escapedTitle}} + {{replace-emoji @channel.title}} {{#if @channel.chatable.read_restricted}} {{d-icon "lock" class="chat-channel-card__read-restricted"}} @@ -47,7 +47,7 @@ {{#if @channel.description}}
- {{replace-emoji @channel.escapedDescription}} + {{replace-emoji @channel.description}}
{{/if}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs index 5fb7fde988..b25ceb099f 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-title.hbs @@ -59,7 +59,7 @@ {{/if}} - {{replace-emoji this.channel.escapedTitle}} + {{replace-emoji this.channel.title}} {{#if (has-block)}} diff --git a/plugins/chat/spec/models/chat_message_spec.rb b/plugins/chat/spec/models/chat_message_spec.rb index de1f70540a..f12d2308e5 100644 --- a/plugins/chat/spec/models/chat_message_spec.rb +++ b/plugins/chat/spec/models/chat_message_spec.rb @@ -287,24 +287,6 @@ describe ChatMessage do COOKED ) expect(message.excerpt).to eq("https://twitter.com/EffinBirds/status/1518743508378697729") - message = - Fabricate.build( - :chat_message, - message: - "wow check out these birbs https://twitter.com/EffinBirds/status/1518743508378697729", - ) - expect(message.excerpt).to eq( - "wow check out these birbs https://twitter.com/Effi...", - ) - end - - it "returns an empty string if PrettyText.excerpt returns empty string" do - message = Fabricate(:chat_message, message: <<~MSG) - [quote="martin, post:30, topic:3179, full:true"] - This is a real **quote** topic with some *markdown* in it I can quote. - [/quote] - MSG - expect(message.excerpt).to eq("") end it "excerpts upload file name if message is empty" do diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index 455a5c17ac..b934db3ad8 100644 --- a/plugins/chat/spec/system/chat_channel_spec.rb +++ b/plugins/chat/spec/system/chat_channel_spec.rb @@ -189,6 +189,34 @@ RSpec.describe "Chat channel", type: :system, js: true do end end + context "when replying to message that has tags" do + fab!(:other_user) { Fabricate(:user) } + fab!(:message_2) do + Fabricate( + :chat_message, + user: other_user, + chat_channel: channel_1, + message: "not marked", + ) + end + + before do + Fabricate(:chat_message, user: other_user, chat_channel: channel_1) + Fabricate(:chat_message, in_reply_to: message_2, user: current_user, chat_channel: channel_1) + channel_1.add(other_user) + channel_1.add(current_user) + sign_in(current_user) + end + + it "escapes the reply-to line" do + chat.visit_channel(channel_1) + + expect(find(".chat-reply .chat-reply__excerpt")["innerHTML"].strip).to eq( + "<mark>not marked</mark>", + ) + end + end + context "when messages are separated by a day" do before do Fabricate(:chat_message, chat_channel: channel_1, created_at: 2.days.ago) diff --git a/spec/lib/final_destination/fast_image_spec.rb b/spec/lib/final_destination/fast_image_spec.rb new file mode 100644 index 0000000000..b04129ec60 --- /dev/null +++ b/spec/lib/final_destination/fast_image_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +describe FinalDestination::FastImage do + before do + # We need to test low-level stuff, switch off WebMock for FastImage + WebMock.enable!(except: [:net_http]) + Socket.stubs(:tcp).never + TCPSocket.stubs(:open).never + Addrinfo.stubs(:getaddrinfo).never + end + + after { WebMock.enable! } + + def expect_tcp_and_abort(stub_addr, &blk) + success = Class.new(StandardError) + TCPSocket.stubs(:open).with { |addr| stub_addr == addr }.once.raises(success) + begin + yield + rescue success + end + end + + def stub_ip_lookup(stub_addr, ips) + FinalDestination::SSRFDetector.stubs(:lookup_ips).with { |addr| stub_addr == addr }.returns(ips) + end + + def stub_tcp_to_raise(stub_addr, exception) + TCPSocket.stubs(:open).with { |addr| addr == stub_addr }.once.raises(exception) + end + + it "uses the first resolved IP" do + stub_ip_lookup("example.com", %w[1.1.1.1 2.2.2.2 3.3.3.3]) + expect_tcp_and_abort("1.1.1.1") do + FinalDestination::FastImage.size(URI("https://example.com/img.jpg")) + end + end + + it "ignores private IPs" do + stub_ip_lookup("example.com", %w[0.0.0.0 2.2.2.2]) + expect_tcp_and_abort("2.2.2.2") do + FinalDestination::FastImage.size(URI("https://example.com/img.jpg")) + end + end + + it "returns a null object when all IPs are private" do + stub_ip_lookup("example.com", %w[0.0.0.0 127.0.0.1]) + expect(FinalDestination::FastImage.size(URI("https://example.com/img.jpg"))).to eq(nil) + end + + it "returns a null object if all IPs are blocked" do + SiteSetting.blocked_ip_blocks = "98.0.0.0/8|78.13.47.0/24|9001:82f3::/32" + stub_ip_lookup("ip6.example.com", %w[9001:82f3:8873::3]) + stub_ip_lookup("ip4.example.com", %w[98.23.19.111]) + expect(FinalDestination::FastImage.size(URI("https://ip4.example.com/img.jpg"))).to eq(nil) + expect(FinalDestination::FastImage.size(URI("https://ip6.example.com/img.jpg"))).to eq(nil) + end + + it "allows specified hosts to bypass IP checks" do + SiteSetting.blocked_ip_blocks = "98.0.0.0/8|78.13.47.0/24|9001:82f3::/32" + SiteSetting.allowed_internal_hosts = "internal.example.com|blocked-ip.example.com" + stub_ip_lookup("internal.example.com", %w[0.0.0.0 127.0.0.1]) + stub_ip_lookup("blocked-ip.example.com", %w[98.23.19.111]) + expect_tcp_and_abort("0.0.0.0") do + FinalDestination::FastImage.size(URI("https://internal.example.com/img.jpg")) + end + expect_tcp_and_abort("98.23.19.111") do + FinalDestination::FastImage.size(URI("https://blocked-ip.example.com/img.jpg")) + end + end +end diff --git a/spec/lib/final_destination/ssrf_detector_spec.rb b/spec/lib/final_destination/ssrf_detector_spec.rb index 6d24a9ca3a..8479f8bf15 100644 --- a/spec/lib/final_destination/ssrf_detector_spec.rb +++ b/spec/lib/final_destination/ssrf_detector_spec.rb @@ -43,9 +43,19 @@ describe FinalDestination::SSRFDetector do expect(subject.ip_allowed?("9001:82f3:8873::3")).to eq(false) end - it "returns false for standard internal IPs" do - expect(subject.ip_allowed?("172.31.100.31")).to eq(false) - expect(subject.ip_allowed?("fd02:77fa:ffea::f")).to eq(false) + %w[0.0.0.0 10.0.0.0 127.0.0.0 172.31.100.31 255.255.255.255 ::1 ::].each do |internal_ip| + it "returns false for '#{internal_ip}'" do + expect(subject.ip_allowed?(internal_ip)).to eq(false) + end + end + + it "returns false for private IPv4-mapped IPv6 addresses" do + expect(subject.ip_allowed?("::ffff:172.31.100.31")).to eq(false) + expect(subject.ip_allowed?("::ffff:0.0.0.0")).to eq(false) + end + + it "returns true for public IPv4-mapped IPv6 addresses" do + expect(subject.ip_allowed?("::ffff:52.52.167.244")).to eq(true) end end diff --git a/spec/lib/freedom_patches/web_push_spec.rb b/spec/lib/freedom_patches/web_push_spec.rb new file mode 100644 index 0000000000..d516425837 --- /dev/null +++ b/spec/lib/freedom_patches/web_push_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +klass = defined?(WebPush) ? WebPush : Webpush + +RSpec.describe klass do + before do + FinalDestination::SSRFDetector.allow_ip_lookups_in_test! + WebMock.enable!(except: [:final_destination]) + end + + after do + WebMock.enable! + FinalDestination::SSRFDetector.disallow_ip_lookups_in_test! + end + + it "should filter endpoint hostname through our SSRF detector" do + klass::Request.any_instance.expects(:encrypt_payload) + klass::Request.any_instance.expects(:headers) + + stub_ip_lookup("example.com", %W[0.0.0.0]) + + expect do + klass.payload_send( + endpoint: "http://example.com", + message: "test", + p256dh: "somep256dh", + auth: "someauth", + vapid: { + subject: "someurl", + public_key: "somepublickey", + private_key: "someprivatekey", + }, + ) + end.to raise_error(FinalDestination::SSRFDetector::DisallowedIpError) + end + + it "should send the right request if endpoint hostname resolves to a public ip address" do + klass::Request.any_instance.expects(:encrypt_payload) + klass::Request.any_instance.expects(:headers) + + stub_ip_lookup("example.com", %W[52.125.123.12]) + + success = Class.new(StandardError) + TCPSocket.stubs(:open).with { |addr| "52.125.123.12" == addr }.once.raises(success) + + expect do + klass.payload_send( + endpoint: "http://example.com", + message: "test", + p256dh: "somep256dh", + auth: "someauth", + vapid: { + subject: "someurl", + public_key: "somepublickey", + private_key: "someprivatekey", + }, + ) + end.to raise_error(success) + end +end diff --git a/spec/services/search_indexer_spec.rb b/spec/services/search_indexer_spec.rb index 9e1b9a8870..13890c884c 100644 --- a/spec/services/search_indexer_spec.rb +++ b/spec/services/search_indexer_spec.rb @@ -220,7 +220,7 @@ RSpec.describe SearchIndexer do Jobs.run_immediately! SiteSetting.max_image_width = 1 - stub_request(:get, "https://meta.discourse.org/some.png").to_return( + stub_request(:get, "https://1.2.3.4/some.png").to_return( status: 200, body: file_from_fixtures("logo.png").read, )