graphql: Add tech detection for GraphQL Engines #5843

kingthorin · 2024-10-23T19:11:20Z

Overview

CHANGELOG > Added note.
build file > Added dependency.
GraphQlFingerprinter > Updated to add Technology matches when GraphQL engines are fingerprinted.
GraphQlFingerprinterUnitTest > Updated for new handling.
DiscoveredGraphQlEngineHandler > An interface to be implemented for consumption/handling of discovered GraphQL engines.
ExtensionTechDetection > Added to facilitate optional dependence.
ExtensionTechDetectionUnitTest > Tests :)
Messages.properties > Added key/value pairs to support the optional dependency.
alert.html > Added note about the new functionality.

Ex: See the Apollo entry:

This seems to be a decent place for testing: https://countries.trevorblades.com/graphql
(Note if you do serial imports you will hit a limit and need to wait a min or so.)

Related Issues

N/A

Checklist

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java

kingthorin · 2024-10-23T19:15:50Z

I did try adding this to GraphQlFingerprinterUnitTest#shouldFingerprintValidData and verify call to ExtensionWappalyzer.addApplicationsToSite but the best I could do was have it work on the first of the parameterized runs, subsequently it would claim zero interactions with the mock 🤷‍♂️

addOns/graphql/graphql.gradle.kts

...ns/graphql/src/main/java/org/zaproxy/addon/graphql/techdetection/ExtensionTechDetection.java

kingthorin · 2024-10-25T01:12:08Z

I should probably add some more tests. But, I'd be glad to have any feedback in the meantime.

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java

...ns/graphql/src/main/java/org/zaproxy/addon/graphql/techdetection/ExtensionTechDetection.java

addOns/graphql/src/test/java/org/zaproxy/addon/graphql/GraphQlFingerprinterUnitTest.java

kingthorin · 2024-10-25T15:28:55Z

addOns/graphql/src/test/java/org/zaproxy/addon/graphql/GraphQlFingerprinterUnitTest.java

+        Object[] arguments = new Object[2];
+        GraphQlFingerprinter.setAppConsumer(
+                (site, engine) -> {
+                    arguments[0] = site.toString();
+                    arguments[1] = engine;
+                });


Used an array because of the whole lambda 'must be final or effectively final' restriction.

I guess alternatively I could have created an assertion method that accepts the two values?

addOns/wappalyzer/src/main/java/org/zaproxy/zap/extension/wappalyzer/ExtensionWappalyzer.java

kingthorin · 2024-10-29T10:42:36Z

Rebased current and deconflicted.

kingthorin · 2024-11-08T19:49:11Z

Previous review has been addressed

kingthorin · 2024-11-11T18:16:47Z

Rebased current and deconflicted, again.

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java

kingthorin · 2024-11-18T00:44:04Z

Rebased current and deconflicted, yet again.

ricekot · 2024-11-19T18:54:37Z

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java


    private final Requestor requestor;
    private final Map<String, HttpMessage> queryCache;

    private HttpMessage lastQueryMsg;
    private String matchedString;
+    private static BiConsumer<URI, Engine> appConsumer = DEFAULT_APP_CONSUMER;


I know I'm late to review but a few notes on the appConsumer...

The Engine (DiscoveredGraphQlEngine?) could include the URI

The appConsumer (discoveredEnginesListener?) could be a functional interface.

GraphQlFingerprinter could keep a list of listeners instead of a single listener.

The raiseFingerPrintingAlert method can also be added as a listener.

This should make the fingerprinter flexible even for future features that rely on GraphQL engines. That being said I'm good with the PR as-is too and this could even be done in a follow-up PR.

I see the vision. I'll tweak for the first item.

I think I'll hold on the listener implementation for another PR. (This one has been dragging I'd like to just get it merged 😇)

Isn't a Consumer already a functional interface? Is there a reason to not just simply use a list of Consumers?

If I'm right it wouldn't be too hard to change for this PR.

@ricekot I'm pretty sure I'm right the more I think about this.

I can create List<Consumer> consumers; Instead of the existing default consumer being noop I could turn it into raiseFingerPrintingAlert being the default. If/when the conditions are proper then the Tech Detection consumer can be added. Instead of calling each separately as things currently are I can do something like: consumers.forEach(consumer -> consumer.accept(discoveredGraphQlEngine));

Sounds good to me.

#5843 (comment)

There are reasons/benefits to use a custom interface rather than Consumer (or any variant of), e.g. if in the future there's the need to add other methods or parameters. If you use Consumer you will not be able to do that, with the custom interface you can rely on default methods.

#5843 (comment)

Never late :) I was not thinking this would be used by/for other purposes in the future, if that's the intent we should expose the functionality through the extension not as static methods in "internal" classes, proper JavaDoc should be added and so on. (Unless you mean as internal functionality?)

Okay I can make a custom interface, thanks for talking through it!

I believe it would only be used 'internally' to the add-on but @ricekot feel free to correct me if you see it differently.

Reworked 😀

Unless you mean as internal functionality?

I meant generally but at the moment I can't think of ways this would be used outside the add-on.

...hql/src/main/java/org/zaproxy/addon/graphql/techdetection/ExtensionTechDetectionGraphQl.java

kingthorin · 2024-11-27T03:10:32Z

Hey crew, is this implementation better?

kingthorin · 2024-12-02T11:27:52Z

@ricekot does this implementation seem better?

kingthorin · 2024-12-08T01:14:09Z

Bump

addOns/graphql/CHANGELOG.md

addOns/graphql/src/test/java/org/zaproxy/addon/graphql/GraphQlFingerprinterUnitTest.java

addOns/graphql/src/main/javahelp/org/zaproxy/addon/graphql/resources/help/contents/alerts.html

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java

kingthorin · 2024-12-13T13:28:25Z

@ricekot got all those.

@thc202 I did tackle all your original comments (I believe) I just didn't resolve them.

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java

addOns/wappalyzer/src/main/java/org/zaproxy/zap/extension/wappalyzer/ExtensionWappalyzer.java

kingthorin · 2024-12-13T19:15:34Z

Feedback addressed.

thc202 · 2024-12-13T19:18:15Z

This one still pending #5843 (comment)

addOns/wappalyzer/src/main/java/org/zaproxy/zap/extension/wappalyzer/ExtensionWappalyzer.java

addOns/graphql/graphql.gradle.kts

- CHANGELOG > Added note. - build file > Added dependency. - GraphQlFingerprinter > Updated to add Technology matches when GraphQL engines are fingerprinted. - GraphQlFingerprinterUnitTest > Updated for new handling. - DiscoveredGraphQlEngineHandler > An interface to be implemented for consumption/handling of discovered GraphQL engines. - ExtensionTechDetection > Added to facilitate optional dependence. - ExtensionTechDetectionUnitTest > Tests :) - Messages.properties > Added key/value pairs to support the optional dependency. - alert.html > Added note about the new functionality. Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>

kingthorin · 2024-12-13T22:39:59Z

Ok I think I got all those now too.

thc202 · 2024-12-14T08:23:50Z

Thank you!

ricekot

Thanks!

kingthorin commented Oct 23, 2024

View reviewed changes

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java Outdated Show resolved Hide resolved

thc202 reviewed Oct 23, 2024

View reviewed changes

addOns/graphql/graphql.gradle.kts Outdated Show resolved Hide resolved

kingthorin force-pushed the gql-td branch 6 times, most recently from 98ca05d to 5a9f162 Compare October 24, 2024 18:41

kingthorin commented Oct 24, 2024

View reviewed changes

...ns/graphql/src/main/java/org/zaproxy/addon/graphql/techdetection/ExtensionTechDetection.java Outdated Show resolved Hide resolved

thc202 reviewed Oct 25, 2024

View reviewed changes

kingthorin force-pushed the gql-td branch 3 times, most recently from d8a916e to 7bd1a00 Compare October 25, 2024 15:25

kingthorin commented Oct 25, 2024

View reviewed changes

addOns/graphql/src/test/java/org/zaproxy/addon/graphql/GraphQlFingerprinterUnitTest.java Show resolved Hide resolved

kingthorin commented Oct 25, 2024

View reviewed changes

addOns/wappalyzer/src/main/java/org/zaproxy/zap/extension/wappalyzer/ExtensionWappalyzer.java Show resolved Hide resolved

kingthorin force-pushed the gql-td branch from 7bd1a00 to 3dfec4a Compare October 29, 2024 10:41

kingthorin force-pushed the gql-td branch from 3dfec4a to 2ddbab3 Compare November 11, 2024 18:16

kingthorin commented Nov 11, 2024

View reviewed changes

addOns/graphql/src/main/java/org/zaproxy/addon/graphql/GraphQlFingerprinter.java Outdated Show resolved Hide resolved

