-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Salesforce OAuth2 Detector #4252
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
Salesforce OAuth2 Detector #4252
Conversation
ff443f0
to
a88e2e8
Compare
a88e2e8
to
4dd42c1
Compare
…r it can be a string of 19 numbers. The consumer PAT can include the characters '+/=' in addition to what is here and it can actually have a character length up to 256.
103167c
to
43f280c
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.
LGTM 🚀
case "invalid_grant": | ||
// This can mean the secret is wrong OR the user isn't configured with the app secret. | ||
// We'll treat it as a VerificationError because the key might be valid but misconfigured. | ||
return false, fmt.Errorf("verification failed: %s", errorResponse.ErrorDescription) |
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.
@shahzadhaider1 in the invalid_grant
case, are the credentials meaningful? I'm scratching my head about whether we should treat this as a verification error
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.
That's a great question. The invalid_grant
error is returned by the API when the Connected App is misconfigured in Salesforce (for example, a "Run As" user hasn't been assigned for the Client Credentials Flow) in two distinct scenarios:
- The Consumer Key is valid, but the Consumer Secret is incorrect.
- Both the Key and Secret are valid.
So, to answer your question: when we receive an invalid_grant
error, the credentials are potentially very meaningful. We know for certain that the Consumer Key is valid, but we can't distinguish between a bad secret and a configuration problem on the server.
Because of this ambiguity, treating it as a VerificationError is the safest and most accurate approach, in my opinion. It correctly tells our user, "We found a valid key, but the full credential couldn't be confirmed. The issue is either a bad secret OR a server-side setup problem."
if errors.Is(verificationErr, errNoHost) { | ||
invalidHosts.Set(domain, struct{}{}) | ||
continue | ||
} |
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.
If the host is invalid, all the combinations of key and secret should also be invalidate like here.
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.
done
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 @shahzadhaider1
Description:
This pull request introduces a new detector for Salesforce OAuth2 Client Credentials. This detector is designed to find and verify the sensitive Consumer Key and Consumer Secret pairs that applications use for server-to-server API authentication with Salesforce.
Implementation Details
The detector was built with a focus on accuracy and robust verification:
*.my.salesforce.com
)3MVG
)It intelligently creates findings for all possible combinations of these three components found within a given data chunk.
API Verification: A comprehensive verification function validates found credentials using the standard OAuth 2.0 Client Credentials Flow. This verifier is built to correctly distinguishing between:
200 OK
).invalid_client
,invalid_client_id
).invalid_grant
).Comprehensive Testing: The detector is supported by a full test suite to ensure reliability:
This new detector provides robust coverage for a critical and common type of Salesforce secret. Ready for review.
Checklist:
make test-community
)?make lint
this requires golangci-lint)?