search

philnash / pwned

😱 An easy, Ruby way to use the Pwned Passwords API.
https://rubygems.org/gems/pwned/
MIT License
424 stars 22 forks source link

Fix crash b/c of nil + … #18

Closed flori closed 4 years ago

flori commented 4 years ago

Apparently chunk_lines can sometimes become an empty array and last_line ends up being nil causing an exception on this string addition. Convert string or nil to string before adding to be sure.

philnash commented 4 years ago

Oh, did this happen to you?

last_line is set by calling chunk_lines.pop. I can only imagine you get nil out of this if last_line + chunk becomes the empty string because then "".lines is[]and[].popisnil`.

I tried to write a test to cover this case, by returning an empty response. That caused

     # NoMethodError:
     #   undefined method `start_with?' for nil:NilClass
     #   ./lib/pwned/password.rb:102:in `block in fetch_pwned_count'

But this fix didn't sort it. Instead, trying this on line 153 sorted it:

last_line = chunk_lines.pop || ''

Are you able to write a test to reproduce this and prove your fix sorts it?

flori commented 4 years ago

Yes, this happened on our server. I suppose that the pop returned nil, and on the next iteration it caused the error when adding to nil. So the last yield (I have overlooked that one) was never called, but now it is. It's weird that the block of stream_response_lines is always called at least once, even if there weren't actually any lines returned. Maybe it would be cleaner to initialize last_line with nil and only call it afterwards if there actually was still a remaining last_line to be processed.

flori commented 4 years ago

I have added a test for this, and apparently what triggers this problem is, if read_body yields an empty chunk twice.

philnash commented 4 years ago

Thanks for the test! Will get this merged and released ASAP.

philnash commented 4 years ago

Merged and released as v2.0.2. Thanks again!