nginx: allow large file uploads for /api/v4/files endpoint#184
Conversation
📝 WalkthroughWalkthroughIn ChangesNginx client body size bump
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nginx/conf.d/default.conf (1)
113-126: Consider addingproxy_request_buffering offandproxy_send_timeoutfor large uploads.The configuration correctly removes the nginx body size limit and increases timeouts. However, for large file uploads, consider these improvements:
proxy_request_buffering off;– Streams the upload directly to the backend without buffering in nginx memory, reducing memory pressure for large files.
proxy_send_timeout– Currently missing; defaults to 60s. For slow uploads of large files, this timeout could be reached while nginx sends the request body to the backend. Consider setting it to matchproxy_read_timeout(600s) or higher.♻️ Suggested improvement
location = /api/v4/files { client_max_body_size 0; client_body_timeout 1200s; + proxy_request_buffering off; proxy_set_header Connection ""; proxy_set_header Host $http_host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Frame-Options SAMEORIGIN; proxy_set_header Early-Data $ssl_early_data; + proxy_send_timeout 600s; proxy_read_timeout 600s; proxy_http_version 1.1; proxy_pass http://backend; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nginx/conf.d/default.conf` around lines 113 - 126, In the location block matching "= /api/v4/files" add proxy_request_buffering off to stream request bodies to the backend (avoid nginx buffering large uploads) and set proxy_send_timeout (e.g., 600s to match proxy_read_timeout) so nginx won't time out while sending the request body to the upstream; update the location = /api/v4/files block to include these two directives alongside the existing proxy_read_timeout and proxy_http_version settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nginx/conf.d/default.conf`:
- Line 113: The nginx config currently has an exact match location "location =
/api/v4/files" which raises client_max_body_size for legacy single-file uploads
but does not cover resumable/chunked uploads under "/api/v4/uploads"; if your
Mattermost uses resumable uploads add a corresponding location block for the
uploads endpoints (e.g. a prefix or regex location for "/api/v4/uploads" and
"/api/v4/uploads/*") and set the same client_max_body_size value as used for
"location = /api/v4/files" so chunk initialization (POST /api/v4/uploads) and
chunk POSTs (POST /api/v4/uploads/{upload_id}) are not rejected. Ensure the new
block(s) use the same directives (client_max_body_size and any other relevant
proxy/fastcgi settings) and test upload flows after deploying the change.
---
Nitpick comments:
In `@nginx/conf.d/default.conf`:
- Around line 113-126: In the location block matching "= /api/v4/files" add
proxy_request_buffering off to stream request bodies to the backend (avoid nginx
buffering large uploads) and set proxy_send_timeout (e.g., 600s to match
proxy_read_timeout) so nginx won't time out while sending the request body to
the upstream; update the location = /api/v4/files block to include these two
directives alongside the existing proxy_read_timeout and proxy_http_version
settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9d0e957-bc4c-4666-8d44-4818d6f8e68a
📒 Files selected for processing (1)
nginx/conf.d/default.conf
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
|
@hanzei Maybe you can take a look at this PR? |
|
@NARSimoes hi! |
aminvakil
left a comment
aminvakil
left a comment
There was a problem hiding this comment.
This requires maintainers to follow api version and change this if it would be changed to /api/v5/ or some other endpoint.
I think 50M suffices many use cases and you can change it locally for yourself (as I did).
This better be documented on how to increase file upload limit size IMHO.
Yes. You're right. But now users are forced to find a way to increase the file upload limit.
In 99% of cases, yes. But in the remaining 1%, files are larger than 50 MB. Plus, the file is stored in Git, which complicates updates if there are conflicts with the pull. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
hanzei
added
2: Dev Review
|
@NARSimoes Are you the right person to review this PR? |
lieut-data
left a comment
lieut-data
left a comment
There was a problem hiding this comment.
@edgarbellot, curious if you have any input for why we would need to enforce a body payload size at the proxy server? It would be ideal if we could just remove the setting below.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nginx/conf.d/default.conf (1)
114-114: ⚡ Quick winAdd an inline comment to flag the sync dependency with Mattermost's
MaxFileSize.The raise to 100M is correct and matches Mattermost's default, but the PR discussion explicitly agreed to document that this value must stay in sync with the admin-configurable Maximum File Size setting in Mattermost. Without any comment here, a future admin who changes Mattermost's
MaxFileSizewill have no prompt to also update this directive.♻️ Proposed inline annotation
- client_max_body_size 100M; + client_max_body_size 100M; # Keep in sync with Mattermost's MaxFileSize (System Console → File Storage → Maximum File Size)Additionally, the PR discussion agreed to update external documentation (README / admin guide) with the same guidance. That docs change does not appear to be included in this PR — please ensure it is either added here or tracked in a follow-up issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nginx/conf.d/default.conf` at line 114, Add an inline comment above or beside the nginx directive client_max_body_size 100M to explicitly state that this value must be kept in sync with Mattermost's admin-configurable "Maximum File Size" (MaxFileSize), e.g., noting the source and that changes to Mattermost's MaxFileSize require updating this directive; also ensure the README/admin guide is updated with the same guidance or create a tracked follow-up issue referencing the sync requirement so maintainers are reminded to update both places when changing MaxFileSize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nginx/conf.d/default.conf`:
- Line 114: Add an inline comment above or beside the nginx directive
client_max_body_size 100M to explicitly state that this value must be kept in
sync with Mattermost's admin-configurable "Maximum File Size" (MaxFileSize),
e.g., noting the source and that changes to Mattermost's MaxFileSize require
updating this directive; also ensure the README/admin guide is updated with the
same guidance or create a tracked follow-up issue referencing the sync
requirement so maintainers are reminded to update both places when changing
MaxFileSize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef1ee9e3-007f-4ca0-8737-1dd3bfbe1289
📒 Files selected for processing (1)
nginx/conf.d/default.conf
|
Thanks again, @madest92! |
6 participants
This prevents 413 (Request Entity Too Large) errors when uploading large files.
Key changes:
Note:
File size limits are still enforced at the application level via Mattermost settings, where administrators can configure the maximum allowed upload size.
This change only removes nginx as a limiting factor and delegates control to the application.