pipes-digital / pipes

Repository for Pipes
https://pipes.digital
GNU Affero General Public License v3.0
254 stars 21 forks source link

Login issues #84

Closed anewuser closed 6 months ago

anewuser commented 2 years ago

I've been getting blank pages on Firefox and this error message on Edge sometimes after trying to log in to Pipes lately:

login issues

I was able to access my account yesterday, but the error is back now.

onli commented 2 years ago

Hi! I can confirm - right now I could not log in with Firefox, but it worked just fine with Chromium. Last time I debugged this issue it looked like the browser forgot the session during the login process. I'm not sure how to solve this, will have to investigate options.

anewuser commented 2 years ago

Thanks. You could add a temporary notice for Firefox users to use a Chromium browser for the meantime.

it worked just fine with Chromium.

I was able to use it on Edge now. Maybe I got that internal server error at that time because I had just tried logging in multiple times with Firefox.

onli commented 2 years ago

I believe to have it fixed now. Will describe it a bit verbose, because there is not much information out there. But the short of it is: Please try it again, but delete your local cookies in Firefox. Confirmation much welcomed!


The long of it: I'm certain this is not a bug in Pipes. We do nothing special, Pipes simply enables the sinatra session and expects to set a nonce that is then available after the redirect to the portier broker.

However, this is what happened:

127.0.0.1 - - [30/Jan/2022:17:04:58 +0100] "GET /favicon-16x16.png HTTP/1.1" 200 4521 0.0011
not authorized
get plan: undefined method `[]' for nil:NilClass
{"session_id"=>"ca8ab14432e537d88160c22a91ea940720d6f44404bd53816242e5a661687651", "tracking"=>{"HTTP_USER_AGENT"=>"9c583c86e69f8a08...eaf5b896"}, "redirect_url"=>"http://localhost:9292/", "nonce"=>"0otCxkp17HndmtBfqi2ieg=="}
127.0.0.1 - - [30/Jan/2022:17:05:04 +0100] "GET /mainlogin HTTP/1.1" 200 2955 0.0046
{"session_id"=>"f64a6005ac71f92cf246410f0848e5267322ea1d4de38d0c59b6ba82cfe964f2", "tracking"=>{"HTTP_USER_AGENT"=>"9c583c86e69f8a08...eaf5b896"}}
127.0.0.1 - - [30/Jan/2022:17:05:09 +0100] "POST /_browserid_assert HTTP/1.1" 403 - 0.1288

This is the log with p session added to the portier gem. When coming back from the broker, without any additional output the sessionid changed and our nonce is gone. Without the nonce, the portier gem will not confirm the otherwise valid login cert (at POST /_browserid_assert).

I have no clear response on why this happens. I found two things though: An unanswered stackoverflow question that goes in the same direction, https://stackoverflow.com/questions/67746571/rack-session-key-not-persisting-after-redirecting, and a closed rack issue about session cookies not working in the current version 2.2.3, https://github.com/rack/rack/issues/1682 and https://github.com/rack/rack/issues/1674.

This gave me the idea to downgrade rack to 2.2.2. Indeed, that fixed it. Now, this setup works again:

set :protection, except: [:http_origin, :remote_token] 

configure do
    enable :sessions
    set :session_secret, ENV.fetch('SESSION_SECRET') { SecureRandom.hex(64) }
    set :session_store, Rack::Session::Pool
    ...
end 

I think this is simply a bug in rack, or potentially a bug in how Sinatra is using rack. I will open bug reports, but that issues like this can exist unreported makes me doubt that this is still a reasonable tech stack - maybe it's time to consider alternatives. We'll see how stable this stays now.

anewuser commented 2 years ago

After several login attempts with my main profile and different settings/extensions (most of which failed), I've found this pattern:

onli commented 2 years ago

For me it works when I delete the cookies before going to the login page Edit: Once. On my dev setup this now again does not work, but nothing changed to yesterday when it did. That's impossible!

I wonder whether setting a new session might be a workaround I could add to the code. In any case, you are right: This still does not work always in Firefox, I was mistaken. Thank you for testing!

dentarg commented 2 years ago

I think it would be interesting to see what broker.portier.io is POST:ing back to https://www.pipes.digital/_browserid_assert

As an example, trying to sign-in with foo@bar as the email also gives Internal Server Error:

Screenshot 2022-02-01 at 00 49 54

Using dev tools like above you can also see what cookies are sent on every request. I've played around in Firefox (with a valid login too) but not managed to get any blank page.

@onli what Ruby version is pipes.digital running on? I tried to decoded the cookie on various Ruby versions but I always got TypeError: incompatible marshal file format (can't be read)

I used this script Use it like: `echo | ./decode_rack_cookie` ```ruby #!/usr/bin/env ruby puts "Loading gems..." require "base64" require "bundler/inline" gemfile do source "https://rubygems.org" gem "rack", require: "rack/session/abstract/id" end def full_message(e) return e.full_message if e.respond_to?(:full_message) "#{e.inspect}\n#{e.backtrace.join("\n")}" end def print_decode_cookie(cookie_value) return if cookie_value.nil? puts puts "Decoded cookie:" puts puts Marshal.load(Base64.decode64(cookie_value)) # rubocop:disable Security/MarshalLoad rescue StandardError => e warn "Invalid cookie value! Error:" warn full_message(e) end puts "All gems loaded. Exit with Ctrl-C (^C)" puts if $stdin.stat.pipe? print_decode_cookie(STDIN.read.chomp) exit end begin loop do puts "Paste the cookie value and press Enter:" print_decode_cookie($stdin.gets) puts end rescue Interrupt exit end ```
dentarg commented 2 years ago

