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

Addeded 403 account block status code handling for gitlab #3471

Conversation

kashifkhan0771
Copy link
Contributor

@kashifkhan0771 kashifkhan0771 commented Oct 18, 2024

Description:

This PR handle 403 status code for Gitlab detector and verify if the user account is blocked or not and add that information in the Extra Data

JIRA Ticket:

CSM-637

Checklist:

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

Sorry, something went wrong.

@kashifkhan0771 kashifkhan0771 requested a review from a team as a code owner October 18, 2024 06:17
}
s1.ExtraData = map[string]string{
"rotation_guide": "https://howtorotate.com/docs/tutorials/gitlab/",
"version": fmt.Sprintf("%d", s.Version()),
}

if verify {
isVerified, verificationErr := s.verifyGitlab(ctx, resMatch)
isVerified, isUserBlocked, verificationErr := s.verifyGitlab(ctx, resMatch)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can verifyGitlab func returns ExtraData rather than just boolean ? IMO code readability will improve and will be consistent with verification func of other detectors.

Copy link
Collaborator

@ahrav ahrav Oct 18, 2024

Choose a reason for hiding this comment

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

I think it makes sense to prioritize consistency across the detectors, especially since we have many of them. One other aspect to consider is that returning multiple values with the same type can cause ambiguity. It's easy to mix up with bool represents what. While named return types can reduce ambiguity, they can also introduce inconsistency if not applied uniformly across the codebase.

I wonder if there is an approach where we can use a specific Error type and perform and errors.Is check?

I'm not at my computer, otherwise I'd have tried to play around with this a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually thought of that first but dropped the idea thinking that extraData will be returned for just one case. Your comments make sense. I will make that change now.

Comment on lines 129 to 135
// check if the user account is blocked or not
var apiResp gitlabMessage
if err := json.Unmarshal(body, &apiResp); err == nil {
if apiResp.Message == blockedUserMessage {
return true, true, nil
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we just find that string in the bodyString ? I would love to see @ahrav's input in this matter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

String matching on errors in general makes me uneasy, but if we’re confident in the response structure and error location, it should work. Alternatively, we could use bytes.Contains to check the entire response after consuming the body. I’m not sure about the performance difference, but I suspect bytes.Contains might be slightly faster. JSON decoding in Go is typically slow due to the standard library’s use of reflection. That said, I’d go with whichever option is clearer to everyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unmarshalling logic

Copy link
Contributor

@rgmz rgmz left a comment

Choose a reason for hiding this comment

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

What about for V2 tokens? Maybe it's worth consolidating the logic like GitHub (linked below)?

s1.Verified = isVerified
// if user account is blocked, add this information in the extra data
if isUserBlocked {
s1.ExtraData["User Account Blocked"] = "Yes"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit wordy. I think just "blocked = true" would be fine.
https://docs.gitlab.com/ee/api/users.html#as-a-regular-user

e.g.,

if userResponse.SiteAdmin {
s1.ExtraData["site_admin"] = "true"
}

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kashifkhan0771 kashifkhan0771 force-pushed the update/gitlab-403-status-handling branch from 92e9bfe to 294a2fe Compare October 21, 2024 07:07
Copy link
Collaborator

@ahrav ahrav left a comment

Choose a reason for hiding this comment

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

Nice!

@zricethezav zricethezav merged commit 97fac39 into trufflesecurity:main Oct 28, 2024
13 checks passed
@kashifkhan0771 kashifkhan0771 deleted the update/gitlab-403-status-handling branch October 29, 2024 05:40
abmussani added a commit to abmussani/trufflehog that referenced this pull request Oct 30, 2024
* main: (76 commits)
  update aws descriptions (trufflesecurity#3529)
  enforce timeout on circleci test (trufflesecurity#3528)
  rm snifftest (trufflesecurity#3527)
  Redact more source credentials (trufflesecurity#3526)
  Create global log redaction capability (trufflesecurity#3522)
  Adding basic "what is trufflehog" to the readme (trufflesecurity#3514)
  Handle custom detector response and include in extra data (trufflesecurity#3411)
  fix: fixed validation logic for `calendarific` (trufflesecurity#3480)
  fix(deps): update github.com/tailscale/depaware digest to 3d7f3b3 (trufflesecurity#3518)
  Move DecoderType into ResultWithMetadata trufflesecurity#3502
  Addeded 403 account block status code handling for gitlab (trufflesecurity#3471)
  updated gcpapplicationdefaultcredentials detector results with RawV2 (trufflesecurity#3499)
  fix(deps): update module github.com/brianvoe/gofakeit/v7 to v7.1.1 (trufflesecurity#3512)
  fix(deps): update module github.com/schollz/progressbar/v3 to v3.17.0 (trufflesecurity#3510)
  fix(deps): update module cloud.google.com/go/secretmanager to v1.14.2 (trufflesecurity#3498)
  Adds a logging section in the contributing guidelines (trufflesecurity#3509)
  fix: fixed verifcation pattern logic for `bulksms` (trufflesecurity#3478)
  Extend `algoliaadminkey` with additional checks (trufflesecurity#3459)
  fix(deps): update module google.golang.org/api to v0.203.0 (trufflesecurity#3497)
  fix: added correct api endpoint for verification & logic for Aeroworkflow (trufflesecurity#3435)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants