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

ref(server): Use multer directly #3978

Merged
merged 7 commits into from
Sep 6, 2024
Merged

ref(server): Use multer directly #3978

merged 7 commits into from
Sep 6, 2024

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Sep 3, 2024

Switches to direct use of multer instead of axum's wrapper. This
allows us to use constraints to configure the maximum body size and
maximum field size directly on the Multipart instance instead of
handling this manually.

In order to keep using the multipart type as an extractor, we define
Remote which is a shallow transparent wrapper around any remote type
we would like to implement FromRequest, FromRequestParts, or
IntoResponse for. This way, we can use the Multipart and
multer::Error types directly in our signatures. Usage of this type is
documented on the type.

Fixes #2218
See also the issue in axum

#skip-changelog

@jan-auer jan-auer marked this pull request as ready for review September 3, 2024 07:44
@jan-auer jan-auer requested a review from a team as a code owner September 3, 2024 07:44
@jan-auer jan-auer self-assigned this Sep 3, 2024
Comment on lines +158 to +160
// Set the single-attachment limit that applies only for raw minidumps. Multipart bypasses the
// limited body and applies its own limits.
post(handle).route_layer(DefaultBodyLimit::max(config.max_attachment_size()))
Copy link
Member Author

Choose a reason for hiding this comment

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

This might be surprising behavior. DefaultBodyLimit applies by default for all extractors that axum defines, since they use with_limited_body() internally. Among others, this applies to the Bytes extractor, which is used in one of the branches of handle.

In our case, we want two different limits:

  1. For raw minidumps, use the single attachment limit
  2. For multipart, use the multiple attachments limit.

Our multipart extractor does that automatically, as it has access to our config, and that also bypasses with_limited_body. Hence, the limit here applies to just one of the two branches.

I've been looking into ways to layer the limit directly onto the Bytes extractor, but haven't found a good way to do so.

headers={
"content-encoding": "gzip",
"content-length": str(size),
"content-type": "application/octet-stream",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an important fix. Before, we didn't set this content type, which had the following effect:

  1. The minidump endpoint did not detect this as a standalone minidump
  2. It went into multipart parsing, so the parser tried to find a boundary
  3. There's no boundary, nor a multipart payload, so the endpoint fails immediately.

This was the 400 status code we received, which had nothing to do with the zipbomp and content sizes. By adding the content type, we force the standalone minidump branch, and at least test that part.

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

A changelog might be helpful, just in case an external user notices a difference in behavior.

relay-server/src/extractors/remote.rs Outdated Show resolved Hide resolved
relay-server/src/extractors/remote.rs Outdated Show resolved Hide resolved
* master: (27 commits)
  build: Update dialoguer and hostname (#4009)
  build: Update opentelemetry-proto to 0.7.0 (#4000)
  build: Update lru to 0.12.4 (#4008)
  build: Update cookie to 0.18.1 (#4007)
  feat(spans): Extract standalone CLS span metrics and performance score (#3988)
  build: Update cadence to 1.4.0 and statsdproxy to 0.2.0 (#4005)
  build: Update maxminddb to 0.24.0 (#4003)
  build: Update multer to 3.1.0 (#4002)
  build: Update regex and aho-corasick (#4001)
  build: Update sentry-kafka-schemas to 1.0.107 (#3999)
  build: Update dev-dependencies (#3998)
  build: Update itertools to 0.13.0 (#3993)
  build: Update brotli, zstd, flate2 (#3996)
  build: Update rdkafka to 0.36.2 (#3995)
  build: Update tikv-jemallocator to 0.6.0 (#3994)
  build: Update minidump to 0.22.0 (#3992)
  build: Update bindgen to 0.70.1 (#3991)
  build: Update chrono to 0.4.38 (#3990)
  feat(spans): initial MongoDB description scrubbing support (#3912)
  fix(spooler): Reduce number of disk reads (#3983)
  ...
@jan-auer jan-auer enabled auto-merge (squash) September 6, 2024 07:32
@jan-auer jan-auer merged commit 4c7c08d into master Sep 6, 2024
23 checks passed
@jan-auer jan-auer deleted the ref/direct-multer branch September 6, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use multer instead of axum's multipart
2 participants