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

chore: enable graphql protection - maxDepth, blockFieldSuggestions, maxCost #2537

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

sabineschaller
Copy link
Member

Changes proposed in this pull request

  • adds graphql armor to enable
    • maxDepth
    • blockFieldSuggestion
    • maxCost

To test maxDepth, run the following query

{
  __schema {
    types {
      name 
      fields {
        name
        type {
          name
          fields {
            name
            type {
              name
              fields {
                name
                type {
                  name
                  fields {
                    name
                    ...
                }
              } 
            }
          }
        }
      }
    }
  }
}

To test blockFieldSuggestion just misspell something slightly in an graphql query.

For the maxCost protection, I enabled it with default values.

Context

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

@github-actions github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Mar 14, 2024
Copy link

netlify bot commented Mar 14, 2024

Deploy Preview for brilliant-pasca-3e80ec failed.

Name Link
🔨 Latest commit 8a05b4c
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65f2d80b67332000082e6927

@github-actions github-actions bot added the pkg: auth Changes in the GNAP auth package. label Mar 14, 2024
Copy link
Contributor

@mkurapov mkurapov left a comment

Choose a reason for hiding this comment

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

Depth test works for me.

To confirm, for the blockFieldSuggestion, should I be seeing

{
  "errors": [
    {
      "message": "Cannot query field \"receivdeAmount\" on type \"OutgoingPayment\". [Suggestion hidden]?",
      "locations": [
        {
          "line": 11,
          "column": 5
        }
      ],
      "extensions": {
        "code": "GRAPHQL_VALIDATION_FAILED"
      }
    }
  ]
}

Comment on lines +167 to +174
costLimit: {
enabled: true,
maxCost: 5000,
objectCost: 2,
scalarCost: 1,
depthCostFactor: 1.5,
ignoreIntrospection: true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we get the cost of a graphql query? is there a way for testing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why I said I used default/"recommended" settings

Copy link
Member Author

Choose a reason for hiding this comment

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

@sabineschaller
Copy link
Member Author

To confirm, for the blockFieldSuggestion, should I be seeing

That's it. the suggestion:hidden part. Otherwise it would say something like "did you mean...?"

@sabineschaller sabineschaller merged commit 71a5d57 into main Mar 14, 2024
18 of 22 checks passed
@sabineschaller sabineschaller deleted the s2-graphql-depth branch March 14, 2024 15:51
Comment on lines +181 to +185
...protection,
plugins: [
...protection.plugins,
ApolloServerPluginDrainHttpServer({ httpServer })
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this is merged but wanted to comment here. We can add new issues for this.

The addition of ...protection breaks introspection. That is, you can't see the queries and mutations in apollo server as http://localhost:3001/graphql. It's working on main (before this was merged) but not here. Commenting out ...protection fixes it.

Maybe this one is a nit, and I know how the docs show it this way, but destructuring protection is probably not the best way to add it IMO because it has a plugins array. If we were to change the order of ...protection and plugins on this object then the plugin array we are forming ourslves would get overridden:

const apolloServer = new ApolloServer({
      schema: schemaWithResolvers,
      plugins: [
        ...protection.plugins,
        ApolloServerPluginDrainHttpServer({ httpServer })
      ],
      ...protection, // overrides the above plugins
      introspection: this.config.env !== 'production'
    })

Again, it's techniaclly fine as-is but it's not obvious how close it is to not fine. I think something like this would be safer:

    const { plugins: protectionPlugins, ...protectionRest } = protection

    this.apolloServer = new ApolloServer({
      schema: schemaWithMiddleware,
      ...protectionRest,
      plugins: [
        ...protectionPlugins,
        ApolloServerPluginDrainHttpServer({ httpServer })
      ],
      introspection: this.config.env !== 'production'
    })

Copy link
Contributor

Choose a reason for hiding this comment

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

it's the maxDepth.ignoreIntrospection breaking it. Although I see that's intentionally set that way to resolve an issue with max depth being ignored. Maybe it can also be controlled by NODE_ENV (not disabled in not prod)?

sabineschaller added a commit that referenced this pull request Mar 19, 2024
…axCost (#2537)

* chore: add graphql depth restriction

* feat: properly enable graphql armor

* style: remove unnecessary space

* chore: add armor to auth admin server
sabineschaller added a commit that referenced this pull request Mar 19, 2024
* feat(backend): webhook max retry

* fix: tighten filter

* Update packages/documentation/src/content/docs/integration/deployment.md

Co-authored-by: Max Kurapov <max@interledger.org>

* test: address review feedback

* chore: add feature requests to contributing.md (#2542)

* chore: enable graphql protection - maxDepth, blockFieldSuggestions, maxCost (#2537)

* chore: add graphql depth restriction

* feat: properly enable graphql armor

* style: remove unnecessary space

* chore: add armor to auth admin server

* fix(deps): update module github.com/aws/aws-sdk-go to v1.50.38 (#2543)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency @interledger/open-payments to v6.7.2 (#2544)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(frontend): add X-Frame-Options header (#2538)

* chore(deps): update dependency @types/lodash to ^4.17.0 (#2546)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency @types/react to ^18.2.66 (#2545)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency @interledger/open-payments to v6.8.0 (#2547)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/aws/aws-sdk-go to v1.51.0 (#2548)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency @interledger/docs-design-system to ^0.3.2 (#2549)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency axios to v1.6.8 (#2556)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency isbot to v5 (#2553)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update typescript-eslint monorepo to v7 (#2552)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update dependency koa to ^2.15.1 (#2551)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency postcss to ^8.4.36 (#2559)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update helm release redis to v18.19.3 (#2561)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat(bruno): use polling in grant continuation (#2550)

* chore: fetch latest OP schemas

* feat(bruno): use polling for grant continuation

* docs: update

* fix: bruno tests

* chore(deps): update pnpm to v8.15.5 (#2563)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat(telemetry): LIVENET now points to new livenet NLB (#2523)

* feat(telemetry): LIVENET now points to new livenet NLB

* Update packages/backend/src/config/app.ts

Co-authored-by: Max Kurapov <max@interledger.org>

* Update packages/backend/src/config/app.ts

Co-authored-by: Max Kurapov <max@interledger.org>

---------

Co-authored-by: Max Kurapov <max@interledger.org>
Co-authored-by: Sabine Schaller <sabine@interledger.org>

* feat(auth): improve GNAP error responses (#2400)

* feat(auth): improve GNAP error responses

* fix: add extra arg to ctx.throw

* feat: middleware error improvements

* feat: add error messages to grant details endpoint

* fix: update error messages in grant routes

* feat: create utility function to generate GNAP errors, create enum for GNAP error codes

* feat: helper function throws instead of generates response

* feat: integration tests (#2380)

* feat: setup basic test env

* feat: seed integration environment

- cmd to start integration environment and run tests
- seeds environment on test run
- extracts common MASE functionality into new mock-account-servicing-lib

* refactor(localenv,integration): move graphql url to config from seed

- moves graphql url to config from seed
- also removes self section which only contained unused properties

* refactor: move testenv configuration into new dir

* refactor: cleanup env vars

* feat: add webhook server, fix env vars

* chore: comment

* feat: start grant request test

* fix: eslint errors

* fix: docker compose env vars

* feat: add --build arg

* fix: incoming-payment grant initiation request

Grant request was failing with invalid signature. This was fixed by directing hte backend to use the same private key being used to create the open payments client.

* fix: ts error

* feat: implement tests through Create Quote

* chore: upgrade op client

* feat: partially implemented grant request outgoing payment test

* feat: rework to host.docker.internal

- switches hostname to host.docker.internal to resolve url mismatch problem
- finishes grant request outgoing payment test
- cleans up some configuration and test variables

* feat: add p2p flow test

* feat: add continuation step with consent mocking

* fix: rm obsolete type cast to any and comment

* feat: add create ougoing payment test

* fix: bad pnpm-lock merge

* feat: build deps in mock ase job

* feat: get non existant wallet address test

* fix: update open payments pkg

* chore: fix lint warnings

* feat: implement continuation polling

* chore: test cleanup

* refactor: generate gql in tests instead of import from lib

* chore: rm old comment

* chore: use latest http-singature-utils to match other deps

* chore: pnpm i

* chore: use latest koa-bodyparser

* chore: rm engine strict

* chore: revert rm engine strict

- fixes netlify ci failure but ultimately not the correct fix

* chore(integration): dont ignore env, rm example env

* refactor: use docker healthcheck for running tests

* chore: move healthcheck to last started docker container

* feat: use latest open payments pkg, no body requird on continuation

* chore: pnpm i, fix broken lockfile (no apollo version)

* refactor: move webhook event enum to types

* refactor: move run integration script to test/integration

* Update packages/mock-account-servicing-lib/package.json

Co-authored-by: Sabine Schaller <sabine@interledger.org>

* refactor: simplify mock-account-servicing-entity ci step

* feat(integration): exit run-tests early if containers fail to start

* feat: use pino logger

* refactor: rename mock account servicing lib

* refactor: rename class files to camel case

* fix: import correct body parser

* refactor: poll instead of delay

* refactor: simplify call to poll condition only

* fix: update filenames

* refactor: change filename name to kebab case

---------

Co-authored-by: Sabine Schaller <sabine@interledger.org>

* fix(deps): update dependency isbot to ^5.1.2 (#2567)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update dependency @types/react to ^18.2.67 (#2566)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/aws/aws-sdk-go to v1.51.2 (#2565)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* chore(deps): update helm release redis to v18.19.4 (#2570)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat(backend): make SPSP optional (#2560)

* feat: flag to enable/disable SPSP

* test: update to cover all cases

* docs: add link to glossary

* chore: fix lockfile

* test: address feedback

* delete lockfile

* add lockfile

* delete lockfile

* fix lockfile this time?

* fix: formatting

---------

Co-authored-by: Max Kurapov <max@interledger.org>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: xplicit <111863110+beniaminmunteanu@users.noreply.github.com>
Co-authored-by: Nathan Lie <lie4nathan@gmail.com>
Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. type: source Changes business logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce maximum depth limit on Graphql queries
3 participants