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

Fix sensitive params filtering #1755

Merged
merged 1 commit into from
Mar 16, 2017

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Mar 3, 2017

  • Use a regular expression to filter only parameters that start and end
    with sensitive keywords.
  • Remove useless assignment of filter_parameters in backend since it's
    already defined in core engine.

ref #1752

Not sure if it makes sense to add some specs here since we are just using the a rails configuration. I'm open to suggestions anyway.

- Use a regular expression to filter only parameters that start and end
  with sensitive keywords.
- Remove useless assignment of filter_parameters in backend since it's
  already defined in core engine.
Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. In my testing this did what was expected. Thanks for picking this issue up, I didn't realize the solution would be so simple.

It's really important that we do filter these values, so I'd like to test them if there's a reasonable way to do so. I'll be giving that some thought.

@kennyadsl
Copy link
Member Author

@jhawthorn I initially started writing something like this but wasn't sure about where is the best place to put it and if it's really useful:

# core/spec/controllers/filtered_parameters_spec.rb

require 'spec_helper'

RSpec.describe ApplicationController, type: :controller do
  controller do
    def index
      head :ok
    end
  end

  describe "Passing sensitive parameters" do
    subject { request.filtered_parameters }

    it "filters correctly" do
      get :index, params: {
        password: 'secret password',
        password_confirmation: 'secret password confirmation',
        number: 'secret credit card number',
        verification_value: 'secret verification value',
        name: 'something not secret',
        number_eq: 'R123456789',
        payment_number_eq: '123',
        answer_number: '42'
      }

      expect(subject["password"]).to eq '[FILTERED]'
      expect(subject["password_confirmation"]).to eq '[FILTERED]'
      expect(subject["number"]).to eq '[FILTERED]'
      expect(subject["verification_value"]).to eq '[FILTERED]'
      expect(subject["verification_value"]).to eq '[FILTERED]'

      expect(subject["name"]).not_to eq '[FILTERED]'
      expect(subject["number_eq"]).not_to eq '[FILTERED]'
      expect(subject["number_eq"]).not_to eq '[FILTERED]'
      expect(subject["answer_number"]).not_to eq '[FILTERED]'
    end
  end
end

I used an anonymous controller that inherits from ApplicationController since this test is not related to a specific controller; the configuration will be valid for the whole application.

My concern is that, if one day we'll change filtered_parameters configuration in frontend or backend engines this test will still be green. A possible solution could be duplicate this test in all engines that need parameters filtering. What do you think about it?

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any reasonable opinions on where and how to test these changes. I sort of lean toward a "really we're just setting a good default configuration for shops", and wouldn't consider testing them that thoroughly.

@jhawthorn jhawthorn merged commit 7e64336 into solidusio:master Mar 16, 2017
@jhawthorn
Copy link
Contributor

Thanks so much @kennyadsl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants