If passed Range shorthand (duck typed), handle as text

[nacengineer]

We ran across a weird corner case when using the blacklight and blacklight_range_limit gems. Where if the text passed to sanitize gets duck typed to a range, then an error is raised.

The range will get past the initial short circuit because it's a valid object but then will fail on the empty? call because range doesn't have the method empty? on it.

I've added a few tests as best I could although I might have missed one or two. The modified the checks to handle a range by checking if it's an instance of Range and if so then calling to_s on it so that it can merrily pass along through the sanitization process.

Any input is appreciated.

[flavorjones]

Thanks for opening this PR to discuss what you're seeing.

I'm afraid I don't understand how or why a Range is being passed to the sanitizer, that seems like a bug in the code that's calling the sanitizer. Can you help me understand why you think this should be fixed in rails-html-sanitizer?

[nacengineer]

oh it's definitely a bug on the blacklight side but I think it highlights an edge case of the sanitization gem as what's being passed is actually just text that is a shorthand range format, e.g. "2001..2005", which is getting duck typed into a range in the santize gem and failing because it passes the truthy test (return unless html) but doesn't pass the empty? test because range doesn't have the empty? method.

[flavorjones]

@nacengineer Thanks for providing some additional context, but I still don't understand how or why a Range object is being passed to the sanitizer. Maybe if you provided a full reproduction to help me understand why you want to make this change?

Unless you can provide more information to change my mind, I would prefer if we either fixed this by performing a type check and raising an ArgumentError if the input isn't a String, (rather than try to add support for new types) or else just didn't fix it at all (this gem's API has remained unchanged since 2013, and it was extracted from a Rails API that was unchanged before that since at least 2008, so I'm not yet convinced this is really a problem).

[nacengineer]

It's not that a range object is being passed to sanitizer. What's being passed is text. It's an edge case where that text also happens to just be text that is a range shorthand, e.g. 2001..2005, so it gets duck typed into a range by ruby.

It will pass your truthy check here

https://github.com/rails/rails-html-sanitizer/pull/203/files#diff-0daf33b9062eb5ccdeae86ed8bf2662a6e8669f1a7db590802b7f3b36ea47426L36

and then fail your call to .empty? here because now it's a range now and empty? doesn't exist on Range

https://github.com/rails/rails-html-sanitizer/pull/203/files#diff-0daf33b9062eb5ccdeae86ed8bf2662a6e8669f1a7db590802b7f3b36ea47426L37

I guess if you're okay with that behaviour then you can close this and move on but that seems like an unhandled boundary condition to me. It's your gem and your call though. I just wanted to bring this to your attention. It reminds me of when you pull a boolean out of params in Rails and instead of 1 or true you get "1" or "true" except the other way. 🤣

[flavorjones]

I think we're talking past each other. Let me try again.

1. The test you wrote here is passing in a literal Range object.
2. There is no code in this gem that I know of that will "duck type" a String into a Range.

My conclusion is that the gem you're using is passing a Range into the sanitizer.

It's not that a range object is being passed to sanitizer

So now I'm confused. Please help me understand what's going on. You may want to provide a reproducible example of some kind that demonstrates the problem you're trying to solve.

[nacengineer]

Ah that's my bad on writing the test.

IIRC I tested it both as a passed range and as passed text and the same results. Let me revisit the PR tests I wrote later and get back to you.

I was a little uncertain as to which tests I should write for the differing methods that could potentially be called so maybe I got lost in the test sauce. I'll pull it back to just a limited number of test.