Decode application/x-www-form-urlencoded bodies #363

plotnick · 2022-06-04T16:19:12Z

This is useful for compatibility with protocols such as OAuth 2.0 and SAML, which frequently use this encoding for request bodies. It is possible to work around this by using UntypedBody and decoding at the application level, but handling it in dropshot seems safer and less error-prone.

ahl

What should our approach be with regard to the OpenAPI description of APIs? Currently we claim they accept application/json only. I don't really want to say that they accept application/x-www-form-urlencoded as well, only because it uglies things up (and will fuck up progenitor, but that's my problem). I'm fine with just having it be un-expressed in OpenAPI, but wanted to get your opinion.

dropshot/src/handler.rs

david-crespo · 2022-06-08T16:47:46Z

What should our approach be with regard to the OpenAPI description of APIs? Currently we claim they accept application/json only. I don't really want to say that they accept application/x-www-form-urlencoded as well, only because it uglies things up (and will fuck up progenitor, but that's my problem). I'm fine with just having it be un-expressed in OpenAPI, but wanted to get your opinion.

Does this make every endpoint accept form-urlencoded or does it let us mark particular ones as accepting it? Seems like the former. I do think it should be in the spec, which is why it's a shame that this affects all endpoints — it will add a lot of noise. Ideally IMO it would a) be in the spec, but b) only on a few relevant endpoints.

I think the TS generator will be unaffected because we're specifically looking for the application/json key. If there was an endpoint that only took form-urlencoded and not json I might have to add a case for that (for now, probably by skipping such endpoints since I don't think the console needs to hit them).

plotnick · 2022-06-08T17:14:28Z

Does this make every endpoint accept form-urlencoded or does it let us mark particular ones as accepting it? ... Ideally IMO it would a) be in the spec, but b) only on a few relevant endpoints.

The current implementation does indeed make every endpoint accept form-urlencoded bodies, and I see now from your and @ahl's comments that that's not really the right approach. I will revise this so that it's opt-in and properly marked in the spec. Thank you both!

ahl · 2022-06-08T17:55:49Z

The current implementation does indeed make every endpoint accept form-urlencoded bodies, and I see now from your and @ahl's comments that that's not really the right approach. I will revise this so that it's opt-in and properly marked in the spec. Thank you both!

I think we have some options. I don't know what endpoints are going to use this behavior--do we want them in the OpenAPI spec? How much do we care that an endpoint must use application/json if the client happens to do form-urlencode?

In short: I think we can be a little loosey goosey here without significant consequence... especially in light of competing priorities.

If elided, defaults to "application/json". The requested content type is verified against the specified one by the handler.

plotnick · 2022-06-19T19:12:19Z

I took updating this as a background task, and after a few tries I think I have something (c903eb8) that satisfies the desiderata. I'm not wild about needing to pass the content-type into the Extractor::metadata method, but I could not find any way around it; I think everything else should be more or less fine, but feedback (as always) would be welcome.

ahl

looking good. thanks for making the changes

two questions?
Are we going to use this in a place where we'll want progenitor support?

How does this work for nested structs? Looking briefly at the serde_urlencoded crate I think it produces a runtime error. Might we want to look at the schema for bodies to give a friendly error when the server starts?

ahl · 2022-06-21T19:05:41Z

dropshot_endpoint/src/lib.rs

+///     // Specifies the media type used to encode the request body
+///     content_type = { "application/json" | "application/x-www-form-urlencoded" }


I guess this is the best approach. The alternative I could think of is to have a wrapper type so that endpoints look something like..

