-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
GraphQL API testing guide #577
Conversation
You have a I will review this in the coming days, as this is a new test, I'll take some time going over it. |
@PauloASilva I believe you'd be interested in having a look at this :) |
Thanks for pinging @ThunderSon. @1OREO may I suggest you to check OWASP/CheatSheetSeries/pull/434 to understand the current testing guide coverage: the PR is full of good references (may more will be available through git history). Cheers, |
@ThunderSon I've resolved the issue you mentioned (using "~"), and some terminology linter issues. |
Note the code fencing still needs a highlighting hint. Current linting issuesdocument/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:39 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
7
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:239:1 MD033/no-inline-html Inline HTML [Element: img]
8
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:240:1 MD033/no-inline-html Inline HTML [Element: img]
9
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:241:1 MD033/no-inline-html Inline HTML [Element: img]
10
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:323 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
11
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:334:4 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
12
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:334 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "``` "]
13
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:387 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
14
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md:450 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
See the template directory for details on embedding images. |
@kingthorin I've resolved all of the linter issues except for the images one. |
I think the images will turnout fine for the web deployment (ex: https://owasp.org/www-project-web-security-testing-guide/latest/6-Appendix/F-Leveraging_Dev_Tools.html) and PDF generation, I wouldn't worry about how they look on GitHub we don't really expect people to use GH as a primary means of consuming that material. |
ack, so I'll convert it to the normal way of importing images and push the update to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the first look, on top of the direct review points:
- We can join the generic attacks under a section saying that. (Injection, AuthZ/N, etc.)
- Please fix the JSON response (like close it properly, we'll see later to how to represent a bigger response). It's breaking on the representation side.
- Let's add the graphQL batching attack: https://lab.wallarm.com/graphql-batching-attack/
- Pointers at mutations with weak access control maybe?
I clearly can see this scenario shorter and more concise, but it looks like a starting place for graphql. This should take a couple of rounds before it gets accepted, so I hope that doesn't really bother you.
Rick should be doing a review soon, just as they're more free with life overall. Everything is open for discussions, and if I can help somehow let me know :)
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
I believe you removed references to these 4 images, so could you remove the files themselves from the PR? ![GraphiQL1](images/GraphiQL1.png)
![GraphiQL2](images/GraphiQL2.png)
![GraphiQL3](images/GraphiQL3.png)
![GraphiQL4](images/GraphiQL4.png) (Unless I missed something, please correct me if I'm wrong.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks ⭐
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
…Testing_GraphQL.md big shebang punctuation Co-authored-by: Luke <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those tweaks
From my beginners PoV, it seems really good to me, it is easy to follow, covers the most important stuff and has references for further understanding and experimenting. Also it is just the right balance between enough information and not being cluttered. Kudos! 👍 💯 |
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Show resolved
Hide resolved
…Testing_GraphQL.md
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
document/4-Web_Application_Security_Testing/12-API_Testing/01-Testing_GraphQL.md
Outdated
Show resolved
Hide resolved
Signed-off-by: kingthorin <[email protected]>
This PR covers issue #553 .
What did this PR accomplish?