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

Improve the atack generation of graphql queries and mutations #122

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

bandronic
Copy link

I have fixed some issues in the generation of the graphql queries.
The issues I faced where that the extension did not create corresponding separate operations and did not remove the comments from the code.

What I basically did was:

  • stripped all the comments
  • set the query type to query or mutation based on the action type
  • used the already existing logic to create multiple operations with different name

@bandronic bandronic changed the base branch from master to dev July 25, 2023 07:18
@bandronic bandronic changed the title Atkrework Improve the atack generation of graphql queries and mutations Jul 25, 2023
@execveat
Copy link
Collaborator

Hey @bandronic and thanks for contributing! I'm a little puzzled about the commit c99e676, as nothing seems to be using this new library. Can you explain the reason for adding it?

Also, I have to warn you that we'll attempt rewriting the bulk of the extension in Kotlin soon. The GraphQL parsing will still be done in Python (for a while), using the https://github.com/doyensec/gqlspection library. So, if you plan to add more improvements to the batching attacks and want to write Python, gqlspection might be a better target.

@bandronic
Copy link
Author

Hey, thanks for getting back to me. Yes, I will remove that library as it was a test for a different parser. In this case, should I continue improving this or should I just forget it? Thanks!

@bandronic
Copy link
Author

I believe that this implementation will fix issue #97 if I'm not mistaking

@execveat
Copy link
Collaborator

We'll start the rewrite in August and we'll try to keep it 1:1 or as close as possible to the existing Python code. So, I'd say if you see other improvement opportunities that are few lines long - go for it (with the understanding that it will likely get translated to Kotlin before the next release). On the other hand, if you have larger features in mind, it's probably best to wait for the rewrite, to make merging things easier.

On the topic of external libraries - once we're in Kotlin-land, I'm fine with bringing external dependencies, so if you have suitable GraphQL parsing libraries (Java/Kotlin) in mind, please share. We could either import them directly to replace GQLSpection or rewrite GQLSpection to Kotlin (possibly replacing core with some upstream libraries). We're focusing on rewriting GUI stuff right now, so I haven't researched this and any help / proof of concepts would be very helpful.

@bandronic
Copy link
Author

I understand. Thanks for the explanation. I was starting to write an improvement for value generation for different data types and expansion of the input arguments as in if you have an input type, expand it to it's most basic scalar types, but in this case I'll wait for the rewrite. Regarding this pull request, in this case is it still valid?

@execveat
Copy link
Collaborator

Oh, that's a great feature! Might be relevant: we have a plan to generate queries with variables (#77), although I'm still not sure about UI.

The pull request is good! I want to take another look at it over the weekend as the Attacker was left behind for a while and there might be more bugs lurking there.

@bandronic
Copy link
Author

Ah yes, I remember I saw a ticket regarding this but I didn't remember which one. Please let me know if I can help in any way. Thanks!

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

2 participants