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

change HandledMethods to AllowGet and cleanup method handling #191

Merged
merged 1 commit into from
Apr 4, 2020

Conversation

Stebalien
Copy link
Member

Allowing methods isn't as simple as just allowing/disallowing them because different methods do different things.

  • HEAD: should be allowed everywhere GET is allowed.
  • OPTIONS:
    • When a CORS request is made, this will be handled by the CORS library.
    • Otherewise, we need to return the allowed methods.
  • POST: always allowed.
  • Everything else: always denied.

Changing HandledMethods to a simple AllowGet makes it easier to "do the right thing".

Allowing methods isn't as simple as just allowing/disallowing them because
different methods do different things.

* HEAD: should be allowed everywhere GET is allowed.
* OPTIONS:
  * When a CORS request is made, this will be handled by the CORS library.
  * Otherewise, we need to return the allowed methods.
* POST: always allowed.
* Everything else: always denied.

Changing HandledMethods to a simple AllowGet makes it easier to "do the right
thing".
@Stebalien Stebalien requested a review from hsanjuan April 4, 2020 20:47
Copy link
Contributor

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

ok this is clearly better

@hsanjuan hsanjuan merged commit 4dfbfd8 into master Apr 4, 2020
@hsanjuan hsanjuan deleted the fix/method-handling branch April 4, 2020 20:55
@hsanjuan
Copy link
Contributor

hsanjuan commented Apr 4, 2020

I'll bubble to go-ipfs

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.

2 participants