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

allow disable limit_payload middleware #1113

Merged
merged 10 commits into from
Jan 5, 2025
Merged

allow disable limit_payload middleware #1113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9f7e964
bugfix: allow disable limit_payload middleware
kaplanelad Dec 29, 2024
902a0c3
fix clippy
kaplanelad Dec 29, 2024
e6cfda3
fix clippy
kaplanelad Dec 29, 2024
9c7eb38
Merge branch 'master' into disable-limit-payload-middleware
kaplanelad Dec 29, 2024
484948a
Merge branch 'master' into disable-limit-payload-middleware
kaplanelad Jan 1, 2025
827d3ae
Merge branch 'master' into disable-limit-payload-middleware
kaplanelad Jan 2, 2025
54781d8
Increase the default payload size instead of disabling the middleware
kaplanelad Jan 2, 2025
f2225fb
Merge branch 'master' into disable-limit-payload-middleware
kaplanelad Jan 5, 2025
5b6fba3
not limit
kaplanelad Jan 5, 2025
2e9e1aa
Merge branch 'master' into disable-limit-payload-middleware
kaplanelad Jan 5, 2025
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
1 change: 0 additions & 1 deletion docs-site/content/docs/getting-started/axum-users.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ In Loco you:
server:
middlewares:
limit_payload:
enable: true
body_limit: 5mb
# .. more middleware below ..
```
Expand Down
27 changes: 16 additions & 11 deletions docs-site/content/docs/the-app/controller.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ This is the stack in `development` mode:
```sh
$ cargo loco middleware --config

limit_payload {"enable":true,"body_limit":2000000}
limit_payload {"body_limit":{"Limit":1000000}}
cors {"enable":true,"allow_origins":["any"],"allow_headers":["*"],"allow_methods":["*"],"max_age":null,"vary":["origin","access-control-request-method","access-control-request-headers"]}
catch_panic {"enable":true}
etag {"enable":true}
Expand All @@ -294,8 +294,6 @@ Take what ever is enabled, and use `enable: false` with the relevant field. If `
```yaml
server:
middlewares:
limit_payload:
enable: false
cors:
enable: false
catch_panic:
Expand All @@ -317,7 +315,6 @@ $ cargo loco middleware --config
powered_by {"ident":"loco.rs"}


limit_payload (disabled)
cors (disabled)
catch_panic (disabled)
etag (disabled)
Expand Down Expand Up @@ -354,7 +351,7 @@ The result:
```sh
$ cargo loco middleware --config

limit_payload {"enable":true,"body_limit":2000000}
limit_payload {"body_limit":{"Limit":1000000}}
cors {"enable":true,"allow_origins":["any"],"allow_headers":["*"],"allow_methods":["*"],"max_age":null,"vary":["origin","access-control-request-method","access-control-request-headers"]}
catch_panic {"enable":true}
etag {"enable":true}
Expand All @@ -372,7 +369,6 @@ Let's change the request body limit to `5mb`. When overriding a middleware confi
```yaml
middlewares:
limit_payload:
enable: true
body_limit: 5mb
```

Expand All @@ -381,7 +377,7 @@ The result:
```sh
$ cargo loco middleware --config

limit_payload {"enable":true,"body_limit":5000000}
limit_payload {"body_limit":{"Limit":5000000}}
cors {"enable":true,"allow_origins":["any"],"allow_headers":["*"],"allow_methods":["*"],"max_age":null,"vary":["origin","access-control-request-method","access-control-request-headers"]}
catch_panic {"enable":true}
etag {"enable":true}
Expand Down Expand Up @@ -480,19 +476,28 @@ To disable the middleware edit the configuration as follows:

## Limit Payload

Restricts the maximum allowed size for HTTP request payloads.
The middleware by default is enabled and configured to 2MB.
The Limit Payload middleware restricts the maximum allowed size for HTTP request payloads. By default, it is enabled and configured with a 2MB limit.

You can disable or customize this behavior in your config file. You can set a few options:
You can customize or disable this behavior through your configuration file.

### Set a custom limit
```yaml
#...
middlewares:
limit_payload:
enable: true
body_limit: 5mb
```
Copy link
Contributor

