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

fix: show 405 error if request is GET and queries are not allowed #3646

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: show 405 error if request is GET and queries are not allowed #3646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7da16a4
fix: show 404 if request is GET and queries are not allowed
alexei Sep 25, 2024
8b622e8
fix: return 405; update tests
alexei Sep 25, 2024
e98048a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 25, 2024
724a89e
fix: formatting
alexei Sep 25, 2024
8ada7a6
fix: add missing RELEASE.md
alexei Sep 25, 2024
c1ac8c9
fix: test
alexei Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Release type: patch

This release fixes an issue where a GET request is processed despite it being disallowed.
Copy link
Member

Choose a reason for hiding this comment

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

Let's be more specific here

Suggested change
This release fixes an issue where a GET request is processed despite it being disallowed.
This release changes the default behavior on queries via GET requests to a 405 error. To enable queries via get, please use `self.allow_queries_via_get`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @erikwrede I appreciate your reviewing this. I agree the message can be improved, but let's be clear about the change. Queries via GET are still allowed by default just like before - I did not touch that, though I believe the default should be more conservative. I changed the behavior when it's turned off:

  • before this change you get a GraphQL query not found error;
  • after this change you get a HTTP method not allowed error.

Copy link
Member

Choose a reason for hiding this comment

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

@alexei so probably we can go with an explanation like that, saying that before, even though queries via GET are allowed by default, when disallowing them you would only get a GraphQL query not found error, but now you should get a 405 error instead.

7 changes: 7 additions & 0 deletions strawberry/http/async_base_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import asyncio
import contextlib
import json
from http import HTTPStatus
from typing import (
Any,
AsyncGenerator,
Expand Down Expand Up @@ -184,6 +185,12 @@ async def run(
else:
raise HTTPException(404, "Not Found")

if request_adapter.method == "GET" and not self.allow_queries_via_get:
raise HTTPException(
HTTPStatus.METHOD_NOT_ALLOWED,
HTTPStatus.METHOD_NOT_ALLOWED.phrase,
)

sub_response = await self.get_sub_response(request)
context = (
await self.get_context(request, response=sub_response)
Expand Down
7 changes: 7 additions & 0 deletions strawberry/http/sync_base_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import abc
import json
from http import HTTPStatus
from typing import (
Any,
Callable,
Expand Down Expand Up @@ -180,6 +181,12 @@ def run(
else:
raise HTTPException(404, "Not Found")

if request_adapter.method == "GET" and not self.allow_queries_via_get:
raise HTTPException(
HTTPStatus.METHOD_NOT_ALLOWED,
HTTPStatus.METHOD_NOT_ALLOWED.phrase,
)

sub_response = self.get_sub_response(request)
context = (
self.get_context(request, response=sub_response)
Expand Down
6 changes: 4 additions & 2 deletions tests/http/test_query_via_get.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from http import HTTPStatus

from .clients.base import HttpClient


Expand Down Expand Up @@ -40,5 +42,5 @@ async def test_fails_if_allow_queries_via_get_false(http_client_class):

response = await http_client.query(method="get", query="{ hello }")

assert response.status_code == 400
assert "queries are not allowed when using GET" in response.text
assert response.status_code == HTTPStatus.METHOD_NOT_ALLOWED
assert HTTPStatus.METHOD_NOT_ALLOWED.phrase in response.text
Loading