-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: added correct verification endpoint & validation logic for alegra #3437
fix: added correct verification endpoint & validation logic for alegra #3437
Conversation
pkg/detectors/alegra/alegra.go
Outdated
@@ -60,7 +60,7 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result | |||
} | |||
|
|||
if verify { | |||
req, err := http.NewRequestWithContext(ctx, "GET", "https://api.alegra.com/api/v1/users", nil) | |||
req, err := http.NewRequestWithContext(ctx, "GET", "https://api.alegra.com/api/v1/users/self", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an account on Alegra and generated a token. Initially, I tested the /users
API endpoint using an API client, and it worked as expected, returning the same response as /users/self
. However, the Alegra documentation only mentions the /users/self
endpoint, and there’s no reference to /users
, so I'm unsure why it’s functioning similarly.
Additionally, I noticed an issue with the detector failing to identify my user ID, which I believe is always the email used during signup. The regex used for detecting IDs has a length constraint of 25 to 30 characters. However, I was able to create an account with an email shorter than this limit, and despite using that email (as my username) along with my token to successfully call the API via the API client, the detector did not pick it up.
I suggest testing and adjusting the regex for detecting user IDs to ensure it covers valid email lengths.
As for the API behavior, I’ll leave that to @zricethezav and @abmussani. Both /users
and /users/self
worked for me, though only the /users/self
endpoint is mentioned in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed review, @kashifkhan0771 !
I have expanded the email regex to include all case for any possible email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would feel more confident approving this if you could also test the current email regex to confirm that it isn't working correctly(I tested it but a second round of testing from the PR owner will be good I believe. You can create an account and test this email regex change). A screenshot or any form of documentation from your testing would be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the testing screenshot, please check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New Changes are picking up the key and ID correctly now and verifying. Wait for @zricethezav and @abmussani to respond about the API change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you apply the label for Hacktoberfest @kashifkhan0771?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the PR is approved. We can add it 😃 - Once again thanks for the all the fixes and contributions!
pkg/detectors/alegra/alegra.go
Outdated
@@ -24,7 +24,7 @@ var ( | |||
|
|||
// Make sure that your group is surrounded in boundary characters such as below to reduce false positives. | |||
keyPat = regexp.MustCompile(detectors.PrefixRegex([]string{"alegra"}) + `\b([a-z0-9-]{20})\b`) | |||
idPat = regexp.MustCompile(detectors.PrefixRegex([]string{"alegra"}) + `\b([a-zA-Z0-9\.\-\@]{25,30})\b`) | |||
idPat = regexp.MustCompile(detectors.PrefixRegex([]string{"alegra"}) + `\b([a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+\.[a-z]+)\b`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: We need to standardize on a email regex. @rgmz you're a regex wizard, got one handy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trufflehog/pkg/common/patterns.go
Line 10 in 86d2c6d
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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I use this, I actually picked this pattern from one of the existing detectors only.
emailPat = regexp.MustCompile(`\b([a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+\.[a-z]+)\b`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please use const EmailPattern
for consistency. We are planning to standardize across all the detectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgmz updated this to alchemy pattern too.
f5c6a30
to
d734bd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are working fine now. I tested locally. One thing I noticed is if we add an email like [email protected]
it actually picks the whole string including alegra_email=
😄 and results in unverified result. If we can somehow avoid that it will be more better. Anyway approving the PR as it is working as expected now.
Thanks @kashifkhan0771 , can you please re-run the workflow again, for some reason it is failing on GitHub, locally works fine. Also I am thinking of a case where we actually have an email like |
Can we merge this @zricethezav ? |
Description:
Fixes #3436
Checklist:
make test-community
)?make lint
this requires golangci-lint)?