Skip to content

Add input validation for HTTP round parameter#17

Open
alienx5499 wants to merge 1 commit intodrand:masterfrom
alienx5499:fix/http-round-parameter-validation
Open

Add input validation for HTTP round parameter#17
alienx5499 wants to merge 1 commit intodrand:masterfrom
alienx5499:fix/http-round-parameter-validation

Conversation

@alienx5499
Copy link
Contributor

Problem

The HTTP handlers for /public/{round}, /{chainHash}/public/{round}, and the V2 API routes (/chains/{chainhash}/rounds/{round}, /beacons/{beaconID}/rounds/{round}) parse the round number from the URL but don't perform comprehensive input validation. While strconv.ParseUint handles some validation (overflow, negative numbers), the API doesn't provide early validation feedback with clear error messages or reasonable maximum limit checks.

Location: routes_handler.go:22-30

Previous Code:

roundStr := chi.URLParam(r, "round")
round, err := strconv.ParseUint(roundStr, 10, 64)
if err != nil {
    w.Header().Set("Cache-Control", "public, max-age=604800, immutable")
    slog.Error("unable to parse round", "error", err)
    http.Error(w, "Failed to parse round. Err: "+err.Error(), http.StatusBadRequest)
    return
}

Solution

  1. Added input validation: Created readRound helper function with:

    • Empty parameter check
    • Improved error messages with context using fmt.Errorf with error wrapping (%w)
    • Reasonable maximum validation (2^60 = 1,152,921,504,606,846,976) to prevent issues with time calculations
  2. Improved error handling: Updated error logging to include client information and request path for better debugging

  3. Added comprehensive test coverage: Added TestReadRound and TestReadRoundDirect test functions with 9 test cases covering:

    • Invalid format (non-numeric strings)
    • Negative numbers
    • Decimal numbers
    • Extremely large numbers (parse errors)
    • Large but parseable numbers exceeding reasonable maximum
    • Valid round numbers
    • Valid large round numbers at maximum
    • Zero round
  4. Fixed test infrastructure: Updated main.go to skip flag parsing during tests to prevent test failures

Changes

  • routes_handler.go:

    • Added readRound helper function with validation and better error messages
    • Updated GetBeacon to use readRound and include client/path in error logs
  • routes_handler_test.go (new file):

    • Added TestReadRound test function with comprehensive test cases
    • Added TestReadRoundDirect for direct function testing
  • main.go:

    • Updated init() to skip flag parsing during tests (prevents test binary flag conflicts)

Testing

  • All new tests pass: go test -v -run TestReadRound
  • Code compiles successfully: go build ./...
  • No linter errors
  • Existing functionality preserved

Impact

This enhancement ensures that:

  • Users receive clear, actionable error messages for invalid round parameters
  • Extremely large round numbers are caught early with descriptive errors
  • The API provides better input validation at the HTTP layer
  • Security is improved by validating inputs before processing
  • Error responses are properly formatted with meaningful messages
  • Better debugging capabilities with enhanced error logging

Related

This addresses the same concern raised in drand/drand#1447, but implemented in the http-relay repository where HTTP handling is now maintained (as recommended by @AnomalRoil). The original PR in drand/drand has been closed in favor of this implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant