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

[kytrinyx]

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

go-github/github/repos_releases.go

Lines 20 to 40 in 29a3681

	// 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]

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.

[dmitshur]

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]

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]

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 Restrict request params of CreateRelease and EditRelease to known subset #997 implements).
4. Do nothing and let GitHub API return 422 error to user (current behavior after GitHub API changes described in Heads up: stricter validation coming soon to the Update a release API endpoint #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?)

[kytrinyx]

I think there is no single best solution, they all have certain advantages and disadvantages

Agreed!

So sorry I missed this whole conversation. We decided to delay turning on strict validation for another 6-ish weeks, which will put us at November 1. Let me go through this issue in detail with my colleague just to make sure we're not missing anything on our end 💙

[kytrinyx]

Having read through the discussions here and in other threads (and the associated PR) I think that the solution in #997 is a good way to address this. Thank you so much for taking the time to think through all the variables!