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

The type checking for parameters could be much more stringent #123

Merged
merged 1 commit into from
Jul 28, 2021
Merged

The type checking for parameters could be much more stringent #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
145 changes: 140 additions & 5 deletions dropshot/src/api_description.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@ use crate::router::route_path_to_segments;
use crate::router::HttpRouter;
use crate::router::PathSegment;
use crate::server::ServerContext;
use crate::type_util::type_is_scalar;
use crate::type_util::type_is_string_enum;
use crate::Extractor;
use crate::CONTENT_TYPE_JSON;
use crate::CONTENT_TYPE_OCTET_STREAM;

use http::Method;
use http::StatusCode;
use std::collections::BTreeMap;
use std::collections::HashSet;

/**
Expand Down Expand Up @@ -232,6 +235,23 @@ impl<Context: ServerContext> ApiDescription<Context> {
{
let e = endpoint.into();

self.validate_path_parameters(&e)?;
self.validate_body_parameters(&e)?;
self.validate_named_parameters(&e)?;

self.router.insert(e);

Ok(())
}

/**
* Validate that the parameters specified in the path match the parameters
* specified by the path parameter arguments to the handler function.
*/
fn validate_path_parameters(
&self,
e: &ApiEndpoint<Context>,
) -> Result<(), String> {
// Gather up the path parameters and the path variable components, and
// make sure they're identical.
let path = route_path_to_segments(&e.path)
Expand Down Expand Up @@ -262,7 +282,7 @@ impl<Context: ServerContext> ApiDescription<Context> {
let vv =
v.iter().map(|s| s.as_str()).collect::<Vec<&str>>().join(",");

return match (p.is_empty(), v.is_empty()) {
match (p.is_empty(), v.is_empty()) {
(false, true) => Err(format!(
"{} ({})",
"path parameters are not consumed", pp,
Expand All @@ -278,9 +298,19 @@ impl<Context: ServerContext> ApiDescription<Context> {
"specified parameters do not appear in the path",
vv,
)),
};
}?;
}

Ok(())
}

/**
* Validate that we have a single body parameter.
*/
fn validate_body_parameters(
&self,
e: &ApiEndpoint<Context>,
) -> Result<(), String> {
// Explicitly disallow any attempt to consume the body twice.
let nbodyextractors = e
.parameters
Expand All @@ -298,7 +328,81 @@ impl<Context: ServerContext> ApiDescription<Context> {
));
}

self.router.insert(e);
Ok(())
}

/**
* Validate that named parameters have appropriate types and their aren't
* duplicates. Parameters must have scalar types except in the case of the
* received for a wildcard path which must be an array of String.
*/
fn validate_named_parameters(
&self,
e: &ApiEndpoint<Context>,
) -> Result<(), String> {
enum SegmentOrWildcard {
Segment,
Wildcard,
}
let path_segments = route_path_to_segments(&e.path)
.iter()
.filter_map(|segment| {
let seg = PathSegment::from(segment);
match seg {
PathSegment::VarnameSegment(v) => {
Some((v, SegmentOrWildcard::Segment))
}
PathSegment::VarnameWildcard(v) => {
Some((v, SegmentOrWildcard::Wildcard))
}
PathSegment::Literal(_) => None,
}
})
.collect::<BTreeMap<_, _>>();

for param in &e.parameters {
/* Skip anything that's not a path or query parameter (i.e. body) */
match &param.metadata {
ApiEndpointParameterMetadata::Path(_)
| ApiEndpointParameterMetadata::Query(_) => (),
_ => continue,
}
/* Only body parameters should have unresolved schemas */
let (schema, dependencies) = match &param.schema {
ApiSchemaGenerator::Static {
schema,
dependencies,
} => (schema, dependencies),
_ => unreachable!(),
};

match &param.metadata {
ApiEndpointParameterMetadata::Path(ref name) => {
match path_segments.get(name) {
Some(SegmentOrWildcard::Segment) => {
type_is_scalar(name, schema, dependencies)?;
}
Some(SegmentOrWildcard::Wildcard) => {
type_is_string_enum(name, schema, dependencies)?;
}
None => {
panic!("all path variables should be accounted for")
}
}
}
ApiEndpointParameterMetadata::Query(ref name) => {
if path_segments.get(name).is_some() {
return Err(format!(
"the parameter '{}' is specified for both query \
and path parameters",
name
));
}
type_is_scalar(name, schema, dependencies)?;
}
_ => (),
}
}

Ok(())
}
Expand Down Expand Up @@ -374,8 +478,6 @@ impl<Context: ServerContext> ApiDescription<Context> {
* Internal routine for constructing the OpenAPI definition describing this
* API in its JSON form.
*/
// TODO: There's a bunch of error handling we need here such as checking
// for duplicate parameter names.
fn gen_openapi(&self, info: openapiv3::Info) -> openapiv3::OpenAPI {
let mut openapi = openapiv3::OpenAPI::default();

Expand Down Expand Up @@ -1181,6 +1283,7 @@ mod test {
use super::ApiEndpoint;
use crate as dropshot; /* for "endpoint" macro */
use crate::endpoint;
use crate::Query;
use crate::TypedBody;
use crate::UntypedBody;
use http::Method;
Expand Down Expand Up @@ -1349,6 +1452,38 @@ mod test {
);
}

#[test]
fn test_dup_names() {
#[derive(Deserialize, JsonSchema)]
struct AStruct {}

#[allow(dead_code)]
#[derive(Deserialize, JsonSchema)]
struct TheThing {
thing: String,
}

#[endpoint {
method = PUT,
path = "/testing/{thing}"
}]
async fn test_dup_names_handler(
_: Arc<RequestContext<()>>,
_: Query<TheThing>,
_: Path<TheThing>,
) -> Result<Response<Body>, HttpError> {
unimplemented!();
}

let mut api = ApiDescription::new();
let error = api.register(test_dup_names_handler).unwrap_err();
assert_eq!(
error,
"the parameter 'thing' is specified for both query and path \
parameters",
);
}

