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

API call changes method on 308 redirect #28058

Open
julianklose opened this issue Oct 14, 2023 · 10 comments
Open

API call changes method on 308 redirect #28058

julianklose opened this issue Oct 14, 2023 · 10 comments

Comments

@julianklose
Copy link

julianklose commented Oct 14, 2023

Current behavior

When calling a POST API endpoint that is redirected by a 308 to another path, cypress changes the method to GET. This is not allowed since RFC 7538.

Desired behavior

Redirect without changing the method when encountering a 307 or 308 redirect.

Test code to reproduce

describe('308 redirect', () => {
  it('POST request to test', () => {
    cy.request("POST", "/test").its("body").should("eq", "POST");
  })
});

Cypress Version

13.3.2

Node version

v18.18.2

Operating System

Windows 10.0.19045.3570

Debug Logs

  1) 308 redirect
       POST request to test:

      Timed out retrying after 4000ms
      + expected - actual

      -'GET'
      +'POST'

Steps to reproduce

You can find the code to reproduce the issue here: https://github.com/julianklose/cypress-308-test

@MikeMcC399
Copy link
Contributor

It looks like @cypress/request implements and tests HTTP 300-307, but not HTTP 308.

RFC 9110 - HTTP Semantics, obsoletes RFC 7538 - The Hypertext Transfer Protocol Status Code 308 (Permanent Redirect, and includes 15.4.9 - HTTP 308 (Permanent Redirect) to describe.

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Oct 18, 2023

@julianklose

Do you have a public URL available which responds with HTTP 308 to method POST to test against? I looked around and I could only find 301 and 307 sites.

@MikeMcC399
Copy link
Contributor

MikeMcC399 commented Oct 20, 2023

@julianklose

It looks like this issue also affects current versions.

The report could be improved to provide an up-to-date reference to the issue:

  • include a log which shows the incorrect behavior
  • use a standard Cypress command such as cy.request() instead of a Custom Command cy.callApi
  • demonstrate the issue on a supported version of Node.js 18, 20 and later. (Node.js 16.19.1 has already reached end-of-life)
  • test using the latest version of Cypress (currently 13.3.2)

@MikeMcC399
Copy link
Contributor

I reproduced this issue with cy.request() using the dev server npm module serve, set up to redirect with HTTP 308 from /api/v1 to /api/v2.

Cypress cy.request() incorrectly sends a GET request to /api/v2 instead of preserving the origin POST request.

RFC 9110 - 15.4 Redirection 3xx states:

307 (Temporary Redirect) and 308 (Permanent Redirect) RFC7538 were later added to unambiguously indicate method-preserving redirects, and status codes 301 and 302 have been adjusted to allow a POST request to be redirected as GET.

Steps to reproduce

A repro of this non-conformance issue is posted to cypress-test-tiny including a README to describe the issue and steps to reproduce.

@julianklose
Copy link
Author

Thanks @MikeMcC399,
I was also able to reproduce it with cy.request and the newest cypress version. I updated the issue to include my test code.

@MikeMcC399
Copy link
Contributor

@julianklose

Thanks for your update.

I tried adding your code:

.its("body").should("eq", "POST"

to my test, however body in my demo returns the whole html code. It's not the method GET or POST.

I used serve for the test because it easily allows setting up a redirect and it conveniently logs all requests, so I think this is sufficient to demonstrate the problem, since the logs clearly show POST followed by the incorrect GET.


I'm not expecting this to get fixed soon, as cy.request() uses the fork @cypress/request of the deprecated https://github.com/request/request which was started in 2011 and deprecated in 2020.

RFC 9110 - 15.4.9 HTTP 308 (Permanent Redirect) includes the text:

Note: This status code is much younger (June 2014) than its sibling codes and thus might not be recognized everywhere. See Section 4 of [RFC7538] for deployment considerations.

cypress-io/request#35 (comment) stated that the Cypress team wanted to move away from using @cypress/request, although at the time of this comment there were no concrete plans to implement.

@cypress-app-bot
Copy link
Collaborator

This issue has not had any activity in 180 days. Cypress evolves quickly and the reported behavior should be tested on the latest version of Cypress to verify the behavior is still occurring. It will be closed in 14 days if no updates are provided.

@cypress-app-bot cypress-app-bot added the stale no activity on this issue for a long period label Apr 20, 2024
@MikeMcC399
Copy link
Contributor

The issue is still reproducible with Cypress 13.8.0 and should remain open pending feedback from the Cypress.io team.

git clone --branch RFC-9110 https://github.com/MikeMcC399/cypress-test-tiny
cd cypress-test-tiny
npm ci
npm test

The log of the test still shows GET incorrectly being used:

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Cypress:        13.8.0                                                                         │
  │ Browser:        Electron 118 (headless)                                                        │
  │ Node Version:   v20.12.2 (/home/mike/n/bin/node)                                               │
  │ Specs:          1 found (spec.cy.js)                                                           │
  │ Searched:       cypress/e2e/**/*.cy.{js,jsx,ts,tsx}                                            │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────

  Running:  spec.cy.js                                                                      (1 of 1)


  cy.request redirect
 HTTP  4/20/2024 9:54:11 AM 127.0.0.1 POST /api/v1
 HTTP  4/20/2024 9:54:11 AM 127.0.0.1 Returned 308 in 1 ms
 HTTP  4/20/2024 9:54:11 AM 127.0.0.1 GET /api/v2
 HTTP  4/20/2024 9:54:11 AM 127.0.0.1 Returned 200 in 1 ms
    ✓ request POST to v1 (140ms)

@cypress-app-bot cypress-app-bot removed the stale no activity on this issue for a long period label Apr 21, 2024
@jennifer-shehane
Copy link
Member

@MikeMcC399 We will have to move off of cypress/request , so it could be resolved when we do that work. But that work isn't currently planned.

@MikeMcC399
Copy link
Contributor

Understood

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

No branches or pull requests

4 participants