Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove hook.data.payload #522

Merged
merged 1 commit into from Jun 8, 2017

Conversation

Projects
None yet
2 participants
@marshallswain
Copy link
Member

marshallswain commented Jun 1, 2017

This removes hook.data.payload because it’s too easy to open a security hole when implementing custom authentication solutions. hook.params.payload is the only default mechanism to customize the JWT, now. A hook can be used to restore this functionality, if needed.

Remove hook.data.payload
This removes hook.data.payload because it’s too easy to open a security hole when implementing custom authentication solutions.  hook.params.payload is the only default mechanism to customize the JWT, now.  A hook can be used to restore this functionality, if needed.
@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 2, 2017

Agreed. I think this was an oversight on my part and was a remnant left over from before I figured out how we should be returning the payload from the auth plugins. See this line as an example.

This may be a breaking change for people. I'm not sure if anyone has been using that but my guess is this probably should be a major release. Thoughts?

This is totally a :shipit: though.

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Jun 8, 2017

This might be a breaking change for one or two people. In order to hit the vulnerability you had to use a custom verifier and not pass any payload out of the verifier. Otherwise, the params.payload always overwrite the data.payload, anyway. I think we should get it into the current release.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 8, 2017

Yeah let’s ship it. Technically it should be a major since we are removing functionality. But that functionality shouldn’t be there and unless you were really monkeying with stuff in a way you shouldn’t be, this PR shouldn’t have an effect.

:shipit:

@marshallswain marshallswain merged commit 17cab82 into master Jun 8, 2017

4 checks passed

codeclimate no new or fixed issues
Details
codeclimate/coverage 84.54%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@daffl daffl deleted the remove-data-payload branch Jul 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.