fix: cap total zip expansion during tar conversion#25877
Conversation
1ffbb54 to
8f18ca0
Compare
| } | ||
|
|
||
| n, err := w.w.Write(p) | ||
| w.remaining -= int64(n) |
There was a problem hiding this comment.
super micro nit: move this after the if err != nil check that returns the error below.
There was a problem hiding this comment.
Thanks! This is intentional: the contract of io.Writer is that Write can return both an error and a valid n, so the wrapper mirrors that behavior by reflecting the best-effort write and then returning the error.
| return ErrInvalidZipContent | ||
| case err != nil: | ||
| return err | ||
| case written != size: |
There was a problem hiding this comment.
I think this case is redundant because io.CopyN docs say
written == n if and only if err == nil
so this is unreachable if err == nil
There was a problem hiding this comment.
Good catch; I was too paranoid here :)
| // entry header then pushes the converted tar output over the | ||
| // file size limit. | ||
| size := int64(coderd.HTTPFileMaxBytes - 1024) | ||
| zipBytes := buildZipWithSizedFile(t, "oversized.txt", size) |
There was a problem hiding this comment.
Seems like we could just generate an archive where the metadata claims the expansion would be huge.
Might be splitting hairs here if the memory usage is not a concern.
There was a problem hiding this comment.
Thanks! I hope it's not a huge concern, because we do want to test the actual streaming enforcement path here (as opposed to the preflight check we'd be testing with a metadata-only test).
jdomeracki-coder
left a comment
There was a problem hiding this comment.
Does the job from security perspective
Enforce aggregate limits when converting uploaded ZIP archives to tar so compressed inputs cannot expand without bound in memory. Also treat malformed ZIP entry metadata and content mismatches as client errors during conversion, returning 400 for invalid archives and 413 when expanded tar output exceeds the upload limit. Ref: https://linear.app/codercom/issue/PLAT-274/zip-upload-decompressed-without-aggregate-size-limit-sec-103
2db3fa1 to
b039620
Compare
Summary
Reject ZIP uploads whose expanded tar output exceeds the file upload limit.
This change adds aggregate size enforcement when converting ZIP uploads to tar,
so small compressed archives can no longer expand without bound in memory. ZIP
uploads that exceed the configured expansion limit now return
413 Request Entity Too Large, and malformed ZIP content now returns400 Bad Requestinstead of surfacing as an internal error.Changes
tar.Writer.Close()errors instead of dropping them413fromPOST /api/v2/fileswhen expanded ZIP content is too large400fromPOST /api/v2/filesfor invalid ZIP archive contentsRef: https://linear.app/codercom/issue/PLAT-274