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

Do Not Merge: v5.0.3 + fixed validation ordering #2

Open
wants to merge 3 commits into
base: v502_fixed_validation_ordering
Choose a base branch
from

Conversation

shirish-pampoorickal
Copy link
Member

@shirish-pampoorickal shirish-pampoorickal commented Nov 11, 2021

NOTE: This PR starts from Doorkeeper 5.0.3 as a base. This is intended to serve as a temporary source for our gem in the interim while we upgrade to Ruby 2.4+. This branch is pointed to from our gemfile, do not merge this PR!

Summary

Impact:
Information disclosure vulnerability. Allows an attacker to see all Doorkeeper::Application model attribute values (including secrets) using authorized applications controller if it's enabled (GET /oauth/authorized_applications.json).

We are fixing redirectable? returning the wrong result as a consequence of a different error name type. This should function correctly independently of the error name type.

In base Doorkeeper, there is a PR and commit that the maintainer is looking to push into the latest 5.2 that fixes the issue in one way doorkeeper-gem@444fdbe where their intent is that validations (and the error response generated) need to happen in a very specific order and would avoid running into this issue.

stefansundin and others added 3 commits May 2, 2020 16:49
* Attempt at fixing information disclosure vulnerability.

* Add `#as_json` method and attrs serialization restriction for Application model

* [ci skip] Add documentation for serialization

Co-authored-by: Nikita Bulai <[email protected]>
- Calling public method redirectable should do the right thing regardless of validation state

Signed-off-by: Kevin Lai <[email protected]>
Co-authored-by: Kevin Lai <[email protected]>
@shirish-pampoorickal shirish-pampoorickal changed the title V503 fixed validation ordering Do Not Merge: V503 fixed validation ordering Nov 11, 2021
@shirish-pampoorickal shirish-pampoorickal changed the title Do Not Merge: V503 fixed validation ordering Do Not Merge: v5.0.3 + fixed validation ordering Nov 11, 2021
Copy link

@urnf urnf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I'm curious if there's a way to keep this from being merged accidentally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants