feat(dropfile): add configurable limits for drop file - #216
Conversation
|
@hinyb can you resolve conflicts? |
Sorry for the late reply, resolved. |
Eldrinn-Elantey
left a comment
There was a problem hiding this comment.
Reviewed the chunked drop file transfer. The chunking itself correctly solves the 32767 byte packet limit, sizes are validated on both sides and sessions are limited to one per player with a TTL, so unbounded session count is not an issue.
There is one blocker: decompression is unbounded, so raising the limit to 4 MB opens a zip bomb path to server OOM. Details inline, together with a few smaller correctness and compatibility notes.
One process note: the DropFile packet format changed incompatibly without a version or flag, so an old client against a new server silently breaks. #215 made clipboard batching opt-in for exactly that reason. If a synchronized client/server update is intended here, it is worth stating that in the PR description.
Eldrinn-Elantey
left a comment
There was a problem hiding this comment.
Second pass over the changes since 403bc8f. All the earlier points are addressed, thanks. The bounded inflate, the on demand buffer, readFully, the session cleanup on logout and the server side name check all look right in the current code, and the replies about the API signature and the hardcoded name limit are fair.
A few new things in the code that was added, one of them a real regression from the rename.
|
One open question about maxDropFileCount. It now carries two meanings: the number of files allowed per drop on the client, and both the bucket size and the refill rate of the server side limiter. With the default of 16, dropping the full 16 files at once empties the bucket, so a second legitimate drop within the same second is rejected part way through and the player gets a chat message per rejected file. Was that intended, or would a separate rate setting (or a bucket with some headroom over maxDropFileCount) fit better? Not blocking either way, I just want to make sure the interaction is deliberate. |
Yeah, it's intentional.I think reusing maxDropFileCount for the rate limit is fine here, cause nobody manually drags and drops files multiple times within a second, so I didn't add a separate config. The 1s throttling is reasonable to me. |
Summary
This PR makes the previously hardcoded 64kb upload size limit configurable, and fixes the
packet too largeerror that occurred with larger files, enabling support for much larger files.Checklist