Skip to content

(fix) Indeterminate verification issue in Clickup detector#4047

Merged
nabeelalam merged 6 commits into
trufflesecurity:mainfrom
abmussani:clickup-definitive-verification-errors
Apr 17, 2025
Merged

(fix) Indeterminate verification issue in Clickup detector#4047
nabeelalam merged 6 commits into
trufflesecurity:mainfrom
abmussani:clickup-definitive-verification-errors

Conversation

@abmussani

Copy link
Copy Markdown
Contributor

Description:

This PR covers all the cases which can cause indeterminate verification issue in Clickup Detector. Also, I noted that Clickup has a very little modification in its token structure.

Checklist:

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

@abmussani abmussani requested a review from a team April 14, 2025 13:58

for _, match := range matches {
resMatch := strings.TrimSpace(match[1])
for resMatch, _ := range uniqueMatches {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Looks like the linter isn't a huge fan of the blank identifier here. We can get rid of it since we only need the key from the map.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Strange part is my golangci-lint version is not raising this issue.

@nabeelalam

Copy link
Copy Markdown
Contributor

Suggestion: We can update the integration test for the failed verification with error case:

{
	name: "found verifiable secret, verification failed due to unexpected API response",
	s:    Scanner{client: common.ConstantResponseHttpClient(404, "")},
	args: args{
		ctx:    context.Background(),
		data:   []byte(fmt.Sprintf("You can find a clickuppersonaltoken secret %s within", secret)),
		verify: true,
	},
	want: []detectors.Result {
		{
			DetectorType: detectorspb.DetectorType_ClickupPersonalToken,
			Verified:     false,
		}
	},
	wantErr: false,
	wantVerificationErr: true,
},

@abmussani

Copy link
Copy Markdown
Contributor Author

Suggestion: We can update the integration test for the failed verification with error case:

{
	name: "found verifiable secret, verification failed due to unexpected API response",
	s:    Scanner{client: common.ConstantResponseHttpClient(404, "")},
	args: args{
		ctx:    context.Background(),
		data:   []byte(fmt.Sprintf("You can find a clickuppersonaltoken secret %s within", secret)),
		verify: true,
	},
	want: []detectors.Result {
		{
			DetectorType: detectorspb.DetectorType_ClickupPersonalToken,
			Verified:     false,
		}
	},
	wantErr: false,
	wantVerificationErr: true,
},

I have added the test case failed verification error.

s := Scanner{}
got, err := s.FromData(tt.args.ctx, tt.args.verify, tt.args.data)
// s := Scanner{}
got, err := tt.s.FromData(tt.args.ctx, tt.args.verify, tt.args.data)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this commented line can be removed

Comment thread pkg/detectors/clickuppersonaltoken/clickuppersonaltoken.go Outdated
removed commented code from integration test.

@nabeelalam nabeelalam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. Thanks @abmussani

@nabeelalam nabeelalam merged commit b9ab701 into trufflesecurity:main Apr 17, 2025
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.

4 participants