Add size constraints to CTIM collection fields (XDR-46972)#483
Conversation
yogsototh
left a comment
There was a problem hiding this comment.
I have only two concerns:
-
why these numbers, to me they are a bit like "magic numbers" and it will eventually breaks in PROD refusing the creation of many entities suddenly without a warning which may create issues with already working integrations. Personally, I think it may be a bit too much to constrain statically here instead of directly into the CTIA application where we could apply different size constraints depending on the source for example. Which will give the time for an integration to adapt to the new constraints. And perhaps instead of a hard reject, we may also integrate a mechanism to just cut the strings/arrays to keep the first one.
-
We should check if adding these constraints does not affect Swagger and Swagger UI. From memory plumatic/schemas and swagger do not support very well certain kind of external constraints like this one. So while the API will work, Swagger UI may be broken, or not show the correct schemas anymore.
Add pred/max-len constraints to seq-of/set-of collection fields in sighting, incident, and common schemas to prevent oversized documents causing ES bulk write failures. - Bump flanders to 1.1.1-SNAPSHOT (adds :spec support on collection types) - Add default-collection-max-len (500) for common collection fields - Add specific limits for sighting fields (observables, relations, targets, data tables) - Constrain external_ids and external_references in base entity entries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace f/any-str with ShortString (1024), MedString (2048), or LongString (5000) across all CTIM schemas based on field semantics and production data analysis (831K sightings, 388K incidents from NAM). - ShortString: identifiers, names, IPs, labels, type fields - MedString: indicator specs (snort, SIOC, OpenIOC), metadata values - LongString: casebook text content - seq-of ShortString: hashes, variables, permissible_IPs Observable :value intentionally kept as f/any-str — production data shows process_args up to 32K chars (6376 values > 1024). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…onstraint - observables: 2000 → 5000 (P99.9 = 4128) - targets: 1000 → 2000 (P99.9 = 1841) - relations: 10000 (unchanged, P99.9 = 9814) - relation_info.actions: new limit of 1000 (addresses 142K action accumulation pattern) - pred/max-len: add metadata for downstream 413 error reporting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bfb1769 to
8237a23
Compare
yogsototh
left a comment
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED -- 1 CRITICAL, 2 HIGH, 3 MEDIUM
This is a clean, well-scoped tightening and the core max-len predicate is correct (inclusive >= bound, nil-safe via string? guards). The blocking issues are about the dependency and where the constraints actually take effect, not the schema design itself.
Findings
-
CRITICAL [cross-cutting] -- SNAPSHOT dependency on unreleased flanders:
project.cljpinsthreatgrid/flanders "1.1.1-SNAPSHOT". flanders 1.1.1 has no git tag, no GitHub release, and Clojars returns 404 for it. The:spec-on-collection-types support that everyf/seq-of/f/mapconstraint in this PR relies on exists only on flanders master, not in 1.1.0. Merging this tomastermakes the ctim build non-reproducible (a SNAPSHOT is mutable / can be GC'd), blocks cutting any stable ctim release, and means the collection constraints are inert until flanders 1.1.1 actually ships. Release flanders 1.1.1 to Clojars and pin the released coordinate before merge. (See inline onproject.clj.) -
HIGH [cross-cutting] -- Collection caps are not enforced in the schema/Swagger target: flanders
schema.cljc(master) does not read:specforSequenceOfType/SetOfType/MapType-- it is consumed only by the clojure.spec target. So the new collection-length caps (external_ids/external_references, sightingtargets/observables/relations/data-table, incidentcategories/assignees/tactics/techniques,relation_info.actions) are present in spec but absent from the plumatic-schema object and the generated Swagger/JSON Schema. This confirms the prior reviewer's Swagger concern at the source level. The string-length caps (ShortString/MedString/LongString) do propagate to both targets. Decide whether flanders should honor:specon collection types in the schema target, or document the collection caps as spec-only and accept that Swagger and any plumatic-based validation will not enforce them. -
HIGH [correctness/cross-cutting] -- Backward-incompatible narrowing rejects previously-valid data: replacing unbounded
f/any-strwith bounded types is a hard reject for documents that validated before. Highest risk isopen_IOC -> MedString (2048)-- the field's own docstring calls it "an XML blob of an openIOC indicator," and OpenIOC XML routinely exceeds 2048 chars;SIOCandsnort_sig(both MedString 2048) are similar. No migration path, grace period, or source-conditional limit is present. Recommend auditing existing PROD data against each new bound and confirmingopen_IOC/SIOCat 2048 is intended. (See inline onindicator.cljc.) -
MEDIUM [security/correctness] --
relation_info.actionscap is trivially bypassable: the:speconly bounds:actionswhen it issequential?; a non-sequential value (JSON object -> map, or a string) returnstrueand passes unbounded, and only the literal:actionskey is inspected in an otherwise free-form map. As a DoS-size guard on an already-open field this is best-effort, but the cap as written does not boundrelation_info. (See inline oncommon.cljc.) -
MEDIUM [quality/cross-cutting] -- Asymmetric bounding: the embedded
SightingDataTablebounds:columns/:rows, but the standaloneDataTableentity (data_table.cljc:53-56) leaves the structurally identical fields unbounded -- the first-class entity is the larger surface and is left open. Same pattern withincident:detection_sources, uncapped while its sibling collection fields were capped. (See inline onincident.cljc.) -
MEDIUM [tests] -- No failure-path tests for the new bounds: the diff touches only
src/. None of the 12 new collection caps (nor the boundary/off-by-one, nor the 3-branchrelation_infospec, nor the new{:max-len}metadata) has a test asserting rejection or at-limit acceptance. The existing generative tests only assert normally-sized documents are accepted (test.check default sizes never approach 500-10000), so "139 tests pass" does not exercise any new bound. Suggest at-N (accept) / at-N+1 (reject) tests per bound.
PASS: Coding Style (only minor :spec indentation drift at sighting.cljc:48,51 and incident.cljc:171; CLJS require parity is correct in all files -- no build break).
Also left 4 inline comments.
Non-blocking follow-ups
max-lennow returns(with-meta fn {:max-len len}); no consumer reads it in this PR, and the metadata is lost once the predicate is composed insidecs/and(which is how every string type uses it). Confirm the intended consumer reads it off the bare predicate.- Centralize or document the convention for where size constants live (
common.cljcdefaults vssighting.cljc-local) and add the P99.9 rationale (currently only in commit messages) as in-code comments. Observable :valueis intentionally leftf/any-str(per commit message: process_args up to ~32K); noted as deliberate, not a defect.count-basedmax-lenis not guarded against a non-counted lazy seq reaching therelation_infospec (in-process/EDN callers only; JSON input is unaffected).
-- auto-review-team (BOT_APPROVABLE gate)
| [threatgrid/clj-momo "0.4.1"] | ||
| [org.mozilla/rhino "1.8.0"] ;threatgrid/flanders > kovacnica/clojure.network.ip | ||
| [threatgrid/flanders "1.1.0"] | ||
| [threatgrid/flanders "1.1.1-SNAPSHOT"] |
There was a problem hiding this comment.
CRITICAL -- SNAPSHOT dependency on unreleased flanders: 1.1.1-SNAPSHOT has no flanders git tag, no GitHub release, and Clojars 404s for it. The :spec support on SequenceOfType/SetOfType/MapType that this entire PR depends on exists only on flanders master, not in 1.1.0. Merging this to master makes the build non-reproducible (a SNAPSHOT is mutable and can be GC'd), blocks any stable ctim release, and leaves these constraints inert until flanders 1.1.1 ships. Release flanders 1.1.1 to Clojars and pin the released version before merge.
| (f/required-entries | ||
| (f/entry :type OpenIOCSpecificationType) | ||
| (f/entry :open_IOC f/any-str)) | ||
| (f/entry :open_IOC c/MedString)) |
There was a problem hiding this comment.
HIGH -- Bound likely too small for documented content: this field's docstring describes it as "an XML blob of an openIOC indicator," and OpenIOC XML documents routinely exceed 2048 chars, so MedString will reject most real open_IOC values. SIOC (line 49) and snort_sig (line 41) share the same 2048 cap and the same risk. More broadly, replacing unbounded f/any-str with a hard cap rejects documents that validated before, with no migration path. Recommend auditing existing PROD data against these bounds and confirming the values are intended for these blob fields.
| :spec (fn [m] | ||
| (let [actions (:actions m)] | ||
| (or (nil? actions) | ||
| (not (sequential? actions)) |
There was a problem hiding this comment.
MEDIUM -- Actions cap is bypassable: the limit only applies when :actions is sequential?. A non-sequential value -- a JSON object (decodes to a map) or a string -- makes (not (sequential? actions)) return true, so it passes unbounded. Only the literal :actions key is inspected; the rest of this free-form f/any map is uncapped. If the goal is to bound oversized relation_info, consider rejecting non-sequential :actions rather than waving them through, and confirm :actions is the real key used in production payloads.
| (f/entry :assignees (f/seq-of c/ShortString | ||
| :spec (pred/max-len c/default-collection-max-len)) | ||
| :description "A set of owners assigned to this incident.") | ||
| (f/entry :detection_sources [c/MedString] |
There was a problem hiding this comment.
MEDIUM -- Uncapped sibling: :detection_sources kept the plain [c/MedString] form while its neighbours :categories, :assignees, :tactics, and :techniques all received (f/seq-of ... :spec (pred/max-len default-collection-max-len)) in this PR. If the size cap is meant to cover incident collection fields, this one was missed; if it is intentionally unbounded, a one-line note would make that explicit. (Same asymmetry exists between the bounded embedded SightingDataTable and the unbounded standalone DataTable entity in data_table.cljc.)
Add
pred/max-lenconstraints to collection fields in sighting, incident, and common schemas.Changes
1.1.1-SNAPSHOT(adds:specsupport onSequenceOfType,SetOfType, andMapType)default-collection-max-len(500) constant incommon.cljcfor shared collection fieldsexternal_idsandexternal_referencesin base entity entriescolumns: 100,rows: 10,000targets: 2,000,observables: 5,000,relations: 10,000categories,assignees,tactics,techniques) to 500relation_info.actionslimit of 1,000{:max-len len}metadata topred/max-lenfor downstream 413 error reportingf/any-strwith typed string constraints (ShortString,MedString,LongString)§ QA
No QA is needed. All 139 existing tests (including generative tests) pass.
§ Release Notes
§ Squashed Commits