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

GraphQL API testing guide #577

Merged
merged 38 commits into from
Nov 15, 2020
Merged

GraphQL API testing guide #577

merged 38 commits into from
Nov 15, 2020

Conversation

OrAsaf
Copy link
Contributor

@OrAsaf OrAsaf commented Oct 6, 2020

This PR covers issue #553 .

  • This PR handles the issue and requires no additional PRs.
  • You have validated the need for this change.

What did this PR accomplish?

  • Created section 12-API testing
  • Created GraphQL testing guide

@OrAsaf OrAsaf mentioned this pull request Oct 6, 2020
1 task
@ThunderSon
Copy link
Collaborator

ThunderSon commented Oct 6, 2020

You have a ~~~ instead of triple ticks, if you can fix that.
So I have a proposition. How about splitting the test inside a folder for GraphQL, where every test scenario takes its own file? More concise, more focused. What do you think? The SQLi is an example, where every DB has its own file

I will review this in the coming days, as this is a new test, I'll take some time going over it.

@ThunderSon ThunderSon self-requested a review October 6, 2020 10:37
@ThunderSon
Copy link
Collaborator

@PauloASilva I believe you'd be interested in having a look at this :)
If you know any additional GraphQL experts as well, be my guest in inviting them!

@PauloASilva
Copy link

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,
Paulo A. Silva

@OrAsaf
Copy link
Contributor Author

OrAsaf commented Oct 7, 2020

@ThunderSon I've resolved the issue you mentioned (using "~"), and some terminology linter issues.
I can understand why separating the file into smaller pieces will be good (they will be more focused as well as easier for more contributors to add GraphQL related test).
I'm fine either way, when you're done reviewing and know how to best proceed, ping me.
I've also looked at the cheatsheet PR, so also let me know if we want to include it as a reference, and add content to the wstg file.

@kingthorin
Copy link
Collaborator

kingthorin commented Oct 7, 2020

Note the code fencing still needs a highlighting hint.
The linter workflow is failing to comment due to an output/content issue (which I’ll look into and address).

Current linting issues
document/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.

@OrAsaf
Copy link
Contributor Author

OrAsaf commented Oct 8, 2020

@kingthorin I've resolved all of the linter issues except for the images one.
The problem I had is the screenshots I took were not very wide, which resulted in being a huge zoomed-in picture and I did not find any other way to resize them (if you'll look at the file, all of the other images are included regularly within markdown using ).
I can revert them back but I think it'll look quite bad.
Is there another solution you can direct me to?

@kingthorin
Copy link
Collaborator

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.

@OrAsaf
Copy link
Contributor Author

OrAsaf commented Oct 9, 2020

ack, so I'll convert it to the normal way of importing images and push the update to the PR.

Copy link
Collaborator

@ThunderSon ThunderSon left a 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:

  1. We can join the generic attacks under a section saying that. (Injection, AuthZ/N, etc.)
  2. 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.
  3. Let's add the graphQL batching attack: https://lab.wallarm.com/graphql-batching-attack/
  4. 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 :)

@kingthorin
Copy link
Collaborator

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.)

@OWASP OWASP deleted a comment from github-actions bot Nov 10, 2020
Copy link
Collaborator

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

Looks good, thanks ⭐

OrAsaf and others added 2 commits November 12, 2020 10:53
…Testing_GraphQL.md


big shebang punctuation

Co-authored-by: Luke <ljm3103@rit.edu>
Copy link
Collaborator

@kingthorin kingthorin left a 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

@ghost
Copy link

ghost commented Nov 13, 2020

@1OREO @ThunderSon

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! 👍 💯

kingthorin
kingthorin previously approved these changes Nov 15, 2020
Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
@kingthorin kingthorin merged commit 617aacb into OWASP:master Nov 15, 2020
@OrAsaf OrAsaf deleted the GraphQL branch November 15, 2020 22:21
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.

None yet

8 participants