Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes: #16292 - Properly restrict GraphQL queries for querys with pk set #17244

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

DanSheps
Copy link
Member

Fixes: #16292 - Properly restrict GraphQL queries for querys with pk set

  • Set name on Schema type to "Query" (required as part of the default resolver generation)
  • Change resolver to regular Field with appropriate type
  • Set DEFAULT_PK_FIELD_NAME in settings.py to id

@DanSheps
Copy link
Member Author

Added bonus, pycharm find and replace regex for this change:

    @strawberry.field
    def (.*)\(self, id: int\) -> (.*):
        return models\..*\.objects\.get\(pk=id\)
    $1: $2 = strawberry_django.field()

netbox/netbox/settings.py Outdated Show resolved Hide resolved
@DanSheps DanSheps self-assigned this Aug 25, 2024
@jeremystretch jeremystretch marked this pull request as ready for review August 27, 2024 17:17
@jeremystretch jeremystretch removed their request for review August 27, 2024 19:33
@jeremystretch
Copy link
Member

I've replicated all of Dan's changes application-wide and it seems to work well. We still need to add a test for this though.

@jeremystretch
Copy link
Member

I've extended the existing GraphQL API tests to check for this.

Also, it seems that GraphQL is logging an exception for requests which query an invalid object ID (but still returning the expected response). I've compensated for this in the test to reduce noise, but it's something we should look into separately.

Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a bit leery of the [name="Query") as I'm not sure if it's needed or has side-effects, but in testing I couldn't find any difference it causes so seems okay.

@jeremystretch jeremystretch merged commit 0464dac into develop Aug 28, 2024
6 checks passed
@jeremystretch jeremystretch deleted the 16292-restrict-graphql-queries branch August 28, 2024 16:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication Bypass in GraphQL Queries for Users/Tokens Lacking Permissions
3 participants