kingthorin force-pushed the gql-td branch from 2ddbab3 to f3bddc1 Compare November 18, 2024 00:42

ricekot reviewed Nov 19, 2024

View reviewed changes

kingthorin force-pushed the gql-td branch 3 times, most recently from d0c49b0 to f64c3ec Compare November 19, 2024 20:45

kingthorin force-pushed the gql-td branch 4 times, most recently from 79e1915 to 29efe57 Compare November 20, 2024 19:11

kingthorin added the backlog label Nov 28, 2024

kingthorin force-pushed the gql-td branch from 29efe57 to 9e0fa19 Compare December 12, 2024 21:47

This comment was marked as outdated.

Sign in to view

ricekot reviewed Dec 13, 2024

View reviewed changes

kingthorin force-pushed the gql-td branch 2 times, most recently from 5488bfd to 558c1bb Compare December 13, 2024 13:26

thc202 reviewed Dec 13, 2024

View reviewed changes

thc202 removed the backlog label Dec 13, 2024

kingthorin force-pushed the gql-td branch from 558c1bb to 8cb8c1b Compare December 13, 2024 19:15

thc202 reviewed Dec 13, 2024

View reviewed changes

addOns/wappalyzer/src/main/java/org/zaproxy/zap/extension/wappalyzer/ExtensionWappalyzer.java Show resolved Hide resolved

kingthorin commented Dec 13, 2024

View reviewed changes

addOns/graphql/graphql.gradle.kts Outdated Show resolved Hide resolved

kingthorin force-pushed the gql-td branch from 1ac87e6 to 03a0902 Compare December 13, 2024 22:38

thc202 approved these changes Dec 14, 2024

View reviewed changes

ricekot approved these changes Dec 14, 2024

View reviewed changes

ricekot merged commit 0e2491b into zaproxy:main Dec 14, 2024
10 checks passed

github-actions bot locked and limited conversation to collaborators Dec 14, 2024

kingthorin deleted the gql-td branch December 14, 2024 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

graphql: Add tech detection for GraphQL Engines #5843

graphql: Add tech detection for GraphQL Engines #5843

kingthorin commented Oct 23, 2024 •

edited

Loading

kingthorin commented Oct 23, 2024

kingthorin commented Oct 25, 2024

kingthorin Oct 25, 2024

kingthorin Oct 25, 2024

kingthorin commented Oct 29, 2024

kingthorin commented Nov 8, 2024

kingthorin commented Nov 11, 2024

kingthorin commented Nov 18, 2024

ricekot Nov 19, 2024

kingthorin Nov 19, 2024

kingthorin Nov 20, 2024

kingthorin Nov 20, 2024 •

edited

Loading

ricekot Nov 20, 2024

thc202 Nov 20, 2024

kingthorin Nov 20, 2024

kingthorin Nov 20, 2024

ricekot Dec 13, 2024 •

edited

Loading

kingthorin commented Nov 27, 2024

kingthorin commented Dec 2, 2024

kingthorin commented Dec 8, 2024

This comment was marked as outdated.

kingthorin commented Dec 13, 2024

kingthorin commented Dec 13, 2024

thc202 commented Dec 13, 2024

kingthorin commented Dec 13, 2024

thc202 commented Dec 14, 2024

ricekot left a comment

graphql: Add tech detection for GraphQL Engines #5843

graphql: Add tech detection for GraphQL Engines #5843

Conversation

kingthorin commented Oct 23, 2024 • edited Loading

Overview

Related Issues

Checklist

kingthorin commented Oct 23, 2024

kingthorin commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingthorin commented Oct 29, 2024

kingthorin commented Nov 8, 2024

kingthorin commented Nov 11, 2024

kingthorin commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingthorin Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricekot Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

kingthorin commented Nov 27, 2024

kingthorin commented Dec 2, 2024

kingthorin commented Dec 8, 2024

This comment was marked as outdated.

kingthorin commented Dec 13, 2024

kingthorin commented Dec 13, 2024

thc202 commented Dec 13, 2024

kingthorin commented Dec 13, 2024

thc202 commented Dec 14, 2024

ricekot left a comment

Choose a reason for hiding this comment

kingthorin commented Oct 23, 2024 •

edited

Loading

kingthorin Nov 20, 2024 •

edited

Loading

ricekot Dec 13, 2024 •

edited

Loading