Operations on Postgresql json/jsonb columns inadequately sanitised

[owenr]

If postgresql is configured with json or jsonb columns, searches are not properly parameterised or escaped. See bug report template gist.
Credit to @bronson for finding the issue.

[jaredbeck]

@owenr @bronson This should be fixed in master by #697 . Please test and we'll try to get a new release cut ASAP.

[bronson]

There looks to be an unrelated error preventing me from running very many iterations:

PaperTrailCleanerTest#test_: `clean_versions!` method No options provided should removes the earliest version(s). :
NoMethodError: undefined method `name' for nil:NilClass
    /Users/bronson/paper_trail/test/unit/cleaner_test.rb:34:in `block (4 levels) in <class:PaperTrailCleanerTest>'
    /Users/bronson/paper_trail/test/unit/cleaner_test.rb:34:in `map'
    /Users/bronson/paper_trail/test/unit/cleaner_test.rb:34:in `block (3 levels) in <class:PaperTrailCleanerTest>'
    shoulda-context (1.2.1) lib/shoulda/context/context.rb:413:in `instance_exec'
    shoulda-context (1.2.1) lib/shoulda/context/context.rb:413:in `block in create_test_from_should_hash'
    minitest (5.8.3) lib/minitest/test.rb:108:in `block (3 levels) in run'
    minitest (5.8.3) lib/minitest/test.rb:205:in `capture_exceptions'
    minitest (5.8.3) lib/minitest/test.rb:105:in `block (2 levels) in run'
    minitest (5.8.3) lib/minitest/test.rb:256:in `time_it'
    minitest (5.8.3) lib/minitest/test.rb:104:in `block in run'
    minitest (5.8.3) lib/minitest.rb:334:in `on_signal'
    minitest (5.8.3) lib/minitest/test.rb:276:in `with_info_handler'
    minitest (5.8.3) lib/minitest/test.rb:103:in `run'
    minitest (5.8.3) lib/minitest.rb:781:in `run_one_method'
    minitest (5.8.3) lib/minitest.rb:308:in `run_one_method'
    minitest (5.8.3) lib/minitest.rb:296:in `block (2 levels) in run'
    minitest (5.8.3) lib/minitest.rb:295:in `each'
    minitest (5.8.3) lib/minitest.rb:295:in `block in run'
    minitest (5.8.3) lib/minitest.rb:334:in `on_signal'
    minitest (5.8.3) lib/minitest.rb:321:in `with_info_handler'
    minitest (5.8.3) lib/minitest.rb:294:in `run'
    minitest (5.8.3) lib/minitest.rb:155:in `block in __run'
    minitest (5.8.3) lib/minitest.rb:155:in `map'
    minitest (5.8.3) lib/minitest.rb:155:in `__run'
    minitest (5.8.3) lib/minitest.rb:129:in `run'
    minitest (5.8.3) lib/minitest.rb:56:in `block in autorun'

Full output: https://gist.github.com/bronson/d597d03e7ca80ac82869

Until I can get 500 iterations, I won't be confident that the injection error is fixed.

I'm curious, why are the tests half minitest and half rspec?

[jaredbeck]

There looks to be an unrelated error preventing me from running very many iterations:

PaperTrailCleanerTest#test_: `clean_versions!` method No options provided should removes the  earliest version(s). :
NoMethodError: undefined method `name' for nil:NilClass
/Users/bronson/paper_trail/test/unit/cleaner_test.rb:34

Are you saying that test fails intermittently? Yeah, I agree, I don't think it's related. Could you open a separate issue for that?

I'm curious, why are the tests half minitest and half rspec?

The usual story, someone (before my time) started converting from one to the other and never finished.

[bronson]

Happy to, #699

Makes total sense. If the project had its choice, would it end up with 100% rspec or 100% minitest?

[jaredbeck]

If the project had its choice, would it end up with 100% rspec or 100% minitest?

They both have their good parts and bad parts. I tend to use rspec more, but I'd be happy with either. Either would be better than both.

[batter]

I'm the guilty party regarding why there are RSpec tests. I started converting the test suite over to RSpec since I was more comfortable with it but then didn't take the time to finish off everything. That being said I think there are actually some pros to having both within the project, since it makes it easier for people comfortable with either of those to contribute and write tests using their framework of choice. I too favor RSpec but I don't think minitest is bad by any means.

[owenr]

👍🏻 @jaredbeck
I added a test locally to ensure it still matches integers (it does).

[bronson]

Well, right now you're requiring authors to understand both frameworks... If you encourage them to use one, eventually the conversion should be complete. Allowing both and prolonging the conversion seems to me like a false economy.

Glad to know which framework is the desired destination, thanks!

[batter]

That may be true, however, the syntax that the 2 frameworks use are similar
enough so that if you understand how to use one of them, it doesn't take
much work to parse the other one, so it hasn't felt like a pressing need
compared to work on the actual functionality of the library itself.

If you would like to make a pull request that converts some of the tests
then I would be happy to review it :)

On Monday, January 18, 2016, Scott Bronson notifications@github.com wrote:

Well, right now you're requiring authors to understand both frameworks...
If you encourage them to use one, eventually the conversion should be
complete. Allowing both and prolonging the conversion seems to me like a
false economy.

Glad to know which framework is the desired destination, thanks!

—
Reply to this email directly or view it on GitHub
#696 (comment)
.

[bronson]

I totally agree that it's not worth slowing down until they're all converted. Just wanted to make sure that the idea is to end up on one or the other.

I'd like to give the tests a good scrub, especially some of these intermittent or first-run failures... maybe next week.

[jaredbeck]

It sounds like our preference is to end up with only RSpec. I agree with Ben, it's not a priority, but I'd be happy to review small, targeted PRs that convert a few tests at a time. If it helps, last year I wrote a cross-compiler that converts one test file at a time: https://github.com/jaredbeck/minitest_to_rspec Even a single file might be too big for a PR though. Some of those test files are huge.

I'd like to give the tests a good scrub, especially some of these intermittent or first-run failures...

That'd be great. Intermittent tests are a pain in the CI :p