Skip to content

Setting GraphQL depth limit to 10#109

Merged
pawiecz merged 13 commits into
roostorg:mainfrom
TomHawk123:mac-graphql_depth_limiter
Mar 31, 2026
Merged

Setting GraphQL depth limit to 10#109
pawiecz merged 13 commits into
roostorg:mainfrom
TomHawk123:mac-graphql_depth_limiter

Conversation

@TomHawk123
Copy link
Copy Markdown
Contributor

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
Comment thread server/api.ts Outdated
@pawiecz
Copy link
Copy Markdown
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
Comment thread server/api.ts Outdated
Comment thread server/iocContainer/utils.ts Outdated
Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Member

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

pawiecz added 2 commits March 24, 2026 15:31
This change addresses ESLint warning about type safety of JSON.stringify.
@pawiecz
Copy link
Copy Markdown
Contributor

pawiecz commented Mar 24, 2026

This PR still has misaligned Betterer results (even after merging recent changes from main).

I'll run Betterer again on this branch, amend the results so that CI doesn't complain about the results getting improved (sigh) and merge this PR.

juanmrad and others added 3 commits March 26, 2026 13:53
…g#115)

* remove sample exchanges

* [HMA][Exchanges] Configure HMA exchanges directly from coop

* tests

* capitalize first value of the keys

* fix tests

* code review changes

* fix eslint

* fix eslint

* fix for real now
juanmrad and others added 3 commits March 26, 2026 13:53
…book main (roostorg#150)

* [Vulnerabilities] Migrate from CRA Storybook to Vite compatible Storybook main

* fix button

* fix betterer and eslint

* fix eslint

* use coop style
* Fix ClickHouse outage crashing all dashboard pages

* fix betterer

* add logging on warehouse error to let back-end know. given empty returns are to prevent front-end issues
I was browsing the docs and noticed this looked weird/inconsistent :)
@TomHawk123 TomHawk123 requested a review from a team as a code owner March 26, 2026 18:53
@pawiecz pawiecz merged commit adb0eeb into roostorg:main Mar 31, 2026
7 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Coop Mar 31, 2026
@TomHawk123 TomHawk123 deleted the mac-graphql_depth_limiter branch March 31, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants