Avoid tracking logins from Devise::Strategies::Rememberable

[y9v]

What does this PR do?
In Devise integration Devise::Strategies::Authenticatable#validate is patched to track login successes and failures.

This method is not only used by Devise::Strategies::DatabaseAuthenticatable, but also by Devise::Strategies::Rememberable that is used for signing in the user using a remember token cookie.

Motivation:
This should reduce the amount of false-positive Login Success events for Devise AppSec integration.

Additional Notes:
I added an alias method for validate to avoid calling super from Rememberable strategy, since the super method is the validate from prepended AuthenticatablePatch.

How to test the change?
I tested this in a simple rails application with devise.
To test that regular login tracking works it's enough to log in via the sign-in form and observe a users.login.success tag on a trace.

To test that logins from Devise::Strategies::Rememberable strategy are not tracked:

• Log in via the sign-in form with checked "Remember Me" checkbox
• Go to any page that requires user authentication and delete the session cookie from Developer Tools
• Reload the page, the user will still be logged in, and the session cookie should reappear
• Observe that the trace was not tagged with users.login.success

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 86.48649% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@63423f9). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
lib/datadog/appsec/contrib/devise/patcher.rb	37.50%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #3867   +/-   ##
=========================================
  Coverage          ?   97.77%           
=========================================
  Files             ?     1286           
  Lines             ?    77059           
  Branches          ?     3807           
=========================================
  Hits              ?    75343           
  Misses            ?     1716           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-09-18 13:26:08

Comparing candidate commit fa16b64 in PR branch appsec-avoid-tracking-devise-rememberable-logins with baseline commit 63423f9 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics.

[ivoanjo]

Feel free to ignore the failing memory leaks test, fix is in #3868

[p-datadog]

I suggest using a prefix/postfix that indicates the purpose and owner of this alias, e.g. something along the lines of __validate_datadog or __validate_datadog_authenticatable. With just a double-underscore prefix there is potential for collisions with other code and it's not clear what the purpose of the method is to someone who might be looking at it in their application for whatever reason (and has no knowledge of dd-trace-rb internals).

[y9v]

Thank you, I renamed the alias in f2fe86a

[lloeki]

Not fixable, but oh my, I hate autoloading so much :(

[y9v]

yeah, especially when autoloaded classes require other classes/modules

[lloeki]

This looks dangerous, as mixing aliasing and prepending can easily lead to obscure issues such as recursive loops.

AIUI the reason this was introduced is that the call stack is:

Foo#validate    # some app class that included ::Devise::Strategies::Rememberable and may implement validate that calls `super` or simply rely on it being provided by a parent
::Devise::Strategies::Rememberable#validate                                   # hypothetical, not there upstream as of yet
::Datadog::AppSec::Contrib::Devise::Patcher::AuthenticatablePatch#validate
::Devise::Strategies::Authenticatable#validate

But just prepending a patch results in this:

Foo#validate    # some app class that included ::Devise::Strategies::Rememberable and may implement validate that calls `super` or simply rely on it being provided by a parent
::Datadog::AppSec::Contrib::Devise::Patcher::RememberablePatch#validate
::Devise::Strategies::Rememberable#validate                                   # hypothetical, not there upstream as of yet
::Datadog::AppSec::Contrib::Devise::Patcher::AuthenticatablePatch#validate
::Devise::Strategies::Authenticatable#validate

But we want to skip some/all of ::Datadog::AppSec::Contrib::Devise::Patcher::AuthenticatablePatch#validate, so this attempts to walk around it with:

Foo#validate    # some app class that included ::Devise::Strategies::Rememberable and may implement validate that calls `super` or simply rely on it being provided by a parent
::Datadog::AppSec::Contrib::Devise::Patcher::RememberablePatch#validate
::Devise::Strategies::Authenticatable#__validate_datadog_authenticatable

The problems are:

• suddenly ::Devise::Strategies::Authenticatable#validate is not called at all (the method body is, but not the name). This can cause issues: a third party instrumenting ::Devise::Strategies::Authenticatable#validate as well would never be called, or if ::Devise::Strategies::Authenticatable#validate's source code calls super it will actually look for __validate_datadog_authenticatable in a parent.
• Between ::Datadog::AppSec::Contrib::Devise::Patcher::RememberablePatch#validate and ::Devise::Strategies::Authenticatable#__validate_datadog_authenticatable the ancestor chain of calls is broken: Ruby "reenters" the instance and starts a new ancestor chain of calls, which creates the opportunity for infinite recursive looping.

I would favour a method where the patches collaborate through context passing: RememberablePatch sets a flag in a context, and AuthenticatablePatch checks for that flag.

[y9v]

Thank you! It is indeed better to use some variable for context. Fixed in 8e60850

[Strech]

Like it!