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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heads up: stricter validation coming soon to the Update a release API endpoint #992

Closed
kytrinyx opened this Issue Sep 4, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@kytrinyx
Contributor

kytrinyx commented Sep 4, 2018

Hi 馃憢 ,

I an engineer on the API team at GitHub. A colleague and I are currently going through GitHub's REST API, tightening up validations.

We noticed that google/go-github is passing undocumented parameters to the Edit a release endpoint.

In particular, we're receiving the following parameters which the endpoint doesn't know about:

  • assets
  • assets_url
  • author
  • created_at
  • html_url
  • id
  • node_id
  • published_at
  • tarball_url
  • url
  • zipball_url

// RepositoryRelease represents a GitHub release in a repository.
type RepositoryRelease struct {
ID *int64 `json:"id,omitempty"`
TagName *string `json:"tag_name,omitempty"`
TargetCommitish *string `json:"target_commitish,omitempty"`
Name *string `json:"name,omitempty"`
Body *string `json:"body,omitempty"`
Draft *bool `json:"draft,omitempty"`
Prerelease *bool `json:"prerelease,omitempty"`
CreatedAt *Timestamp `json:"created_at,omitempty"`
PublishedAt *Timestamp `json:"published_at,omitempty"`
URL *string `json:"url,omitempty"`
HTMLURL *string `json:"html_url,omitempty"`
AssetsURL *string `json:"assets_url,omitempty"`
Assets []ReleaseAsset `json:"assets,omitempty"`
UploadURL *string `json:"upload_url,omitempty"`
ZipballURL *string `json:"zipball_url,omitempty"`
TarballURL *string `json:"tarball_url,omitempty"`
Author *User `json:"author,omitempty"`
NodeID *string `json:"node_id,omitempty"`
}

Currently the backend code is ignoring unknown parameters, but we're shortly going to change the validation to return a 422 Unprocessable Entity if an undocumented parameter is passed.

We're still discussing our timeline for this, but wanted to give you a heads up so you're not caught by surprise. If you have an ETA for the fix and an idea of how long people would typically need in order to update their dependencies, that information would be very useful for us to have.

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Sep 7, 2018

Collaborator

Hi @kytrinyx... sorry for the delay, and thank you for this information!

Looking through the code, I see we use this struct heavily as a return value, but also use it as a request parameter both for CreateRelease and for EditRelease.

I'm thinking that the best option for fixing this would be to leave the API alone so that users of the repo can still easily get a RepositoryRelease response and then turn around and use it as a request parameter, but then internally to these two endpoints, we could copy the contents into a private struct that has all the extra "unknown" fields removed.

We could possibly get this working today and hopefully reviewed and merged by early next week, but then comes the problem of updating all the users of this repo to the latest version, which we have no control over.

Collaborator

gmlewis commented Sep 7, 2018

Hi @kytrinyx... sorry for the delay, and thank you for this information!

Looking through the code, I see we use this struct heavily as a return value, but also use it as a request parameter both for CreateRelease and for EditRelease.

I'm thinking that the best option for fixing this would be to leave the API alone so that users of the repo can still easily get a RepositoryRelease response and then turn around and use it as a request parameter, but then internally to these two endpoints, we could copy the contents into a private struct that has all the extra "unknown" fields removed.

We could possibly get this working today and hopefully reviewed and merged by early next week, but then comes the problem of updating all the users of this repo to the latest version, which we have no control over.

@gmlewis gmlewis self-assigned this Sep 7, 2018

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 7, 2018

Member

As @gmlewis said, that struct is one of many that are used both for for output and input. For input, the intention is for users to only use a subset of the fields that are supported by the GitHub API (per documentation).

The good news is that users of go-github that correctly use the API aren't going to be affected. The bad news is that this API is easy to accidentally misuse.

If the GitHub API returns descriptive errors when new validation checks fail, that'll make it easier for anyone who's currently misusing the API to update it to avoid sending unsupported fields.

Member

dmitshur commented Sep 7, 2018

As @gmlewis said, that struct is one of many that are used both for for output and input. For input, the intention is for users to only use a subset of the fields that are supported by the GitHub API (per documentation).

The good news is that users of go-github that correctly use the API aren't going to be affected. The bad news is that this API is easy to accidentally misuse.

If the GitHub API returns descriptive errors when new validation checks fail, that'll make it easier for anyone who's currently misusing the API to update it to avoid sending unsupported fields.

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Sep 7, 2018

Collaborator

Unless there are any objections, I plan on labeling this repo v18.1.0 after a fix is merged, indicating that the API is not changed (if indeed that is the case, which is true for #997) but should be upgraded to in order to fix this issue.

Maybe the code itself should directly point to version v18.1.0 (in the CreateRelease and EditRelease endpoint comments) in case people start seeing the 422 error?

Collaborator

gmlewis commented Sep 7, 2018

Unless there are any objections, I plan on labeling this repo v18.1.0 after a fix is merged, indicating that the API is not changed (if indeed that is the case, which is true for #997) but should be upgraded to in order to fix this issue.

Maybe the code itself should directly point to version v18.1.0 (in the CreateRelease and EditRelease endpoint comments) in case people start seeing the 422 error?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 8, 2018

Member

I think we have the following choices of what to do when a user incorrectly sets an unsupported field for CreateRelease and EditRelease endpoints:

  1. Make it an impossible via API (by adding new structs containing only the supported fields). This requires a breaking API change.
  2. Detect it locally and return an error (without sending a request to GitHub API).
  3. Detect it locally and drop/ignore those fields, passing only the supported fields to GitHub API (what PR #997 implements).
  4. Do nothing and let GitHub API return 422 error to user (current behavior after GitHub API changes described in #992).

I think there is no single best solution, they all have certain advantages and disadvantages. My personal favorite is probably option 4 (it helps users by reporting incorrect API usage rather than hiding it). But I'm not opposed to option 3 either (it doesn't change current behavior, even if users are incorrectly setting unsupported fields). I'm okay with whichever decision @gmlewis makes.

(It's also worth considering the approach the library takes in many other places; is it viable to use whatever choice we go with everywhere else too?)

Member

dmitshur commented Sep 8, 2018

I think we have the following choices of what to do when a user incorrectly sets an unsupported field for CreateRelease and EditRelease endpoints:

  1. Make it an impossible via API (by adding new structs containing only the supported fields). This requires a breaking API change.
  2. Detect it locally and return an error (without sending a request to GitHub API).
  3. Detect it locally and drop/ignore those fields, passing only the supported fields to GitHub API (what PR #997 implements).
  4. Do nothing and let GitHub API return 422 error to user (current behavior after GitHub API changes described in #992).

I think there is no single best solution, they all have certain advantages and disadvantages. My personal favorite is probably option 4 (it helps users by reporting incorrect API usage rather than hiding it). But I'm not opposed to option 3 either (it doesn't change current behavior, even if users are incorrectly setting unsupported fields). I'm okay with whichever decision @gmlewis makes.

(It's also worth considering the approach the library takes in many other places; is it viable to use whatever choice we go with everywhere else too?)

@gmlewis gmlewis closed this in #997 Sep 12, 2018

gmlewis added a commit that referenced this issue Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment