From 0392c5017408cac6e63b71dd13b7e318350fd225 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 27 Aug 2021 11:25:24 -0400 Subject: [PATCH 1/3] api: close HTTP response body on all paths --- api/api.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/api/api.go b/api/api.go index 83510d8c5..25faf5724 100644 --- a/api/api.go +++ b/api/api.go @@ -44,6 +44,7 @@ func (a *API) GetUser(ctx context.Context) (*User, error) { if err != nil { return nil, fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { @@ -86,6 +87,7 @@ func (a *API) GetRepository(ctx context.Context, nwo string) (*Repository, error if err != nil { return nil, fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { @@ -152,6 +154,7 @@ func (a *API) ListCodespaces(ctx context.Context, user *User) (Codespaces, error if err != nil { return nil, fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { @@ -199,6 +202,7 @@ func (a *API) GetCodespaceToken(ctx context.Context, ownerLogin, codespaceName s if err != nil { return "", fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { @@ -232,6 +236,7 @@ func (a *API) GetCodespace(ctx context.Context, token, owner, codespace string) if err != nil { return nil, fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { @@ -261,10 +266,13 @@ func (a *API) StartCodespace(ctx context.Context, token string, codespace *Codes } req.Header.Set("Authorization", "Bearer "+token) - _, err = a.client.Do(req) + resp, err := a.client.Do(req) if err != nil { return fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() + + // TODO: check status code? return nil } @@ -283,12 +291,15 @@ func (a *API) GetCodespaceRegionLocation(ctx context.Context) (string, error) { if err != nil { return "", fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { return "", fmt.Errorf("error reading response body: %v", err) } + // TODO: check status code? + var response getCodespaceRegionLocationResponse if err := json.Unmarshal(b, &response); err != nil { return "", fmt.Errorf("error unmarshaling response: %v", err) @@ -320,12 +331,15 @@ func (a *API) GetCodespacesSkus(ctx context.Context, user *User, repository *Rep if err != nil { return nil, fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("error reading response body: %v", err) } + // TODO: check status code? + response := struct { Skus Skus `json:"skus"` }{} @@ -359,6 +373,7 @@ func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repos if err != nil { return nil, fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() b, err := ioutil.ReadAll(resp.Body) if err != nil { @@ -388,6 +403,7 @@ func (a *API) DeleteCodespace(ctx context.Context, user *User, token, codespaceN if err != nil { return fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() if resp.StatusCode > http.StatusAccepted { b, err := ioutil.ReadAll(resp.Body) @@ -419,6 +435,7 @@ func (a *API) GetCodespaceRepositoryContents(ctx context.Context, codespace *Cod if err != nil { return nil, fmt.Errorf("error making request: %v", err) } + defer resp.Body.Close() if resp.StatusCode == http.StatusNotFound { return nil, nil From 90f3ac6f56a5b44fc65da1dd3003e97b2f4b378b Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 27 Aug 2021 14:23:33 -0400 Subject: [PATCH 2/3] check status codes --- api/api.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/api/api.go b/api/api.go index 25faf5724..fb386a57b 100644 --- a/api/api.go +++ b/api/api.go @@ -272,7 +272,17 @@ func (a *API) StartCodespace(ctx context.Context, token string, codespace *Codes } defer resp.Body.Close() - // TODO: check status code? + b, err := ioutil.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("error reading response body: %v", err) + } + + // TODO(adonovan): the status code proxied from VSCS may distinguish + // "already running" from "fresh start". Find out what code it uses + // and allow it too. + if resp.StatusCode != http.StatusOK { + return a.errorResponse(b) + } return nil } @@ -298,7 +308,9 @@ func (a *API) GetCodespaceRegionLocation(ctx context.Context) (string, error) { return "", fmt.Errorf("error reading response body: %v", err) } - // TODO: check status code? + if resp.StatusCode != http.StatusOK { + return "", a.errorResponse(b) + } var response getCodespaceRegionLocationResponse if err := json.Unmarshal(b, &response); err != nil { @@ -338,7 +350,9 @@ func (a *API) GetCodespacesSkus(ctx context.Context, user *User, repository *Rep return nil, fmt.Errorf("error reading response body: %v", err) } - // TODO: check status code? + if resp.StatusCode != http.StatusOK { + return nil, a.errorResponse(b) + } response := struct { Skus Skus `json:"skus"` From dcf4f041e9817e7aed0f2e584f7c00884c85f258 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 27 Aug 2021 18:01:52 -0400 Subject: [PATCH 3/3] deal with Start errors, non-JSON --- api/api.go | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/api/api.go b/api/api.go index fb386a57b..c86de79f7 100644 --- a/api/api.go +++ b/api/api.go @@ -1,5 +1,11 @@ package api +// For descriptions of service interfaces, see: +// - https://online.visualstudio.com/api/swagger (for visualstudio.com) +// - https://docs.github.com/en/rest/reference/repos (for api.github.com) +// - https://github.com/github/github/blob/master/app/api/codespaces.rb (for vscs_internal) +// TODO(adonovan): replace the last link with a public doc URL when available. + import ( "bytes" "context" @@ -29,10 +35,6 @@ type User struct { Login string `json:"login"` } -type errResponse struct { - Message string `json:"message"` -} - func (a *API) GetUser(ctx context.Context) (*User, error) { req, err := http.NewRequest(http.MethodGet, githubAPI+"/user", nil) if err != nil { @@ -52,7 +54,7 @@ func (a *API) GetUser(ctx context.Context) (*User, error) { } if resp.StatusCode != http.StatusOK { - return nil, a.errorResponse(b) + return nil, jsonErrorResponse(b) } var response User @@ -63,8 +65,10 @@ func (a *API) GetUser(ctx context.Context) (*User, error) { return &response, nil } -func (a *API) errorResponse(b []byte) error { - var response errResponse +func jsonErrorResponse(b []byte) error { + var response struct { + Message string `json:"message"` + } if err := json.Unmarshal(b, &response); err != nil { return fmt.Errorf("error unmarshaling error response: %v", err) } @@ -95,7 +99,7 @@ func (a *API) GetRepository(ctx context.Context, nwo string) (*Repository, error } if resp.StatusCode != http.StatusOK { - return nil, a.errorResponse(b) + return nil, jsonErrorResponse(b) } var response Repository @@ -162,7 +166,7 @@ func (a *API) ListCodespaces(ctx context.Context, user *User) (Codespaces, error } if resp.StatusCode != http.StatusOK { - return nil, a.errorResponse(b) + return nil, jsonErrorResponse(b) } response := struct { @@ -210,7 +214,7 @@ func (a *API) GetCodespaceToken(ctx context.Context, ownerLogin, codespaceName s } if resp.StatusCode != http.StatusOK { - return "", a.errorResponse(b) + return "", jsonErrorResponse(b) } var response getCodespaceTokenResponse @@ -244,7 +248,7 @@ func (a *API) GetCodespace(ctx context.Context, token, owner, codespace string) } if resp.StatusCode != http.StatusOK { - return nil, a.errorResponse(b) + return nil, jsonErrorResponse(b) } var response Codespace @@ -277,11 +281,12 @@ func (a *API) StartCodespace(ctx context.Context, token string, codespace *Codes return fmt.Errorf("error reading response body: %v", err) } - // TODO(adonovan): the status code proxied from VSCS may distinguish - // "already running" from "fresh start". Find out what code it uses - // and allow it too. if resp.StatusCode != http.StatusOK { - return a.errorResponse(b) + // Error response is numeric code and/or string message, not JSON. + if len(b) > 100 { + b = append(b[:97], "..."...) + } + return fmt.Errorf("failed to start codespace: %s", b) } return nil @@ -309,7 +314,7 @@ func (a *API) GetCodespaceRegionLocation(ctx context.Context) (string, error) { } if resp.StatusCode != http.StatusOK { - return "", a.errorResponse(b) + return "", jsonErrorResponse(b) } var response getCodespaceRegionLocationResponse @@ -351,7 +356,7 @@ func (a *API) GetCodespacesSkus(ctx context.Context, user *User, repository *Rep } if resp.StatusCode != http.StatusOK { - return nil, a.errorResponse(b) + return nil, jsonErrorResponse(b) } response := struct { @@ -395,7 +400,7 @@ func (a *API) CreateCodespace(ctx context.Context, user *User, repository *Repos } if resp.StatusCode > http.StatusAccepted { - return nil, a.errorResponse(b) + return nil, jsonErrorResponse(b) } var response Codespace @@ -424,7 +429,7 @@ func (a *API) DeleteCodespace(ctx context.Context, user *User, token, codespaceN if err != nil { return fmt.Errorf("error reading response body: %v", err) } - return a.errorResponse(b) + return jsonErrorResponse(b) } return nil @@ -461,7 +466,7 @@ func (a *API) GetCodespaceRepositoryContents(ctx context.Context, codespace *Cod } if resp.StatusCode != http.StatusOK { - return nil, a.errorResponse(b) + return nil, jsonErrorResponse(b) } var response getCodespaceRepositoryContentsResponse