search

mirumee / ariadne

Python library for implementing GraphQL servers using schema-first approach.
https://ariadnegraphql.org
BSD 3-Clause "New" or "Revised" License
2.21k stars 179 forks source link

Mounted App (on Starlette) Doesn't Check Path when Serving Requests #1072

Open cancan101 opened 1 year ago

cancan101 commented 1 year ago

Following the docs here https://ariadnegraphql.org/docs/starlette-integration#mounting-asgi-application:

app.mount("/graphql/", GraphQL(schema, debug=True))

Means the app will respond on <app>/graphql/ but also anything starting with that prefix, such as <app>/graphql/v2.

The is probably not desirable.

rafalp commented 1 year ago

If it's not desirable, why it's not desirable? Because search engines will index GraphiQL under odd paths like /graphql/wp-admin.php?

cancan101 commented 1 year ago

Because search engines will index GraphiQL under odd paths like /graphql/wp-admin.php

That is one issue. Another being a consumer might accidentally code against the "wrong" URL not realizing that it wrong because it works. At some point in the future, if the URL is made stricter (for example a new server / proxy or even just moving to the Route approach in Starlette), all of a sudden what they thought was a working URL will cease to function.

rafalp commented 1 year ago

Web crawlers can be told to bugger off via either robots.txt or disabling explorer view in Ariadne.

And how would you want to handle posts made for wrong paths? The fix would require having GraphQL URL specified on both the app you are mounting in and Ariadne's GraphQL APP, wouldn't?

cancan101 commented 1 year ago

If I understand this code in Starlette: https://github.com/encode/starlette/blob/01aa49a379520d3bbe6bc1aecc48a48169e6b004/starlette/routing.py#L386-L407, it bite off the portion of the path that it matches against. Given that, wouldn't Ariadne's GraphQL APP just have to match against /, ie the portion that remains. If that is the case then I don't think the GraphQL URL needs to be specified in both locations.

rafalp commented 1 year ago

What about other ASGI applications? FastAPI? Starlite? Etc. Ect? I would suspect this is also the case for them, but would you be willing to verify this and open a PR with the change?