@BWStearns BWStearns Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe add an example of disabling the limit so they don't need to dig/guess for the param?

Could we allow a disable here where the middleware wouldn't be disabled but we use DefaultBodyLimitKind::Disable in the apply function? As far as I can tell the current PR doesn't let a user fully disable upload size. Something like below?

#[derive(Debug, Clone, Deserialize, Serialize)]
pub struct LimitPayload {
    #[serde(default)]
    pub enable: bool,
    #[serde(deserialize_with = "deserialize_body_limit")]
    #[serde(default = "default_body_limit")]
    pub body_limit: usize,
    pub no_limit: Option<bool>,
}

// ......

impl MiddlewareLayer for LimitPayload {

    /// Applies the payload limit middleware to the application router by adding
    /// a `DefaultBodyLimit` layer.
    fn apply(&self, app: AXRouter<AppContext>) -> Result<AXRouter<AppContext>> {
        match self.no_limit {
            Some(true) => Ok(app.layer(axum::extract::DefaultBodyLimit::disable())),
            _ => Ok(app.layer(axum::extract::DefaultBodyLimit::max(self.body_limit))),
        }
    }

### Remove the limit

_Caution: this can be dangerous because it would allow arbitrarily large files to be uploaded._

```yaml
#...
  middlewares:
    limit_payload:
      no_limit: true
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right, this was part of the initial commit. After discussing it with @jondot, we wanted to understand why users might want the option to disable the limits.

We reviewed the middleware implementation and found no performance concerns with keeping the limits enabled. To ensure protection for users, we've decided to set a reasonable default limit, as it was originally. If a user needs a larger limit, they can easily increase it to suit their needs.

The goal is to strike a balance between safety and flexibility.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, in practice it's probably fine to set a stupidly large limit when you want "unlimited", but stating clearly that it's unlimited is more clear for future readers, vs wondering why 100GB is set as a limit. It's more about communication of intent than a strict requirement I guess.

For my use case I might be uploading kind of arbitrarily large log files, but they're realistically not often going to be more than a couple GB. I am all onboard with sensible defaults, and leaving the safeties on as a default is a good idea, but for some applications it might be nice to be able to switch them off.

Since DefaultBodyLimit::disable is available in Axum I think it makes sense to allow ergonomic access to it via the middleware settings as long as it is flagged as "this is potentially a massive footgun, think before disabling" in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BWStearns i agree,
reverted to the first implementation


### Disable payload size limitation
To remove the restriction entirely, set `body_limit` to `disable`:
```yaml
#...
middlewares:
limit_payload:
body_limit: disable
```


##### Usage
In your controller parameters, use `axum::body::Bytes`.
```rust
Expand Down
50 changes: 33 additions & 17 deletions src/controller/middleware/limit_payload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,39 +16,50 @@ use serde::{Deserialize, Deserializer, Serialize};

use crate::{app::AppContext, controller::middleware::MiddlewareLayer, Result};

#[derive(Debug, Clone, Copy, Deserialize, Serialize)]
pub enum DefaultBodyLimitKind {
Disable,
Limit(usize),
}

#[derive(Debug, Clone, Deserialize, Serialize)]
pub struct LimitPayload {
#[serde(default)]
pub enable: bool,
#[serde(deserialize_with = "deserialize_body_limit")]
#[serde(default = "default_body_limit")]
pub body_limit: usize,
#[serde(
default = "default_body_limit",
deserialize_with = "deserialize_body_limit"
)]
pub body_limit: DefaultBodyLimitKind,
}

impl Default for LimitPayload {
fn default() -> Self {
Self {
enable: false,
body_limit: default_body_limit(),
}
}
}