I tried to decoded the cookie on various Ruby versions but I always got

Ah, for some reason the cookie value was very short, 64 chars. Not sure why I got such cookies? Now I get real rack.session cookies, that are much longer (454 chars) and looks like they should, and it works to decode them.

onli commented 2 years ago

@dentarg I find it incredibly hard to debug this properly. I make changes (like just now I provided a different and stable session secret) and it works, I write a comment to explain that maybe this was it, test it again and Firefox refused to login, despite working before five times in a row, both locally and on the server. In short: I'm very thankful for any help you can provide.

I think it would be interesting to see what broker.portier.io is POST:ing back to https://www.pipes.digital/_browserid_assert

This should be easier to debug when providing a real email, ideally a gmail to make the login faster. https://github.com/portier/portier-broker is the broker that runs there, it will provide different data then. The broker does an extensive address check and will not continue if it thinks it's not valid. The data that it is supposed to provide is a jwt at params[:id_token], which will be read at https://github.com/portier/sinatra-portier/blob/fa259e4d2503c483d38f2d6f8b21ac807b295572/lib/sinatra/browserid.rb#L49;

@onli what Ruby version is pipes.digital running on?

ruby 2.7.0. I'm testing locally with ruby 3.0.3, which does not seem to change anything.

dentarg commented 2 years ago

Yeah, I've tested with a valid email too. And I was actually reading the code in https://github.com/portier/sinatra-portier before. :-)

I make changes (like just now I provided a different and stable session secret) and it works, I write a comment to explain that maybe this was it, test it again and Firefox refused to login, despite working before five times in a row, both locally and on the server

How did it fail? Did you inspect it with dev tools? I was actually rate limited by broker.portier.io when I tried with a@a as email one too many times, but I got a nice message about that. Perhaps they have other protections too.

onli commented 2 years ago

How did it fail?

It's always failing the same way for me: The login redirects to the broker, the broker redirects me to _browserid_assert, but that page stays blank instead of redirecting me further. It stays blank because the session id changed and the nonce is gone -> the portier token check fails.

Did you inspect it with dev tools?

I'm checking the session content with a p session inside the portier gem. Also with the dev tools the storage tab shows that the value of the rack.session cookie did change.

I was actually rate limited by broker.portier.io when I tried with a@a as email one too many times, but I got a nice message about that.

Same. It should always be clear about that :)

onli commented 2 years ago

Maybe something to add: When I launched Pipes, I ran into a very similar problem. Back then it was chrome that did not work, and the session got nuked by something in rack-protection. That's why two of these measures are still disabled (even in the old version, https://github.com/pipes-digital/pipes/blob/38e7dbd4a3c2452b47611f5c78d652318ee88c15/server.rb#L87). Is there a new one that got introduced recently that could get triggered here, and would they nuke the session without any log output?

I tried to disable rack-protection completely, but https://github.com/sinatra/sinatra/issues/1685 blocks that.

onli commented 2 years ago

This should be solved now, actually by giving up on the session nonce.

I pushed https://github.com/portier/sinatra-portier/commit/08aa5617d60b8afe699db9cae8ea604ba9541ae7 and added it to pipes. The commit stored the nonce in an in-memory cache at the server, avoiding the session nonce. This approach stems from the awesome Portier maintainer https://github.com/stephank.

In my tests it worked perfectly now, in both Chrome and Firefox. I hope desperately that this will still hold up tomorrow morning. @anewuser, could you try to confirm again?

