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

Broken GraphQL endpoint on last Strapi versions #19073

Closed
daniele-orlando opened this issue Dec 15, 2023 · 13 comments
Closed

Broken GraphQL endpoint on last Strapi versions #19073

daniele-orlando opened this issue Dec 15, 2023 · 13 comments
Assignees
Labels
issue: bug Issue reporting a bug issue: discussion A general discussion issue issue: help wanted severity: medium If it breaks the basic use of the product but can be worked around source: plugin:graphql Source is plugin/graphql package status: pending reproduction Waiting for free time to reproduce the issue, or more information

Comments

@daniele-orlando
Copy link

daniele-orlando commented Dec 15, 2023

Bug report

Required System information

  • Strapi version: 4.15.5

Describe the bug

In last Strapi 4 versions, GraphQL queries sent over HTTP GET requests receive a Forbidden access error.
Everything works as expected running the same queries over POST requests.

The GraphQL endpoint is /graphql and serves successfully POST queries/mutations and the Playground Web Interface.

This is NOT a feature request, but a regression on the GET endpoint after commit 4436f59. It was working as expected until version 4.3. Upgrading to last Strapi version broke my GraphQL ecommerce and I had to rollback to previous version.

Steps to reproduce the behavior

  1. Open a browser console
  2. Run
fetch("https://<HOST>/graphql?query=query%7Bpages%7Bdata%7Bid%20attributes%7Bslug%7D%7D%7D%7D", {
  "method": "GET",
}).then(it => it.json()).then(console.log)

Expected behavior

I should receive the GraphQL data as response.

Actual behavior

I receive the error:

{
    "errors": [
        {
            "message": "Forbidden access",
            "extensions": {
                "error": {
                    "name": "ForbiddenError",
                    "message": "Forbidden access",
                    "details": {}
                },
                "code": "FORBIDDEN"
            }
        }
    ],
    "data": {
        "pages": null
    }
}

Additional context

The issue is related to this portion of code that skips the authentication logic.

if (ctx.request.method === 'GET') return next();

Removing this block of code, the GET requests work as expected and the Playground is still reachable on the same endpoint.

-          // allow graphql playground to load without authentication
-          if (ctx.request.method === 'GET') return next();
@daniele-orlando daniele-orlando changed the title Broken GraphQL endpoint on latest Strapi versions Broken GraphQL endpoint on last Strapi versions Dec 15, 2023
@derrickmehaffy
Copy link
Member

This needs to be determined by product if it's intended as generally it's anti-pattern and considered a bad practice to use GraphQL over GET requests.

As far as I'm aware the GET route was only ever intended for use with the playground and actual queries/mutations -should always- be done over POST.

@derrickmehaffy derrickmehaffy added issue: bug Issue reporting a bug issue: discussion A general discussion issue severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve issue: help wanted source: plugin:graphql Source is plugin/graphql package status: pending reproduction Waiting for free time to reproduce the issue, or more information labels Dec 15, 2023
@derrickmehaffy
Copy link
Member

Related discussion: graphql/graphql-over-http#123

@derrickmehaffy
Copy link
Member

Likewise specific graphql specification does not require supporting GET: https://graphql.github.io/graphql-over-http/draft/#sec-Request

@daniele-orlando
Copy link
Author

daniele-orlando commented Dec 17, 2023

Using GraphQL over GET requests is not an anti-pattern at all. I think you confused mutations which must use POST method. GraphQL queries instead should use an idempotent verb like GET and, in order to be cached by the browser, they must use an idempotent verb like GET. POST requests can not be cached by browsers.

A GraphQL server should accept GET and POST requests and should not enforce one method over the other, because it's up to the client to decide the best strategy for its use cases.

Your GraphQL HTTP server should handle the HTTP GET and POST methods.
Serving GraphQL over HTTP

My queries are served over GET method using Cache-Control, ETag and Last-Modified headers, which make them cachable by the browser. Switching to POST request would break completely the browser caching on any browser, which is not acceptable.

@Hadrien-DELAITRE
Copy link

Facing the exactly same issue.

I was trying to implement the APQ documented by Apollo: https://www.apollographql.com/docs/apollo-server/v3/performance/apq
These kind of queries only work with GET HTTP request method:

Persisted queries are especially effective when clients send queries as GET requests.

It seems weird that:

queries/mutations -should always- be done over POST.

Maybe expected features of graphQL is not correctly understood by Apollo & Strapi ?

@daniele-orlando
Copy link
Author

Anyone from the Strapi team can give an update about the planned timeline for the resolution of this bug, please?

cc: @Eventyret

@daniele-orlando
Copy link
Author

daniele-orlando commented Feb 15, 2024

@Eventyret can you increase the severity from severity:low to severity:high, because:
a) there is no workaround except downgrading Strapi (requisite to be low not met)
b) "it breaks the basic use of the product" (requisite to be high met)

@derrickmehaffy derrickmehaffy added severity: medium If it breaks the basic use of the product but can be worked around and removed severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve labels Feb 16, 2024
@derrickmehaffy
Copy link
Member

We can raise it to medium but we still need clarification from our product team if GET requests were ever intended to work as from what I understood they were not.

We specially advise users and especially EE customers to use REST where possible if caching is important.

@daniele-orlando
Copy link
Author

Hi dear Strapi team, do we have any update about the resolution of this issue?

@derrickmehaffy
Copy link
Member

GET requests are not supported in Strapi 4 and have been added in Strapi 5

@derrickmehaffy
Copy link
Member

#19916

@derrickmehaffy
Copy link
Member

Actually it looks like a community pr was accepted in 4.22.0 to allow this in v4, so marking as closed.

@derrickmehaffy
Copy link
Member

#19168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug issue: discussion A general discussion issue issue: help wanted severity: medium If it breaks the basic use of the product but can be worked around source: plugin:graphql Source is plugin/graphql package status: pending reproduction Waiting for free time to reproduce the issue, or more information
Projects
Status: Fixed/Shipped
Status: Done
Development

No branches or pull requests

4 participants