fn default_body_limit() -> usize {
2_000_000
/// Returns the default body limit in bytes (2MB).
fn default_body_limit() -> DefaultBodyLimitKind {
DefaultBodyLimitKind::Limit(2_000_000)
}

fn deserialize_body_limit<'de, D>(deserializer: D) -> Result<usize, D::Error>
fn deserialize_body_limit<'de, D>(deserializer: D) -> Result<DefaultBodyLimitKind, D::Error>
where
D: Deserializer<'de>,
{
Ok(
byte_unit::Byte::from_str(String::deserialize(deserializer)?)
.map_err(|err| serde::de::Error::custom(err.to_string()))?
.get_bytes() as usize,
)
}
let s: String = String::deserialize(deserializer)?;

match s.as_str() {
"disable" => Ok(DefaultBodyLimitKind::Disable),
limit => {
let bytes = byte_unit::Byte::from_str(limit)
.map_err(|err| serde::de::Error::custom(err.to_string()))?
.get_bytes();
Ok(DefaultBodyLimitKind::Limit(bytes as usize))
}
}
}
impl MiddlewareLayer for LimitPayload {
/// Returns the name of the middleware
fn name(&self) -> &'static str {
Expand All @@ -57,7 +68,7 @@ impl MiddlewareLayer for LimitPayload {

/// Returns whether the middleware is enabled or not
fn is_enabled(&self) -> bool {
self.enable
true
}

fn config(&self) -> serde_json::Result<serde_json::Value> {
Expand All @@ -67,6 +78,11 @@ impl MiddlewareLayer for LimitPayload {
/// Applies the payload limit middleware to the application router by adding
/// a `DefaultBodyLimit` layer.
fn apply(&self, app: AXRouter<AppContext>) -> Result<AXRouter<AppContext>> {
Ok(app.layer(axum::extract::DefaultBodyLimit::max(self.body_limit)))
let body_limit_layer = match self.body_limit {
DefaultBodyLimitKind::Disable => axum::extract::DefaultBodyLimit::disable(),
DefaultBodyLimitKind::Limit(limit) => axum::extract::DefaultBodyLimit::max(limit),
};

Ok(app.layer(body_limit_layer))
}
}
11 changes: 1 addition & 10 deletions src/controller/middleware/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ pub mod static_assets;
pub mod timeout;

use axum::Router as AXRouter;
use limit_payload::LimitPayload;
use serde::{Deserialize, Serialize};

use crate::{app::AppContext, environment::Environment, Result};
Expand Down Expand Up @@ -76,15 +75,7 @@ pub fn default_middleware_stack(ctx: &AppContext) -> Vec<Box<dyn MiddlewareLayer

vec![
// Limit Payload middleware with a default if none
Box::new(
middlewares
.limit_payload
.clone()
.unwrap_or_else(|| LimitPayload {
enable: true,
..Default::default()
}),
),
Box::new(middlewares.limit_payload.clone().unwrap_or_default()),
// CORS middleware with a default if none
Box::new(middlewares.cors.clone().unwrap_or_else(|| cors::Cors {
enable: false,
Expand Down
23 changes: 12 additions & 11 deletions tests/controller/middlewares.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,19 +215,17 @@ async fn cors(
}

#[rstest]
#[case(true)]
#[case(false)]
#[case(middleware::limit_payload::DefaultBodyLimitKind::Limit(0x1B))]
#[case(middleware::limit_payload::DefaultBodyLimitKind::Disable)]
#[tokio::test]
#[serial]
async fn limit_payload(#[case] enable: bool) {
async fn limit_payload(#[case] limit: middleware::limit_payload::DefaultBodyLimitKind) {
configure_insta!();

let mut ctx: AppContext = tests_cfg::app::get_app_context().await;

ctx.config.server.middlewares.limit_payload = Some(middleware::limit_payload::LimitPayload {
enable,
body_limit: 0x1B,
});
ctx.config.server.middlewares.limit_payload =
Some(middleware::limit_payload::LimitPayload { body_limit: limit });

let handle = infra_cfg::server::start_from_ctx(ctx).await;

Expand All @@ -238,10 +236,13 @@ async fn limit_payload(#[case] enable: bool) {
.await
.expect("valid response");

if enable {
assert_eq!(res.status(), StatusCode::PAYLOAD_TOO_LARGE);
} else {
assert_eq!(res.status(), StatusCode::OK);
match limit {
middleware::limit_payload::DefaultBodyLimitKind::Disable => {
assert_eq!(res.status(), StatusCode::OK);
}
middleware::limit_payload::DefaultBodyLimitKind::Limit(_) => {
assert_eq!(res.status(), StatusCode::PAYLOAD_TOO_LARGE);
}
}

handle.abort();
Expand Down
Loading