Review fixes

[ScriptSmith]

No description provided.

[greptile-apps]

Greptile Summary

This PR is a broad security hardening pass addressing findings from a previous review. Key changes include: DNS-pinning for all JWKS/OIDC fetches (closing the rebinding window), tenant-scoped cache keys and vector store filtering, bootstrap-auth brute-force protection, stripping of reserved _-prefixed roles from IdP claims and proxy headers, proxy auth failing closed when trusted_proxies is unconfigured, and a dedicated low-rate-limit middleware for /auth/discover.

Confidence Score: 5/5

Safe to merge; no P0/P1 findings. Two P2 issues (discover rate-limit "unknown" bucket and a misleading doc comment) do not block correctness.

All findings are P2. The discover rate-limit "unknown" bucket issue is a consistency gap with the bootstrap-auth fix but is not a security regression relative to the base branch (which had no rate limit at all on this endpoint). All P0/P1 concerns from the previous review threads have been addressed.

src/middleware/layers/rate_limit.rs — discover rate-limit "unknown" fallback bucket.

Important Files Changed

Filename	Overview
src/auth/jwt.rs	Replaces per-request client with a per-fetch DNS-pinned reqwest client via pinned_reqwest_client, closing the JWKS DNS-rebinding window; adds audience empty-string validation.
src/auth/gateway_jwt.rs	Adds per-IP lazy-load rate limiting and LRU negative-cache eviction (VecDeque order index); replaces with_client with with_options to propagate SSRF validation options.
src/auth/oidc.rs	Pins OIDC discovery and JWKS fetches with DNS-pinned clients; adds issuer-mismatch check per OIDC §4.3; SSRF-validates all endpoints from the discovery document.
src/middleware/layers/admin.rs	Adds bootstrap-auth per-IP brute-force protection; strips reserved _-prefix roles from bearer tokens and proxy auth headers; proxy auth now fails closed when no trusted_proxies is configured.
src/middleware/layers/rate_limit.rs	Adds dedicated /auth/discover rate-limit middleware (10 req/min); uses shared "unknown" bucket for IP-less requests, inconsistent with bootstrap-auth fix that skips limiting when IP is absent.
src/cache/keys.rs	Introduces CacheTenantScope mixed into all cache key hashes so cross-tenant collisions are impossible; adds bootstrap rate-limit and lockout key helpers.
src/cache/vector_store/pgvector.rs	Rewrites search query to include org/project tenant filters at the DB layer, using dynamic parameter binding to avoid SQL injection; None maps to IS NULL so unscoped requests cannot match scoped entries.
src/cache/vector_store/mod.rs	Adds VectorTenantFilter with matches() for post-query tenant validation; trait doc misleadingly says "None fields match any value" when the semantics are NULL-only matching.
migrations_sqlx/postgres/20250101000000_initial.sql	Drops daily_spend table; replaces UNIQUE(vector_store_id, file_id) constraint with a partial unique index scoped to deleted_at IS NULL to allow soft-delete re-adds.
src/validation/url.rs	Adds pinned_reqwest_client that binds reqwest DNS resolution to addresses resolved at validation time; wasm32 builds fall back to an unpinned client with a clear comment.
src/config/auth.rs	Adds auth_state_ttl_secs to SessionConfig, configurable auth-state TTL; validates audience is non-empty; rejects SameSite=None without Secure; adds ProxyAuthJwtConfig::validate.
src/routing/mod.rs	Model string validation removes trailing space from allowed characters; error reporting changes from last to first routing failure for better debuggability.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/middleware/layers/rate_limit.rs
Line: 336-380

Comment:
**Shared `"unknown"` bucket on `/auth/discover`**

`discover_rate_limit_middleware` falls back to the literal string `"unknown"` when no client IP is available, collapsing all IP-less requests into a single rate-limit bucket with the low cap of `DISCOVER_REQUESTS_PER_MINUTE = 10`. One client behind an IP-stripping reverse proxy can exhaust the quota and block every other client in the same environment.

The bootstrap-auth fix in this same PR explicitly skips rate-limiting when the IP is absent (`let ip_str = connecting_ip.map(|ip| ip.to_string()); if let (Some(ip_str), Some(cache)) = …`), citing the same concern. The discover middleware should apply the same treatment — either skip entirely when the IP is unknown, or raise the "unknown" bucket threshold significantly.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/cache/vector_store/mod.rs
Line: 315-332

Comment:
**Misleading doc: `None` matches NULL, not "any value"**

The `VectorBackend::search` doc comment says "`None` fields match any value", but the implementation and the following sentence both describe the opposite semantics: `None` means "match only entries stored without a value for that field" (i.e., `IS NULL`). This is confirmed by `VectorTenantFilter::matches` and the `pgvector.rs` query builder (`None => where_clauses.push("organization_id IS NULL")`). The phrase "match any value" should be replaced with something like "match only entries with no value for that field."

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (5): Last reviewed commit: "Fix e2e tests" | Re-trigger Greptile}

[ScriptSmith]

@greptile-apps