Skip to content

Initial depth check setup, testing has also been completed - #141

Merged
nsdeschenes merged 13 commits into
masterfrom
graphql-cycle-security-check
Mar 4, 2020
Merged

Initial depth check setup, testing has also been completed#141
nsdeschenes merged 13 commits into
masterfrom
graphql-cycle-security-check

Conversation

@nsdeschenes

Copy link
Copy Markdown
Contributor

This PR will bring in a new security check that will not all users to submit a query of depth 10 or greater. The depth can be changed at any time inside the api/backend/depth_check.py file.

  • Added backend directory to contain any backends we may choose to implement
  • Added depth check backend to help mitigate cycle attacks
  • Tests created to test depth check functionality

@peacheym peacheym left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work!

@sleepycat sleepycat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few suggestions, but some great stuff in here!

Comment thread api/app.py Outdated
Comment thread api/backend/depth_check.py Outdated
Comment thread api/tests/test_depth_check.py Outdated
@nsdeschenes
nsdeschenes force-pushed the graphql-cycle-security-check branch from 2d138fc to 54c8b2d Compare March 2, 2020 11:20
@nsdeschenes

Copy link
Copy Markdown
Contributor Author

Also added in a default value of 10, this should make it easier to change and customize depending on usage.

@nsdeschenes
nsdeschenes requested a review from sleepycat March 2, 2020 14:00
@nsdeschenes

Copy link
Copy Markdown
Contributor Author

Hold off on review, I think I have something good to add in

@nsdeschenes

Copy link
Copy Markdown
Contributor Author

New Changes

  • Added in cost checking
  • Split depth check, and cost functions into their own files

@nsdeschenes
nsdeschenes requested a review from peacheym March 3, 2020 11:13

@peacheym peacheym left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Good to have the depth check to make sure we don't get a huge (& slow) API call! :)

@sleepycat sleepycat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks awesome! 🚢

Comment thread api/app.py

db.init_app(app)

backend = SecurityAnalysisBackend(max_depth=10, max_cost=1000)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😻

@Ethanljf Ethanljf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Syntactically sound. Great stuff!

@nsdeschenes
nsdeschenes merged commit 696fc6b into master Mar 4, 2020
@nsdeschenes
nsdeschenes deleted the graphql-cycle-security-check branch March 4, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants