CUD mutation global ID parsing and validation leaks implementation details

[SupImDos]

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:

1. The id provided to retrieve the instance in update / delete CUD mutations
2. The id provided as input for related fields in create / update CUD 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

Upvote & Fund

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[bellini666]

Hey @SupImDos ,

I see your point and I totally agree with you!

Since you already opened a PR recently and are somewhat more familiar with the code, do you want to try to open a PR to solve this? You can ping me on discord if you need any help with this.

Otherwise I'll try to tackle this once I get some spare time

[SupImDos]

@bellini666 Thanks, we're glad you agree! At the moment, this issue isn’t at the top of our priority list, but we're happy to look into opening a PR when we have the bandwidth. If you get a chance to tackle it before then, we’d be grateful for your help!