async fn demo_handler_args_2urlencoded( _rqctx: RequestCtx, body: TypedBody<DemoJsonBody, FormUrlEncodedMarkerTypeOrWhatever>, ) -> Result<Response<Body>, HttpError> { // ... }

I don't think that's actually better--it just moves the metadata from the macro to the type. It would be both messier and harder to see.

@davepacheco do you have any thoughts here?

ahl · 2022-06-21T19:08:29Z

dropshot_endpoint/src/lib.rs

+    let content_type =
+        metadata.content_type.unwrap_or_else(|| "application/json".to_string());


Can we validate that this is application/x-www-form-urlencoded? What do you think about a trybuild test (the bad_endpointNNN.rs tests) to catch an invalid content type at build time?

So, ApiEndpointBodyContentType::from_mime_type raises an error on mime types that it doesn't recognize, but you're right that it's not at build time. I thought we should have only one place that knows which types are acceptable, but was hesitant to move ApiEndpointBodyContentType and the associated constants into dropshot_endpoint. fdc22c8 adds a simple check at build time and a new bad_endpoint16.rs test, but does duplicate the list of acceptable types.

Looks good; two small nits, but up to you if you want to change them

ahl · 2022-06-21T19:09:42Z

dropshot/src/api_description.rs

+    /** application/x-www-form-urlencoded */
+    UrlEncoded,


do we care that the user says "application/x-www-form-urlencoded" or could we just let them give us any string and have the variant be Yolo(String)?

In all but the UntypedBody case, we do care because we need to know how to decode the body.

sorry: I know we care what it is for decoding, but can we stuff that into a string rather than having Json and UrlEncoded as the two enumerated options?

Mmm, we could, but I'm not sure I see the advantage. We could certainly add another Yolo variant for arbitrary types, but since these are the ones that we handle specially I think it makes sense to have variants for them. Otherwise why not just use strings for content type everywhere?

plotnick · 2022-06-22T16:53:11Z

Are we going to use this in a place where we'll want progenitor support?

In the immediate use case I have, we won't need progenitor to handle the urlencoded endpoints (since it's for OAuth, for which we'll use third-party libs). So... maybe?

How does this work for nested structs?

That's an excellent question; I hadn't thought of that at all. Is there an easy way to check for those? And could we take that as a follow-up?

ahl · 2022-06-22T20:27:33Z

How does this work for nested structs?

That's an excellent question; I hadn't thought of that at all. Is there an easy way to check for those? And could we take that as a follow-up?

I'm fine with this being just filed as an issue for future work.

I think the way to do it would be to look at the JsonSchema and make sure it's an object whose properties are all scalars.

plotnick · 2022-06-22T21:17:46Z

maybe we just return here to avoid the extra boilerplate?

Yep, we sure can; thanks! Fixed in d1963f7.

plotnick · 2022-06-22T21:18:34Z

I don't think this is valid, is it?

Indeed not, thank you. Also fixed in d1963f7.

plotnick added 3 commits June 3, 2022 20:59

Decode application/x-www-form-urlencoded bodies

14d701d

Tests for application/x-www-form-urlencoded bodies

a781028

fmt

850f24e

ahl reviewed Jun 8, 2022

View reviewed changes

dropshot/src/handler.rs Outdated Show resolved Hide resolved

Merge branch 'main' into urlencoded

e14906b

plotnick added 2 commits June 8, 2022 11:04

trim_end

f235c0a

fmt

8819b15

plotnick mentioned this pull request Jun 10, 2022

API client authentication and token granting oxidecomputer/omicron#1194

Merged

7 tasks

Optionally specify content type in endpoints

c903eb8

If elided, defaults to "application/json". The requested content type is verified against the specified one by the handler.

plotnick added 3 commits June 19, 2022 13:12

fmt

94ef971

Merge branch 'main' into urlencoded

bf7b2f4

clippy

b62403f

plotnick requested a review from ahl June 21, 2022 18:51

ahl reviewed Jun 21, 2022

View reviewed changes

Check endpoint content type at build time

fdc22c8

Better error reporting for invalid endpoint content type

d1963f7

Merge branch 'main' into urlencoded

a5dae5f

plotnick mentioned this pull request Jun 22, 2022

Raise a build time error for nested structs with URL-encoded bodies #382

Open

plotnick merged commit e75844e into main Jun 23, 2022

plotnick deleted the urlencoded branch June 23, 2022 15:15

plotnick mentioned this pull request Jun 27, 2022

Support endpoints without application/json bodies oxidecomputer/oxide.ts#119

Merged

		/// // Specifies the media type used to encode the request body
		/// content_type = { "application/json" \| "application/x-www-form-urlencoded" }

		let content_type =
		metadata.content_type.unwrap_or_else(\|\| "application/json".to_string());

Decode application/x-www-form-urlencoded bodies #363

Decode application/x-www-form-urlencoded bodies #363

Uh oh!

Conversation

plotnick commented Jun 4, 2022

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-crespo commented Jun 8, 2022

Uh oh!

plotnick commented Jun 8, 2022

Uh oh!

ahl commented Jun 8, 2022

Uh oh!

plotnick commented Jun 19, 2022

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plotnick commented Jun 22, 2022

Uh oh!

ahl commented Jun 22, 2022

Uh oh!

plotnick commented Jun 22, 2022

Uh oh!

plotnick commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants