Skip to content

Harden Search API correctness, resource controls, and delivery validation - #119

Closed
afoote-mitre wants to merge 33 commits into
devfrom
af/0.4.0-codex-review
Closed

afoote-mitre wants to merge 33 commits into
devfrom
af/0.4.0-codex-review

Conversation

@afoote-mitre

@afoote-mitre afoote-mitre commented Aug 3, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

This PR hardens the Search API after the 0.4.0 multi-skill review. The changes are intentionally grouped around failure modes: invalid input reaching query construction, incomplete OpenSearch responses being treated as success, malformed semantic CPE candidates producing unreliable results, oversized responses and expensive searches exhausting process resources, shared-operation telemetry being attributed incorrectly, shutdown/readiness races, unvalidated runtime configuration, and delivery checks that did not verify the runtime image or production OpenSearch contract.

It also removes stale fallback paths and consolidates tests so the additional safety behavior remains maintainable rather than leaving duplicate coverage in multiple layers.

Why These Changes Were Needed

1. Fail Closed on Incomplete or Malformed OpenSearch Results

Why: OpenSearch may return an HTTP success while reporting a timeout, failed shards, malformed hit structures, or a page that does not contain every requested CVE. Treating those responses as successful can expose incomplete totals, missing records, duplicate records, or stale cached semantic results.

What changed: OpenSearch responses now require explicit non-timeout and zero-failed-shard metadata, operation-appropriate totals, and valid hit collections. Semantic candidates and page-fetch records are validated before use. Invalid upstream responses map to sanitized 502 or 504 responses, and incomplete page fetches evict the associated cache entry.

Primary review locations:

2. Reject Invalid Search Input Before Query Construction

Why: Middleware previously validated some fields only superficially, while the query builder could silently ignore malformed CPE values or create filters from inputs that bypassed the expected route path. Explicit empty CPE components could be interpreted like omitted trailing components, and arbitrary non-empty strings could reach OpenSearch as CWE filters. Invalid CVE IDs, CWE IDs, CVSS vectors, date ranges, pagination values, and virtual match ranges need one deterministic contract and a second fail-closed boundary at query construction.

What changed: /search now rejects unknown fields, wrong JSON types, empty values, oversized values, excessive keyword/CVSS complexity, malformed CVE and CWE IDs, invalid date ordering, explicit empty CPE components, invalid CPE parts, inconsistent version-bound pairs, impossible ranges, and pagination outside the supported window. cweId requires a positive decimal identifier without leading zeros, accepts a case-insensitive prefix, and is normalized to uppercase. Shared CVE, CWE, and CVSS parsers keep validation and DSL construction aligned. Query construction throws when invalid CWE, CPE, or range data reaches it instead of omitting the filter.

Primary review locations:

3. Make Pagination and Semantic Page Fetches Deterministic

Why: Pagination metadata must describe the records actually returned, and repeated searches need stable ordering. Semantic CPE searches also fetch full records in a second operation, so missing, duplicate, unexpected, or reordered CVE IDs could otherwise produce incorrect pages.

What changed: Standard searches use deterministic sorting and exact total tracking. Pagination reports the current page, returned page size, and accessible page count consistently. Semantic searches paginate filtered CVE IDs first, fetch the exact page by ID without unnecessary sorting or total counting, validate one unique record per requested ID, and restore requested order locally.

Primary review locations:

4. Bound Expensive Search Work and Abandoned Requests

Why: Broad semantic CPE searches can retrieve and evaluate up to 10,000 candidates, while otherwise valid searches can serialize large pages of complete CVE records. Without bounded concurrency, queue depth, waiter counts, cancellation checks, request deadlines, and response-size enforcement, bursts or disconnected clients can consume memory, event-loop time, network capacity, and OpenSearch capacity after the caller is gone.

What changed: Cold semantic candidate requests retain the proof-of-concept admission queue but now fail closed when full or expired. Identical candidate and page-fetch work is shared with bounded waiter counts and is canceled when no waiters remain. Semantic filtering validates incrementally, yields to the event loop, and checks cancellation between chunks. OpenSearch requests are abortable and bounded by both per-operation and overall request deadlines. Successful /search responses default to a 10 MiB serialized UTF-8 limit, recheck the overall deadline after serialization, and return one complete HTTP 422 SEARCH_RESPONSE_TOO_LARGE error before any bytes are sent. Exact totals and the 500-record page cap remain unchanged. A disabled-by-default process-wide /search rate limiter provides a fallback safety control.

Primary review locations:

5. Enforce the Stored CPE Applicability Contract

Why: Semantic filtering is only reliable when every candidate has the projected CVE ID and direct CNA cpeApplicability.nodes[].cpeMatch shape the application evaluates. Invalid CPE parts, contradictory range boundaries, missing vulnerability flags, nested-only structures, or combining matches across entries can create false positives or false negatives.

What changed: Candidate evaluation validates each direct CPE Match Criteria entry before applying filters. cpeName, vulnerability, platform components, exact versions, and range versions must be satisfied by compatible individual entries. CPE parts and range structures are validated explicitly, and the documented behavior now states that the application evaluates direct entries independently rather than complete node logic.

Primary review locations:

6. Make Runtime Startup, Readiness, and Shutdown Predictable

Why: Invalid timeout relationships or malformed environment values should stop startup rather than fail later under traffic. Readiness must not return success after shutdown begins, startup errors must not become unhandled rejections, and shutdown must stop new traffic while allowing active requests and OpenSearch clients to drain within a bounded deadline.

What changed: Runtime configuration is parsed once, type-checked, range-checked, relationship-checked, and frozen. HTTP timeouts, search deadlines, queue settings, logging controls, feature flags, and rate limits use the same resolved configuration. The server tracks active requests, gates traffic during shutdown, performs a bounded graceful close, handles synchronous and asynchronous startup failures, and keeps liveness available while readiness fails during shutdown.

Primary review locations:

7. Add Correlated, Sanitized Operational Evidence

Why: Queue saturation, OpenSearch throttling, timeouts, client disconnects, and event-loop pressure are difficult to distinguish from generic slow requests without structured metrics. Logging full URLs, queries, or upstream errors would create a separate data-exposure risk.

What changed: Requests receive bounded correlation IDs. Access logs redact query strings and sample successful traffic while retaining failures. Search performance events identify strategy, outcome, queue/cache/waiter state, OpenSearch operation counts, cancellation causes, and rolling event-loop delay without logging search terms, CPE values, credentials, bodies, or raw upstream errors. Candidate-discovery and page-fetch metrics are now captured on the shared operation and merged into each active waiter independently. Cache entries retain result data only, so later cache hits do not replay historical queue, OpenSearch, or post-filter timings. Oversized-response failures log the attempted byte count, configured maximum, and serialization duration.

Primary review locations:

8. Verify Delivery, Runtime Packaging, and the Production Index Contract

Why: Unit tests cannot prove that the production image contains every runtime module, runs as the intended non-root user, remains free of fixable severe image vulnerabilities, or that a separately owned OpenSearch index exposes the fields and mappings this API requires.

What changed: CI now runs dependency review, quality checks, a pinned production image build, two-stage Trivy reporting/enforcement, runtime startup, health/OpenAPI smoke checks, and user/command validation. A protected, manually dispatched workflow performs read-only OpenSearch field-capability and representative query checks with secrets scoped only to the contract step. The contract check now discovers a real record with direct CNA CPE applicability, validates its projected candidate shape through the production candidate processor, extracts an actual criteria value, and executes a positive CPE candidate query. Dockerfiles copy cohesive runtime directories into a pinned distroless non-root image. Owned dependencies and transitive overrides were updated, including brace-expansion 5.0.9; the one cve-core-owned adm-zip exception is explicit and time-bounded.

Primary review locations:

Maintainability and Test Scope

Legacy query fallbacks, stale maintenance exports, obsolete base URL configuration, and duplicated validation helpers were removed so unsupported paths cannot silently diverge from the public POST contract. Tests were moved to the modules that own the behavior and consolidated where controller, utility, workflow, and OpenAPI suites duplicated the same assertions.

Primary review locations:

Deferred Decisions

Item Why it matters Why it remains deferred
PIT and search_after for semantic CPE searches The current 10,000-candidate boundary can omit later semantic matches and report incomplete totals for broad queries. Requires team agreement on pagination semantics, PIT lifecycle, resource cost, and compatibility with semantic post-filtering. The current limitation is explicit in README.md.
Remove /webSearch The deprecated endpoint retains older raw-query behavior and should not remain part of the long-term public surface. Consumer migration and policy approval are still pending. It can be disabled at runtime with WebSearchEnabled=false.
Replace the process-local cache and queue Per-process state does not provide consistent behavior across ECS tasks and is not the intended production architecture. The current implementation is a proof of concept. A shared AWS Valkey design, or removal of the cache/queue, will be handled separately.
Update cve-core transitive dependency cve-core owns the remaining adm-zip pin and its remediation. This repository cannot safely change another repository's dependency contract; the temporary exception expires on 2026-10-31.

@afoote-mitre
afoote-mitre marked this pull request as draft August 3, 2026 18:25
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