Repository navigation
fix: bound request sizes and recurrence expansion, and set server timeouts (#39) - #42
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
The endpoint had no limit anywhere: any client could make the server buffer an
unbounded body, and it was built on
http.ListenAndServe, i.e. anhttp.Serverwith no timeout at all. Verified against a local build before thefix:
PUTof a valid iCalendar of 64 MiB -> 201 after 0.13 s, 67109032bytes written to the volume.
MaxBytesReader,LimitReader,MaxHeaderBytes,ReadHeaderTimeout,ReadTimeout,WriteTimeout,IdleTimeoutincmd/and
internal/: zero results.io.ReadAllcalls read straight fromr.Body.The worst case was not the disk: recurrence expansion happens inside the
library's
caldav.Match, which has no bound of its own, so a single object withRRULE:FREQ=SECONDLYqueried over a decade kept a request runningindefinitely. Measured on the built binary with the cap disabled
(
-max-expand 2000000000): the samecalendar-querydid not finish in 25seconds; with the default cap it answers 207 in 0.0016 s.
What this changes
limitRequestBodywraps every request body withhttp.MaxBytesReaderbefore any handler sees it, andhandlePutwraps itagain with the object cap, so an oversized object is rejected while it is
being read and no partial file is written. Oversized bodies answer 413
(mapped from
http.MaxBytesErrorinbodyError, used by PUT, REPORT,PROPFIND, PROPPATCH and MKCALCOL). Defaults mirror Radicale:
-max-request-size100 MiB (max_content_length),-max-item-size10 MiB(
max_resource_size), also settable asDAVCLOUD_MAX_REQUEST_SIZEandDAVCLOUD_MAX_ITEM_SIZE.max-resource-sizein PROPFIND, in theCalDAV or CardDAV namespace as appropriate, so a well-behaved client can
refuse to send what will not be stored (Radicale and sabre/dav both do this).
internal/caldavestimates, before delegatinga time-range query, how many instances an object's recurrence would produce
(
instanceBudget, countingFREQ,INTERVAL,COUNT,UNTILagainst therequested range). Over
-max-expand(default 10000, Radicale'smax_expand_occurrence) the object is matched on its plain dates, which isthe same fallback the code already used for recurrence data it cannot parse,
and a warning is logged. Trade-off, stated on purpose: for ranges wide enough
to need more than 10000 instances (a daily event over more than 27 years, or
a sub-hourly rule over a wide range) an object whose base date falls outside
the range is no longer matched; in exchange the query always finishes.
newServerbuilds thehttp.ServerwithReadHeaderTimeout10 s,ReadTimeout/WriteTimeout/IdleTimeout2 min andMaxHeaderBytes64 KiB instead of relying on Go's defaults, which set none(Radicale has a 30 s socket timeout and caps concurrent connections).
ES README typo in the rollback sentence is fixed on the way.
Tests
internal/caldav:TestInstanceBudget(15 cases, includingCOUNT,UNTIL,lowercase keys, unknown and empty rules),
TestOverInstanceBudgetandTestQueryRanges(the range lives in the nested VEVENT filter).cmd/server:TestPutOverItemSizeIsRejected(413, nothing left on disk, anda smaller object still accepted byte for byte),
TestOversizedRequestIsRejected(413 on a PROPFIND body, 207 within the limit),
TestPropfindAdvertisesMaxResourceSize(both namespaces),TestServerHasTimeouts, andTestPathologicalRecurrenceQueryIsBounded, whichdoes not finish without the cap.
Also checked end to end against a copy of the production volume (temporary
directory, production untouched): with
-max-item-size 1024, a 2000 byte objectanswers 413 and leaves no file, a 100 byte object answers 201, the collection
lists
max-resource-size>1024, and the real items keep being served.Closes #39