Skip to content

fix(config): validate port range and non-negative router integers - #108

Open
norascott407 wants to merge 1 commit into
Continuum-AI-Corp:mainfrom
norascott407:fix/config-field-validation
Open

norascott407 wants to merge 1 commit into
Continuum-AI-Corp:mainfrom
norascott407:fix/config-field-validation

Conversation

@norascott407

@norascott407 norascott407 commented Sep 14, 2026

Copy link
Copy Markdown

Orca-Code-Review — push 1

Severity Count
P0 0
P1 1
P2 0
P3 0

❌ 1 finding blocks merge

Settings accepted any integer for port, router_cooldown_seconds, router_allowed_fails, router_num_retries_default, and router_num_retries_auto. Invalid values like port=0, port=99999, or negative retry counts were silently accepted and only failed at uvicorn bind time or inside LiteLLM Router with a confusing error.

Adds Pydantic field_validator for each field:

  • port: must be 1-65535
  • router_cooldown_seconds: must be >= 0
  • router_allowed_fails: must be >= 0
  • router_num_retries_default / router_num_retries_auto: must be >= 0

Misconfiguration is now caught at startup with a clear error message before any request is served.

Includes 13 regression tests covering boundary values, env-sourced rejection, and the existing test-suite (434 unit tests) passes clean.

Settings accepted any integer for port, router_cooldown_seconds,
router_allowed_fails, router_num_retries_default, and
router_num_retries_auto. Invalid values (port 0 or 99999, negative
retry counts) were silently accepted and only failed at uvicorn bind
time or inside LiteLLM Router with a confusing error.

Add Pydantic field_validator for each field:
- port: must be 1–65535
- router_cooldown_seconds: must be >= 0
- router_allowed_fails: must be >= 0
- router_num_retries_default/auto: must be >= 0

Clear error messages at startup so misconfiguration is caught before
any request is served.

@orcacode-review orcacode-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐳 OrcaCode Review

Found 1 issue in this PR: 🟠 1 P1.

Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.


packages/auth/encryption.py (line 83): 🟠 P1 New Settings ValidationError is swallowed by encryption/hashing fallbacks, silently dropping to the public dev key and un-peppered hashes

This commit adds new ways for Settings() to raise (pydantic.ValidationError from _port_in_range, _cooldown_non_negative, _allowed_fails_non_negative, _retries_non_negative) — a bad value in any router/port field now aborts get_settings(). Two callers resolve security-critical material through get_settings() inside a bare except Exception: pass:

  • packages/auth/encryption.py:77-83 — the CREDENTIAL_ENCRYPTION_KEY lookup. If get_settings() raises, key_hex stays "" and the code falls through to os.environ.get("CREDENTIAL_ENCRYPTION_KEY"). The docstring right above (lines 71-75) notes that pydantic-settings does NOT export .env into os.environ, so on the README/.env.example path — key written in .env — the lookup misses and _resolve_key_material() returns the dev-fallback key (sha256(b"orcarouter-lite-dev-key")), i.e. every provider credential is sealed with a publicly-known key and the "who am I using" signal flips to insecure.
  • packages/auth/hashing.py:23-28 — same shape for API_KEY_PEPPER: the pepper is dropped and hash_api_key() silently falls back to plain SHA-256 for newly minted keys (a mixed pepper/no-pepper key table, which the dual-hash lookup is explicitly there to work around).

So a misconfigured router knob (e.g. ROUTER_NUM_RETRIES_AUTO=-1, which this commit now rejects) can degrade credential handling instead of failing loudly — a check that cannot complete fails open. I am marking this low confidence because in the standard boot path the same get_settings() call in the lifespan (app/main.py:49) raises first and uvicorn exits, so this is only reachable where the app is served without the lifespan (e.g. ASGI-transport embedding/tests) or after a successful cache then a cleared cache; the code path itself is real but not routinely exercised.

The swallowed exception was written for "Settings may not be importable in some isolated test contexts", which only justifies ImportError. Concrete fix: narrow the handler and treat a configuration error as fatal, e.g.

try:
    from app.config import get_settings
    key_hex = get_settings().credential_encryption_key or ""
except ImportError:
    pass  # isolated import contexts only

(and the analogous change in packages/auth/hashing.py:26), so that an invalid Settings can never silently select the dev key or the un-peppered hash.

OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 389 calls · 21.3M tokens · 98% cached

❤️ Share · Install OrcaCode Review

Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.

Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter

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