fixed sentry auth token detector #3827
Conversation
| func verifyToken(ctx context.Context, client *http.Client, token string) (map[string]string, bool, error) { | ||
| // api docs: https://docs.sentry.io/api/organizations/ | ||
| // this api will return 200 for user auth tokens with scope of org:<> | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, "https://sentry.io/api/0/organizations/", nil) |
There was a problem hiding this comment.
/projects API require project:<> scope and /organizations API require org:<> scope. I updated to organization API because we capture the organizations from response for extraData.
There was a problem hiding this comment.
How common is it for a token to have org: scope versus project: scope?
|
|
||
| return isVerified, err | ||
| // if response contain the forbiddenError message it means the token is active but does not have the right scope for this API call | ||
| if strings.Contains(fmt.Sprintf("%v", responseBody), forbiddenError) { |
There was a problem hiding this comment.
In case the token is Active and does not have the org:<> scope the API returns 403 with a specific error message. In case token is removed the API return 401
| @@ -20,7 +20,7 @@ import ( | |||
| func TestSentryToken_FromChunk(t *testing.T) { | |||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) | |||
| defer cancel() | |||
| testSecrets, err := common.GetSecret(ctx, "trufflehog-testing", "detectors3") | |||
There was a problem hiding this comment.
detectors3 vault limit is full
| wantVerificationErr: true, | ||
| }, | ||
| { | ||
| name: "found, good key but wrong scope", |
There was a problem hiding this comment.
Removed some tests which are not necessary.
| // Ensure the Scanner satisfies the interface at compile time. | ||
| var _ detectors.Detector = (*Scanner)(nil) | ||
|
|
||
| var ( | ||
| defaultClient = 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{"sentry"}) + `\b([a-f0-9]{64})\b`) | ||
| keyPat = regexp.MustCompile(`\b(sntryu_[a-f0-9]{64})\b`) |
There was a problem hiding this comment.
It's still possible to find tokens with the old format. You should create a new detector version, even if it calls the same code from v1.
There was a problem hiding this comment.
Are you referring to legacy API keys? According to the documentation, the legacy authentication method using API keys is still supported. However, it states that these must be passed as Basic Auth. Does this mean the functionality was broken? 😕
There was a problem hiding this comment.
I'm not sure. Either way, we should still detect old keys without the sntryu_ prefix.
There was a problem hiding this comment.
I'm not sure. Either way, we should still detect old keys without the
sntryu_prefix.
Done Added new changes to version 2 ✅
| func verifyToken(ctx context.Context, client *http.Client, token string) (map[string]string, bool, error) { | ||
| // api docs: https://docs.sentry.io/api/organizations/ | ||
| // this api will return 200 for user auth tokens with scope of org:<> | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, "https://sentry.io/api/0/organizations/", nil) |
There was a problem hiding this comment.
How common is it for a token to have org: scope versus project: scope?
| bytes, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, false, err |
There was a problem hiding this comment.
This is unnecessary, you can read the body directly with json.NewDecoder(res.Body).Decode(...) instead of json.Unmarshal(...).
There was a problem hiding this comment.
How common is it for a token to have
org:scope versusproject:scope?
It depends on the task the user is creating the token for. When generating a token, users have the option to add any scope they need, with at least one scope being mandatory. The reason for using the organization API in this case is that we're trying to retrieve some organization-level data for extraData.
Co-authored-by: Richard Gomez <[email protected]>
kashifkhan0771
force-pushed
the
fix/github-issue-3575
branch
from
52dc3cb to
b5ed85a
Compare
Description:
This Pull request fixes github issue #3575
Checklist:
make test-community)?make lintthis requires golangci-lint)?