Security sinks in Ruby on Rails Part 1 - XSS

Szymon Lipka — Thu, 06 Apr 2023 20:20:37 +0000

In Security, there are such concepts as sinks and sources. Sinks are dangerous functions in the code, whereas sources are arguments of the sinks.

In this case, there is a dangerous function eval and a source is params[:name].

In code reviews, I tend to search for such functions and analyze what can be their argument/source of data. This helps me identify potential vulnerabilities before they reach production.

I want to go with you through some basic sinks, that should turn on the yellow light.

Cross Site Scripting(XSS)

Let's go through Cross Site Scripting first. XSS is a vulnerability that allows execution scripts in browsers with malicious sources. More about them can be read here. The most popular ways of allowing this security issue are methods raw and html_safe. By default in Rails, everything that is being returned by <%= %> is being interpreted as a text, but when you use those methods, you tell that it can be interpreted as HTML.

For example, on code review you see a line in a .erb file:

raw t('user_name', name: user.name)

t('user_name', name: user.name).html_safe

# config/locales/en.yml
en:
  user_name: "<p>User name is %{name}</p>"

This is I would say common case because it's from my experience often that we want to introduce HTML into translation. It might result in XSS. user.name is a source and it shouldn't be user input. If it is, a malicious user might define it for example <script>alert(1)</script> and the alert pops at the time of rendering the erb file.

In general, I would avoid html_safe or raw methods, unless they are obligatory.

Sometimes you might say that you know that it is XSS, but it is Self-XSS(the person who can be affected by XSS is the person who writes the script). I don't encourage you to ignore such cases, because in the future there might be a different business logic, which would result in Self-XSS becoming XSS.

When it comes to XSS, it's very important to mention CSP - a policy that is a last resort against XSS. It's rather simple to implement it in Rails, docs you can find here, and more about the policy itself you can find here.

To be continued

Let's expect change and reload!

Szymon Lipka — Mon, 03 Apr 2023 17:44:09 +0000

It's often that we create a service that is supposed to change an attribute on an ActiveRecord object. Testing such a service is tricky at times.

Expect change

Let's say we have an incorrect service that is supposed to update
a name of a user in a database, which is looking like this:



class UpdateUserName
  def initialize(user)
    @user = user
  end

  def call
    # TODO: Will be done in close future, until 2200
  end

  private

  attr_reader :user
end

And we have a spec that is supposed to test it:



require 'rails_helper'

RSpec.describe UpdateUserName do
  it "should update user name" do
    user = create(:user)

    described_class.new(user).call

    expect(user.name).to eq "funny name"
  end
end

What happens when we run the spec?

Why has it passed you shall ask. The name of the user was always "funny name" although the call of service did not change anything, the spec passed.

We shall rewrite it to something like this:



require 'rails_helper'

RSpec.describe UpdateUserName do
  it "should update user name" do
    user = create(:user)

    expect { described_class.new(user).call }.to change(user, :name).to "funny name"
  end
end

Awesome! We've discovered an incorrect implementation with a spec!

Reload an object

Going forward with the same example of spec:



require 'rails_helper'

RSpec.describe UpdateUserName do
  it "should update user name" do
    user = create(:user, name: "not so funny name")

    expect { described_class.new(user).call }.to change(user, :name).to "funny name"
  end
end

but we have upgraded our service and it is looking like this:



class UpdateUserName
  def initialize(user)
    @user = user
  end

  def call
    user.name = "funny name"
  end

  private

  attr_reader :user
end

What happens if I run the test?

Is the service implemented correctly? In this particular case, we expect it to persist changes to database, so it is not correct, but spec is passing.

Let me fix the spec:



require 'rails_helper'

RSpec.describe UpdateUserName do
  it "should update user name" do
    user = create(:user, name: "not so funny name")

    expect { described_class.new(user).call }.to change { user.reload.name }.to "funny name"
  end
end

So now we've discovered incorrect implementation again! Let me fix the service:



class UpdateUserName
  def initialize(user)
    @user = user
  end

  def call
    user.name = "funny name"
    user.save
  end

  private

  attr_reader :user
end

Conclusion

Usually, when we want to test services, we should expect a change of an attribute and reload it to see if the change really happened or maybe it is only on the object passed.