fix(api): reject NaN/Inf in min-savings query params

[cristim]

Problem

parseMinSavingsParam (internal/api/validation.go) parses the min_savings_usd / min_savings_pct query parameters with strconv.ParseFloat, which accepts NaN, Inf, +Inf and Infinity (case-insensitively). The only guard was v < 0, which is false for NaN. Downstream:

• a NaN min_savings_usd bound into monthly_savings >= $n excludes every row, returning an empty list with HTTP 200 instead of a 400
• a NaN min_savings_pct makes pct < filter.MinSavingsPct always false, a silently inert floor
• +Inf excludes everything

Review finding COR-09 (docs/reviews/codebase-review-2026-06-10.md). Closes #1183.

Fix

Reject non-finite values at the input boundary: err != nil || math.IsNaN(v) || math.IsInf(v, 0) now returns a 400 client error ("must be a finite non-negative number"). Negative finite values keep their existing 400. Docstring updated to state the non-finite rejection.

Test evidence

Extended TestParseMinSavingsParam with table cases for NaN, nan, +Inf, Inf, Infinity, -Inf, and the pct-path (min_savings_pct) variants.

• Pre-fix (fix stashed, new tests only): all non-finite cases FAIL with "An error is expected but got nil"
• Post-fix: go test ./internal/api/ -run TestParseMinSavingsParam passes (20 subtests)
• Full package: go test ./internal/api/ passes (1635 tests); go build ./... clean

Summary by CodeRabbit

• Bug Fixes

  • Improved validation for minimum savings parameters to properly reject non-finite numeric values (NaN, Infinity) and provide clearer error messages indicating only finite non-negative numbers are accepted.

• Tests

  • Added regression test coverage for invalid non-finite numeric inputs to ensure they are properly rejected.

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 11 minutes and 32 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8c94ba0d-546b-4d86-8cf3-f7ab84dffca3

📥 Commits

Reviewing files that changed from the base of the PR and between cd6470a and 091d868.

📒 Files selected for processing (2)

• internal/api/validation.go
• internal/api/validation_test.go

📝 Walkthrough

Walkthrough

The PR tightens validation in parseMinSavingsParam to explicitly reject non-finite float values (NaN and ±Inf). The math package import supports new finiteness checks. Documentation clarifies the finite non-negative requirement. Validation logic and corresponding test cases ensure the parameter accepts only valid numeric inputs.

Changes

Non-finite input validation for min_savings parameter

Layer / File(s)	Summary
Validation tightening implementation internal/api/validation.go	Added math import to support finiteness checks. Updated parseMinSavingsParam documentation to specify values must be finite non-negative numbers. Enhanced validation to return HTTP 400 when the parsed value is NaN or ±Inf in addition to existing parse and negative checks.
Test coverage for non-finite inputs internal/api/validation_test.go	Extended TestParseMinSavingsParam test table with explicit test cases for NaN, nan, +Inf, Inf, Infinity, and -Inf across both min_savings_usd and min_savings_pct parameters, each asserting ClientError with status code 400.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Poem

A rabbit hops through floats so fine,
Rejecting NaN on the line,
Infinity banned, a fence so tight,
Only finite values feel so right! 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main change: rejecting NaN/Inf in min-savings query parameters.
Linked Issues check	✅ Passed	The PR fully addresses issue #1183 by implementing the recommended fix to reject non-finite values in parseMinSavingsParam with appropriate HTTP 400 error responses.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the validation.go and validation_test.go files to fix NaN/Inf rejection as specified in issue #1183; no unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cor-09-fix

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 24 seconds before sending another message.

[cristim]

Adversarial review (PR #1233)

Reviewed against the brief's risk surfaces (input vs output layer, sibling float params, SQL-injection adjacent, FE pass-through, status-code class, fail-loud semantics, test coverage). The fix is correct, complete-in-scope, and the only follow-up is defense-in-depth on the FE side, filed separately. Nothing blocking.

Verified

• Fixed at the INPUT layer (correct). parseMinSavingsParam rejects NaN, +Inf, -Inf, Infinity with a 400 before the value is bound into SQL or the in-process pct floor. The brief's output-layer concern (Go's encoding/json refuses NaN/Inf and would 500 on serialization) is moot here because the value is killed upstream of any persistence or computation. ✓
• NewClientError(400, ...) actually returns HTTP 400 (handler_router.go:71-73 clientError{code: 400}; response writer in handler.go honors ce.code via IsClientError). feedback_http_status_classification.md: NaN/Inf is a client fault → 400, not a 5xx. ✓
• SQL injection adjacent: MinSavingsUSD flows as a parameterized arg (buildRecommendationFilter at internal/config/store_postgres_recommendations.go:275-277: add("monthly_savings >= $%d", filter.MinSavingsUSD) — len(args)+1 is the placeholder index, the value is appended to args and bound by the pq driver). No fmt.Sprintf of the user value into raw SQL. ✓
• Sibling float-typed query params are also safe. Grepped every strconv.ParseFloat under internal/:
  • internal/api/validation.go:618 — this PR.
  • internal/api/handler_ri_exchange.go:431 (parseThresholdParam) already guards err != nil || math.IsNaN(f) || math.IsInf(f, 0) || f < 0 || f > 100, same shape.
  • internal/server/app.go:950 (getEnvFloat) is env-var, not user input — os.Getenv, only operator-controlled, lower risk.
  • No other float-typed query params exist (no max-cost, percentile, etc. — verified with grep -n 'params\[' internal/api/handler_*.go filtered for numeric names). Scope of the fix is complete; nothing else needs the same hardening today.
