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 AC::Parameters not being sanitized for query methods. #21539

Merged
merged 1 commit into from Oct 2, 2015

Conversation

Projects
None yet
5 participants
@tgxworld
Contributor

tgxworld commented Sep 8, 2015

Actually do we need to sanitize inputs for query methods?

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Sep 8, 2015

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

Show outdated Hide outdated activerecord/lib/active_record/relation/query_methods.rb
@@ -42,6 +42,7 @@ def initialize(scope)
# User.where.not(name: "Jon", role: "admin")
# # SELECT * FROM users WHERE name != 'Jon' AND role != 'admin'
def not(opts, *rest)
opts = @scope.send(:sanitize_opts, opts)

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

We only need to do it on where!

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

We only need to do it on where!

This comment has been minimized.

@tgxworld

tgxworld Sep 8, 2015

Contributor

hmm we can't. where! is not called if it is called the following way: where.not(params).

@tgxworld

tgxworld Sep 8, 2015

Contributor

hmm we can't. where! is not called if it is called the following way: where.not(params).

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

where.not don't need to be sanitized, only where

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

where.not don't need to be sanitized, only where

This comment has been minimized.

@tgxworld

tgxworld Sep 9, 2015

Contributor

Why does where.not need not be sanitized? 💭

@tgxworld

tgxworld Sep 9, 2015

Contributor

Why does where.not need not be sanitized? 💭

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 9, 2015

Member

Because the arguments passed to where.not are not used to initialize the object when you do where.not(admin: true).first_or_create

@rafaelfranca

rafaelfranca Sep 9, 2015

Member

Because the arguments passed to where.not are not used to initialize the object when you do where.not(admin: true).first_or_create

This comment has been minimized.

@tgxworld

tgxworld Sep 9, 2015

Contributor

Got it 😄 Thanks!

@tgxworld

tgxworld Sep 9, 2015

Contributor

Got it 😄 Thanks!

Show outdated Hide outdated activerecord/test/cases/calculations_test.rb
@@ -681,4 +681,17 @@ def test_sum_uses_enumerable_version_when_block_is_given
end
assert block_called
end
def test_having_with_strong_parameters
params = ActionController::Parameters.new(credit_limit: '50')

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

We can't use ActionController here. Let just use the ProtectedParams but change it to not inherit from HWIA

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

We can't use ActionController here. Let just use the ProtectedParams but change it to not inherit from HWIA

This comment has been minimized.

@tgxworld

tgxworld Sep 8, 2015

Contributor

ok 👍

@tgxworld

tgxworld Sep 8, 2015

Contributor

ok 👍

@@ -558,11 +559,8 @@ def where(opts = :chain, *rest)
end
def where!(opts, *rest) # :nodoc:
if Hash === opts

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

if Hash === opts || opts.respond_to?(:permitted)

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

if Hash === opts || opts.respond_to?(:permitted)

This comment has been minimized.

@tgxworld

tgxworld Sep 8, 2015

Contributor

hmm do we need to sanitize if the input is a hash?

@tgxworld

tgxworld Sep 8, 2015

Contributor

hmm do we need to sanitize if the input is a hash?

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

We don't need to sanitize but we need to call references

@rafaelfranca

rafaelfranca Sep 8, 2015

Member

We don't need to sanitize but we need to call references

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
Member

rafaelfranca commented Oct 2, 2015

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Oct 2, 2015

r+ @schneems. This pull request was approved by one of the reviewers and it is waiting your decision

r+ @schneems. This pull request was approved by one of the reviewers and it is waiting your decision

@rails-bot rails-bot assigned schneems and unassigned rafaelfranca Oct 2, 2015

@rails-bot rails-bot added the Approved label Oct 2, 2015

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 2, 2015

Member

@tgxworld @schneems this was just a test.

Member

rafaelfranca commented Oct 2, 2015

@tgxworld @schneems this was just a test.

@rafaelfranca rafaelfranca removed the Approved label Oct 2, 2015

@rafaelfranca rafaelfranca assigned rafaelfranca and unassigned schneems Oct 2, 2015

@tgxworld

This comment has been minimized.

Show comment
Hide comment
@tgxworld

tgxworld Oct 2, 2015

Contributor

Ah I forgot about this. Let me get it sorted by this weekend. @rafaelfranca

Contributor

tgxworld commented Oct 2, 2015

Ah I forgot about this. Let me get it sorted by this weekend. @rafaelfranca

@tgxworld

This comment has been minimized.

Show comment
Hide comment
@tgxworld

tgxworld Oct 2, 2015

Contributor

@rafaelfranca Updated

Contributor

tgxworld commented Oct 2, 2015

@rafaelfranca Updated

sgrif added a commit that referenced this pull request Oct 2, 2015

Merge pull request #21539 from tgxworld/parameters_not_sanitized
Fix AC::Parameters not being sanitized for query methods.

@sgrif sgrif merged commit a9f66de into rails:master Oct 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 2, 2015

Member

❤️ 💚 💙 💛 💜

Member

rafaelfranca commented Oct 2, 2015

❤️ 💚 💙 💛 💜

@tgxworld tgxworld deleted the tgxworld:parameters_not_sanitized branch Jun 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment