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

Update README.md #1056

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update README.md #1056

wants to merge 1 commit into from

Conversation

bradgessler
Copy link

I spent an hour trying to figure out why /auth/developer wasn't working from the README, and its because the :get method wasn't allowed for OmniAuth.config.allowed_request_methods

Is there a better way to do this per provider instead of globally for all providers? That would be ideal, but if not, what's the danger of adding :get to the default list vs the change I'm proposing?

Regardless of the path, this change will prevent future devs from spending lots of time trying to figure out why /auth/developer doesn't work.

I spent an hour trying to figure out why `/auth/developer` wasn't working from the README, and its because the `:get` method wasn't allowed for `OmniAuth.config.allowed_request_methods`

Is there a better way to do this per provider instead of globally for all providers? That would be ideal, but if not, what's the danger of adding `:get` to the default list vs the change I'm proposing?

Cheers
@BobbyMcWho
Copy link
Member

Hi @bradgessler, there's a lot of history w/ using GET, and a long standing vulnerability that was fixed last year around it. GHSA-ww4x-rwq6-qpgf

I don't particularly wish to recommend folks to use GET, even in dev, since it's not recommended to use in prod. Routes should be set up using a post, and there are examples in the wiki on how to set it up.

If the readme isn't clear as far as setting up the post button, we should clarify that

@bradgessler
Copy link
Author

Makes sense, I figured there was a security issue. If that's the case I'd simply eliminate :get

Based on your response, the issue is that the developer strategy won't work unless :get is allowed, which I configured in the README for the development environment. At this point, I won't be able to meaningfully contribute to this PR since this seems to be a question for the maintainers of this project.

The issue stands that as documented (prior to this PR), the /auth/developer strategy doesn't work out of the box and fails silently. I suppose one option would be to show an error message on a :get request w/ the proper HTTP code.

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

Successfully merging this pull request may close these issues.

None yet

2 participants