Nested mutations on 1-to-1 relationships require relation's PK
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 3.5k
- Forks
- 468
- Avg merge
- 3h 9m
- Merged PRs (30d)
- 2
Description
Slack thread: https://lighthouse-php.slack.com/archives/CB28A070S/p1579310262008300
Describe the bug
When using a nested mutation, such as update, you're required to pass in the related model's PK, even if the relationship is 1-1.
mutation {
updateUser(input: {
id: 1
username: "test"
profile: {
update: {
user_id: 2 # This is the undocumented bit and I wouldn't expect to be allowed to use the wrong PK here.
email: "test@test.com"
}
}
}){
id
username
profile {
email
}
}
}
- This seems undocumented, since no update example is shown in the docs for the 1-1 relationship types.
- This allows users to create malformed mutations that, at best, trigger laravel or db exceptions. Not sure if it's a valid vector for screwing up the db, though.
Expected behavior/Solution
-
It should at least be documented in the 1-1 portion of the nested mutation docs that the related model's PK is needed. That the related model instance isn't using the model's relationship method.
-
It'd seem natural that 1-1 relationships shouldn't need to specify all PKs involved. If a
usermodel only has oneprofilerelationship, that shouldn't need to be manually specified by the client. -
I'd expect users of the API to not be allowed to supply malformed mutations that seem to break laravel's normal relationship rules.
I don't have a proof-of-concept that this could lead to operating on models that don't actually belong to the root model but I wouldn't be surprised if it did. For instance, what if a relationship has extra restrictions set in the model's method that are ignored by supplying incorrect PKs, which Lighthouse seems to be taking at face value?
If nothing else, it's a side-channel attack that leaks which PKs exist in the related model's table, which could compromise slug-based or multi-tenant systems.
Steps to reproduce
- Create schema per https://lighthouse-php.com/master/eloquent/nested-mutations.html#belongs-to
- Call a nested update with incorrect PKs
mutation {
updateUser(input: {
id: 1
username: "test"
profile: {
update: {
user_id: 2 # This is the undocumented bit and I wouldn't expect to be allowed to use the wrong PK here.
email: "test@test.com"
}
}
}){
id
username
profile {
email
}
}
}
Output/Logs
Click to expand
For PK clashes:
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '4' for key 'PRIMARY' (SQL: update `profiles` set `user_id` = 4, `email` = test@test.com where `user_id` = 3)
For PKs that don't exist:
No query results for model [App\\Models\\Profile] 5
Environment
Lighthouse Version: 4.8.1
Laravel Version: 6.11.0
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the nested mutation documentation, especially the belongs-to and 1-to-1 sections, and reproduce the update mutation using mismatched or missing related primary keys. Compare the observed database and model errors with the documented behavior. Done should include an agreed validation or relationship-handling behavior and documentation of the required input, with coverage for the reported malformed mutation cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- graphql, laravel, php
- Domain
- api, backend, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100