-
Notifications
You must be signed in to change notification settings - Fork 469
Allow reading the request body in both security and main server logic #5491
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
923b5c0
dd127cf
bbf5628
e01ab10
a55c602
09897b1
9235564
e6c183f
7d11bbd
3d31b12
83a427a
c283c58
7c23c68
a858658
de0f833
9205c42
3780461
f610a75
c51a401
374f41a
9dc02e8
087e1a8
2bf63a9
2f91e54
7c31bee
d62a81d
2784357
ab1b081
90a7f13
5ef8634
9aba4f0
54fe795
e0f70cf
76ae8d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package sttp.tapir.client.sttp4 | ||
|
|
||
| import org.scalatest.flatspec.AnyFlatSpec | ||
| import org.scalatest.matchers.should.Matchers | ||
| import sttp.client4.Request | ||
| import sttp.model.Uri._ | ||
| import sttp.tapir._ | ||
|
|
||
| class SecondaryBodyClientTest extends AnyFlatSpec with Matchers { | ||
| it should "send only the primary body, ignoring the secondary one" in { | ||
| // the secondary body is on `in`, processed after `securityIn` - so an unskipped one would overwrite the primary | ||
| val e = endpoint.post | ||
| .in("people") | ||
| .securityIn(stringBody) | ||
| .in(stringBody.asSecondary) | ||
| .out(stringBody) | ||
|
|
||
| val request: Request[_] = | ||
| SttpClientInterpreter().toSecureRequestThrowDecodeFailures(e, Some(uri"http://example.com"))("sent")("ignored") | ||
|
|
||
| request.body.show should include("sent") | ||
| request.body.show should not include ("ignored") | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| package sttp.tapir | ||
|
|
||
| import java.io.InputStream | ||
| import java.nio.ByteBuffer | ||
| import scala.annotation.implicitNotFound | ||
|
|
||
| /** Attribute value marking a body input as a secondary definition: decoded from the request on the server, but not part of the API | ||
| * contract. Secondary bodies are excluded from documentation and ignored by client interpreters, which allows the request body to be | ||
| * decoded more than once - e.g. in `serverSecurityLogic` and again in the main logic. | ||
| * | ||
| * Set using [[EndpointIO.Body.asSecondary]]. | ||
| */ | ||
| case class SecondaryBody() | ||
|
|
||
| object SecondaryBody { | ||
| val attributeKey: AttributeKey[SecondaryBody] = new AttributeKey[SecondaryBody]("sttp.tapir.SecondaryBody") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude-generated review
|
||
| } | ||
|
|
||
| /** Evidence that a raw body type can be re-read from buffered bytes, and is therefore usable as a secondary body. */ | ||
| @implicitNotFound( | ||
| "Cannot use a body with raw type ${R} as a secondary body. Only bodies which can be re-read from buffered bytes " + | ||
| "are supported: string, byte array, byte buffer, input stream. File, multipart and streaming bodies cannot be " + | ||
| "read twice." | ||
| ) | ||
| trait ReplayableRawBody[R] | ||
|
|
||
| object ReplayableRawBody { | ||
| private val instance: ReplayableRawBody[Any] = new ReplayableRawBody[Any] {} | ||
| private def of[R]: ReplayableRawBody[R] = instance.asInstanceOf[ReplayableRawBody[R]] | ||
|
|
||
| implicit val forString: ReplayableRawBody[String] = of | ||
| implicit val forByteArray: ReplayableRawBody[Array[Byte]] = of | ||
| implicit val forByteBuffer: ReplayableRawBody[ByteBuffer] = of | ||
| implicit val forInputStream: ReplayableRawBody[InputStream] = of | ||
| implicit val forInputStreamRange: ReplayableRawBody[InputStreamRange] = of | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| package sttp.tapir.server | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude-generated review The module is right —
Two smaller things: |
||
|
|
||
| import sttp.model.Method | ||
| import sttp.tapir.internal._ | ||
| import sttp.tapir.{AnyEndpoint, EndpointIO, EndpointInput, RawBodyType} | ||
|
|
||
| /** Errors make an endpoint unserveable; warnings describe one that works, but whose published contract probably isn't what was intended. */ | ||
| private[tapir] case class EndpointBodyProblems(errors: List[String], warnings: List[String]) { | ||
| def ++(other: EndpointBodyProblems): EndpointBodyProblems = | ||
| EndpointBodyProblems(errors ++ other.errors, warnings ++ other.warnings) | ||
| } | ||
|
|
||
| private[tapir] object EndpointBodyProblems { | ||
| val Empty: EndpointBodyProblems = EndpointBodyProblems(Nil, Nil) | ||
| } | ||
|
|
||
| /** Verifies that endpoint descriptions are structurally serveable. Run by server interpreters when routes are constructed; warnings are not | ||
| * logged anywhere, so call this directly to assert on them. | ||
| */ | ||
| private[tapir] object EndpointBodyVerifier { | ||
| def verify(endpoints: List[AnyEndpoint]): EndpointBodyProblems = | ||
| endpoints.map(verifyOne).foldLeft(EndpointBodyProblems.Empty)(_ ++ _) | ||
|
|
||
| private[tapir] def throwOnErrors(problems: EndpointBodyProblems): Unit = | ||
| if (problems.errors.nonEmpty) throw new IllegalArgumentException(problems.errors.mkString("\n")) | ||
|
|
||
| def verifyOne(endpoint: AnyEndpoint): EndpointBodyProblems = { | ||
| val securityInputs = endpoint.securityInput.asVectorOfBasicInputs() | ||
| val ordinaryInputs = endpoint.input.asVectorOfBasicInputs() | ||
| val inputs = securityInputs ++ ordinaryInputs | ||
|
|
||
| val secondary = inputs.collect { case b: EndpointIO.Body[?, ?] if b.isSecondary => b } | ||
| def primaryBodiesOf(basics: Vector[EndpointInput.Basic[?]]): Vector[EndpointInput.Basic[?]] = basics.collect { | ||
| case b: EndpointIO.Body[?, ?] if !b.isSecondary => b | ||
| case b: EndpointIO.OneOfBody[?, ?] => b | ||
| case b: EndpointIO.StreamBodyWrapper[?, ?] => b | ||
| } | ||
| val securityPrimaryBodies = primaryBodiesOf(securityInputs) | ||
| val inPrimaryBodies = primaryBodiesOf(ordinaryInputs) | ||
| val primaryBodies = securityPrimaryBodies ++ inPrimaryBodies | ||
| def asAtoms(body: EndpointInput.Basic[?]): Vector[EndpointInput.Basic[?]] = body match { | ||
| case ob: EndpointIO.OneOfBody[?, ?] => ob.variants.map(_.bodyAsAtom).toVector | ||
| case other => Vector(other) | ||
| } | ||
| val primaryBodyAtoms: Vector[EndpointInput.Basic[?]] = primaryBodies.flatMap(asAtoms) | ||
| val streamingPrimary = primaryBodyAtoms.exists(_.isInstanceOf[EndpointIO.StreamBodyWrapper[?, ?]]) | ||
| val nonReplayablePrimary = primaryBodyAtoms.exists { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude-generated review "Which raw types can be replayed" is now stated three times: Matching positively, or a shared |
||
| case b: EndpointIO.Body[?, ?] => | ||
| b.bodyType match { | ||
| case RawBodyType.FileBody => true | ||
| case _: RawBodyType.MultipartBody => true | ||
| case _ => false | ||
| } | ||
| case _ => false | ||
| } | ||
| val shown = endpoint.showShort | ||
|
|
||
| // asSecondary can be called on a variant, as oneOfBody takes bodies, but the server interpreters only look for | ||
| // the marker on a top-level body input - so accepting it here would silently fall back to reading the body once | ||
| val secondaryInsideOneOfBody: List[String] = | ||
| inputs | ||
| .collect { case ob: EndpointIO.OneOfBody[?, ?] => ob } | ||
| .collect { | ||
| case ob if ob.variants.map(_.bodyAsAtom).exists { case b: EndpointIO.Body[?, ?] => b.isSecondary; case _ => false } => | ||
| s"Endpoint $shown marks a oneOfBody variant as secondary. Only a body input used on its own can be " + | ||
| s"secondary; a oneOfBody is always part of the API contract." | ||
| } | ||
| .toList | ||
|
|
||
| val tooManyPrimaries: List[String] = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude-generated review Needs a decision before merge. This also rejects the hidden-schema pattern from #3820: endpoint.post
.securityIn(byteArrayBody.schema(_.hidden(true)))
.in("api" / "echo")
.in(jsonBody[FruitAmount])
It's the existing workaround for exactly the problem this PR solves, so either treat a hidden-schema body as secondary, or call it out explicitly in the release notes. The current message only mentions |
||
| if (secondaryInsideOneOfBody.nonEmpty) Nil | ||
| else if (securityPrimaryBodies.nonEmpty && inPrimaryBodies.nonEmpty) | ||
| List( | ||
| s"Endpoint $shown declares a request body in both securityIn and in. Only one may be part of the API " + | ||
| s"contract. If both should decode the same request body, mark the securityIn one: " + | ||
| s"stringBody.asSecondary." | ||
| ) | ||
| else if (securityPrimaryBodies.size > 1) | ||
| List( | ||
| s"Endpoint $shown declares more than one request body in securityIn. Only one request body may be part " + | ||
| s"of the API contract." | ||
| ) | ||
| else if (inPrimaryBodies.size > 1) | ||
| List( | ||
| s"Endpoint $shown declares more than one request body in in. Only one request body may be part of the " + | ||
| s"API contract." | ||
| ) | ||
| else Nil | ||
|
|
||
| val streamWithSecondary = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude-generated review Smaller things in this file:
|
||
| if (streamingPrimary && secondary.nonEmpty) | ||
| List( | ||
| s"Endpoint $shown combines a streaming body with a secondary body. The request body can either be " + | ||
| s"streamed lazily or buffered for repeated reads, not both." | ||
| ) | ||
| else Nil | ||
|
|
||
| val nonReplayableWithSecondary = | ||
| if (nonReplayablePrimary && secondary.nonEmpty) | ||
| List( | ||
| s"Endpoint $shown combines a file or multipart body with a secondary body. Reading the secondary body " + | ||
| s"consumes the request; the file or multipart body would then be read from an already-drained request." | ||
| ) | ||
| else Nil | ||
|
|
||
| val bodyCarryingMethod = endpoint.method.exists(m => m == Method.POST || m == Method.PUT || m == Method.PATCH) | ||
| val secondaryWithoutPrimary = | ||
| if (secondary.nonEmpty && primaryBodies.isEmpty && bodyCarryingMethod) | ||
| List( | ||
| s"Endpoint $shown reads a secondary request body, but no request body is part of the API contract: it " + | ||
| s"will be absent from the documentation and clients will not send it. Either declare the body in `in` " + | ||
| s"as well, or drop asSecondary and use the body input directly." | ||
| ) | ||
| else Nil | ||
|
|
||
| val uselessMetadata = | ||
| secondary.filter(b => b.info.description.isDefined || b.info.examples.nonEmpty).map { b => | ||
| s"Endpoint $shown sets a description or example on the secondary body ${b.show}, which never reaches the " + | ||
| s"documentation, as secondary bodies are excluded from it." | ||
| } | ||
|
|
||
| EndpointBodyProblems( | ||
| errors = secondaryInsideOneOfBody ++ tooManyPrimaries ++ streamWithSecondary ++ nonReplayableWithSecondary, | ||
| warnings = (secondaryWithoutPrimary ++ uselessMetadata).toList | ||
| ) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package sttp.tapir | ||
|
|
||
| import org.scalatest.flatspec.AnyFlatSpec | ||
| import org.scalatest.matchers.should.Matchers | ||
|
|
||
| class SecondaryBodyTest extends AnyFlatSpec with Matchers { | ||
| it should "mark a string body as secondary" in { | ||
| stringBody.asSecondary.attribute(SecondaryBody.attributeKey) shouldBe Some(SecondaryBody()) | ||
| } | ||
|
|
||
| it should "mark a json-style string body as secondary" in { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Claude-generated review Some trimming:
Worth a comment on line 39: |
||
| val body = stringBodyUtf8AnyFormat(Codec.string) | ||
| body.asSecondary.attribute(SecondaryBody.attributeKey) shouldBe Some(SecondaryBody()) | ||
| } | ||
|
|
||
| it should "leave a plain body unmarked" in { | ||
| stringBody.attribute(SecondaryBody.attributeKey) shouldBe None | ||
| stringBody.isSecondary shouldBe false | ||
| } | ||
|
|
||
| it should "report a marked body through isSecondary" in { | ||
| stringBody.asSecondary.isSecondary shouldBe true | ||
| } | ||
|
|
||
| it should "preserve the codec and body type" in { | ||
| val secondary = byteArrayBody.asSecondary | ||
| secondary.bodyType shouldBe RawBodyType.ByteArrayBody | ||
| secondary.codec shouldBe byteArrayBody.codec | ||
| } | ||
|
|
||
| it should "not compile for file bodies" in { | ||
| assertDoesNotCompile("fileBody.asSecondary") | ||
| } | ||
|
|
||
| it should "not compile for multipart bodies" in { | ||
| assertDoesNotCompile("multipartBody.asSecondary") | ||
| } | ||
|
|
||
| it should "not compile for oneOfBody" in { | ||
| assertDoesNotCompile("""oneOfBody(stringBody, stringBody).asSecondary""") | ||
| } | ||
|
|
||
| it should "render a secondary body distinctly in show" in { | ||
| stringBody.asSecondary.show shouldBe "{secondary body as text/plain (UTF-8)}" | ||
| } | ||
|
|
||
| it should "render a plain body unchanged in show" in { | ||
| stringBody.show shouldBe "{body as text/plain (UTF-8)}" | ||
| } | ||
|
|
||
| it should "report secondary bodies through the internal predicate" in { | ||
| import sttp.tapir.internal._ | ||
| isSecondaryBodyInput(stringBody.asSecondary) shouldBe true | ||
| isSecondaryBodyInput(stringBody) shouldBe false | ||
| isSecondaryBodyInput(query[String]("q")) shouldBe false | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both SecondaryBody & ReplayableRawBody aren't really directly used by users. So maybe they don't have to be in the main namespace, which is auto-imported by
import sttp.tapir.*. Instead, maybe we could put them e.g. insideEndpointIO.Bodycompanion object? Or maybe there's a better place existing in the hierarchy?