Warn on dropping sessions #1900
Conversation
jdelStrother
force-pushed
the
session-hijack-logging
branch
from
476ed1f to
166295e
Compare
|
Oh sorry - turns out we do. I'll add one for SessionHijacking too |
jdelStrother
force-pushed
the
session-hijack-logging
branch
2 times, most recently
from
1d0cbc4 to
d2f2633
Compare
|
Looks like only You can configure the Oh, Line 1767 in 8ec83e3 Will this be too much noise in production? 🤔 Any thoughts from @sinatra/sinatras-helpers? |
|
@dentarg yeah I'm not sure which way to go on this. I can see how it would be nice to get this warning, but it also could potentially be very noisy. I suppose all rack protection logging being configured to use its own file would be a decent option tho if that is a problem. |
| session(env).clear if session? env | ||
| return unless session? env | ||
|
|
||
| warn env, "session dropped by #{self.class}" |
There was a problem hiding this comment.
I think we need to make it easy to silence this warning. Maybe via an ENV?
WDYT @jkowens ?
There was a problem hiding this comment.
That sounds like a really good idea 👍
There was a problem hiding this comment.
@jdelStrother any interest in continuing here?
There was a problem hiding this comment.
I don't really get why dropping sessions might be a noisy warning that needs silencing, but deny and report warnings are fine and should always be printed. Could you clarify?
If we're adding an ENV var, any thoughts on naming? RACK_PROTECTION_WARN_ON_DROPPED_SESSIONS=0 ?
There was a problem hiding this comment.
because it is the default in Sinatra
We can warn by default but there should be an easy way to get back the previous behavior (no warning)
So it should be named "silence ..." something. The RACK_PROTECTION prefix is a good call 👍🏻
There was a problem hiding this comment.
You don't think it should just silence all warnings, along the lines of this ?
diff --git a/rack-protection/lib/rack/protection/base.rb b/rack-protection/lib/rack/protection/base.rb
index 6f7e81fb..1ffc37c6 100644
--- a/rack-protection/lib/rack/protection/base.rb
+++ b/rack-protection/lib/rack/protection/base.rb
@@ -10,7 +10,7 @@ module Rack
module Protection
class Base
DEFAULT_OPTIONS = {
- reaction: :default_reaction, logging: true,
+ reaction: :default_reaction, logging: ENV["RACK_PROTECTION_SILENCE_REACTIONS"] != "1",
message: 'Forbidden', encryptor: Digest::SHA1,
session_key: 'rack.session', status: 403,
allow_empty_referrer: true,
There was a problem hiding this comment.
No, I think it should be specific to drop session
There was a problem hiding this comment.
OK, see what you think to the latest push.
jdelStrother
force-pushed
the
session-hijack-logging
branch
from
d2f2633 to
a91bd23
Compare
Co-authored-by: Patrik Ragnarsson <patrik@starkast.net>
jdelStrother
force-pushed
the
session-hijack-logging
branch
from
b1865c3 to
56c669e
Compare
It took me quite a while to discover that the reason my sessions would suddenly get dropped was from rack-protection's SessionHijacking. How about warning when that happens (in common with the other reactions like
denyandreport) ?