[MM-69606] Limit decompressed size on archive import - #220
Conversation
parseVersionFile and archive image imports read zip entries without bounds, allowing small compressed payloads to allocate large amounts of memory. Apply the existing importMaxFileSize cap consistently across all entry types and cap the upload request body via MaxBytesReader.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configured size limits to archive import requests and ZIP entries, maps oversized or malformed uploads to structured errors, and cleans up partial oversized saves. It also adds multipart client support and expands application, API, and integration coverage. ChangesArchive import size enforcement
Multipart upload integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MultipartClient
participant ArchiveAPI
participant ImportArchive
participant FileBackend
Client->>MultipartClient: Build multipart archive POST
MultipartClient->>ArchiveAPI: Send bounded upload
ArchiveAPI->>ImportArchive: Parse archive request
ImportArchive->>FileBackend: Save bounded ZIP entries
FileBackend-->>ImportArchive: Return save result
ImportArchive-->>ArchiveAPI: Return success or size-limit error
ArchiveAPI-->>Client: Return JSON HTTP response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (3)
server/app/import.go (1)
95-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "+1 sentinel / N<=0" bounded-read pattern across three sites.
The same "wrap in
io.LimitedReader{N: limit+1}, then checkN<=0" logic is repeated inline (fileReader), indiscardLimited, and inparseVersionFile. Extracting a small shared helper (e.g. one that returns a*io.LimitedReaderand a companionexceeded()check) would reduce the chance of the off-by-one sentinel being applied inconsistently in future edits.Also applies to: 100-107, 479-494, 496-505
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/import.go` around lines 95 - 99, The bounded-read logic is duplicated across fileReader, discardLimited, and parseVersionFile, including the repeated limit+1 sentinel and N<=0 check. Refactor this pattern into a small shared helper used by fileReader, discardLimited, and parseVersionFile so they all construct the same io.LimitedReader and use one companion exceeded check consistently. Keep the existing behavior intact while centralizing the off-by-one handling to avoid future drift.server/api/archive.go (2)
160-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the new size-limit-to-413 mapping.
The ZIP-entry-level limits gained tests in
import_test.go, but this handler-levelMaxBytesReader/413-mapping change has no accompanying test. Consider adding a handler test that simulates an oversized multipart body and asserts amodel.ErrRequestEntityTooLargeresponse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/archive.go` around lines 160 - 172, The archive upload handler in a.app’s request flow maps http.MaxBytesReader failures to model.ErrRequestEntityTooLarge, but there is no test covering this path. Add a handler test around the UploadFormFileKey multipart parsing in archive.go that sends an oversized body with MaxFileSize enabled and verifies errorResponse returns model.ErrRequestEntityTooLarge rather than a generic bad request.
166-170: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
http.MaxBytesErrorinstead of matching the error string. The module targets Go 1.24.6, soerrors.As(err, &maxBytesErr)is available here and is safer thanstrings.HasSuffix(err.Error(), "http: request body too large"), which can break if the message changes or gets wrapped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/archive.go` around lines 166 - 170, The request-body size check in archive handling should not rely on matching the error string; update the error handling in the archive endpoint flow to detect `http.MaxBytesError` using `errors.As` before falling back to `model.NewErrBadRequest`. Keep the existing `a.errorResponse` behavior in the same branch, but use the typed error path in the relevant handler function so oversized uploads map to `model.ErrRequestEntityTooLarge` safely even if the message is wrapped or changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/app/import.go`:
- Around line 100-107: The oversized import path in aSaveFile handling currently
returns errSizeLimitExceeded without cleaning up the partially written object.
Update the import flow in import.go, using the SaveFile result (newFileName) and
the fileBackend RemoveFile(path string) method, to delete the saved file
whenever fileReader.N indicates the size limit was exceeded before returning the
error.
---
Nitpick comments:
In `@server/api/archive.go`:
- Around line 160-172: The archive upload handler in a.app’s request flow maps
http.MaxBytesReader failures to model.ErrRequestEntityTooLarge, but there is no
test covering this path. Add a handler test around the UploadFormFileKey
multipart parsing in archive.go that sends an oversized body with MaxFileSize
enabled and verifies errorResponse returns model.ErrRequestEntityTooLarge rather
than a generic bad request.
- Around line 166-170: The request-body size check in archive handling should
not rely on matching the error string; update the error handling in the archive
endpoint flow to detect `http.MaxBytesError` using `errors.As` before falling
back to `model.NewErrBadRequest`. Keep the existing `a.errorResponse` behavior
in the same branch, but use the typed error path in the relevant handler
function so oversized uploads map to `model.ErrRequestEntityTooLarge` safely
even if the message is wrapped or changes.
In `@server/app/import.go`:
- Around line 95-99: The bounded-read logic is duplicated across fileReader,
discardLimited, and parseVersionFile, including the repeated limit+1 sentinel
and N<=0 check. Refactor this pattern into a small shared helper used by
fileReader, discardLimited, and parseVersionFile so they all construct the same
io.LimitedReader and use one companion exceeded check consistently. Keep the
existing behavior intact while centralizing the off-by-one handling to avoid
future drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49c26b08-514c-40df-80c6-77c37aacfe6f
📒 Files selected for processing (3)
server/api/archive.goserver/app/import.goserver/app/import_test.go
Clean up partial files on size-limit errors, use typed MaxBytesError handling, and add coverage for image/orphan limits and API 413/400 responses. Update permissions and integration tests to send valid multipart uploads after the handler started rejecting non-multipart bodies.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/integrationtests/import_archive_test.go (2)
35-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the actual error response body, not just headers.
Both subtests only check status code and
Content-Type; neither decodes the body to confirm it's a well-formed standard API error object (e.g. has anid/message), which is the core behavior this PR changes (raw error write →errorResponse).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/integrationtests/import_archive_test.go` around lines 35 - 84, The import archive error tests only verify status and Content-Type, but they should also confirm the response body is the expected JSON API error object. In TestImportArchiveAPIErrors, update both subtests to parse the body returned by ImportArchive/DoAPIPost and assert standard error fields such as id and message, using the existing test helpers and responses from th.Client and setupClients. This will validate the errorResponse behavior instead of only checking headers.
54-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
minimalImportArchiveZipinstead of duplicating zip construction.♻️ Suggested fix
- var buf bytes.Buffer - zw := zip.NewWriter(&buf) - w, err := zw.Create("version.json") - require.NoError(t, err) - _, err = w.Write([]byte(`{"version":2}`)) - require.NoError(t, err) - require.NoError(t, zw.Close()) - - resp := th.Client.ImportArchive(teamID, bytes.NewReader(buf.Bytes())) + resp := th.Client.ImportArchive(teamID, bytes.NewReader(minimalImportArchiveZip(t)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/integrationtests/import_archive_test.go` around lines 54 - 60, The test is manually rebuilding the same minimal import archive zip contents instead of reusing the shared helper. Replace the inline zip construction in this test with a call to minimalImportArchiveZip so the archive setup stays centralized and consistent. Keep the test focused on its assertions and use the existing helper as the single source for creating the version.json-based archive.server/client/client.go (1)
100-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate multipart-upload logic.
This duplicates the multipart-building code already present in
ImportArchiveandTeamUploadFilein this same file, differing only in field/file names.♻️ Suggested consolidation
+func (c *Client) doMultipartPost(url, fieldName, fileName string, data io.Reader) (*http.Response, error) { + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, err := writer.CreateFormFile(fieldName, fileName) + if err != nil { + return nil, err + } + if _, err = io.Copy(part, data); err != nil { + return nil, err + } + if err = writer.Close(); err != nil { + return nil, err + } + + opt := func(r *http.Request) { + r.Header.Set("Content-Type", writer.FormDataContentType()) + } + + return c.doAPIRequestReader(http.MethodPost, c.APIURL+url, body, "", opt) +} + func (c *Client) DoAPIPostMultipart(url, fieldName, fileName string, data []byte) (*http.Response, error) { - body := &bytes.Buffer{} - writer := multipart.NewWriter(body) - part, err := writer.CreateFormFile(fieldName, fileName) - if err != nil { - return nil, err - } - if _, err = part.Write(data); err != nil { - return nil, err - } - if err = writer.Close(); err != nil { - return nil, err - } - - opt := func(r *http.Request) { - r.Header.Set("Content-Type", writer.FormDataContentType()) - } - - return c.doAPIRequestReader(http.MethodPost, c.APIURL+url, body, "", opt) + return c.doMultipartPost(url, fieldName, fileName, bytes.NewReader(data)) }
ImportArchiveandTeamUploadFilecould similarly delegate todoMultipartPost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/client/client.go` around lines 100 - 119, The multipart upload logic in DoAPIPostMultipart is duplicated with the existing multipart-building flow used by ImportArchive and TeamUploadFile. Refactor by centralizing the shared request construction into doMultipartPost and make DoAPIPostMultipart delegate to it, passing through the field name, file name, and payload. Then update ImportArchive and TeamUploadFile to use the same helper so all multipart POST paths share one implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/client/client.go`:
- Around line 100-119: The multipart upload logic in DoAPIPostMultipart is
duplicated with the existing multipart-building flow used by ImportArchive and
TeamUploadFile. Refactor by centralizing the shared request construction into
doMultipartPost and make DoAPIPostMultipart delegate to it, passing through the
field name, file name, and payload. Then update ImportArchive and TeamUploadFile
to use the same helper so all multipart POST paths share one implementation.
In `@server/integrationtests/import_archive_test.go`:
- Around line 35-84: The import archive error tests only verify status and
Content-Type, but they should also confirm the response body is the expected
JSON API error object. In TestImportArchiveAPIErrors, update both subtests to
parse the body returned by ImportArchive/DoAPIPost and assert standard error
fields such as id and message, using the existing test helpers and responses
from th.Client and setupClients. This will validate the errorResponse behavior
instead of only checking headers.
- Around line 54-60: The test is manually rebuilding the same minimal import
archive zip contents instead of reusing the shared helper. Replace the inline
zip construction in this test with a call to minimalImportArchiveZip so the
archive setup stays centralized and consistent. Keep the test focused on its
assertions and use the existing helper as the single source for creating the
version.json-based archive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8be52b65-bf33-4660-82ae-63454d773cb1
📒 Files selected for processing (7)
server/api/archive.goserver/api/archive_test.goserver/app/import.goserver/app/import_test.goserver/client/client.goserver/integrationtests/import_archive_test.goserver/integrationtests/permissions_test.go
✅ Files skipped from review due to trivial changes (1)
- server/api/archive_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/app/import.go
- server/app/import_test.go
jgheithcock
added
2: Dev Review
|
Hey @jgheithcock I have a branch for MM-69607 that overlaps a lot with this #218. Same: Different: Your 70 MB per-entry cap already covers the disk-exhaustion scenario from MM-69607 so my archive specific changes are redundant with yours. How do you want to reconcile? I close mine and you land as is or merge my SaveFile cap into this PR. |
|
@edgarbellot how do you think we can reconcile both changes? |
|
@nang2049 - IIUC, your PR handles my situation, but with configs that I prefer, so I vote to close this one. @edgarbellot , any objection? |
|
@jgheithcock can we reopen this? we need to see how to reconcile both PR |
|
@nang2049 - what is the plan for reconciling these? |
| continue | ||
| } | ||
| newFileName, err := a.SaveFile(zr, opt.TeamID, board.ID, filename, board.IsTemplate) | ||
| fileReader := newLimitedReader(zr, importMaxFileSize) |
There was a problem hiding this comment.
@jgheithcock @nang2049 This PR correctly fixes both issues. I think we can apply the remaining change described below and close PR #218.
The gap worth closing before merging: importMaxFileSize is a hardcoded 70 MB constant and doesn't respect the server's configured MaxFileSize. If an admin sets MaxFileSize below 70 MB, archive imports can still write up to 70 MB per entry.
The fix is straightforward - derive the effective cap from both values wherever importMaxFileSize is used (L95, L100, L497):
maxEntry := importMaxFileSize
if cfgMax := a.app.GetConfig().MaxFileSize; cfgMax > 0 && cfgMax < maxEntry {
maxEntry = cfgMax
}
fileReader := newLimitedReader(zr, maxEntry)
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
Thank you! We can close #218 since this PR addresses both.
|
/cherry-pick release-9.3 |
|
Cherry pick is scheduled. |
* [MM-69606] Limit decompressed size when importing board archives. parseVersionFile and archive image imports read zip entries without bounds, allowing small compressed payloads to allocate large amounts of memory. Apply the existing importMaxFileSize cap consistently across all entry types and cap the upload request body via MaxBytesReader. * [MM-69606] Address review feedback and fix archive import tests. Clean up partial files on size-limit errors, use typed MaxBytesError handling, and add coverage for image/orphan limits and API 413/400 responses. Update permissions and integration tests to send valid multipart uploads after the handler started rejecting non-multipart bodies. * Honor configured MaxFileSize when stricter than importMaxFileSize * fix linter * tets --------- (cherry picked from commit a807c9e) Co-authored-by: JG Heithcock <jgheithcock@gmail.com> Co-authored-by: Nevyana Angelova <nevyangelova@192.168.1.10>
|
/cherry-pick release-9.2 |
|
Cherry pick is scheduled. |
5 participants
Summary
importMaxFileSize(70 MB) decompressed-size cap toversion.jsonand archive image entries during importhttp.MaxBytesReader, matching the file upload handlerTicket Link
https://mattermost.atlassian.net/browse/MM-69606
Release Note
Fixed an issue where importing a board archive could consume excessive server memory.
Test plan
go test ./app/... ./api/...passesTestParseVersionFile— validversion.jsonaccepted; oversized payload rejectedTestImportArchiveVersionFileSizeLimit— valid zip archive accepted; oversizedversion.jsonentry rejected.boardarchivefile via UI or API and confirm successChange Impact: 🟠 Medium
Regression Risk: Medium—this changes user-facing archive import behavior by enforcing stricter request/body and per-entry decompressed size limits, adjusts ZIP stream handling for orphan entries (now actively discarded), and standardizes API error mapping (400/413). While blast radius is largely limited to archive import, it’s a core business flow with behavior-contract changes that can affect edge-case imports and cleanup.
QA Recommendation: Limited manual QA recommended: validate a successful small-to-medium archive import, verify 413 for oversized uploads and correct JSON/content-type responses, verify 400 for non-multipart bodies, and confirm cleanup/absence of partially imported oversized assets for both board-mapped entries and orphan entries.
Generated by CodeRabbitAI