Merge pull request #147 from basecamp/fix-sgid-decoding

Try to decodde SGIDs in multiple ways

app/assets/stylesheets/buttons.css

@@ -127,6 +127,13 @@
     }
   }
 
+  &:has(input[type=checkbox][disabled]) {
+    cursor: not-allowed;
+    filter: grayscale(100%) contrast(0.5);
+    opacity: 0.5;
+    pointer-events: none;
+  }
+
   [disabled] &[type=submit],
   &[type=submit]:disabled {
     cursor: not-allowed;

app/controllers/accounts_controller.rb

@@ -3,7 +3,9 @@ class AccountsController < ApplicationController
   before_action :set_account
 
   def edit
-    set_page_and_extract_portion_from account_users.ordered, per_page: 500
+    users = account_users.ordered.without_bots
+    @administrators, @members = users.partition(&:administrator?)
+    set_page_and_extract_portion_from users, per_page: 500
   end
 
   def update

app/controllers/first_runs_controller.rb

@@ -11,6 +11,8 @@ class FirstRunsController < ApplicationController
     user = FirstRun.create!(user_params)
     start_new_session_for user
 
+    redirect_to root_url
+  rescue ActiveRecord::RecordNotUnique
     redirect_to root_url
   end
 

app/controllers/rooms/involvements_controller.rb

@@ -1,8 +1,6 @@
 class Rooms::InvolvementsController < ApplicationController
   include RoomScoped
 
-  before_action :ensure_can_administer, only: %i[ update ]
-
   def show
     @involvement = @membership.involvement
   end

app/views/accounts/edit.html.erb

@@ -100,7 +100,13 @@
 
     <menu class="flex flex-column gap margin-none pad">
       <turbo-frame id="account_users">
-        <%= render partial: "accounts/users/user", collection: @page.records, as: :user %>
+        <%= render partial: "accounts/users/user", collection: @administrators, as: :user %>
+
+        <% if @administrators.any? && @members.any? %>
+          <hr class="separator full-width" style="--border-style: solid">
+        <% end %>
+
+        <%= render partial: "accounts/users/user", collection: @members, as: :user %>
         <%= render "accounts/users/next_page_container", page: @page.next_param unless @page.last? %>
       </turbo-frame>
     </menu>

app/views/accounts/users/_user.html.erb

@@ -9,23 +9,26 @@
 
   <hr class="separator" aria-hidden="true">
 
-  <% if Current.user.can_administer? && user != Current.user && user.active? %>
+  <% if Current.user.can_administer? && user.active? %>
     <% unless user.bot? %>
       <%= form_with model: user, url: account_user_path(user), data: { controller: "form" }, method: :patch do | form | %>
         <label class="btn txt-small flex-item-no-shrink" for="<%= dom_id(user, :role) %>">
           <span class="for-screen-reader">Role: <%= user.administrator? ? "Administrator" : "Member" %></span>
           <%= image_tag "crown.svg", size: 20, aria: { hidden: "true" } %>
-          <%= form.check_box :role, { data: { action: "form#submit" }, hidden: true, id: dom_id(user, :role) }, "administrator", "member" %>
+          <%= form.check_box :role, { data: { action: "form#submit" }, hidden: true, id: dom_id(user, :role), disabled: user == Current.user }, "administrator", "member" %>
         </label>
       <% end %>
     <% end %>
 
-    <%= button_to account_user_path(user), method: :delete, class: "btn txt-small flex-item-no-shrink btn--negative", data: {
-          turbo_confirm: "Are you sure you want to permanently remove this person from the account? This can’t be undone."} do %>
-      <%= image_tag "minus.svg", size: 20, aria: { hidden: "true" } %>
-      <span class="for-screen-reader">Delete <%= user.name %></span>
+    <% unless user == Current.user %>
+      <%= button_to account_user_path(user), method: :delete, class: "btn txt-small flex-item-no-shrink btn--negative", data: {
+            turbo_confirm: "Are you sure you want to permanently remove this person from the account? This can’t be undone."} do %>
+        <%= image_tag "minus.svg", size: 20, aria: { hidden: "true" } %>
+        <span class="for-screen-reader">Delete <%= user.name %></span>
+      <% end %>
     <% end %>
-  <% elsif user == Current.user %>
+  <% end %>
+  <% if user == Current.user %>
     <%= link_to user_profile_path, class: "btn txt-small flex-item-no-shrink", target: "_top" do %>
       <%= image_tag "pencil.svg", size: 20, aria: { hidden: "true" } %>
       <span class="for-screen-reader">My settings</span>

app/views/users/_ban_button.html.erb

@@ -9,7 +9,7 @@
   <%= button_to user_ban_path(user), method: :delete,
         class: "btn btn--negative full-width",
         data: { turbo_confirm: "Are you sure you want to remove the ban on this user?" } do %>
-    <%= image_tag "cancel.svg", aria: { hidden: "true", label: "Ban #{@user.name}" } %>
+    <%= image_tag "cancel.svg", aria: { hidden: "true", label: "Remove Ban #{@user.name}" } %>
     <span>Remove ban</span>
   <% end %>
 <% end %>

bin/release

@@ -30,6 +30,17 @@ EXPORT_DEPLOY_LOCATION = "/local/app/product_releases/"
 
 abort "You must specify a version, e.g. `bin/release 1.0.0`" unless VERSION
 
+current_branch = `git rev-parse --abbrev-ref HEAD`.strip
+if current_branch != "main"
+  print "\e[1;33mWarning: You are on branch '#{current_branch}', not 'main'. Continue? [y/N] \e[0m"
+  abort "Release aborted." unless $stdin.gets.strip.downcase.chars.first == "y"
+end
+
+unless `git status --porcelain`.strip.empty?
+  print "\e[1;33mWarning: You have uncommitted changes. Continue? [y/N] \e[0m"
+  abort "Release aborted." unless $stdin.gets.strip.downcase.chars.first == "y"
+end
+
 def announcing(message)
   puts "\n\e[1;36m#{message}\e[0m"
   yield

db/migrate/20251212154340_add_singleton_constraint_to_accounts.rb

@@ -0,0 +1,6 @@
+class AddSingletonConstraintToAccounts < ActiveRecord::Migration[8.2]
+  def change
+    add_column :accounts, :singleton_guard, :integer, default: 0, null: false
+    add_index :accounts, :singleton_guard, unique: true
+  end
+end

db/schema.rb

@@ -10,14 +10,16 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.2].define(version: 2025_11_26_130131) do
+ActiveRecord::Schema[8.2].define(version: 2025_12_12_154340) do
   create_table "accounts", force: :cascade do |t|
     t.datetime "created_at", null: false
     t.text "custom_styles"
     t.string "join_code", null: false
     t.string "name", null: false
     t.json "settings"
