Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Runtime] Improve parameter handling of MIME types in content types #113

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[Runtime] Improve parameter handling of MIME types in content types #113

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e8a8e9
[Runtime] Improve parameter handling of MIME types in content types
czechboy0 Aug 13, 2024
ba9d50c
Merge branch 'main' into hd-mime-type-params
czechboy0 Sep 17, 2024
71849ae
WIP on rewriting, doesn't build yet
czechboy0 Sep 23, 2024
0950840
Merge branch 'main' into hd-mime-type-params
czechboy0 Oct 4, 2024
6604f96
Fix up the new approach based on PR feedback
czechboy0 Oct 4, 2024
4b0b0b8
Merge branch 'hd-mime-type-params' of github.com:czechboy0/swift-open…
czechboy0 Oct 4, 2024
b240a07
Reformat
czechboy0 Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions Sources/OpenAPIRuntime/Conversion/Converter+Server.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,34 @@ extension Converter {
// Drop everything after the optional semicolon (q, extensions, ...)
value.split(separator: ";")[0].trimmingCharacters(in: .whitespacesAndNewlines).lowercased()
}

if acceptValues.isEmpty { return }
if acceptValues.contains("*/*") { return }
if acceptValues.contains("\(substring.split(separator: "/")[0].lowercased())/*") { return }
if acceptValues.contains(where: { $0.localizedCaseInsensitiveContains(substring) }) { return }
guard let parsedSubstring = OpenAPIMIMEType(substring) else {
throw RuntimeError.invalidAcceptSubstring(substring)
}
guard case .concrete(let substringType, let substringSubtype) = parsedSubstring.kind else {
// If the substring content type has a wildcard, just let it through.
// It's not well defined how such a case should behave, so be permissive.
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this case correct?

The function is supposed to throw an error if the Accept header is present but incompatible with the provided content type. The comments here indicate that this case is handling where the header is wildcards, and there is such a case in the deleted part of the diff, but it looks like this handles any .concrete with arbitrary associated values for substringType and substringSubtype. Is this missing some use of .any values?

Currently, adding the following to the tests fails,

 let cases: [(HTTPFields, String, Bool)] = [
+            ([.accept: "text/plain"], "application/*", false),

Copy link
Collaborator Author

@czechboy0 czechboy0 Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this case isn't handled correctly in this shape, I tried to explain that in the comment in the else block. In general we don't have clarity on what to do with e.g. application/* (a partial wildcard) being the content type in the OpenAPI doc (as opposed to wildcards being part of the Accept header, which is well-defined). That's why I opted to go more relaxed here, as I don't know what to back up with being stricter.

I guess it comes down to whether we think of application/* as a content type that we have any information about whatsoever (so far I've though the answer is no, and treated it equally to */*). Maybe that's not the right place to land, and we can come up with rules that constrain it more. We just run the risk of getting it slightly wrong at which point users won't have an escape hatch. But I could be convinced of going in either direction.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have thought that if we have a partial, we would validate that the top-level types match in the validate function.

I can see that a partial wildcard, e.g. application/*, is a strange thing to return in the Content-Type header field of a HTTP response, but it's clear that it doesn't match an Accept header of text/plain, or text/*, or anything that doesn't start with application/.

For this reason, I think the least surprising semantics of this validate function is to reject it.

To pull on this thread a little more though:

  1. Is a partial wildcard content-type for a response valid OpenAPI (according to OAS)?
  2. If yes, then is that just an oversight? I.e. if it is valid, is it typically always a mistake, or is there a genuine use case for it?

The answers to these two questions might yield some more validation and/or warning generation when the generator runs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenAPI does allow partial and full wildcards in the content type, but I've not come up with a way to treat that any differently to raw data that we know nothing about.

Agreed that we should do at least the type match. I'll update the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From chatting with folks in the team and consulting the RFCs, I think we need to adjust this differently.

For Content-Type: a/*, I suggested it should be permitted if Accept: a/b but we should only permit if Accept: a/* because we should treat a/* as a concrete MIME type with * being an unrecognised, but concrete (not wildcard) subtype, when in the Content-Type header field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, updated with this more correct matching. Check out the unit tests, I can add more if there's a case I missed.


// Look for the first match.
for acceptValue in acceptValues {
// Fast path.
if acceptValue == substring { return }
guard let parsedAcceptValue = OpenAPIMIMEType(acceptValue) else {
throw RuntimeError.invalidExpectedContentType(acceptValue)
}
Comment on lines +67 to +69
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How permissive would we like to be here? If a client sends an Accept header with a valid content-type that the server supports, and an invalid one, should the server permit it? IIUC, this guard statement suggests not, but is that the desired behaviour?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we fail to even parse the header value in its entirety, I suspect something very wrong is happening and I think we should fail, yeah. If we skip it silently, it could lead to harder-to-debug cases where the user is getting unexpected content back, and we're not catching that on the server (even though we could).

Or do you have cases in mind where this could be too strict?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to consider the case where someone is calling the server from a not-generated client and might make mistakes and provide a malformed content-type followed by a valid one in the accept header. Having them get back a response of the valid one seems like a nicer experience than failing the request altogether, but I appreciate your opinion might differ here.

Copy link
Collaborator Author

@czechboy0 czechboy0 Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I genuinely wonder, if they're typing in a malformed content type, whether they'd want us to fall back to the other one, or to error out (since there's probably a reason they added it in the first place).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed more offline, we'll leave it this strict, but separately I also filed apple/swift-openapi-generator#644 so that we actually return 400, not 500 (as we'd return today when any error is thrown, including input validation error).

switch parsedAcceptValue.kind {
case .any: return
case .anySubtype(type: let type): if substringType.lowercased() == type.lowercased() { return }
case .concrete(type: let type, subtype: let subtype):
if type.lowercased() == substringType.lowercased()
&& subtype.lowercased() == substringSubtype.lowercased()
{
return
}
}
}
throw RuntimeError.unexpectedAcceptHeader(acceptHeader)
}

Expand Down
2 changes: 2 additions & 0 deletions Sources/OpenAPIRuntime/Errors/RuntimeError.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ internal enum RuntimeError: Error, CustomStringConvertible, LocalizedError, Pret
case invalidServerURL(String)
case invalidServerVariableValue(name: String, value: String, allowedValues: [String])
case invalidExpectedContentType(String)
case invalidAcceptSubstring(String)
case invalidHeaderFieldName(String)
case invalidBase64String(String)

Expand Down Expand Up @@ -85,6 +86,7 @@ internal enum RuntimeError: Error, CustomStringConvertible, LocalizedError, Pret
return
"Invalid server variable named: '\(name)', which has the value: '\(value)', but the only allowed values are: \(allowedValues.map { "'\($0)'" }.joined(separator: ", "))"
case .invalidExpectedContentType(let string): return "Invalid expected content type: '\(string)'"
case .invalidAcceptSubstring(let string): return "Invalid Accept header content type: '\(string)'"
case .invalidHeaderFieldName(let name): return "Invalid header field name: '\(name)'"
case .invalidBase64String(let string):
return "Invalid base64-encoded string (first 128 bytes): '\(string.prefix(128))'"
Expand Down
9 changes: 7 additions & 2 deletions Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Server.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,13 @@ final class Test_ServerConverterExtensions: Test_Runtime {
.accept: "text/html, application/xhtml+xml, application/xml;q=0.9, image/webp, */*;q=0.8"
]
let multiple: HTTPFields = [.accept: "text/plain, application/json"]
let params: HTTPFields = [.accept: "application/json; foo=bar"]
let cases: [(HTTPFields, String, Bool)] = [
// No Accept header, any string validates successfully
(emptyHeaders, "foobar", true),

// Accept: */*, any string validates successfully
(wildcard, "foobar", true),
// Accept: */*, any MIME type validates successfully
(wildcard, "foobaz/bar", true),

// Accept: text/*, so text/plain succeeds, application/json fails
(partialWildcard, "text/plain", true), (partialWildcard, "application/json", false),
Expand All @@ -58,6 +59,10 @@ final class Test_ServerConverterExtensions: Test_Runtime {

// Multiple values
(multiple, "text/plain", true), (multiple, "application/json", true), (multiple, "application/xml", false),

// Params
(params, "application/json; foo=bar", true), (params, "application/json; charset=utf-8; foo=bar", true),
(params, "application/json", true), (params, "text/plain", false),
]
for (headers, contentType, success) in cases {
if success {
Expand Down