[5.3] Update TokenGuard.php to look for key in query string items only. #14985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because in Larvel's combined input system, the body items take precedence over query string items. If an item appears in the body that uses the same key as the one being used for the API token, then this body item is then assumed to be the token which could lead to authentication errors especially if the key is being set to a more generic custom name with a high risk of conflict, e.g. 'password'. This file has been edited to restrict the API token to being in the query string only by using request->query instead of request->input which I think is the expected behaviour for token authentication.
| * | ||
| * @var string | ||
| */ | ||
| protected $inputKey; | ||
| protected $queryKey; |
There was a problem hiding this comment.
Doesn't this could potentially be breaking BC, if one extending Illuminate\Auth\TokenGuard and changing the $inputKey.
There was a problem hiding this comment.
Thanks I updated the description to mention that potential issue.
There was a problem hiding this comment.
No, you need to stick with the old variable name or send this pull request to master branch (for Laravel 5.4), breaking BC is only permitted on major release.
There was a problem hiding this comment.
Thanks for letting me know that information. I added a commit reverting back to the old variable name. So now all thats changed is the request->query access and the variable's comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because in Larvel's combined input system, the body items take precedence over query string items. If an item appears in the body that uses the same key as the one being used for the API token, then this body item is then assumed to be the token which could lead to authentication errors especially if the key is being set to a more generic custom name with a high risk of conflict, e.g. 'password'. This file has been edited to restrict the API token to being in the query string only by using request->query instead of request->input which I think is the expected behaviour for token authentication.
After checking the query string it still falls back to looking in the headers for a bearer token or basic auth password as it was originally designed.
I also renamed the variable to queryKey to convey it is only relevant to the query string rather than the combined input, however, given 5.3 is already released this would be a breaking change if anyone has already subclassed TokenGuard to rename the token key. Thus it might be better to leave the variable named inputKey.