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.

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

feat(servers): add max_response_size #711

Merged
merged 2 commits into from
Mar 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 16 additions & 1 deletion http-server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ pub struct Builder<M = ()> {
access_control: AccessControl,
resources: Resources,
max_request_body_size: u32,
max_response_body_size: u32,
keep_alive: bool,
/// Custom tokio runtime to run the server on.
tokio_runtime: Option<tokio::runtime::Handle>,
Expand All @@ -66,6 +67,7 @@ impl Default for Builder {
fn default() -> Self {
Self {
max_request_body_size: TEN_MB_SIZE_BYTES,
max_response_body_size: TEN_MB_SIZE_BYTES,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would it make more sense to define this as MAX_RESPONSE_BODY_SIZE instead of TEN_MB_SIZE_BYTES, to decouple naming from underlying value? But I think this is outside the scope of this PR, as the constant was already there 😄

Copy link
Member

Choose a reason for hiding this comment

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

yeah, could make sense :)

resources: Resources::default(),
access_control: AccessControl::default(),
keep_alive: true,
Expand Down Expand Up @@ -111,6 +113,7 @@ impl<M> Builder<M> {
pub fn set_middleware<T: Middleware>(self, middleware: T) -> Builder<T> {
Builder {
max_request_body_size: self.max_request_body_size,
max_response_body_size: self.max_response_body_size,
resources: self.resources,
access_control: self.access_control,
keep_alive: self.keep_alive,
Expand All @@ -125,6 +128,12 @@ impl<M> Builder<M> {
self
}

/// Sets the maximum size of a response body in bytes (default is 10 MiB).
pub fn max_response_body_size(mut self, size: u32) -> Self {
self.max_response_body_size = size;
self
}

/// Sets access control settings.
pub fn set_access_control(mut self, acl: AccessControl) -> Self {
self.access_control = acl;
Expand Down Expand Up @@ -190,6 +199,7 @@ impl<M> Builder<M> {
local_addr,
access_control: self.access_control,
max_request_body_size: self.max_request_body_size,
max_response_body_size: self.max_response_body_size,
resources: self.resources,
tokio_runtime: self.tokio_runtime,
middleware: self.middleware,
Expand Down Expand Up @@ -261,6 +271,8 @@ pub struct Server<M = ()> {
local_addr: Option<SocketAddr>,
/// Max request body size.
max_request_body_size: u32,
/// Max response body size.
max_response_body_size: u32,
/// Access control
access_control: AccessControl,
/// Tracker for currently used resources on the server
Expand All @@ -279,6 +291,7 @@ impl<M: Middleware> Server<M> {
/// Start the server.
pub fn start(mut self, methods: impl Into<Methods>) -> Result<ServerHandle, Error> {
let max_request_body_size = self.max_request_body_size;
let max_response_body_size = self.max_response_body_size;
let access_control = self.access_control;
let (tx, mut rx) = mpsc::channel(1);
let listener = self.listener;
Expand Down Expand Up @@ -341,6 +354,7 @@ impl<M: Middleware> Server<M> {
methods,
resources,
max_request_body_size,
max_response_body_size,
)
.await?;

Expand Down Expand Up @@ -429,6 +443,7 @@ async fn process_validated_request(
methods: Methods,
resources: Resources,
max_request_body_size: u32,
max_response_body_size: u32,
) -> Result<hyper::Response<hyper::Body>, HyperError> {
let (parts, body) = request.into_parts();

Expand All @@ -446,7 +461,7 @@ async fn process_validated_request(

// NOTE(niklasad1): it's a channel because it's needed for batch requests.
let (tx, mut rx) = mpsc::unbounded::<String>();
let sink = MethodSink::new_with_limit(tx, max_request_body_size);
let sink = MethodSink::new_with_limit(tx, max_response_body_size);

type Notif<'a> = Notification<'a, Option<&'a RawValue>>;

Expand Down
43 changes: 43 additions & 0 deletions http-server/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -405,3 +405,46 @@ async fn run_forever() {
server_handle.handle.take();
server_handle.with_timeout(TIMEOUT).await.unwrap();
}

#[tokio::test]
async fn can_set_the_max_request_body_size() {
let addr = "127.0.0.1:0";
// Rejects all requests larger than 100 bytes
let server = HttpServerBuilder::default().max_request_body_size(100).build(addr).unwrap();
let mut module = RpcModule::new(());
module.register_method("anything", |_p, _cx| Ok("a".repeat(100))).unwrap();
let addr = server.local_addr().unwrap();
let uri = to_http_uri(addr);
let handle = server.start(module).unwrap();

// Invalid: too long
let req = format!(r#"{{"jsonrpc":"2.0", "method":{}, "id":1}}"#, "a".repeat(100));
let response = http_request(req.into(), uri.clone()).with_default_timeout().await.unwrap().unwrap();
assert_eq!(response.body, oversized_request());

// Max request body size should not override the max response size
let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#;
let response = http_request(req.into(), uri.clone()).with_default_timeout().await.unwrap().unwrap();
assert_eq!(response.body, ok_response(JsonValue::String("a".repeat(100)), Id::Num(1)));

handle.stop().unwrap();
}

#[tokio::test]
async fn can_set_the_max_response_size() {
let addr = "127.0.0.1:0";
// Set the max response size to 100 bytes
let server = HttpServerBuilder::default().max_response_body_size(100).build(addr).unwrap();
let mut module = RpcModule::new(());
module.register_method("anything", |_p, _cx| Ok("a".repeat(101))).unwrap();
let addr = server.local_addr().unwrap();
let uri = to_http_uri(addr);
let handle = server.start(module).unwrap();

// Oversized response.
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above was done above test, is it worth also checking that the request size is not an issue when only the response size is limited?

Copy link
Member

@niklasad1 niklasad1 Mar 21, 2022

Choose a reason for hiding this comment

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

you mean that actual max response works when the actual response_size <= max_response_body_size?

Copy link
Member

Choose a reason for hiding this comment

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

it won't work unless you register another method for that.

let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#;
let response = http_request(req.into(), uri.clone()).with_default_timeout().await.unwrap().unwrap();
assert_eq!(response.body, oversized_response(Id::Num(1), 100));

handle.stop().unwrap();
}
15 changes: 13 additions & 2 deletions ws-server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ where
methods.clone(),
resources.clone(),
cfg.max_request_body_size,
cfg.max_response_body_size,
stop_monitor.clone(),
middleware,
id_provider,
Expand All @@ -290,6 +291,7 @@ async fn background_task(
methods: Methods,
resources: Resources,
max_request_body_size: u32,
max_response_body_size: u32,
stop_server: StopMonitor,
middleware: impl Middleware,
id_provider: Arc<dyn IdProvider>,
Expand All @@ -303,7 +305,7 @@ async fn background_task(
let close_notify_server_stop = close_notify.clone();

let stop_server2 = stop_server.clone();
let sink = MethodSink::new_with_limit(tx, max_request_body_size);
let sink = MethodSink::new_with_limit(tx, max_response_body_size);

middleware.on_connect();

Expand Down Expand Up @@ -476,7 +478,7 @@ async fn background_task(
// request in the batch and read the results off of a new channel, `rx_batch`, and then send the
// complete batch response back to the client over `tx`.
let (tx_batch, mut rx_batch) = mpsc::unbounded();
let sink_batch = MethodSink::new_with_limit(tx_batch, max_request_body_size);
let sink_batch = MethodSink::new_with_limit(tx_batch, max_response_body_size);
if let Ok(batch) = serde_json::from_slice::<Vec<Request>>(&d) {
tracing::debug!("recv batch len={}", batch.len());
tracing::trace!("recv: batch={:?}", batch);
Expand Down Expand Up @@ -624,6 +626,8 @@ impl AllowedValue {
struct Settings {
/// Maximum size in bytes of a request.
max_request_body_size: u32,
/// Maximum size in bytes of a response.
max_response_body_size: u32,
/// Maximum number of incoming connections allowed.
max_connections: u64,
/// Policy by which to accept or deny incoming requests based on the `Origin` header.
Expand All @@ -638,6 +642,7 @@ impl Default for Settings {
fn default() -> Self {
Self {
max_request_body_size: TEN_MB_SIZE_BYTES,
quake marked this conversation as resolved.
Show resolved Hide resolved
max_response_body_size: TEN_MB_SIZE_BYTES,
max_connections: MAX_CONNECTIONS,
allowed_origins: AllowedValue::Any,
allowed_hosts: AllowedValue::Any,
Expand Down Expand Up @@ -680,6 +685,12 @@ impl<M> Builder<M> {
self
}

/// Set the maximum size of a response body in bytes. Default is 10 MiB.
pub fn max_response_body_size(mut self, size: u32) -> Self {
self.settings.max_response_body_size = size;
self
}

/// Set the maximum number of connections allowed. Default is 100.
pub fn max_connections(mut self, max: u64) -> Self {
self.settings.max_connections = max;
Expand Down
24 changes: 23 additions & 1 deletion ws-server/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ async fn can_set_the_max_request_body_size() {
init_logger();

let addr = "127.0.0.1:0";
// Rejects all requests larger than 10 bytes
// Rejects all requests larger than 100 bytes
let server = WsServerBuilder::default().max_request_body_size(100).build(addr).await.unwrap();
let mut module = RpcModule::new(());
module.register_method("anything", |_p, _cx| Ok("a".repeat(100))).unwrap();
Expand All @@ -197,6 +197,28 @@ async fn can_set_the_max_request_body_size() {
let response = client.send_request_text(req).await.unwrap();
assert_eq!(response, oversized_request());

// Max request body size should not override the max response body size
let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#;
let response = client.send_request_text(req).await.unwrap();
assert_eq!(response, ok_response(JsonValue::String("a".repeat(100)), Id::Num(1)));

handle.stop().unwrap();
}

#[tokio::test]
async fn can_set_the_max_response_body_size() {
init_logger();

let addr = "127.0.0.1:0";
// Set the max response body size to 100 bytes
let server = WsServerBuilder::default().max_response_body_size(100).build(addr).await.unwrap();
let mut module = RpcModule::new(());
module.register_method("anything", |_p, _cx| Ok("a".repeat(101))).unwrap();
let addr = server.local_addr().unwrap();
let handle = server.start(module).unwrap();

let mut client = WebSocketTestClient::new(addr).await.unwrap();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as my above comment; should we check that setting one hasn't impacted the other here too?

// Oversized response.
let req = r#"{"jsonrpc":"2.0", "method":"anything", "id":1}"#;
let response = client.send_request_text(req).await.unwrap();
Expand Down