nuwave / nuwave/lighthouse

Nested mutations on 1-to-1 relationships require relation's PK

Open
#1,158 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

discussion enhancement
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
        }
    }
}
  1. This seems undocumented, since no update example is shown in the docs for the 1-1 relationship types.
  2. 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

  1. 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.

  2. It'd seem natural that 1-1 relationships shouldn't need to specify all PKs involved. If a user model only has one profile relationship, that shouldn't need to be manually specified by the client.

  3. 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

  1. Create schema per https://lighthouse-php.com/master/eloquent/nested-mutations.html#belongs-to
  2. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.