Overview
The parsing and validation of Relay global IDs trusts the input too much. Malformed payloads can be constructed to cause various exceptions which precipitate in error messages that expose various levels of implementation details.
Describe the Bug
Global IDs appear to be parsed and validated slightly differently depending on the context in which they are used. As such, I've investigated providing malformed global IDs in the following contexts:
- The
idprovided to retrieve the instance inupdate/deleteCUD mutations - The
idprovided as input for related fields increate/updateCUD mutations
The current behaviour for these contexts is outlined in the tables below:
Scenario 1: The id provided to retrieve the instance in update / delete CUD mutations
| Global ID | Resultant Error | Notes |
|---|---|---|
| Type for correct model : Non-existent primary key | <Correct Model> matching query does not exist. |
Good |
| Type for correct model : Garbage | Field 'id' expected a number but got 'Garbage'. |
Not bad |
| Type for incorrect model : Existing primary key | Cannot resolve. GlobalID requires <Correct Model>, received <Incorrect Model Instance>. Verify that the supplied ID is intended for this Query/Mutation/Subscription. |
The error message leaks information about the incorrectly retrieved record |
| Type for incorrect model : Non-existent primary key | <Incorrect Model> matching query does not exist. |
Not bad, but we probably shouldn't have tried to retrieve the record for the incorrect type to begin with |
| Type for incorrect model : Garbage | Field 'id' expected a number but got 'Garbage'. |
Again, not bad but we probably shouldn't have tried to retrieve the record for the incorrect type to begin with |
| Garbage : Garbage | Cannot resolve. GlobalID requires a GraphQL type, received ``Garbage``. |
We probably don't need to expose implementation details of the Global ID validation here? |
| Garbage | Expected value of type 'GlobalID!', found \"R2FyYmFnZQ==\"; ['Garbage'] expected to contain only 2 items |
We probably don't need to expose implementation details of the Global ID validation here? |
| Empty | Expected value of type 'GlobalID!', found \"\"; [''] expected to contain only 2 items |
We probably don't need to expose implementation details of the Global ID validation here? |
Scenario 2: The id provided as input for related fields in create / update CUD mutations
| Malformed Global ID | Resultant Error | Notes |
|---|---|---|
| Type for correct model : Non-existent primary key | <Correct Model> matching query does not exist. |
Good |
| Type for correct model : Garbage | Field 'id' expected a number but got 'Garbage'. |
Not bad |
| Type for incorrect model : Existing primary key | An unknown error occurred. |
This is caused by an assert here |
| Type for incorrect model : Non-existent primary key | <Incorrect Model> matching query does not exist. |
Not bad, but we probably shouldn't have tried to retrieve the record for the incorrect type to begin with |
| Type for incorrect model : Garbage | Field 'id' expected a number but got 'Garbage'. |
Again, not bad but we probably shouldn't have tried to retrieve the record for the incorrect type to begin with |
| Garbage : Garbage | Cannot resolve. GlobalID requires a GraphQL type, received ``Garbage``. |
We probably don't need to expose implementation details of the Global ID validation here? |
| Garbage | Expected value of type 'GlobalID!', found \"R2FyYmFnZQ==\"; ['Garbage'] expected to contain only 2 items |
We probably don't need to expose implementation details of the Global ID validation here? |
| Empty | Expected value of type 'GlobalID!', found \"\"; [''] expected to contain only 2 items |
We probably don't need to expose implementation details of the Global ID validation here? |
Extra notes
An extra factor to consider here is which of these cause errors handled by handle_django_errors=True and which don't.
Currently, the errors which result in "<Model> matching query does not exist." messages (i.e., ObjectDoesNotExist exceptions) result in an OperationInfo result, whereas the others are top-level GraphQL errors.
It would be convenient if the errors precipitating from these malformed global IDs were consistent.
Expected Behaviour
I would expect the parsing and validation of global IDs to be more strict and not trust the input as much as it does now.
In particular:
- The GraphQL type parsed from the global ID should be checked against an expected type before continuing.
- The primary key should be parsed and validated against the expected type's Django model before continuing.
This would prevent information leakage and any other possible security concerns due to malformed global IDs.
System Information
- Operating system: MacOS
- Strawberry version (if applicable): 0.47.2
How does funding with Polar work?
Pay now to fund the work behind this issue.
Get updates on progress being made.
Maintainer is rewarded once the issue is completed.
Backer
You're funding impactful open source efforts
Contributor
You want to contribute to this effort
Maintainer
You want to get funding like this too