Handle custom detector response and include in extra data #3411

kashifkhan0771 · 2024-10-15T06:27:17Z

Description:

This PR handle custom detector response and include it in the ExtraData

Issue:

#3385

Checklist:

Tests passing (make test-community)?
Lint passing (make lint this requires golangci-lint)?

zricethezav · 2024-10-15T13:41:30Z

@kashifkhan0771 could you take a look at the test failure:

panic: assignment to entry in nil map

goroutine 191 [running]:
github.com/trufflesecurity/trufflehog/v3/pkg/custom_detectors.(*CustomRegexWebhook).createResults(0xc0007f4770, {0x7fd2f8a84330, 0xc0014fc240}, 0xc0014fc330, 0x1, 0xc0014255e0)
	/home/runner/work/trufflehog/trufflehog/pkg/custom_detectors/custom_detectors.go:208 +0xb05
github.com/trufflesecurity/trufflehog/v3/pkg/custom_detectors.(*CustomRegexWebhook).FromData.func1()
	/home/runner/work/trufflehog/trufflehog/pkg/custom_detectors/custom_detectors.go:97 +0x2b
golang.org/x/sync/errgroup.(*Group).Go.func1()
	/home/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:78 +0x50
created by golang.org/x/sync/errgroup.(*Group).Go in goroutine 172
	/home/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:75 +0x96
