config: reject overflowing seconds in CustomDuration.UnmarshalJSON - #1703
Open
RajeshRajendiran wants to merge 2 commits into
Open
config: reject overflowing seconds in CustomDuration.UnmarshalJSON#1703RajeshRajendiran wants to merge 2 commits into
RajeshRajendiran wants to merge 2 commits into
Conversation
CustomDuration's JSON wire format is documented as seconds (unlike its YAML format, which uses time.Duration's own "10s"-style parser). When a caller instead supplies a value on the order of 1e10 (e.g. mistaking it for nanoseconds, which is the usual convention for time.Duration), multiplying by time.Second overflows int64 and silently wraps into a large negative time.Duration. That negative duration is passed straight into net.Dialer.Timeout for originRequest.connectTimeout, so the dial context is already expired before net.Dialer is ever invoked: dials fail instantly with "i/o timeout" and zero socket()/connect() syscalls, with no indication the config value itself was invalid. Bounds-check the parsed seconds value against the int64 range before multiplying, returning a descriptive unmarshal error instead of silently producing a corrupted duration. Fixes cloudflare#1702
This repo's golangci-lint config uses whole-files: true, so editing
configuration.go/configuration_test.go surfaces lint issues across
the entire file, not just the diff. Address them:
- errcheck: check file.Close() via defer func() { _ = file.Close() }()
- staticcheck: de-capitalize ErrNoConfigFile message (ST1005), drop
redundant embedded-field selectors in CustomDuration (QF1008)
- testifylint: use require.NoError for error assertions that
subsequent lines depend on, assert.InDelta for float comparison,
assert.True instead of assert.Equal(true, ...)
- gosec: nolint with justification for G304/G703 (paths are local
config search paths, not attacker-controlled) and G301 (pre-existing
directory permissions; tightening them is a separate behavior
change out of scope for this PR)
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
1 participant
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.
Summary
CustomDuration.UnmarshalJSON(config/configuration.go) parses the JSON wire value as seconds, then multiplies bytime.Secondto build atime.Duration. This is intentional (see the comment aboveCustomDuration) so remote/API-pushed configs aren't limited by JS's 32-bit-safe-integer nanosecond convention, and it's mirrored by existing tests (TestMarshalUnmarshalOriginRequest).1e10(e.g. assuming nanoseconds, the usualtime.Durationconvention) overflowsint64when multiplied by1e9, silently wrapping into a large negativetime.Duration.net.Dialer.TimeoutfororiginRequest.connectTimeout(and any otherCustomDurationfield arriving via remote JSON config). A negative/expired deadline makesnet.Dialer.DialContextfail instantly withdial tcp: ... i/o timeout— before anysocket()/connect()syscall — with no indication that the config value was the problem.int64range before multiplying, returning a clear unmarshal error instead of silently producing a corrupted, confusing timeout..golangci.yamlsetswhole-files: true, so editing a file surfaces lint findings across the entire file, not just the diff.Test plan
TestCustomDurationUnmarshalJSONOverflowinconfig/configuration_test.gocovering: the reported repro value, a normal value, the boundary value, and one past the boundary.go build ./...go test -race ./config/... ./orchestration/...gofmt -l/go vet ./config/...make lint(golangci-lint run ./config/...-> 0 issues)Fixes #1702