From 50be611f01ba137e89ce8d68c7beb42f36c1d845 Mon Sep 17 00:00:00 2001 From: Lewis Buckley Date: Mon, 8 Dec 2025 20:57:01 +0000 Subject: [PATCH 1/5] Allow configuring SSL in production with ENV vars --- config/environments/production.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/environments/production.rb b/config/environments/production.rb index 7b29a00d1..86b66b968 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -54,10 +54,10 @@ Rails.application.configure do # Assume all access to the app is happening through a SSL-terminating reverse proxy. # Can be used together with config.force_ssl for Strict-Transport-Security and secure cookies. - config.assume_ssl = true + config.assume_ssl = ENV.fetch("ASSUME_SSL", "true") == "true" # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. - config.force_ssl = true + config.force_ssl = ENV.fetch("FORCE_SSL", "true") == "true" # Log to STDOUT by default config.logger = ActiveSupport::Logger.new(STDOUT) From 8eb01da05769fd6de39f0c95bfa17fd76fc71d9a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 8 Dec 2025 15:26:42 -0800 Subject: [PATCH 2/5] Process blob variants using local files Rails does post-upload variant processing using the same file upload in https://github.com/rails/rails/pull/56327 Unrelated Lexxy bump: Pulls in https://github.com/basecamp/lexxy/pull/493 to work around `dom_id` removal from ActionText tag in https://github.com/rails/rails/pull/51238 --- Gemfile | 5 +++-- Gemfile.lock | 15 ++++++++++----- Gemfile.saas.lock | 15 ++++++++++----- config/initializers/active_storage.rb | 7 +++---- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/Gemfile b/Gemfile index 7f7a463bb..e8a5f8514 100644 --- a/Gemfile +++ b/Gemfile @@ -1,8 +1,9 @@ source "https://rubygems.org" -gem "rails", github: "rails/rails", branch: "main" git_source(:bc) { |repo| "https://github.com/basecamp/#{repo}" } +gem "rails", github: "rails/rails", branch: "ast-immediate-variants-process-locally" + # Assets & front end gem "importmap-rails" gem "propshaft" @@ -27,7 +28,7 @@ gem "rqrcode" gem "redcarpet" gem "rouge" gem "jbuilder" -gem "lexxy" +gem "lexxy", bc: "lexxy" gem "image_processing", "~> 1.14" gem "platform_agent" gem "aws-sdk-s3", require: false diff --git a/Gemfile.lock b/Gemfile.lock index e4f364c1c..ea4794b5a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,10 @@ +GIT + remote: https://github.com/basecamp/lexxy + revision: d292280b6d2c5d8a924327adf8bb9f0b25953539 + specs: + lexxy (0.1.23.beta) + rails (>= 8.0.2) + GIT remote: https://github.com/basecamp/useragent revision: 433ca320a42db1266c4b89df74d0abdb9a880c5e @@ -6,8 +13,8 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 - branch: main + revision: bf81d40a91880c059ce9d0447219385a2c8e4a15 + branch: ast-immediate-variants-process-locally specs: actioncable (8.2.0.alpha) actionpack (= 8.2.0.alpha) @@ -237,8 +244,6 @@ GEM logger (~> 1.6) letter_opener (1.10.0) launchy (>= 2.2, < 4) - lexxy (0.1.23.beta) - rails (>= 8.0.2) lint_roller (1.1.0) logger (1.7.0) loofah (2.24.1) @@ -518,7 +523,7 @@ DEPENDENCIES jbuilder kamal letter_opener - lexxy + lexxy! mission_control-jobs mittens mocha diff --git a/Gemfile.saas.lock b/Gemfile.saas.lock index f093ac677..aabe04258 100644 --- a/Gemfile.saas.lock +++ b/Gemfile.saas.lock @@ -41,6 +41,13 @@ GIT yabeda-puma-plugin yabeda-rails (>= 0.10) +GIT + remote: https://github.com/basecamp/lexxy + revision: d292280b6d2c5d8a924327adf8bb9f0b25953539 + specs: + lexxy (0.1.23.beta) + rails (>= 8.0.2) + GIT remote: https://github.com/basecamp/queenbee-plugin revision: 15faf03a876c5e66b67753d2e1ddb24f1eb5abb2 @@ -75,8 +82,8 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 - branch: main + revision: bf81d40a91880c059ce9d0447219385a2c8e4a15 + branch: ast-immediate-variants-process-locally specs: actioncable (8.2.0.alpha) actionpack (= 8.2.0.alpha) @@ -313,8 +320,6 @@ GEM logger (~> 1.6) letter_opener (1.10.0) launchy (>= 2.2, < 4) - lexxy (0.1.23.beta) - rails (>= 8.0.2) lint_roller (1.1.0) logger (1.7.0) loofah (2.24.1) @@ -653,7 +658,7 @@ DEPENDENCIES jbuilder kamal letter_opener - lexxy + lexxy! mission_control-jobs mittens mocha diff --git a/config/initializers/active_storage.rb b/config/initializers/active_storage.rb index 4a4a0343d..74461c795 100644 --- a/config/initializers/active_storage.rb +++ b/config/initializers/active_storage.rb @@ -11,10 +11,9 @@ end # before the User model's upload callback, causing FileNotFoundError when # using `process: :immediately` for variants. # See: https://github.com/rails/rails/issues/53694 -# -# ActiveSupport.on_load(:active_storage_record) do -# configure_replica_connections -# end +ActiveSupport.on_load(:active_storage_record) do + configure_replica_connections +end module ActiveStorageControllerExtensions extend ActiveSupport::Concern From bec46e15a97f7533c2ee1562a82e7c9f3603296e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 9 Dec 2025 00:36:39 -0800 Subject: [PATCH 3/5] Drop defunct Account upload attachments (#2030) Controller and routes removed in 6a62df470cfcdacbe193716344a3b26bbb766fba --- app/models/account.rb | 2 - app/views/uploads/create.json.jbuilder | 4 - .../convert_markdowns_to_rich_text.rb | 115 ------------------ 3 files changed, 121 deletions(-) delete mode 100644 app/views/uploads/create.json.jbuilder delete mode 100755 script/migrations/convert_markdowns_to_rich_text.rb diff --git a/app/models/account.rb b/app/models/account.rb index 61a636adb..0bb4d4111 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -10,8 +10,6 @@ class Account < ApplicationRecord has_many :columns, dependent: :destroy has_many :exports, class_name: "Account::Export", dependent: :destroy - has_many_attached :uploads - before_create :assign_external_account_id after_create :create_join_code diff --git a/app/views/uploads/create.json.jbuilder b/app/views/uploads/create.json.jbuilder deleted file mode 100644 index 984afba49..000000000 --- a/app/views/uploads/create.json.jbuilder +++ /dev/null @@ -1,4 +0,0 @@ -json.message "File uploaded successfully" -json.fileName @upload.filename.to_s -json.mimetype @upload.content_type -json.fileUrl @upload.slug_url diff --git a/script/migrations/convert_markdowns_to_rich_text.rb b/script/migrations/convert_markdowns_to_rich_text.rb deleted file mode 100755 index e093730f6..000000000 --- a/script/migrations/convert_markdowns_to_rich_text.rb +++ /dev/null @@ -1,115 +0,0 @@ -#!/usr/bin/env ruby - -require_relative "../config/environment" -require "redcarpet" -require "nokogiri" - -class ActionText::Markdown < ApplicationRecord - belongs_to :record, polymorphic: true -end - -class MarkdownToActionTextConverter - ATTACHMENT_URL_REGEX = %r{/u/(?[^\/\s\)]+)} - - def initialize(html) - @doc = Nokogiri::HTML::DocumentFragment.parse(html) - @attachments = [] - end - - def convert - process_images - process_links - [ @doc.to_html, @attachments ] - end - - private - def process_images - @doc.css("img").each do |img| - src = img["src"].presence - if src && match = src.match(ATTACHMENT_URL_REGEX) - if (attachment = find_attachment(match[:slug])) - img.replace(build_attachment_node(attachment)) - @attachments << attachment - end - end - end - end - - def process_links - @doc.css("a").each do |link| - href = link["href"].presence - - if href && match = href.match(ATTACHMENT_URL_REGEX) - if (attachment = find_attachment(match[:slug])) - link.replace(build_attachment_node(attachment)) - @attachments << attachment - end - end - end - end - - def build_attachment_node(attachment) - html = ActionText::Attachment.from_attachable(attachment).to_html - fragment = Nokogiri::HTML::DocumentFragment.parse(html) - - node = fragment.at_css("action-text-attachment") - node["url"] = Rails.application.routes.url_helpers.rails_blob_path(attachment.blob, only_path: true) - - fragment - end - - def find_attachment(slug) - ActiveStorage::Attachment.find_by(slug: slug) - end -end - -class RedcarpetRenderer - def self.render(markdown) - renderer = Redcarpet::Render::HTML.new - markdowner = Redcarpet::Markdown.new(renderer, - autolink: true, - tables: true, - fenced_code_blocks: true, - strikethrough: true, - superscript: true, - ) - markdowner.render(markdown.to_s) - end -end - -def process_all(klass, field) - klass.find_each do |record| - markdown = ActionText::Markdown.find_by(record: record, name: field) - next unless markdown - - puts "markdown.id=#{markdown.id}" - next unless markdown.record - - html = RedcarpetRenderer.render(markdown.content.to_s) - converter = MarkdownToActionTextConverter.new(html) - rich_text_html, attachments = converter.convert - - rich_text = ActionText::RichText.create!( - name: markdown.name, - record: markdown.record, - body: rich_text_html - ) - - attachments.each do |attachment| - attachment.update!(record: rich_text) - end - - puts "✓ Created rich text for #{markdown.record_type}##{markdown.record_id} (#{markdown.name})" - rescue => e - warn "✗ Failed to process markdown ##{markdown.id}: #{e.class} - #{e.message}" - end -end - -ApplicationRecord.with_each_tenant do |tenant| - puts "Processing tenant: #{tenant}" - - ActionText::RichText.delete_all - - process_all(Card, :description) - process_all(Comment, :body) -end From ea8a78cc2294cc6825c5fc1ec479a3f81689c182 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 9 Dec 2025 01:01:03 -0800 Subject: [PATCH 4/5] Fix stale-read race when creating push subscriptions (Caught one such uniqueness exception in the wild) * 200 OK -> 204 No Content, default status * No need to touch the subscription when found * Drop superflous test --- .../users/push_subscriptions_controller.rb | 8 +------- .../push_subscriptions_controller_test.rb | 20 +------------------ 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index bf5e5f87e..b511a6b19 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -5,13 +5,7 @@ class Users::PushSubscriptionsController < ApplicationController end def create - if subscription = @push_subscriptions.find_by(push_subscription_params) - subscription.touch - else - @push_subscriptions.create! push_subscription_params.merge(user_agent: request.user_agent) - end - - head :ok + @push_subscriptions.create_with(user_agent: request.user_agent).create_or_find_by!(push_subscription_params) end def destroy diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb index c41d8313e..4b4dd0a09 100644 --- a/test/controllers/users/push_subscriptions_controller_test.rb +++ b/test/controllers/users/push_subscriptions_controller_test.rb @@ -14,30 +14,12 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest post user_push_subscriptions_path(users(:david)), params: { push_subscription: subscription_params }, headers: { "HTTP_USER_AGENT" => "Mozilla/5.0" } - assert_response :ok + assert_response :no_content assert_equal subscription_params, users(:david).push_subscriptions.last.attributes.slice("endpoint", "p256dh_key", "auth_key") assert_equal "Mozilla/5.0", users(:david).push_subscriptions.last.user_agent end - test "touch existing subscription" do - existing_subscription = users(:david).push_subscriptions.create!( - endpoint: "https://fcm.googleapis.com/fcm/send/abc123", - p256dh_key: "123", - auth_key: "456" - ) - - assert_no_difference -> { users(:david).push_subscriptions.count } do - assert_changes -> { existing_subscription.reload.updated_at } do - post user_push_subscriptions_path(users(:david)), params: { - push_subscription: existing_subscription.attributes.slice("endpoint", "p256dh_key", "auth_key") - } - end - end - - assert_response :ok - end - test "destroy a push subscription" do subscription = users(:david).push_subscriptions.create!( endpoint: "https://fcm.googleapis.com/fcm/send/abc123", From 29c7926307f55028ac9380d893371938928657c1 Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Tue, 9 Dec 2025 11:25:12 +0100 Subject: [PATCH 5/5] Stream response in webhook request manually to check size This addresses a DoS vulnerability where the response might be massive leading to OOM errors, as the response is read in full in memory by default. To prevent this, we need to read the body in chunks, checking the size of the chunks we've read and raising if we go over a certain limit. I've set the limit to 100 KB because the responses to these requests should be fairly small or even empty, and we only care about the status code in the end. --- app/models/webhook/delivery.rb | 21 ++++++++++++++++--- test/models/webhook/delivery_test.rb | 31 +++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/app/models/webhook/delivery.rb b/app/models/webhook/delivery.rb index c5dab1497..3ebe49b06 100644 --- a/app/models/webhook/delivery.rb +++ b/app/models/webhook/delivery.rb @@ -1,7 +1,10 @@ class Webhook::Delivery < ApplicationRecord + class ResponseTooLarge < StandardError; end + STALE_TRESHOLD = 7.days USER_AGENT = "fizzy/1.0.0 Webhook" ENDPOINT_TIMEOUT = 7.seconds + MAX_RESPONSE_SIZE = 100.kilobytes belongs_to :account, default: -> { webhook.account } belongs_to :webhook @@ -52,12 +55,16 @@ class Webhook::Delivery < ApplicationRecord if resolved_ip.nil? { error: :private_uri } else - response = http.request( - Net::HTTP::Post.new(uri, headers).tap { |request| request.body = payload } - ) + request = Net::HTTP::Post.new(uri, headers).tap { |request| request.body = payload } + + response = http.request(request) do |net_http_response| + stream_body_with_limit(net_http_response) + end { code: response.code.to_i } end + rescue ResponseTooLarge + { error: :response_too_large } rescue Resolv::ResolvTimeout, Resolv::ResolvError, SocketError { error: :dns_lookup_failed } rescue Net::OpenTimeout, Net::ReadTimeout, Errno::ETIMEDOUT @@ -68,6 +75,14 @@ class Webhook::Delivery < ApplicationRecord { error: :failed_tls } end + def stream_body_with_limit(response) + bytes_read = 0 + response.read_body do |chunk| + bytes_read += chunk.bytesize + raise ResponseTooLarge if bytes_read > MAX_RESPONSE_SIZE + end + end + def resolved_ip return @resolved_ip if defined?(@resolved_ip) @resolved_ip = SsrfProtection.resolve_public_ip(uri.host) diff --git a/test/models/webhook/delivery_test.rb b/test/models/webhook/delivery_test.rb index 30f91390e..d4d95528c 100644 --- a/test/models/webhook/delivery_test.rb +++ b/test/models/webhook/delivery_test.rb @@ -296,12 +296,15 @@ class Webhook::DeliveryTest < ActiveSupport::TestCase stub_dns_resolution(PUBLIC_TEST_IP) # Verify Net::HTTP.new is called with the pinned IP + response_mock = stub(code: "200") + response_mock.stubs(:read_body) + http_mock = mock("http") http_mock.stubs(:use_ssl=) http_mock.stubs(:ipaddr=) http_mock.stubs(:open_timeout=) http_mock.stubs(:read_timeout=) - http_mock.stubs(:request).returns(stub(code: "200")) + http_mock.stubs(:request).yields(response_mock).returns(response_mock) Net::HTTP.expects(:new).with("example.com", 443).returns(http_mock) @@ -310,6 +313,32 @@ class Webhook::DeliveryTest < ActiveSupport::TestCase assert delivery.succeeded? end + test "handles response too large error" do + delivery = webhook_deliveries(:pending) + + large_body = "x" * 200.kilobytes + stub_request(:post, delivery.webhook.url).to_return(status: 200, body: large_body) + + delivery.deliver + + assert_equal "completed", delivery.state + assert_equal "response_too_large", delivery.response[:error] + assert_not delivery.succeeded? + end + + test "allows responses within size limit" do + delivery = webhook_deliveries(:pending) + + small_body = "x" * 50.kilobytes + stub_request(:post, delivery.webhook.url).to_return(status: 200, body: small_body) + + delivery.deliver + + assert_equal "completed", delivery.state + assert_equal 200, delivery.response[:code] + assert delivery.succeeded? + end + private def stub_dns_resolution(*ips) dns_mock = mock("dns")