Prevent strategy from running when the current path matches a dispatc… #60
Conversation
lib/warden/jwt_auth/strategy.rb
Outdated
| return false if path_is_dispatch_request_path? | ||
|
|
||
| token_exists? && issuer_claim_valid? |
There was a problem hiding this comment.
| return false if path_is_dispatch_request_path? | |
| token_exists? && issuer_claim_valid? | |
| token_exists? && issuer_claim_valid? && !path_is_dispatch_request_path? |
We can keep it as a one-liner. WDYT?
lib/warden/jwt_auth/strategy.rb
Outdated
| def path_is_dispatch_request_path? | ||
| current_path = EnvHelper.path_info(env) | ||
| dispatch_requests.any? do |tuple| | ||
| current_path.match(tuple.last) | ||
| end | ||
| end |
There was a problem hiding this comment.
Should we also check the request method?
There was a problem hiding this comment.
Yeah you're quite right here, if a user visits a dispatch request path via a GET for example, we want the strategy to actually run and indicate to devise that they are already logged in and trigger a redirect without inserting a fresh token into the headers. I will tweak my code so we also check the request method like it is done inside the hooks.
|
Also, please, could you rebase it from master? I just enabled GH actions to reenable CI here 🙏 |
k-p-jones
force-pushed
the
prevent-strat-from-running-on-dispatch-paths
branch
from
9763b96 to
52c528c
Compare
|
@waiting-for-dev thank you for your feedback! I have made the requested changes for you to take a look at and I have also rebased against master. |
|
I appear to have upset the linter. I will sort that out later today |
k-p-jones
force-pushed
the
prevent-strat-from-running-on-dispatch-paths
branch
from
52c528c to
f8bd7b9
Compare
|
@waiting-for-dev Linting errors should be fixed now |
|
Thanks!! |
|
Available in v0.11.0 |
Summary
As mentioned in the following issue waiting-for-dev/devise-jwt#274, it is possible for a client to hit the sign in endpoint of an app with a valid token and obtain a fresh one, which would allow an attacker to infinitely extend the period of time they have access after stealing a token, without having to provide valid credentials.
This PR attempts to address that issue by preventing the strategy from running if our current path matches any of the token dispatch request paths. Presumably the desired behaviour here is for the request to carry on cascading through the strategy chain until it either passes or fails authentication. This means that if a client provides correct credentials and a token, it will fall through and still grant them a new one via the
after_set_userhook.I'm unsure if this warrants a readme update, please let me know if it does.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):