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.

Sign up for GitHub

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

Jump to bottom

HTTP API uses redirects with 301 status, which leads to GET subsequent request in some clients #1415

Closed
Gozala opened this issue Jul 20, 2021 · 1 comment · Fixed by #1445
Closed

HTTP API uses redirects with 301 status, which leads to GET subsequent request in some clients #1415

Gozala opened this issue Jul 20, 2021 · 1 comment · Fixed by #1445
Assignees
@hsanjuan
Labels
effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/in-progress In progress
Milestone
Release v0.14.1

Comments

@Gozala
Copy link

Gozala commented Jul 20, 2021

Describe the bug:

niftysave uses cluster's HTTP API to pin content discovered on chain using IPFS path. On paths e.g. like /api/pins/ipfs/bafybeihqrzwradmal4ok5irhpxhst6fedq7kd5kumx724v4lekh2daj4eq/ cluster responds with 301 status redirecting request to /api/pins/ipfs/bafybeihqrzwradmal4ok5irhpxhst6fedq7kd5kumx724v4lekh2daj4eq (no trailing slash), which causes node fetch implementation to make subsequent GET request which fails (because API only accepts POST requests).

Initially I thought it was fetch implementation bug web-std/io#11, however browser implementation seems to fail as well.

As it turns out rfc7231 has note about it (quoting inline below)

Note: For historical reasons, a user agent MAY change the request method from POST to GET for the subsequent request. If this behavior is undesired, the 307 (Temporary Redirect) status code can be used instead.

Browser implementation seems to do exactly that and perhaps unsurprisingly node (re)implementation of that API does the same, both failing due to reasons described above.

** Proposed fix **

Use 307 status code instead as per note in the spec to avoid this issue.
@Gozala Gozala added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Jul 20, 2021
@welcome
Copy link

welcome bot commented Jul 20, 2021

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

  • "Priority" labels will show how urgent this is for the team.
  • "Status" labels will show if this is ready to be worked on, blocked, or in progress.
  • "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

Gozala added a commit to nftstorage/nft.storage that referenced this issue Jul 20, 2021
@Gozala
fix: workaround ipfs-cluster/ipfs-cluster#1415
4b5e18b
Gozala added a commit to nftstorage/nft.storage that referenced this issue Jul 22, 2021
@Gozala
fix: unblock niftysave (#257)
7fc56bd 
* chore: add more logging
* feat: extend ipfs-url module
* fix: use timeouts and original CID versions
* fix: Turn statusText to string
* fix: workaround ipfs-cluster/ipfs-cluster#1415
* fix: pass metadata properly
* chore: upgrade to web-std/fetch containing fix
@hsanjuan hsanjuan added this to the Release v0.14.1 milestone Jul 23, 2021
@hsanjuan hsanjuan added effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers P1 High: Likely tackled by core team if no one steps up good first issue Good issue for new contributors help wanted Seeking public contribution on this issue and removed need/triage Needs initial labeling and prioritization labels Jul 23, 2021
@hsanjuan hsanjuan self-assigned this Aug 11, 2021
@hsanjuan hsanjuan added status/in-progress In progress and removed help wanted Seeking public contribution on this issue good first issue Good issue for new contributors labels Aug 11, 2021
hsanjuan added a commit that referenced this issue Aug 11, 2021
@hsanjuan
restapi: fix #1415: paths ending in / get 307-redirect instead of 301
70386ea 
The Gorilla muxer StrictSlash option uses a 301 permanent redirect, which
results in POST requests becoming GET requests in most clients.  Thus we use
our own middleware that performs a 307 redirect.  See issue #1415 for more
details.
@hsanjuan hsanjuan mentioned this issue Aug 11, 2021
Merged
hsanjuan added a commit that referenced this issue Aug 12, 2021
@hsanjuan
Merge pull request #1445 from ipfs/fix/1415-307-redirect
4e61722 
restapi: fix #1415: paths ending in / get 307-redirect instead of 301
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hsanjuan hsanjuan

Labels
effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/in-progress In progress
Projects
None yet
Milestone
Release v0.14.1
Development

Successfully merging a pull request may close this issue.

restapi: fix #1415: paths ending in / get 307-redirect instead of 301 ipfs-cluster/ipfs-cluster
2 participants
@Gozala @hsanjuan