Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#signed_id outputs URL unsafe strings #49505

Closed
terracatta opened this issue Oct 5, 2023 · 0 comments · Fixed by #49507
Closed

#signed_id outputs URL unsafe strings #49505

terracatta opened this issue Oct 5, 2023 · 0 comments · Fixed by #49507

Comments

@terracatta
Copy link
Contributor

terracatta commented Oct 5, 2023

Today, calling Model.signed_id there is a chance of generating URL unsafe strings.

From my research, this has always been true, but I believe it may be more likely to happen due to changes in how ruby objects might be serialized after #47916 was merged (which IMO is a great change).

In my view, when @dhh introduced signed_ids he always intended them to be url safe.

Since the intent of signed_ids is to be included in things like URL params, we should be passing url_safe: true option to MessageVerifier.

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "~> 7.1.0"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Base.signed_id_verifier_secret = "foobar"

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
  end
end

class User < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def check_for_unsafe_base64_characters
    user = User.create!
    signed_id = user.signed_id(purpose: "~~~~~~~~~")
    # `+` is not a url-safe character
    assert !signed_id.include?("+")
  end
end

Expected behavior

user.signed_id should generate base64 strings that are url safe

Actual behavior

The + is included which is not safe for URLs.

System configuration

Rails version: 7.1.0

Ruby version: 3.2.2

Monkey Patch

Here is the patch I am using in my app that fixes the issue.

module ActiveRecord
  module SignedId
    module ClassMethods
      # Overriding this method because we want Rails to support signed_ids that are url_safe
      def signed_id_verifier
        @signed_id_verifier ||= begin
          secret = signed_id_verifier_secret
          secret = secret.call if secret.respond_to?(:call)

          if secret.nil?
            raise ArgumentError, "You must set ActiveRecord::Base.signed_id_verifier_secret to use signed ids"
          else
            ActiveSupport::MessageVerifier.new secret, digest: "SHA256", serializer: JSON, url_safe: true
          end
        end
      end
    end
  end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants