fix: bound OAuth2 request bodies and report the rejection per RFC - #28175
Draft
BobbyHo wants to merge 3 commits into
Draft
fix: bound OAuth2 request bodies and report the rejection per RFC#28175BobbyHo wants to merge 3 commits into
BobbyHo wants to merge 3 commits into
Conversation
POST /oauth2/tokens and POST /oauth2/revoke authenticate the client from the request body, so they carry no API key middleware and r.ParseForm ran before any authorization decision. net/http caps an unwrapped urlencoded body at its own 10 MiB maxFormSize, 2.5x the ceiling every other endpoint carries, and /oauth2 is mounted outside apiRateLimiter. The middleware now installs a MaxBytesReader at DefaultMaxRequestBodyBytes ahead of every reader, which net/http defers to when it finds one. tokens.go and revoke.go translate the same error, since they perform the first read when client_id arrived in the query string and the middleware had no reason to parse. These endpoints answer under RFC 6749 rather than through httpapi.Read, so the rejection is written as an invalid_request. RFC 6749 defines no error code for a transport rejection, so that is the closest compliant framing. The middleware also discarded its ParseForm error, so an oversized body reported the client_id it may well have carried as missing. That is fixed here: a size failure is now reported as one, and any other parse failure still falls through, since the client_id may arrive through HTTP Basic.
POST /oauth2/register is reachable pre-authentication and decoded through httpapi.Read, which reports a codersdk.Response. That was the one protocol-inconsistent response in a handler that is otherwise RFC 7591 compliant, and it becomes visible once httpapi.Read starts answering 413. readOAuth2ClientRegistrationRequest bounds and decodes locally so the rejection is an RFC 7591 error. http.MaxBytesReader surfaces the limit through the decoder's error, so the error shape belongs to whoever decodes; that is why the decode moves into the handler rather than passing a limit to httpapi.ReadLimit. RFC 7591 section 3.2.2 defines invalid_client_metadata and friends for semantic validation rather than transport rejection, so invalid_request is the closest compliant framing. The limit is DefaultMaxRequestBodyBytes, unchanged from what httpapi.Read would have applied. Only the error shape and the recorded limit differ.
readOAuth2ClientRegistrationRequest reported every malformed body as a bare "Request body must be valid JSON", so a client integrator saw one message whether a proxy had returned HTML or redirect_uris carried a string where an array belongs. The decoder's own text names the offending field and the type it expected, it describes the caller's own bytes so it discloses nothing, and httpapi.Read has exposed it on every other endpoint for as long as it has existed. This handler routes every other error through err.Error() too. That 400 also had no test. It is a shape the previous commit introduced on purpose: a malformed body used to produce a codersdk.Response and now produces an RFC 7591 error, which is the whole reason the decode is local to the handler rather than httpapi.Read. A future refactor routing it back through httpapi.Read would have regressed the protocol shape silently. TestOAuth2SpecificErrorScenarios/InvalidJSONStructure was an empty subtest claiming coverage happened implicitly through typed request structs, which by construction cannot produce a decode failure. It now posts raw bodies, an unterminated object and a mistyped field, and asserts the status, the invalid_request code, and that the decoder's text survives into error_description. The doc comment for writeOAuth2RegistrationError had been left above readOAuth2ClientRegistrationRequest when that function was inserted, so godoc attributed it to the wrong function and writeOAuth2RegistrationError had none of its own.
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bounds the two OAuth2 paths from
r.Bodyto a decoded value that do not go throughhttpapi.Read, and reports each rejection in the error shape its caller expects rather than thecodersdk.Responsethathttpapi.Readwrites.Second of three PRs split out of #28048. Stacked on #28168, which adds the
httpapi.ReadLimitprimitive andRecordRequestBodyLimitthat this PR builds on. Review that one first; the diff shown here is against its branch. The SCIM remediation and the lint rule follow in a third PR.Refs PLAT-463. Part of the remediation for SEC-416 (CWE-770, CVSS 7.5).
Problem
httpapi.Readis not the only path from a request body to a decoded value.POST /oauth2/tokensandPOST /oauth2/revokeauthenticate the client from the request body, so they carry no API key middleware andr.ParseFormran before any authorization decision. These were not unbounded, sincenet/httpcaps an unwrapped urlencoded body at its own 10 MiBmaxFormSize, but that is an asymmetric pre-auth ceiling 2.5x the one every other endpoint carries, on a prefix mounted outsideapiRateLimiter.POST /oauth2/registeris also reachable pre-authentication. It decoded throughhttpapi.Read, which reports acodersdk.Response, making it the one protocol-inconsistent response in a handler that is otherwise RFC 7591 compliant. That inconsistency becomes visible oncehttpapi.Readstarts answering 413.Fix
The form endpoints get a
MaxBytesReaderatDefaultMaxRequestBodyBytesinstalled inextractOAuth2ProviderAppBase, ahead of every reader;net/httpdefers to one when it finds it, so this replacesmaxFormSize.tokens.goandrevoke.gotranslate the same error, because the middleware parses the form only whenclient_idis absent from the query string, and when it is present those handlers perform the first read./oauth2/registerbounds and decodes locally inreadOAuth2ClientRegistrationRequest.http.MaxBytesReadersurfaces the limit through the decoder's error, so the error shape belongs to whoever decodes; that is why the decode moves into the handler rather than passing a limit tohttpapi.ReadLimit. The limit itself is unchanged from whathttpapi.Readapplied.RFC 6749 defines no error code for a transport rejection, and RFC 7591 section 3.2.2 defines
invalid_client_metadataand friends for semantic validation rather than transport rejection, soinvalid_requestis the closest compliant framing in both.Two fixes here that are not about body size
Called out separately so they are not skimmed past while checking the size bounds. Each is its own commit.
The middleware discarded its
ParseFormerror, so an oversized body reported theclient_idit may well have carried as missing. A size failure is now reported as one; any other parse failure still falls through, since theclient_idmay arrive through HTTP Basic.The registration 400 carried a fixed sentence, so an integrator saw one message whether a proxy had returned HTML or
redirect_uriscarried a string where an array belongs. It now carries the decoder's own text, which names the offending field and the expected type, describes the caller's own bytes so it discloses nothing, and is whathttpapi.Readhas exposed on every other endpoint for as long as it has existed.TestOAuth2SpecificErrorScenarios/InvalidJSONStructurewas an empty subtest claiming coverage happened implicitly through typed request structs, which by construction cannot produce a decode failure; it now posts raw bodies and asserts the shape.Behavior change
POST /oauth2/tokensandPOST /oauth2/revokenow answer 413 with an RFC 6749invalid_requestonce a form body passes 4 MiB. Previously such a body ran tonet/http's 10 MiB cap and was reported as a 400 namingclient_idas missing.POST /oauth2/registernow answers 413 as an RFC 7591 error rather than acodersdk.Response. Its 400 for a malformed body now carries the JSON decoder's text inerror_description; the status and the RFC 7591 error code are unchanged.