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

fix: corrected verification endpoint & validation logic for bombbomb #3462

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sahil9001
Copy link
Contributor

@sahil9001 sahil9001 commented Oct 17, 2024

Description:

Fixes #3461 , tests:
Screenshot 2024-10-18 at 1 36 31 AM

Checklist:

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

@sahil9001 sahil9001 changed the title fix: corrected verification endpoint for bombbomb fix: corrected verification endpoint & validation logic for bombbomb Oct 17, 2024
@@ -20,7 +21,7 @@ var (
client = common.SaneHttpClient()

// Make sure that your group is surrounded in boundary characters such as below to reduce false positives.
keyPat = regexp.MustCompile(detectors.PrefixRegex([]string{"bombbomb"}) + `\b([a-zA-Z0-9-._]{704})\b`)
keyPat = regexp.MustCompile(detectors.PrefixRegex([]string{"bombbomb"}) + `\b(eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,})\b`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you had a chance to generate an API key from the BombBomb portal? I'm not entirely sure if I might have made a mistake, but when I copy the API key from the BombBomb integrations page, it doesn't seem to match the regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is because it is not the integrations API Key, the endpoint corresponds to user information, for that you can get token by Network Tab on Chrome

Copy link
Contributor Author

@sahil9001 sahil9001 Oct 21, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-10-21 at 4 33 29 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not exactly sure about this. I'll let someone else review and comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well you can try this though, go to this website : https://developer.bombbomb.com/api#

and then click on authenticate button, it will give you an API token to test

Screenshot 2024-10-21 at 6 10 08 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kashifkhan0771 can you check and reply here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll check today @sahil9001

@@ -20,7 +21,7 @@ var (
client = common.SaneHttpClient()

// Make sure that your group is surrounded in boundary characters such as below to reduce false positives.
keyPat = regexp.MustCompile(detectors.PrefixRegex([]string{"bombbomb"}) + `\b([a-zA-Z0-9-._]{704})\b`)
keyPat = regexp.MustCompile(detectors.PrefixRegex([]string{"bombbomb"}) + `\b(eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,})\b`)
Copy link
Contributor

Choose a reason for hiding this comment

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

The generated secrets are JWTs, and I recommend using a standard JWT regex pattern: \b[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\b. Additionally, I suggest adding this pattern to the common patterns here so that it can be utilized by other detectors that look for JWT secrets.

@zricethezav @mcastorina @abmussani thoughts on adding JWT in common patterns? ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please check @kashifkhan0771

@sahil9001 sahil9001 requested a review from a team as a code owner October 28, 2024 14:45
@@ -8,6 +8,7 @@ import (
)

const EmailPattern = `\b(?:[a-z0-9!#$%&'*+/=?^_\x60{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_\x60{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9]))\.){3}(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9])|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])\b`
const JWTPattern = `\b[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\b`
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sahil9001 this is too loose. Take a look at this for a reference https://github.com/gitleaks/gitleaks/blob/43fae355e6fe4d99d2a7b240a224b85e2903aeb4/config/gitleaks.toml#L2311 or even

keyPat = regexp.MustCompile(detectors.PrefixRegex([]string{"formio"}) + `\b(eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9\.[0-9A-Za-z]{220,310}\.[0-9A-Z-a-z\-_]{43}[ \r\n]{1})`)
. The first section might be consistent across all bombomb tokens.

Copy link
Contributor Author

@sahil9001 sahil9001 Oct 28, 2024

Choose a reason for hiding this comment

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

No, it changes with each refresh, apparently there is a jti field which changes each time which eventually changes the first part each time as well.

Screenshot 2024-10-28 at 9 04 41 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Issue with incorrect verification endpoint and validation logic for Bombbomb
4 participants