doyensec / inql

InQL is a robust, open-source Burp Suite extension for advanced GraphQL testing, offering intuitive vulnerability detection, customizable scans, and seamless Burp integration.
https://doyensec.com/
Apache License 2.0
1.53k stars 158 forks source link

Bug fix (header scraping and grapiql custom headers) #83

Closed matteoldani closed 1 year ago

matteoldani commented 1 year ago

This pull request aims to fix the bug related to those issues:

The main contribution was made to address the first bug. However, with the new shared dictionaries, I did not see any race condition during the tests and the custom headers set in the scanner tab are not used when a query is sent to GraphiQL.

The information used to build the request to send the generated queries is taken directly for the introspection request made during the scan. No more data is scraped from the requests intercepted by the proxy. The default headers used in the scan process are augmented with custom headers set in the extension scanner tab.

thypon commented 1 year ago

Is the auto-guessing still working?

matteoldani commented 1 year ago

Is the auto-guessing still working?

I'm not sure what is considered "auto-guessing" and what was actually working before.

In the version I submitted, the traffic throughout the proxy is not considered so auth headers are not automatically added when a query is sent to the repeater. However, even in the current version, the introspection query sent from the InQL scanner tab does not include any scraped headers so you already need to add custom headers if required to make the scan. Those headers will be carried around if you send the request to repeater/GraphiQL. In addition, while testing the current version I noticed that headers are scraped only from traffic that happened after the scan query. The execution of the following steps does not lead to augmented headers:

While the following, as mentioned in the bug report, will result in additional headers being added:

To conclude, can I have a better definition of what the auto-guesser is supposed to do? I'm fairly new to this extension so I may have misunderstood some behaviours

thypon commented 1 year ago

The idea is inheriting the same headers the web-app is using, after you scrape it navigating it. Your definition seems to be right. The addition was because the users were not understanding why InQL was not working, this includes James Kettle. We removed 70/80% of requests for "it's not working" so I'd rather not remove that feature and fix the underlying race.

thypon commented 1 year ago

To be more clear, when this feature works 30% of the times, it will remove 30% of the bogus bug reports.

matteoldani commented 1 year ago

I might have to rethink something but, to preserve the feature while giving it some intuitiveness, what if I can add a popup window at the moment of the pressing of the "load" button in the scanner? The idea is to let the user choose which of the scraped headers wants to use for the scan. Those headers will be then added to the custom headers that can be modified and included in every subsequent forward of the request.

Will this approach be acceptable?

If not, I'm a bit lost because I don't have any idea (yet) on how to integrate the auto-guessing with the ability of a user to choose which headers to use

thypon commented 1 year ago

An idea, 2 cases:

matteoldani commented 1 year ago

Yes, thanks. It is similar to what I had in mind

execveat commented 1 year ago

We just had a call with @matteoldani and decided to proceed in three stages.

  1. remove the header scraping completely and clean up the code
  2. add the bare minimum functionality that is necessary for InQL to work
  3. add the convenience functionality like auto-guessing the headers

The goal is to get code simplified and modularized for the pending Kotlin rewrite, while at the same time assuring that at least base functionality is solid and extension keeps being usable.

The steps 1) and 2) are a must have by the next release while 3) is set for the version 5.1.

@matteoldani in this pull request please focus on achieving the step 1) - that is fixing #67 and #69 as you mentioned (as well as cleaning the code while you're at it). I will open a new issue listing the use cases that should work to fulfill step 2) and you'll implement those in a separate pull request.

execveat commented 1 year ago

@matteoldani here are the requirements for the second pull request: https://github.com/doyensec/inql/issues/84

matteoldani commented 1 year ago

I should have removed the old scraping method and fixed the custom headers for requests sent with GraphiQL. Meanwhile, I restructured the HTTPMutator and the HTTP handler for the local host server that handles GraphiQL requests.

execveat commented 1 year ago

Good job! I'll merge this PR to push further refactoring on top of it. Please submit a second PR for the rest.