#[test]
fn test_additional_properties() {
#[allow(dead_code)]
Expand Down
151 changes: 4 additions & 147 deletions dropshot/src/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,8 +755,8 @@ fn schema2parameters(
) -> Vec<ApiEndpointParameter> {
/*
* We ignore schema.metadata, which includes things like doc comments, and
* schema.extensions. We call these out explicitly rather than .. since we
* match all other fields in the structure.
* schema.extensions. We call these out explicitly rather than eliding them
* as .. since we match all other fields in the structure.
*/
match schema {
/* We expect references to be on their own. */
Expand All @@ -779,7 +779,8 @@ fn schema2parameters(
generator,
required,
),
// Match objects and subschemas.

/* Match objects and subschemas. */
schemars::schema::Schema::Object(schemars::schema::SchemaObject {
metadata: _,
instance_type: Some(schemars::schema::SingleOrVec::Single(_)),
Expand All @@ -801,8 +802,6 @@ fn schema2parameters(
if let Some(object) = object {
parameters.extend(object.properties.iter().map(
|(name, schema)| {
validate_parameter_schema(loc, name, schema, generator);

// We won't often see referenced schemas here, but we may
// in the case of enumerated strings. To handle this, we
// package up the dependencies to include in the top-
Expand Down Expand Up @@ -880,148 +879,6 @@ fn schema2parameters(
}
}

/**
* Confirm that the parameter schema type is either an atomic value (i.e.
* string, number, boolean) or an array of strings (to accommodate wild card
* paths).
*/
fn validate_parameter_schema(
loc: &ApiEndpointParameterLocation,
name: &String,
schema: &schemars::schema::Schema,
generator: &schemars::gen::SchemaGenerator,
) {
match schema {
/*
* We expect references to be on their own.
*/
schemars::schema::Schema::Object(schemars::schema::SchemaObject {
metadata: _,
instance_type: None,
format: None,
enum_values: None,
const_value: None,
subschemas: None,
number: None,
string: None,
array: None,
object: None,
reference: Some(_),
extensions: _,
}) => validate_parameter_schema(
loc,
name,
generator.dereference(schema).expect("invalid reference"),
generator,
),

/*
* Match atomic value types.
*/
schemars::schema::Schema::Object(schemars::schema::SchemaObject {
instance_type:
Some(schemars::schema::SingleOrVec::Single(instance_type)),
subschemas: None,
array: None,
object: None,
reference: None,
..
}) if match instance_type.as_ref() {
InstanceType::Boolean
| InstanceType::Number
| InstanceType::String
| InstanceType::Integer => true,
_ => false,
} =>
{ /* All good. */ }

/*
* For path parameters only, match array types and validate that their
* items are strings.
*/
schemars::schema::Schema::Object(schemars::schema::SchemaObject {
metadata: _,
instance_type:
Some(schemars::schema::SingleOrVec::Single(instance_type)),
format: None,
enum_values: None,
const_value: None,
subschemas: None,
number: None,
string: None,
array: Some(array_validation),
object: None,
reference: None,
extensions: _,
}) if matches!(loc, ApiEndpointParameterLocation::Path)
&& matches!(instance_type.as_ref(), InstanceType::Array) =>
{
match array_validation.as_ref() {
schemars::schema::ArrayValidation {
items:
Some(schemars::schema::SingleOrVec::Single(item_schema)),
additional_items: None,
..
} => validate_string_schema(name, item_schema, generator),
_ => panic!("parameter {} has an invalid array type", name),
}
}

_ => panic!("parameter {} has an invalid type", name),
}
}

/**
* Validate that the given schema is for a simple string type.
*/
fn validate_string_schema(
name: &String,
schema: &schemars::schema::Schema,
generator: &schemars::gen::SchemaGenerator,
) {
match schema {
/*
* We expect references to be on their own.
*/
schemars::schema::Schema::Object(schemars::schema::SchemaObject {
metadata: _,
instance_type: None,
format: None,
enum_values: None,
const_value: None,
subschemas: None,
number: None,
string: None,
array: None,
object: None,
reference: Some(_),
extensions: _,
}) => validate_string_schema(
name,
generator.dereference(schema).expect("invalid reference"),
generator,
),

/*
* Match string value types.
*/
schemars::schema::Schema::Object(schemars::schema::SchemaObject {
instance_type:
Some(schemars::schema::SingleOrVec::Single(instance_type)),
subschemas: None,
array: None,
object: None,
reference: None,
..
}) if matches!(instance_type.as_ref(), InstanceType::String) => { /* All good. */
}

_ => {
panic!("parameter {} is an array of a type other than string", name)
}
}
}

/*
* TypedBody: body extractor for formats that can be deserialized to a specific
* type. Only JSON is currently supported.
Expand Down
1 change: 1 addition & 0 deletions dropshot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,7 @@ mod logging;
mod pagination;
mod router;
mod server;
mod type_util;

pub mod test_util;

Expand Down
Loading