Skip to content

Auth hardening follow-ups from #260 and #262 #337

@raullenchai

Description

@raullenchai

Carry-over P1 findings from codex review of #260 and #262 (both merged as critical bypass fixes — these items are hardening of an already-correct fix, not unfixed bypasses).

From #260 (vllm_mlx/routes/health.py)

  • Probe doc update: When --api-key is set, k8s/Docker liveness probes calling GET /health and /health/ready will now get 401. Update docs/guides/server.md to document this and provide probe-config examples (Bearer token in httpHeaders for k8s liveness probes, or split unauthenticated liveness from authenticated management/cache routes).
  • Strengthen positive-case test assertions in test_health_router_accepts_valid_api_key: replace assert r.status_code != 401 with explicit per-route expected status codes — the current assertion would pass on 5xx or a broken handler.

From #262 (vllm_mlx/middleware/auth.py)

  • Bearer token whitespace normalization: _extract_bearer_token should .strip() the token after partition(\" \"). Currently Bearer test-secret and Bearer test-secret are auth-equivalent (HTTPBearer normalizes for auth) but route to different rate-limit buckets.
  • Duplicate x-api-key header: request.headers.get(\"x-api-key\") only evaluates one value. Use request.headers.getlist(\"x-api-key\") and either require exactly one value or require all to match — the "both must match" rule should extend to multi-valued headers.
  • Empty companion header: _verify_api_key_values() filters falsy values, so Authorization: Bearer test-secret plus x-api-key: (empty) is accepted. If the rule is "if a credential header is present, it must be valid," presence needs to be checked separately from extracted credential value.
  • Constant-time comparison short-circuit: all(secrets.compare_digest(...)) short-circuits on first mismatch, leaking which header failed via timing. Replace with non-short-circuit accumulation (valid &= compare_digest(...)).
  • Non-ASCII header → 500 instead of 401: secrets.compare_digest(str, str) raises TypeError for non-ASCII strings. An invalid x-api-key with non-ASCII bytes can produce a 500 instead of 401. Encode to bytes with strict policy or catch TypeError and reject as 401.
  • Test fixture cleanup not exception-safe during setup: tests/test_anthropic_route_auth.py anthropic_client fixture restores sys.modules and parent attrs only after yield. If an import or setup step fails before yield, parent module attributes can remain monkeypatched. Wrap setup in try/finally or use monkeypatch for parent attrs too.

Why follow-up, not block-merge

None of these are exploitable bypasses. The base merged PRs close active CRITICAL bypasses (anyone could DELETE the cache, anyone could call Anthropic routes); the bypass cost > the residual hardening cost. These items are tracked here so the security posture keeps improving.

Refs: #189 (closed by #260), #188 (closed by #262), and the new SOP §0–§9 in docs/development/pr_merge_sop.md.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestgood first issueGood for newcomerssecuritySecurity vulnerability or hardening

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions