lynndylanhurley / lynndylanhurley/devise_token_auth

`set_user_token`: Incompatibility between the Authorization header and having a param clashing with the auth header names.

Open
#1,648 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Ruby
Stars
3.6k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

Hi,

There seems to be a bug in how we read the authentication attributes in [concerns/set_user_by_token.rb](https://github.com/lynndylanhurley/devise_token_auth/blob/aedef3ca68d4e69bdc4a3e8987fa9d5f45420eec/app/controllers/devise_token_auth/concerns/set_user_by_token.rb#L52).

On line 52 and after this is how it reads:
```ruby
uid = request.headers[uid_name] || params[uid_name] || parsed_auth_cookie[uid_name] || decoded_authorization_token[uid_name]
other_uid = other_uid_name && request.headers[other_uid_name] || params[other_uid_name] || parsed_auth_cookie[other_uid_name]
@token = DeviseTokenAuth::TokenFactory.new unless @token
@token.token ||= request.headers[access_token_name] || params[access_token_name] || parsed_auth_cookie[access_token_name] || decoded_authorization_token[access_token_name]
@token.client ||= request.headers[client_name] || params[client_name] || parsed_auth_cookie[client_name] || decoded_authorization_token[client_name]
```

The problem exists where: if someone uses the `decoded_authorization_token` values coming from the one `Authorization` header instead of relying on the other 4 headers, the order of reading values is interrupted by reading from the `params`.

As a result, if either a path, query or body param contains an attribute with a name clashing with the defined `client_name`, `uid_name` or `access_token_name`, this will get evaluated before we can even try to evaluate the `decoded_authorization_token` (and this would be the same with the cookie).

As a result, auth fails for these requests.

```
GET /api/protected/some_resource?client=123
Authorization: Bearer a3dxd.....
===> Fails because client = 123 instead of the value in the Bearer token.
```

Could we issue a fix where we read from `params` after any other option has been tried ?

```ruby
uid = request.headers[uid_name] || parsed_auth_cookie[uid_name] || decoded_authorization_token[uid_name] || params[uid_name]
other_uid = other_uid_name && request.headers[other_uid_name] || parsed_auth_cookie[other_uid_name] || params[other_uid_name]
@token = DeviseTokenAuth::TokenFactory.new unless @token
@token.token ||= request.headers[access_token_name] || parsed_auth_cookie[access_token_name] || decoded_authorization_token[access_token_name] || params[access_token_name]
@token.client ||= request.headers[client_name] || parsed_auth_cookie[client_name] || decoded_authorization_token[client_name] || params[client_name]
```

Contributor guide

Open the contributing guide

Research direction

Start in app/controllers/devise_token_auth/concerns/set_user_by_token.rb around line 52 and trace how authentication attributes are selected from headers, cookies, the decoded Authorization token, and params. Reproduce the documented request with a colliding client parameter, then verify that authentication still uses the Authorization token while preserving fallback behavior for params.

Written by the indexing model from the issue text.

Assessment

Tech stack
rails, ruby
Domain
api, authentication, backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.