+    t.integer "singleton_guard", default: 0, null: false
     t.datetime "updated_at", null: false
+    t.index ["singleton_guard"], name: "index_accounts_on_singleton_guard", unique: true
   end
 
   create_table "action_text_rich_texts", force: :cascade do |t|

lib/rails_ext/action_text_attachables.rb

@@ -12,21 +12,32 @@ ActiveSupport.on_load(:action_text_content) do
 
         def attachable_from_possibly_expired_sgid(sgid)
           if message = sgid&.split("--")&.first
-            encoded_message = JSON.parse Base64.strict_decode64(message)
+            encoded_message = JSON.parse(decode_base64(message))
 
             decoded_gid = if data = encoded_message.dig("_rails", "data")
               data
+            elsif data = encoded_message.dig("_rails", "message")
+              # Rails 7 used an older format of GID that serialized the payload using Marshall
+              # Since we intentionally skip signature verification, we can't safely unmarshal the data
+              # To work around this, we manually extract the GID from the marshaled data
+              decode_base64(data).match(%r{(gid://campfire/[^/]+/\d+)})&.to_s
             else
               nil
             end
 
-            model = GlobalID.find(decoded_gid)
-
-            model.model_name.to_s.in?(ATTACHABLES_PERMITTED_WITH_INVALID_SIGNATURES) ? model : nil
+            if model = GlobalID.find(decoded_gid)
+              model.model_name.to_s.in?(ATTACHABLES_PERMITTED_WITH_INVALID_SIGNATURES) ? model : nil
+            end
           end
         rescue ActiveRecord::RecordNotFound
           nil
         end
+
+        def decode_base64(message)
+          Base64.strict_decode64(message)
+        rescue => _e
+          Base64.urlsafe_decode64(message)
+        end
     end
   end
 end

lib/restricted_http/private_network_guard.rb

@@ -16,7 +16,7 @@ module RestrictedHTTP
 
     def private_ip?(ip)
       IPAddr.new(ip).then do |ipaddr|
-        ipaddr.private? || ipaddr.loopback? || LOCAL_IP.include?(ipaddr)
+        ipaddr.private? || ipaddr.loopback? || ipaddr.link_local? || ipaddr.ipv4_mapped? || LOCAL_IP.include?(ipaddr)
       end
     rescue IPAddr::InvalidAddressError
       true

test/controllers/accounts_controller_test.rb

@@ -10,6 +10,38 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest
     assert_response :ok
   end
 
+  test "edit groups administrators separately from members with a divider" do
+    get edit_account_url
+
+    assert_response :ok
+
+    # Verify the divider exists between administrator and member sections
+    assert_select "turbo-frame#account_users hr.separator.full-width"
+
+    # Verify administrators appear before the divider and members appear after
+    # by checking the order of user names in the response body
+    administrators = users(:david, :jason).map(&:name)
+    members = users(:jz, :kevin).map(&:name)
+
+    response_body = response.body
+
+    # Find positions of divider and user names
+    divider_position = response_body.index('hr class="separator full-width"')
+    assert divider_position, "Divider should exist in the response"
+
+    administrators.each do |name|
+      name_position = response_body.index("<strong>#{name}</strong>")
+      assert name_position, "Administrator #{name} should appear in the response"
+      assert name_position < divider_position, "Administrator #{name} should appear before the divider"
+    end
+
+    members.each do |name|
+      name_position = response_body.index("<strong>#{name}</strong>")
+      assert name_position, "Member #{name} should appear in the response"
+      assert name_position > divider_position, "Member #{name} should appear after the divider"
+    end
+  end
+
   test "update" do
     assert users(:david).administrator?
 

test/controllers/first_runs_controller_test.rb

@@ -30,4 +30,28 @@ class FirstRunsControllerTest < ActionDispatch::IntegrationTest
 
     assert parsed_cookies.signed[:session_token]
   end
+
+  test "create is not vulnerable to race conditions" do
+    num_attackers = 5
+    url = first_run_url
+    barrier = Concurrent::CyclicBarrier.new(num_attackers)
+
+    num_attackers.times.map do |i|
+      Thread.new do
+        session = ActionDispatch::Integration::Session.new(Rails.application)
+        barrier.wait  # All threads wait here, then fire simultaneously
+
+        session.post url, params: {
+          user: {
+            name: "Attacker#{i}",
+            email_address: "attacker#{i}@example.com",
+            password: "password123"
+          }
+        }
+      end
+    end.each(&:join)
+
+    assert_equal 1, Account.count, "Race condition allowed #{Account.count} accounts to be created!"
+    assert_equal 1, User.where(role: :administrator).count, "Race condition allowed multiple admin users!"
+  end
 end

test/controllers/rooms/involvements_controller_test.rb

@@ -43,4 +43,13 @@ class Rooms::InvolvementsControllerTest < ActionDispatch::IntegrationTest
     end
     end
   end
+
+  test "a non-admin can update their room involvement" do
+    sign_in :jz
+
+    assert_changes -> { memberships(:jz_designers).reload.involvement }, from: "everything", to: "mentions" do
+      put room_involvement_url(rooms(:designers)), params: { involvement: "mentions" }
+      assert_redirected_to room_involvement_url(rooms(:designers))
+    end
+  end
 end

test/lib/rails_ext/action_text_attachables_test.rb

@@ -0,0 +1,39 @@
+require "test_helper"
+
+class ActionText::AttachmentTest < ActiveSupport::TestCase
+  setup do
+    @user = users(:david)
+  end
+
+  test "from_node" do
+    html = %Q(<action-text-attachment sgid="#{@user.attachable_sgid}"></action-text-attachment>)
+    node = ActionText::Fragment.wrap(html).find_all(ActionText::Attachment.tag_name).first
+
+    attachment = ActionText::Attachment.from_node(node)
+    assert_equal @user, attachment.attachable
+  end
+
+  test "from_node with a Rails 7 SGID" do
+    gid = @user.to_gid.to_s
+    marshaled_gid = Base64.urlsafe_encode64(Marshal.dump(gid))
+    rails7_payload = { "_rails" => { "message" => marshaled_gid, "exp" => nil, "pur" => "attachable" } }
+    rails7_message = Base64.strict_encode64(JSON.generate(rails7_payload))
+    rails7_sgid = "#{rails7_message}--invalidsignature"
+
+    html = %Q(<action-text-attachment sgid="#{rails7_sgid}"></action-text-attachment>)
+    node = ActionText::Fragment.wrap(html).find_all(ActionText::Attachment.tag_name).first
+
+    attachment = ActionText::Attachment.from_node(node)
+    assert_equal @user, attachment.attachable
+  end
+
+  test "from_node with an invalid SGID" do
+    room = rooms(:pets).tap { |r| r.extend ActionText::Attachable }
+
+    html = %Q(<action-text-attachment sgid="#{room.attachable_sgid}invalid"></action-text-attachment>)
+    node = ActionText::Fragment.wrap(html).find_all(ActionText::Attachment.tag_name).first
+
+    attachment = ActionText::Attachment.from_node(node)
+    assert_kind_of ActionText::Attachables::MissingAttachable, attachment.attachable
+  end
+end

test/models/opengraph/location_test.rb

@@ -21,6 +21,28 @@ class Opengraph::LocationTest < ActiveSupport::TestCase
     assert_equal [ "is not public" ], location.errors[:url]
   end
 
+  test "link-local addresses are blocked" do
+    Resolv.stubs(:getaddress).with("metadata.internal").returns("169.254.169.254")
+
+    location = Opengraph::Location.new("https://metadata.internal")
+    assert_not location.valid?
+    assert_equal [ "is not public" ], location.errors[:url]
+  end
+
+  test "IPv6 addresses mapped to IPv4 addresses are blocked" do
+    Resolv.stubs(:getaddress).with("metadata.internal").returns("::ffff:192.168.1.1")
+
+    location = Opengraph::Location.new("https://metadata.internal")
+    assert_not location.valid?
+    assert_equal [ "is not public" ], location.errors[:url]
+
+    Resolv.stubs(:getaddress).with("metadata.internal").returns("::ffff:c0a8:0101")
+
+    location = Opengraph::Location.new("https://metadata.internal")
+    assert_not location.valid?
+    assert_equal [ "is not public" ], location.errors[:url]
+  end
+
   test "avoid reading file urls when expecting HTML" do
     large_file = Opengraph::Location.new("https://www.example.com/100gb.zip")