fix: Close integer validation bypass and query parser differential#109
Merged
Conversation
Two validation bypasses surfaced in a security review of the request pipeline: - checkInteger narrowed numbers with longValue() before bound-checking, so a fractional Double (4.9) satisfied `type: integer` and a value above 2^64 wrapped past a declared maximum. It now rejects non-integral numbers and compares the full magnitude, keeping an allocation-free long path for the common case and falling back to BigDecimal/BigInteger otherwise. - Query parameters were validated from the already-decoded URI.getQuery() but delivered to handlers from getRawQuery(), so an encoded separator (%26) could smuggle a value past a schema constraint. Both paths now share QueryParams.parse over the raw query, so the value validated is the value the handler receives. Adds exploit tests asserting both are rejected.
The SonarCloud new-code coverage gate failed on PR #109: extracting the bound checks into checkIntegerBounds turned the exclusive-bound and beyond-long paths into "new" lines that no test exercised. - Simplify the beyond-long branch to delegate to the long path when the value fits, and otherwise reject against whichever bound lies on its side (or an int32/int64 format), collapsing the per-bound arms. - Add tests for exclusive bounds, values above 2^53 that still fit in a long, every beyond-long rejection path, and QueryParams edge cases (empty pairs, keys without '=', encoded separators).
SonarCloud flagged S1244 (floating-point equality) on the `d == Math.rint(d)` integrality test in checkInteger's fast double path, dropping New Code reliability to C. Remove the double fast path entirely: doubles reaching an integer schema are either fractional (rejected) or rare integral doubles, both handled correctly by converting through BigDecimal.toBigIntegerExact(). Integral wrappers keep the allocation-free long path, so the common case is unaffected.
- Extract MINIMUM_KEYWORD/MAXIMUM_KEYWORD constants (S1192: each literal now appears 3 times across the integer and number checks). - Use an unnamed catch variable for the discarded parse exception. - Replace try/catch + fail() with assertDoesNotThrow() in the query test.
|
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.
Fixes two request-validation bypasses found in a security review of the Java library.
Finding 1 —
type: integervalidation bypass (HIGH)DefaultValidator.checkIntegernarrowed numbers withlongValue()before bound-checking, so the check ran on a truncated value:4.9→longValue()4→ passed{minimum:1, maximum:5}. Non-integral values slipped throughtype: integer.2^64 + 50→ low 64 bits50→ passed{maximum:1000}. The upper bound was wrapped.Fix: reject non-integral numbers and compare the full magnitude. An allocation-free
longfast path stays for the common case (integral wrappers, exact integral doubles ≤ 2^53); fractional / non-finite / beyond-2^53 values fall back toBigDecimal/BigInteger.Finding 2 — query-parameter parser differential (MEDIUM)
Parameter validation parsed the already-decoded
URI.getQuery()(split on&/=after decode), while handlers parsedgetRawQuery()(split first, then per-component decode). An encoded separator let the two disagree:?q1=abc%26x=y→ validation sawq1=abc(passing a^[a-z]+$constraint) while the handler receivedabc&x=y. Anypattern/enum/maxLength/formaton the delivered value could be bypassed.Fix: a single shared
internal.QueryParams.parseover the raw query, called by both the validation filter andRequest— so the value validated is exactly the value the handler receives. Net removes both private parser copies.Tests (TDD — written failing first)
StringIntegerNumberTest:integerRejectsFractionalDouble,integerRejectsValueOverflowingLongPastMaximum,integerAcceptsIntegralDouble(regression guard).QueryParamSmugglingTest:?q1=abc%26x=yagainst a^[a-z]+$param now returns 400 (was 200).Full suite: 527/527 pass.
Not included
$refsibling keywords are dropped bySchemaParser(a separate correctness gap, low real-world exposure since tooling emitsallOf). Left for a follow-up.