-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop dependency on secure_headers, fix response headers #15712
Drop dependency on secure_headers, fix response headers #15712
Conversation
One minor downside to this approach is that the “secure” cookie might be sent over HTTP even without going through the Tor Hidden Service, if such a service is configured in one of the alternate domains. Such a misconfiguration is highly unlikely though, and it's slightly better than sending the same cookie with the “secure” flag stripped, anyway. cc @cohosh @dr-bonez @jtracey I don't have a Tor Hidden Service set up to try that out, would be cool if any of you could confirm it works as intended |
I have a onion service test deployment I can try this out on but it won't be until later tonight (about 3 hours from now). |
Converted back to a draft as the initializer is called too late for the |
Instead of using LOCAL_DOMAIN, etc, why not decide based on the |
Because that would require a middleware or monkey-patching, the config option rails use is set at application boot. |
The monkey-patching would be really minimal, but would touch a private method of module ActionDispatch
module CookieJarExtensions
def write_cookie?(cookie)
request.headers["Host"].ends_with?('.onion') || super
end
end
end
ActionDispatch::Cookies::CookieJar.prepend(ActionDispatch::CookieJarExtensions) I'm open to both solutions. |
558305d
to
d5f69c6
Compare
Pushed the monkey-patch which should work as well. I have no strong preference to either alternative, the monkey-patching might be cleaner functionality-wise, but involves patching a private method. |
It looks like this isn't quite working. I deployed this to my hidden service instance, and tried to sign up a user, and I'm getting:
The browser I'm using does recognize it as a secure context. |
My config uses |
I finally got around to testing this and it's failing for me too :/ looking into it now... |
Can you check in the network monitor whether the |
Yeah I did that and I'm not seeing the cookies being sent still. I even set
in the monkey patch and it didn't work. After more debugging, it looks like the monkey patch is getting called, but that the onion service is throwing the 422 error before it even gets to the patch. I do think the monkey patch was necessary to get this working but it seems like there's something else going on as well. |
Okay I just tracked down more information. There are three different cookies Mastodon uses: When I turn off the secure flag for only Here's the code for how I got it working, btw (just for debugging purposes, I know we don't want to actually do this):
|
Okay I found it! We need another monkey patch for this function: When I patch that, everything works :) I'll clean this up and submit another PR. |
* Drop dependency on secure_headers, use always_write_cookie instead * Fix cookies in Tor Hidden Services by moving configuration to application.rb * Instead of setting always_write_cookie at boot, monkey-patch ActionDispatch
Alternate take at fixing #15560 without adding a middleware or additional dependency.
This should set it so “secure” cookies are sent if an “.onion” domain is configured as either
LOCAL_DOMAIN
,WEB_DOMAIN
or any of theALTERNATE_DOMAINS
, without modifying the cookie itself.