diff --git a/cmd/src/code_intel_upload.go b/cmd/src/code_intel_upload.go index dfc7a9190e..303e2199e0 100644 --- a/cmd/src/code_intel_upload.go +++ b/cmd/src/code_intel_upload.go @@ -71,7 +71,7 @@ func handleCodeIntelUpload(args []string) error { } } if err != nil { - return handleUploadError(cfg.AccessToken, err) + return err } client := cfg.apiClient(codeintelUploadFlags.apiFlags, io.Discard) @@ -212,17 +212,22 @@ func (e errorWithHint) Error() string { return fmt.Sprintf("%s\n\n%s\n", e.err, e.hint) } -// handleUploadError writes the given error to the given output. If the -// given output object is nil then the error will be written to standard out. -// -// This method returns the error that should be passed back up to the runner. +// handleUploadError attaches actionable hints to upload errors and returns +// the enriched error. Only called for actual upload failures (not flag validation). func handleUploadError(accessToken string, err error) error { - if errors.Is(err, upload.ErrUnauthorized) { - err = attachHintsForAuthorizationError(accessToken, err) + if httpErr := findAuthError(err); httpErr != nil { + isUnauthorized := httpErr.Code == 401 + isForbidden := httpErr.Code == 403 + + displayErr := errors.Newf("upload failed: %s", uploadFailureReason(httpErr)) + + err = errorWithHint{ + err: displayErr, + hint: uploadHints(accessToken, isUnauthorized, isForbidden), + } } if codeintelUploadFlags.ignoreUploadFailures { - // Report but don't return the error fmt.Println(err.Error()) return nil } @@ -230,76 +235,112 @@ func handleUploadError(accessToken string, err error) error { return err } -func attachHintsForAuthorizationError(accessToken string, originalError error) error { - var actionableHints []string +// findAuthError searches the error chain (including multi-errors from retries) +// for a 401 or 403 ErrUnexpectedStatusCode. Returns nil if none is found. +func findAuthError(err error) *ErrUnexpectedStatusCode { + // Check if it's a multi-error and scan all children. + if multi, ok := err.(errors.MultiError); ok { + for _, e := range multi.Errors() { + if found := findAuthError(e); found != nil { + return found + } + } + return nil + } + + var httpErr *ErrUnexpectedStatusCode + if errors.As(err, &httpErr) && (httpErr.Code == 401 || httpErr.Code == 403) { + return httpErr + } + return nil +} + +// uploadHints builds hint paragraphs for the Sourcegraph access token, +// code host tokens, and a docs link. +func uploadHints(accessToken string, isUnauthorized, isForbidden bool) string { + var causes []string - likelyTokenError := accessToken == "" - if _, parseErr := accesstoken.ParsePersonalAccessToken(accessToken); accessToken != "" && parseErr != nil { - likelyTokenError = true - actionableHints = append(actionableHints, - "However, the provided access token does not match expected format; was it truncated?", - "Typically the access token looks like sgp_<40 hex chars> or sgp__<40 hex chars>.") + if h := sourcegraphAccessTokenHint(accessToken, isUnauthorized, isForbidden); h != "" { + causes = append(causes, "- "+h) } - if likelyTokenError { - return errorWithHint{err: originalError, hint: strings.Join(mergeStringSlices( - []string{"A Sourcegraph access token must be provided via SRC_ACCESS_TOKEN for uploading SCIP data."}, - actionableHints, - []string{"For more details, see https://sourcegraph.com/docs/cli/how-tos/creating_an_access_token."}, - ), "\n")} + for _, h := range codeHostTokenHints(isUnauthorized) { + causes = append(causes, "- "+h) } - needsGitHubToken := strings.HasPrefix(codeintelUploadFlags.repo, "github.com") - needsGitLabToken := strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com") + var parts []string + parts = append(parts, "Possible causes:\n"+strings.Join(causes, "\n")) + parts = append(parts, "For more details on uploading SCIP indexes, see https://sourcegraph.com/docs/cli/references/code-intel/upload.") - if needsGitHubToken { - if codeintelUploadFlags.gitHubToken != "" { - actionableHints = append(actionableHints, - fmt.Sprintf("The supplied -github-token does not indicate that you have collaborator access to %s.", codeintelUploadFlags.repo), - "Please check the value of the supplied token and its permissions on the code host and try again.", - ) - } else { - actionableHints = append(actionableHints, - fmt.Sprintf("Please retry your request with a -github-token=XXX with collaborator access to %s.", codeintelUploadFlags.repo), - "This token will be used to check with the code host that the uploading user has write access to the target repository.", - ) + return strings.Join(parts, "\n\n") +} + +// sourcegraphAccessTokenHint returns a hint about the Sourcegraph access token +// based on the error type and token state. +func sourcegraphAccessTokenHint(accessToken string, isUnauthorized, isForbidden bool) string { + if isUnauthorized { + if accessToken == "" { + return "No Sourcegraph access token was provided. Set the SRC_ACCESS_TOKEN environment variable to a valid token." } - } else if needsGitLabToken { - if codeintelUploadFlags.gitLabToken != "" { - actionableHints = append(actionableHints, - fmt.Sprintf("The supplied -gitlab-token does not indicate that you have write access to %s.", codeintelUploadFlags.repo), - "Please check the value of the supplied token and its permissions on the code host and try again.", - ) - } else { - actionableHints = append(actionableHints, - fmt.Sprintf("Please retry your request with a -gitlab-token=XXX with write access to %s.", codeintelUploadFlags.repo), - "This token will be used to check with the code host that the uploading user has write access to the target repository.", - ) + if _, parseErr := accesstoken.ParsePersonalAccessToken(accessToken); parseErr != nil { + return "The provided Sourcegraph access token does not match the expected format (sgp_<40 hex chars> or sgp__<40 hex chars>). Was it copied incorrectly or truncated?" } - } else { - actionableHints = append(actionableHints, - "Verification is supported for the following code hosts: github.com, gitlab.com.", - "Please request support for additional code host verification at https://github.com/sourcegraph/sourcegraph/issues/4967.", - ) + return "The Sourcegraph access token may be invalid, expired, or you may be connecting to the wrong Sourcegraph instance." + } + if isForbidden { + return "You may not have sufficient permissions on this Sourcegraph instance." } + return "" +} - return errorWithHint{err: originalError, hint: strings.Join(mergeStringSlices( - []string{"This Sourcegraph instance has enforced auth for SCIP uploads."}, - actionableHints, - []string{"For more details, see https://docs.sourcegraph.com/cli/references/code-intel/upload."}, - ), "\n")} +// codeHostTokenHints returns hints about GitHub or GitLab tokens. +func codeHostTokenHints(isUnauthorized bool) []string { + if codeintelUploadFlags.gitHubToken != "" || strings.HasPrefix(codeintelUploadFlags.repo, "github.com") { + return []string{gitHubTokenHint(isUnauthorized)} + } + if codeintelUploadFlags.gitLabToken != "" || strings.HasPrefix(codeintelUploadFlags.repo, "gitlab.com") { + return []string{gitLabTokenHint(isUnauthorized)} + } + return []string{"Code host verification is supported for github.com and gitlab.com repositories."} } -// emergencyOutput creates a default Output object writing to standard out. -func emergencyOutput() *output.Output { - return output.NewOutput(os.Stdout, output.OutputOpts{}) +// gitHubTokenHint returns a hint about the GitHub token. +// Only called when gitHubToken is set or repo starts with "github.com". +func gitHubTokenHint(isUnauthorized bool) string { + if codeintelUploadFlags.gitHubToken == "" { + return fmt.Sprintf("No -github-token was provided. If this Sourcegraph instance enforces code host authentication, retry with -github-token= for a token with access to %s.", codeintelUploadFlags.repo) + } + if isUnauthorized { + return "The supplied -github-token may be invalid." + } + return "The supplied -github-token may lack the required permissions." } -func mergeStringSlices(ss ...[]string) []string { - var combined []string - for _, s := range ss { - combined = append(combined, s...) +// gitLabTokenHint returns a hint about the GitLab token. +// Only called when gitLabToken is set or repo starts with "gitlab.com". +func gitLabTokenHint(isUnauthorized bool) string { + if codeintelUploadFlags.gitLabToken == "" { + return fmt.Sprintf("No -gitlab-token was provided. If this Sourcegraph instance enforces code host authentication, retry with -gitlab-token= for a token with access to %s.", codeintelUploadFlags.repo) + } + if isUnauthorized { + return "The supplied -gitlab-token may be invalid." } + return "The supplied -gitlab-token may lack the required permissions." +} + +// uploadFailureReason returns the server's response body if available, or a +// generic reason derived from the HTTP status code. +func uploadFailureReason(httpErr *ErrUnexpectedStatusCode) string { + if httpErr.Body != "" { + return httpErr.Body + } + if httpErr.Code == 401 { + return "unauthorized" + } + return "forbidden" +} - return combined +// emergencyOutput creates a default Output object writing to standard out. +func emergencyOutput() *output.Output { + return output.NewOutput(os.Stdout, output.OutputOpts{}) } diff --git a/cmd/src/code_intel_upload_test.go b/cmd/src/code_intel_upload_test.go new file mode 100644 index 0000000000..fd8c390c94 --- /dev/null +++ b/cmd/src/code_intel_upload_test.go @@ -0,0 +1,352 @@ +package main + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/sourcegraph/sourcegraph/lib/errors" +) + +// validSGToken is a well-formed (but not real) Sourcegraph personal access token. +const validSGToken = "sgp_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + +func TestUploadFailureReason(t *testing.T) { + tests := []struct { + name string + err *ErrUnexpectedStatusCode + want string + }{ + {"401 with body", &ErrUnexpectedStatusCode{Code: 401, Body: "Invalid access token."}, "Invalid access token."}, + {"401 without body", &ErrUnexpectedStatusCode{Code: 401}, "unauthorized"}, + {"403 with body", &ErrUnexpectedStatusCode{Code: 403, Body: "no write permission"}, "no write permission"}, + {"403 without body", &ErrUnexpectedStatusCode{Code: 403}, "forbidden"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, uploadFailureReason(tt.err)) + }) + } +} + +func TestSourcegraphAccessTokenHint(t *testing.T) { + tests := []struct { + name string + accessToken string + isUnauthorized bool + isForbidden bool + wantContains string + }{ + { + name: "401 no token", + isUnauthorized: true, + wantContains: "No Sourcegraph access token was provided", + }, + { + name: "401 malformed token", + accessToken: "not-a-valid-token", + isUnauthorized: true, + wantContains: "does not match the expected format", + }, + { + name: "401 valid format token", + accessToken: validSGToken, + isUnauthorized: true, + wantContains: "may be invalid, expired", + }, + { + name: "403", + isForbidden: true, + wantContains: "may not have sufficient permissions", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sourcegraphAccessTokenHint(tt.accessToken, tt.isUnauthorized, tt.isForbidden) + assert.Contains(t, got, tt.wantContains) + }) + } +} + +func TestCodeHostTokenHints(t *testing.T) { + tests := []struct { + name string + repo string + gitHubToken string + gitLabToken string + isUnauthorized bool + wantContains []string + }{ + { + name: "github repo no token 401", + repo: "github.com/org/repo", + isUnauthorized: true, + wantContains: []string{"No -github-token was provided", "github.com/org/repo"}, + }, + { + name: "github repo no token 403", + repo: "github.com/org/repo", + isUnauthorized: false, + wantContains: []string{"No -github-token was provided"}, + }, + { + name: "github repo with token 401", + repo: "github.com/org/repo", + gitHubToken: "ghp_xxx", + isUnauthorized: true, + wantContains: []string{"-github-token may be invalid"}, + }, + { + name: "github repo with token 403", + repo: "github.com/org/repo", + gitHubToken: "ghp_xxx", + isUnauthorized: false, + wantContains: []string{"-github-token may lack the required permissions"}, + }, + { + name: "gitlab repo no token 401", + repo: "gitlab.com/org/repo", + isUnauthorized: true, + wantContains: []string{"No -gitlab-token was provided", "gitlab.com/org/repo"}, + }, + { + name: "gitlab repo no token 403", + repo: "gitlab.com/org/repo", + isUnauthorized: false, + wantContains: []string{"No -gitlab-token was provided"}, + }, + { + name: "gitlab repo with token 401", + repo: "gitlab.com/org/repo", + gitLabToken: "glpat-xxx", + isUnauthorized: true, + wantContains: []string{"-gitlab-token may be invalid"}, + }, + { + name: "gitlab repo with token 403", + repo: "gitlab.com/org/repo", + gitLabToken: "glpat-xxx", + isUnauthorized: false, + wantContains: []string{"-gitlab-token may lack the required permissions"}, + }, + { + name: "other repo no tokens", + repo: "bitbucket.org/org/repo", + isUnauthorized: true, + wantContains: []string{"Code host verification is supported for github.com and gitlab.com"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + saved := codeintelUploadFlags + defer func() { codeintelUploadFlags = saved }() + + codeintelUploadFlags.repo = tt.repo + codeintelUploadFlags.gitHubToken = tt.gitHubToken + codeintelUploadFlags.gitLabToken = tt.gitLabToken + + hints := codeHostTokenHints(tt.isUnauthorized) + joined := strings.Join(hints, "\n") + for _, s := range tt.wantContains { + assert.Contains(t, joined, s) + } + }) + } +} + +func TestUploadHints(t *testing.T) { + tests := []struct { + name string + accessToken string + repo string + gitHubToken string + gitLabToken string + isUnauthorized bool + isForbidden bool + wantContains []string + }{ + { + name: "401 no SG token github repo", + repo: "github.com/org/repo", + isUnauthorized: true, + wantContains: []string{ + "Possible causes:", + "- No Sourcegraph access token was provided", + "- No -github-token was provided", + "sourcegraph.com/docs/cli/references/code-intel/upload", + }, + }, + { + name: "401 valid SG token github token supplied", + accessToken: validSGToken, + repo: "github.com/org/repo", + gitHubToken: "ghp_xxx", + isUnauthorized: true, + wantContains: []string{ + "Possible causes:", + "- The Sourcegraph access token may be invalid, expired", + "- The supplied -github-token may be invalid", + "sourcegraph.com/docs/cli/references/code-intel/upload", + }, + }, + { + name: "403 gitlab token supplied", + accessToken: validSGToken, + repo: "gitlab.com/org/repo", + gitLabToken: "glpat-xxx", + isForbidden: true, + wantContains: []string{ + "Possible causes:", + "- You may not have sufficient permissions", + "- The supplied -gitlab-token may lack the required permissions", + "sourcegraph.com/docs/cli/references/code-intel/upload", + }, + }, + { + name: "401 bitbucket repo catch-all", + accessToken: validSGToken, + repo: "bitbucket.org/org/repo", + isUnauthorized: true, + wantContains: []string{ + "Possible causes:", + "- The Sourcegraph access token may be invalid, expired", + "- Code host verification is supported for github.com and gitlab.com", + "sourcegraph.com/docs/cli/references/code-intel/upload", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + saved := codeintelUploadFlags + defer func() { codeintelUploadFlags = saved }() + + codeintelUploadFlags.repo = tt.repo + codeintelUploadFlags.gitHubToken = tt.gitHubToken + codeintelUploadFlags.gitLabToken = tt.gitLabToken + + got := uploadHints(tt.accessToken, tt.isUnauthorized, tt.isForbidden) + for _, s := range tt.wantContains { + assert.Contains(t, got, s) + } + }) + } +} + +func TestHandleUploadError(t *testing.T) { + tests := []struct { + name string + accessToken string + repo string + gitHubToken string + err error + wantContains []string + wantNil bool + }{ + { + name: "401 with server body", + accessToken: validSGToken, + repo: "github.com/org/repo", + err: &ErrUnexpectedStatusCode{Code: 401, Body: "Invalid access token."}, + wantContains: []string{ + "upload failed: Invalid access token.", + "Possible causes:", + "- The Sourcegraph access token may be invalid, expired", + }, + }, + { + name: "401 without body", + accessToken: "", + repo: "github.com/org/repo", + err: &ErrUnexpectedStatusCode{Code: 401}, + wantContains: []string{ + "upload failed: unauthorized", + "Possible causes:", + "- No Sourcegraph access token was provided", + }, + }, + { + name: "403 with server body", + accessToken: validSGToken, + repo: "github.com/org/repo", + gitHubToken: "ghp_xxx", + err: &ErrUnexpectedStatusCode{Code: 403, Body: "no write permission"}, + wantContains: []string{ + "upload failed: no write permission", + "Possible causes:", + "- You may not have sufficient permissions", + }, + }, + { + name: "500 passthrough", + accessToken: validSGToken, + repo: "github.com/org/repo", + err: &ErrUnexpectedStatusCode{Code: 500, Body: "internal error"}, + wantContains: []string{"unexpected status code: 500"}, + }, + { + name: "non-http error passthrough", + accessToken: validSGToken, + repo: "github.com/org/repo", + err: errors.New("connection refused"), + wantContains: []string{"connection refused"}, + }, + { + name: "combined 502 + 403 from retries", + accessToken: validSGToken, + repo: "github.com/org/repo", + gitHubToken: "ghp_xxx", + err: errors.CombineErrors( + &ErrUnexpectedStatusCode{Code: 502}, + &ErrUnexpectedStatusCode{Code: 403, Body: "no write permission"}, + ), + wantContains: []string{ + "upload failed: no write permission", + "Possible causes:", + "- You may not have sufficient permissions", + }, + }, + { + name: "combined 502 + 401 from retries", + accessToken: validSGToken, + repo: "github.com/org/repo", + err: errors.CombineErrors( + &ErrUnexpectedStatusCode{Code: 502}, + &ErrUnexpectedStatusCode{Code: 401, Body: "Invalid access token."}, + ), + wantContains: []string{ + "upload failed: Invalid access token.", + "Possible causes:", + "- The Sourcegraph access token may be invalid, expired", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + saved := codeintelUploadFlags + defer func() { codeintelUploadFlags = saved }() + + codeintelUploadFlags.repo = tt.repo + codeintelUploadFlags.gitHubToken = tt.gitHubToken + codeintelUploadFlags.ignoreUploadFailures = false + + got := handleUploadError(tt.accessToken, tt.err) + require.NotNil(t, got) + for _, s := range tt.wantContains { + assert.Contains(t, got.Error(), s) + } + }) + } +} + +func TestHandleUploadErrorIgnoreFailures(t *testing.T) { + saved := codeintelUploadFlags + defer func() { codeintelUploadFlags = saved }() + + codeintelUploadFlags.repo = "github.com/org/repo" + codeintelUploadFlags.ignoreUploadFailures = true + + got := handleUploadError(validSGToken, &ErrUnexpectedStatusCode{Code: 401, Body: "bad token"}) + assert.Nil(t, got) +} diff --git a/cmd/src/code_intel_upload_vendored.go b/cmd/src/code_intel_upload_vendored.go index b5e4897af0..a1a98fe86a 100644 --- a/cmd/src/code_intel_upload_vendored.go +++ b/cmd/src/code_intel_upload_vendored.go @@ -339,8 +339,20 @@ type uploadRequestOptions struct { Done bool // Whether the request is a multipart finalize } -// ErrUnauthorized occurs when the upload endpoint returns a 401 response. -var ErrUnauthorized = errors.New("unauthorized upload") +// ErrUnexpectedStatusCode is returned for HTTP error responses from the upload +// endpoint. It carries the status code and response body so callers can inspect +// them without parsing the error string. +type ErrUnexpectedStatusCode struct { + Code int + Body string +} + +func (e *ErrUnexpectedStatusCode) Error() string { + if e.Body != "" { + return fmt.Sprintf("unexpected status code: %d (%s)", e.Code, e.Body) + } + return fmt.Sprintf("unexpected status code: %d", e.Code) +} // performUploadRequest performs an HTTP POST to the upload endpoint. The query string of the request // is constructed from the given request options and the body of the request is the unmodified reader. @@ -419,17 +431,13 @@ func performRequest(ctx context.Context, req *http.Request, httpClient upload.Cl // returns a boolean flag indicating if the function can be retried on failure (error-dependent). func decodeUploadPayload(resp *http.Response, body []byte, target *int) (bool, error) { if resp.StatusCode >= 300 { - if resp.StatusCode == http.StatusUnauthorized { - return false, ErrUnauthorized - } - - suffix := "" - if !bytes.HasPrefix(bytes.TrimSpace(body), []byte{'<'}) { - suffix = fmt.Sprintf(" (%s)", bytes.TrimSpace(body)) + detail := "" + if trimmed := bytes.TrimSpace(body); len(trimmed) > 0 && trimmed[0] != '<' { + detail = string(trimmed) } // Do not retry client errors - return resp.StatusCode >= 500, errors.Errorf("unexpected status code: %d%s", resp.StatusCode, suffix) + return resp.StatusCode >= 500, &ErrUnexpectedStatusCode{Code: resp.StatusCode, Body: detail} } if target == nil {