• feedback_no_silent_fallbacks.md: the pre-fix behavior was the silent failure mode the policy targets — monthly_savings >= NaN is FALSE for every finite row in Postgres, so the API returned an empty result with HTTP 200 ("looks fine" to the client). For the pct path, pct < NaN is also FALSE, making the floor a silent no-op. The fix surfaces this as an explicit 400, exactly the policy's "fail loud" prescription on a money-adjacent filter. ✓
• feedback_empty_string_vs_error.md: the semantic distinction is preserved — absent / empty / whitespace / "0" returns (0, nil) meaning "no filter" (permissive), while a present but non-finite value returns 400 (rejection). The raw == "" || raw == "0" guard fires before ParseFloat, so "0" never reaches the new IsNaN/IsInf check. ✓
• The 0 ≤ pct ≤ 100 bound check in parseRecommendationFilter would NOT have caught NaN. Verified at handler_recommendations.go:69-71: if minSavingsPct < 0 || minSavingsPct > 100 — both NaN < 0 and NaN > 100 evaluate to false in Go (IEEE 754 NaN comparison semantics), so NaN would have slipped past the range check entirely. The PR fix correctly places the rejection at the parse boundary, not at the bound check. ✓
• No magic strings / SDK-literal regressions (feedback_no_hardcoded_magic_values.md, feedback_sdk_enum_string_literals.md): the fix adds math.IsNaN/math.IsInf (stdlib primitives), uses the existing NewClientError(400, ...) constructor, and updates only the error-message phrasing. No new string literals on hot paths. ✓
• Tests:
  • Positive coverage: valid integer / float / large integer / pct integer / pct float — all pass.
  • Negative coverage: NaN, lowercase nan, +Inf, bare Inf, Infinity, -Inf, plus the pct-path duplicates (pct NaN, pct infinity). All assert IsClientError returns code 400.
  • Missing: empty / whitespace / "0" — assert (0, nil).
  • Pre-fix verification: ran git checkout HEAD~1 -- internal/api/validation.go then go test ./internal/api/ -run TestParseMinSavingsParam. The new non-finite subtests fail with "An error is expected but got nil" (NaN), and the +Inf / Infinity / -Inf / pct variants fail the same way. Post-fix: all 20 subtests pass. Meets feedback_no_panic_in_test_setup.md (no panics, no time.Sleep, no goroutines). ✓
• Docstring updated to state the non-finite rejection explicitly ("NaN and +/-Inf are rejected: a NaN floor silently disables or inverts the filter downstream"). Truthful and current after the change. ✓

Out of scope, filed as follow-ups

• FE defense-in-depth for non-finite filter values — frontend/src/api/recommendations.ts:17-19 builds the query string via String(filters.minSavingsUsd) / String(filters.minSavingsPct) with no Number.isFinite() guard. If a future caller ever sets these from a malformed source (a parseFloat of arbitrary text), the client would emit min_savings_usd=NaN and the server would now correctly 400 — but the user-visible failure mode is opaque. Today there is no UI input that sets these fields (no consumer of minSavingsUsd / minSavingsPct outside the API wrapper and tests), so this is genuinely defense-in-depth, not a live bug. Filed for hardening per feedback_strict_int_parse.md (strict numeric parsing on the FE).
• getEnvFloat in internal/server/app.go:948 silently accepts NaN/Inf env values. Operator-controlled only, low blast radius, but the same math.IsNaN/math.IsInf guard would close the gap.

CI UNSTABLE state

Lint Code, Integration Tests, Security Scanning, CI Success — same failure shape as the recent adversarial-review rounds (#1232, #1253, #1258); pre-existing on main, not caused by this 2-file diff (internal/api/validation.go + internal/api/validation_test.go). Unit Tests, Build Docker Image, Validate Terraform (aws/gcp/azure), Snyk Security Scan, E2E Tests, pre-commit, Azure role actions parity, sanity (AWS/Azure) all SUCCESS. Not blocking.

Verdict

LGTM. CR re-ping posted.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.