@dentarg The Firefox/Chrome discrepancy and that the rack downgrade did not really help makes me think that this might be primarily or uniquely a browser issue. On the other hand, it's still possible that something like rack-protection is eating the session again, and that sinatra might have some influence here. I really think that this issue might still bite in other situations than just the specific one Pipes ran into. So if I can support with the sinatra issue please let me know - from providing a minimal test case to updating this repo to contain the prior broken and the (hopefully) new fixed app state. In any case, thanks a lot for the aid already provided.

anewuser commented 2 years ago

Thank you, it seems to be fixed! I was able to log in successfully at different times.

dentarg commented 2 years ago

I'm checking the session content with a p session inside the portier gem. Also with the dev tools the storage tab shows that the value of the rack.session cookie did change.

If it doesn't change it sounds like it is sending the session cookie without the nonce?

The Firefox/Chrome discrepancy and that the rack downgrade did not really help makes me think that this might be primarily or uniquely a browser issue.

Yes, I think so too, for some reasons, in some situations, Firefox is sending the old session cookie (without the nonce)

if I can support with the sinatra issue please let me know - from providing a minimal test case

I think the Sinatra issue should be closed, as I don't think the session is lost. Providing the code that exhibits this issue would be good, it is hard to explain it without seeing the actual code. I am interested in figuring it out (eventually, when I have the time) but that's hard when I don't have the complete code.

onli commented 2 years ago

If it doesn't change it sounds like it is sending the session cookie without the nonce?

Right, I think. It's initializing a new session, so the nonce is not part of it anymore.

Firefox is sending the old session cookie (without the nonce)

It's generating a new one here, the id is different :/

I think the Sinatra issue should be closed, as I don't think the session is lost.

But the session is lost. After the redirect the nonce is gone since the old session is gone. I'll create a minimal example for you today, let's hope it triggers the bug as well.

onli commented 2 years ago

The example app in the sinatra-portier gem is a perfect test case I forgot we had. https://github.com/sinatra/sinatra/issues/1742#issuecomment-1032886213 explains the small details.

dentarg commented 2 years ago

The example app in the sinatra-portier gem is a perfect test case I forgot we had. sinatra/sinatra#1742 (comment) explains the small details.

@onli just tried it and it works for me in Firefox 96.0.3 (using macOS). I used a Gmail address. Is there some special settings to touch in Firefox?

(I checked out https://github.com/portier/sinatra-portier/commit/ded3b6a94e9920d58e8800ed85d6913c06b0f011 and pointed my Gemfile to it, in-case I would need to debug. An idea would be to push tags to the git repo corresponding to the RubyGems releases.)

onli commented 2 years ago

Okay, that narrows its down - it's the Enhanced Tracking Protection. :/ I must have messed up when I tried to test without that. Also ruled it out since it happened to multiple users and since it does not match the description of what the tracking protection does. Set it to custom and the "cross site tracking cookies" rule kills us.

This solves a mystery for me. Thank you for your involvement here!

dentarg commented 2 years ago

Is there a new one that got introduced recently that could get triggered here, and would they nuke the session without any log output?

Recently I don't know but SessionHijacking is on by default and looks to "drop session" without logging that (https://github.com/sinatra/sinatra/blob/v2.1.0/rack-protection/lib/rack/protection.rb#L51)

onli commented 2 years ago

Maybe that interacts with the FF setting somehow?

Because the strange thing is that I definitely disabled tracking protection when testing for pipes.digital, via the shield at the top left. And it 100% did not work. So while I can completely reliably (currently) break this on the example site via the enhanced tracking protection in Firefox, I'm still not sure that this is the only answer.

So SessionHijacking is a great candidate for having played a role here. There really is no log?

dentarg commented 2 years ago

I'm not able to break the example app even when I try different things with the Enhanced Tracking Protection 🤔

dentarg commented 2 years ago

There really is no log?

Nope, see https://github.com/sinatra/sinatra/blob/v2.1.0/rack-protection/lib/rack/protection/session_hijacking.rb#L15 and https://github.com/sinatra/sinatra/blob/v2.1.0/rack-protection/lib/rack/protection/base.rb#L89-L91 – if you run locally you can add your own puts I guess. I haven't done this as I haven't seen it break yet.

onli commented 2 years ago

These are the settings that break it for me:

session_breaking_small

When I change this to standard or strict the example works. FF 96.0.2.

(To re-open the issue was a misclick, but I'll leave this open for a bit for better visibility).

onli commented 6 months ago

Since I got no new reports about login issues in a while now, and also experienced none myself, I will close here again.