nextron_thor_apt_scanner: quality improvements#19931
Conversation
6ab2a2a to
b758bdd
Compare
✅ Elastic Docs Style Checker (Vale)No issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
| { | ||
| "events": [], | ||
| "offset": state.more_pages ? (int(state.offset) + int(state.batch_size)) : 0, | ||
| "want_more": state.more_pages, |
There was a problem hiding this comment.
Severity: 🟠 High confidence: medium path: packages/nextron_thor/data_stream/thor_forwarding/agent/stream/cel.yml.hbs:179
A scan-search HTTP error is silently swallowed when the worklist was drained in the prior cycle; guard the second stage so the error is surfaced instead of overwritten.
Details
The program runs in two stages against a single, merged state. Stage 1 (scan search) sets worklist/more_pages on success, or on a non-200 returns the single-object error map {"events": {"error": {...}}, "offset": 0, "want_more": false} WITHOUT a worklist key. Stage 2 then runs unconditionally as state.with(<expr>).
Because state.with() preserves keys not present in the merged map, the worklist from the previous cycle survives a Stage 1 error. In normal steady state that surviving value is the empty list [] left by tail(state.worklist, 1) after the last log was processed. So after a search error, Stage 2 evaluates has(state.worklist) = true and size(state.worklist) > 0 = false, and falls into the empty-worklist branch (this line). That branch returns {"events": [], "offset": ..., "want_more": state.more_pages, "cursor": {...}}, which is merged over the state and OVERWRITES events (the error object) with [].
Result: the scan-search API error is never indexed (no error event is emitted), and want_more/offset are driven by the stale state.more_pages from the prior successful search. When that stale more_pages is true, want_more becomes true and offset advances by batch_size on an error cycle, so the program skips past the failed page instead of retrying it. This defeats the error handling that Stage 1 carefully constructs and can cause silent data loss on transient search-API failures.
Recommendation:
Short-circuit the second stage when the first stage already produced an error, so the single-object error map is returned as-is instead of being overwritten by the empty-worklist branch:
).as(state,
// If the scan-search stage already produced an error, surface it and halt;
// do not let the empty-worklist branch overwrite it with an empty page.
(has(state.events) && type(state.events) == map) ?
state
:
state.with(
!has(state.worklist) ?
state
: (size(state.worklist) > 0) ?
request(
"GET",
state.url.trim_right("/") + "/v1/scan/log?" + {
"scan": [state.worklist[0].id],
"log": ["thor.json"],
}.format_query()
)
// ... unchanged ...
:
{
"events": [],
"offset": state.more_pages ? (int(state.offset) + int(state.batch_size)) : 0,
"want_more": state.more_pages,
"cursor": {
?"latest_scan_ts": state.?cursor.latest_scan_ts,
?"latest_scan_id": state.?cursor.latest_scan_id,
},
}
)
)
(On a successful search the first stage sets worklist/more_pages and does not set events, so the guard only triggers on the error map.)
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| "worklist": tail(state.worklist, 1), | ||
| // offset is a variable used to control paging on the scan search API that fills 'worklist' | ||
| // increment when the worklist is empty | ||
| "offset": (size(state.worklist) == 1) ? (int(state.offset) + int(state.batch_size)) : state.offset, |
There was a problem hiding this comment.
Severity: 🟠 High confidence: medium path: packages/nextron_thor/data_stream/thor_forwarding/agent/stream/cel.yml.hbs:150
The scan-search offset is advanced but never reset when a collection cycle ends on a partial last page, so the next periodic run skips scans; only advance offset when more_pages is true, else reset it to 0.
Details
When the worklist drains its last item (size(state.worklist) == 1) this branch sets offset = int(state.offset) + int(state.batch_size) unconditionally, and sets want_more = size(state.worklist) > 1 || state.more_pages. On the final, partial page of a cycle more_pages is false (body.data.size() < batch_size), so evaluation terminates here with want_more=false AND a non-zero offset. offset is a top-level (non-cursor) state key, so it is retained across periodic evaluations in the running input session. The offset is only ever reset to 0 in the empty-worklist fallthrough branch and the error branches, neither of which is reached on this path. Consequently the next periodic run re-initializes start_time from the cursor but issues /v1/scan/search with the stale offset, causing the server to skip the first offset matching scans of the new time window — those are new, unprocessed scans (the already-processed one is dropped separately by the s.id != latest_scan_id client filter), so they are silently lost. This only self-corrects when the final page is exactly full (total a multiple of batch_size), because then an extra empty-page query routes through the reset branch. The typical partial-final-page case loses data every cycle after the first.
Recommendation:
Only advance offset when there is genuinely a next page (more_pages), and reset it to 0 when the current page is the last one, so a fresh periodic cycle starts at offset 0:
"offset": (size(state.worklist) == 1) ?
(state.more_pages ? (int(state.offset) + int(state.batch_size)) : 0)
:
state.offset,
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
There was a problem hiding this comment.
Good catch. offset is retained across periodic evaluations within a session. Line 150 unconditionally advances offset when draining the last worklist item, so a partial final page can leave a non-zero offset into the next interval.
b4b9d68 to
6a3b847
Compare
🚀 Benchmarks reportTo see the full report comment with |
|
No issues across the latest commits dbbf919. Review summaryIssues found across earlier commits [b4b9d68](https://github.com/elastic/integrations/commit/b4b9d685b8e343896c34b8710622af04fc5e4738) — 1 high
Issues found across earlier commits [b758bdd](https://github.com/elastic/integrations/commit/b758bdd45158edede2dc507fb419fe3e7d326ec3) — 1 high
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
|
✅ All changelog entries have the correct PR link. |
💚 Build Succeeded
History
|
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots