Allow session cookie handling to work with localhost

[aburgel]

/domain @jmileham @samandmoore @smudge @effron
/no-platform

PublicSuffix currently blows up with a PublicSuffix::DomainInvalid exception if you attempt to parse localhost. This makes it impossible to run an app with this gem via rails s (since that starts a server on localhost:3000). This modifies session cookie handling to fallback to request.host if PublicSuffix is not able to parse the host.

It seems like we are intentionally relying on PublicSuffix to blow up to protect against invalid host names. I'm not sure we need this gem to provide that sort of security feature, so I'm removing it (since it gets in the way of this fix). But please push back if I'm missing something.

[nanda-prbot]

Needs somebody from @jmileham, @samandmoore, @smudge, and @effron to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

[samandmoore]

i don't quite follow:

It seems like we are intentionally relying on PublicSuffix to blow up to protect against invalid host names. I'm not sure we need this gem to provide that sort of security feature, so I'm removing it (since it gets in the way of this fix)

you are removing it, or you are not removing it?

i think @jmileham might have more valuable input on this PR than me. i would just like to see this work properly locally and also when the localhost is ipv6 which i've seen error with public suffix in other places before.

[jmileham]

PublicSuffix was integrated so we could reliably do wildcarding correctly - e.g. we www.foo.co.uk should wildcard to .foo.co.uk, not .co.uk. But I think we went overboard with the behavior (and those tests), when we added the non-wildcard flag. You're right. I see no exploit chain in passing a bogus Host header just so you can get a Set-Cookie header back with a malformed host name in it.

<< domain lgtm!

[nanda-prbot]

Approved! 👻 🎆 👍

[jmileham]

(feel free to bump version and release)

[aburgel]

Added an issue to replace PublicSuffix with some Rails builtins now that we no longer depend on its security properties #68

[aburgel]

you are removing it, or you are not removing it?

My bad. I meant I'm leaving the gem, but removing the bit of code that blows up on a bad Host header.

i would just like to see this work properly locally and also when the localhost is ipv6 which i've seen error with public suffix in other places before.

I think we correctly account for ipv6 addresses, since the regex we use to detect an ip also matches on ipv6. I haven't tested this though.