FAIL	github.com/trufflesecurity/trufflehog/v3/pkg/engine	1.[302](https://github.com/trufflesecurity/trufflehog/actions/runs/11340538845/job/31537134772#step:4:303)s

kashifkhan0771 · 2024-10-15T15:16:16Z

@kashifkhan0771 could you take a look at the test failure:
@zricethezav It is fixed now.

mcastorina · 2024-10-15T15:52:10Z

pkg/custom_detectors/custom_detectors.go

+			if err != nil {
+				return fmt.Errorf("failed to read response body: %v", err)
+			}


This looks like it's returning an error for the FromData call, is that right? I think that may not be what we want to do for two reasons:

it will break the loop so other configs aren't tested

it seems like it would be a non-determinant error (if those exist in custom detectors), and should maybe set the verification error

There other parts that also return an error, this is just one example.

I think I did that out of habit. I’ve fixed it now, and we’re continuing to the next step instead of returning an error.

mcastorina · 2024-10-15T15:53:28Z

pkg/custom_detectors/custom_detectors.go

+// helper function to handle JSON response
+func handleJSONResponse(body []byte) (string, error) {
+	var respBody interface{}
+	err := json.Unmarshal(body, &respBody)
+	if err != nil {
+		return "", fmt.Errorf("failed to unmarshal JSON: %v", err)
+	}
+
+	// convert JSON map to a formatted string
+	jsonString, err := json.MarshalIndent(respBody, "", "  ")
+	if err != nil {
+		return "", fmt.Errorf("failed to marshal JSON: %v", err)
+	}
+
+	return strings.TrimSpace(string(jsonString)), nil
+}


Why does this function unmarshal then marshal?

Ah, I think I might have over-engineered this, even though @ahrav warned me not to! 😅 I was trying to get the response into a nicely formatted JSON string, but I realize now that it's not really necessary. We can just stick to simple string conversion instead.

abmussani · 2024-10-15T16:15:45Z

pkg/custom_detectors/custom_detectors.go

+
+			// read the Content-Type header and response body
+			respContentType := resp.Header.Get("Content-Type")
+			body, err := io.ReadAll(resp.Body)


@mcastorina I could not recall where we had a discussion about PII. I remember, we decided not to include PII in extraData. Will this needed to be care here as well ?

That's a great point. Custom detector servers are user supplied (or at least user configured), so I don't think PII would be an issue here.. @zricethezav what are your thoughts?

see https://github.com/trufflesecurity/trufflehog/pull/3411/files#r1809008021

zricethezav · 2024-10-21T15:06:37Z

pkg/custom_detectors/custom_detectors.go

+			responseStr := string(body)
+
+			// store the processed response in ExtraData
+			result.ExtraData["response"] = responseStr


@kashifkhan0771 Thought: storing the entire response could lead to some large outputs. This could be annoying for users who already are using custom detectors.

One option could be limit the response to 200 characters. I don't feel comfortable merging this PR as is.

@kashifkhan0771 additionally, I tried spinning up a verification server and testing this myself but found that the only thing that could printed in the output was the name of the customer verifier: "ExtraData":{"name":"HogTokenDetector"} when using the --json flag. A mock test might help here, although I have a suspicion there might be some other issues considering I'm not able to surface additional ExtraData past the name of the custom verifier in the output.

@CameronLonsdale what is your use case for ExtraData? How are you using TruffleHog to surface this data (what command and flags)?

For me, I always run trufflehog with --json --only-verified. I wanted to surface data from my custom patterns that are the responses from verifying the credential against the API. So this would typically be a JSON blob which tells me info about the user which corresponds to the token I just verified.

I wrap trufflehog around custom code to sync results to Jira etc so hence why JSON. But I would expect the extraData to show up in CLI output aswell, so truncating it to be readable at the terminal makes sense.

@zricethezav @CameronLonsdale I added the limit of 200 chars for response. I am not sure how to test the custom detector. If anyone of you can guide me how to do it. I can test locally to understand why it is not printing the extra data in output.

Thank you, @mcastorina, for your guidance on running the custom detector. I successfully tested it and identified the issue that was preventing the results from printing. I've fixed it now. Please review my changes.

kashifkhan0771 · 2024-10-23T08:20:54Z

pkg/custom_detectors/custom_detectors.go

-		// NOTE: I don't believe this is being set anywhere else, hence the map assignment.
-		result.ExtraData = map[string]string{
-			"name": c.GetName(),
-		}


This was the reason the response result wasn't being printed! 😛 @zricethezav

Let me know if the name is something we want to print. I can make the change to append it.

kashifkhan0771 requested a review from a team as a code owner October 15, 2024 06:27

kashifkhan0771 linked an issue Oct 15, 2024 that may be closed by this pull request

Include ExtraData from customDetector http response #3385

Open

mcastorina reviewed Oct 15, 2024

View reviewed changes

abmussani reviewed Oct 15, 2024

View reviewed changes

kashifkhan0771 requested review from mcastorina and abmussani October 16, 2024 15:50

zricethezav reviewed Oct 21, 2024

View reviewed changes

kashifkhan0771 added 5 commits October 22, 2024 11:18

Handle custom detector response and include in extra data

fb478d4

Added todo

3662976

fixed panic

670eb3d

simplicity is always good

6dc944a

limit the response to 200 chars

36c6519

kashifkhan0771 force-pushed the update/custom-detector-resp branch from b854936 to 36c6519 Compare October 22, 2024 06:21

results should print now in output

949426f

kashifkhan0771 commented Oct 23, 2024

View reviewed changes

kashifkhan0771 requested a review from zricethezav October 23, 2024 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle custom detector response and include in extra data #3411

Handle custom detector response and include in extra data #3411

kashifkhan0771 commented Oct 15, 2024 •

edited

Loading

zricethezav commented Oct 15, 2024

kashifkhan0771 commented Oct 15, 2024

mcastorina Oct 15, 2024

mcastorina Oct 15, 2024

kashifkhan0771 Oct 15, 2024

mcastorina Oct 15, 2024

kashifkhan0771 Oct 15, 2024

abmussani Oct 15, 2024

mcastorina Oct 15, 2024

zricethezav Oct 21, 2024

zricethezav Oct 21, 2024

CameronLonsdale Oct 21, 2024

kashifkhan0771 Oct 22, 2024

kashifkhan0771 Oct 23, 2024

kashifkhan0771 Oct 23, 2024

kashifkhan0771 Oct 23, 2024

Handle custom detector response and include in extra data #3411

Are you sure you want to change the base?

Handle custom detector response and include in extra data #3411

Conversation

kashifkhan0771 commented Oct 15, 2024 • edited Loading

Description:

Issue:

Checklist:

zricethezav commented Oct 15, 2024

kashifkhan0771 commented Oct 15, 2024

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

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

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

kashifkhan0771 commented Oct 15, 2024 •

edited

Loading