Skip to content

Setting GraphQL depth limit to 10#109

Open
TomHawk123 wants to merge 4 commits intoroostorg:mainfrom
TomHawk123:mac-graphql_depth_limiter
Open

Setting GraphQL depth limit to 10#109
TomHawk123 wants to merge 4 commits intoroostorg:mainfrom
TomHawk123:mac-graphql_depth_limiter

Conversation

@TomHawk123
Copy link

Context & Requests for Reviewers

This PR answers Issue #108

Tests

Ran locally and slammed the server with an overly complex query to confirm crashing of service. Implemented depth limit of 10 and tested:

Expected to Fail:

curl -s -X POST http://localhost:8080/graphql \
  -H 'Content-Type: application/json' \
  -d '{"query":"{ jobs { edges { node { item { submittedBy { roles { org { policies { rules { conditions { id } } } } } } } } } } }"}' \
  | jq .

Expected to Succeed:

curl -s -X POST http://localhost:8080/graphql \
  -H 'Content-Type: application/json' \
  -d '{"query":"{ __typename }"}' \
  | jq .

(Optional) Rollout Plan

After testing by reviewers, this should be able to rollout immediately.

pawiecz
pawiecz previously requested changes Mar 9, 2026
@pawiecz
Copy link
Contributor

pawiecz commented Mar 10, 2026

@TomHawk123 it looks like the package-lock.json was generated with npm <v24.13.1 - this could be the reason for locks desync in the CI checks: https://github.com/roostorg/coop/actions/runs/22804564190/job/66321426089?pr=109#step:3:79

I'll submit locking the .nvmrc at v24.14.0 for the time being to keep the CI consistent.

@pawiecz pawiecz added this to Coop Mar 10, 2026
@github-project-automation github-project-automation bot moved this to Backlog in Coop Mar 10, 2026
@pawiecz pawiecz moved this from Backlog to In review in Coop Mar 10, 2026
@pawiecz pawiecz added this to the v1 milestone Mar 10, 2026
Used safe var helper to ensure proper integer is being used for GraphQL depth

Using Node 24.14.0, as directed to make CI happy
Copy link
Contributor

@pawiecz pawiecz left a comment

Choose a reason for hiding this comment

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

Patch looks good!

I'll merge #112 so we could address the CI warning.

@TomHawk123
Copy link
Author

TomHawk123 commented Mar 19, 2026

Patch looks good!

I'll merge #112 so we could address the CI warning.

I don't think I can merge until @juanmrad approves the PR after addressing his change request and/or until the CI warning can be addressed.

@juanmrad
Copy link
Member

@TomHawk123 can you merge main to re-trigger CI and make sure all passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants