fix: reject empty/whitespace-only vulnerability_id in /v3/exploit-intelligence/analyze endpoint - #2567
Merged
Merged
Conversation
Contributor
Reviewer's GuideThis PR adds validation to the /v3/exploit-intelligence/analyze endpoint to reject empty or whitespace-only vulnerability_id values with a 400 Bad Request, wires a new BadRequest error type through the exploit-intelligence module, and ensures the runner treats such errors as permanent while covering the behavior with an endpoint test. Sequence diagram for updated exploit_intelligence_analyze request handlingsequenceDiagram
actor Client
participant AnalyzeEndpoint as analyze
participant EIService as ei_service
participant DatabaseRW as db_rw
participant DatabaseRO as db_ro
Client->>AnalyzeEndpoint: POST /v3/exploit-intelligence/analyze
AnalyzeEndpoint->>ei_service: runtime()
ei_service-->>AnalyzeEndpoint: Option<Runtime>
alt [runtime is None]
AnalyzeEndpoint-->>Client: Error::Unavailable (503)
else [runtime is Some]
AnalyzeEndpoint->>AnalyzeEndpoint: vulnerability_id.trim()
alt [vulnerability_id.is_empty()]
AnalyzeEndpoint-->>Client: Error::BadRequest (400)
else [vulnerability_id not empty]
AnalyzeEndpoint->>DatabaseRW: begin()
DatabaseRW-->>AnalyzeEndpoint: tx
AnalyzeEndpoint->>EIService: create_job(sbom_id, vulnerability_id, tx)
EIService-->>AnalyzeEndpoint: Result<Job, Error>
alt [Ok(job)]
AnalyzeEndpoint-->>Client: 201 AnalyzeResponse
else [Err(e)]
AnalyzeEndpoint->>DatabaseRW: rollback(tx)
AnalyzeEndpoint->>DatabaseRO: begin()
DatabaseRO-->>AnalyzeEndpoint: ro_tx
AnalyzeEndpoint->>EIService: find_active_job(sbom_id, vulnerability_id, ro_tx)
EIService-->>AnalyzeEndpoint: Option<Job>
alt [Some(job)]
AnalyzeEndpoint-->>Client: 200 AnalyzeResponse
else [None]
AnalyzeEndpoint-->>Client: e (mapped to HTTP)
end
end
end
end
Sequence diagram for Error to AnalysisError conversion with BadRequestsequenceDiagram
participant Runner
participant AnalysisError
Runner->>AnalysisError: From<crate::Error>::from(Error::BadRequest)
AnalysisError-->>Runner: AnalysisError::Permanent
Runner->>AnalysisError: From<crate::Error>::from(Error::Database)
AnalysisError-->>Runner: AnalysisError::Retryable
Runner->>AnalysisError: From<crate::Error>::from(Error::Unavailable)
AnalysisError-->>Runner: AnalysisError::Permanent
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
Error::BadRequest’sResponseErrorimplementation you pass the raw message string toErrorInformation::newrather thanself, which is inconsistent with the other variants and may drop useful context; consider aligning this with the other error responses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Error::BadRequest`’s `ResponseError` implementation you pass the raw message string to `ErrorInformation::new` rather than `self`, which is inconsistent with the other variants and may drop useful context; consider aligning this with the other error responses.
## Individual Comments
### Comment 1
<location path="modules/exploit-intelligence/src/endpoints/test.rs" line_range="774-775" />
<code_context>
+ })
+ .to_request();
+
+ let resp = test::call_service(&app, req).await;
+ assert_eq!(resp.status(), 400, "expected 400 for vulnerability_id={blank:?}");
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Assert the error response payload, not just the HTTP status, to lock in the BadRequest contract.
Given that `Error::BadRequest` now returns a structured `ErrorInformation`, consider deserializing the response body and asserting its `kind` (e.g. `"BadRequest"`) and message (e.g. `"vulnerability_id must not be empty"`). This will ensure the test validates the full error contract, not just the status code.
Suggested implementation:
```rust
for blank in ["", " "] {
let req = TestRequest::post()
.uri("/v3/exploit-intelligence/analyze")
.set_json(AnalyzeRequest {
sbom_id: Uuid::now_v7(),
vulnerability_id: blank.to_string(),
})
.to_request();
let resp = test::call_service(&app, req).await;
assert_eq!(
resp.status(),
StatusCode::BAD_REQUEST,
"expected 400 for vulnerability_id={blank:?}"
);
let body = actix_web::body::to_bytes(resp.into_body()).await?;
let error: ErrorInformation = serde_json::from_slice(&body)?;
assert_eq!(
error.kind,
"BadRequest",
"unexpected error kind for vulnerability_id={blank:?}"
);
assert_eq!(
error.message,
"vulnerability_id must not be empty",
"unexpected error message for vulnerability_id={blank:?}"
);
}
```
1. Ensure the test module imports the `StatusCode` and `ErrorInformation` types, and `serde_json`:
- Add `use actix_web::http::StatusCode;`
- Add the appropriate import for `ErrorInformation` (e.g. `use trustify_error::ErrorInformation;` or wherever it is defined).
- Add `use serde_json;` if not already present.
2. Confirm that `ErrorInformation` has `kind` and `message` fields with the expected types; adjust the field names in the assertions if the actual struct uses different names (e.g. `error_type`, `detail`, etc.).
3. If the error payload nests these fields (e.g. `{ "error": { "kind": "...", "message": "..." } }`), update the deserialization and assertions accordingly to match the actual JSON structure.
</issue_to_address>
### Comment 2
<location path="modules/exploit-intelligence/src/endpoints/test.rs" line_range="746-747" />
<code_context>
Ok(())
}
+/// Verifies that POSTing an analyze request with an empty or whitespace-only
+/// vulnerability_id returns 400 Bad Request.
+#[test_context(TrustifyContext)]
+#[test(actix_web::test)]
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that verify blank vulnerability_id errors are treated as permanent (non-retryable) analysis errors.
This test only checks that the endpoint returns HTTP 400, but doesn’t verify that `Error::BadRequest` is treated as a permanent analysis error. Please add or update tests in the analysis/runner suite to cover the `BadRequest` variant and assert it maps to `AnalysisError::Permanent`, so the background analysis behavior matches the bugfix intent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…elligence/analyze endpoint
Contributor
|
Looks good. |
i386x
approved these changes
Aug 12, 2026
|
Successfully created backport PR for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes TC-5535
Summary by Sourcery
Validate and reject exploit intelligence analyze requests with blank vulnerability IDs and surface them as client errors.
Bug Fixes:
Enhancements: