Feature: Add Indicator To AddIOCtoIOCCollectionAction (1315, 1314)#2064
Open
Feature: Add Indicator To AddIOCtoIOCCollectionAction (1315, 1314)#2064
Conversation
vg-svitla
changed the title
Contributor
Reviewer's GuideAllow AddIOCtoIOCCollectionAction to accept a single indicator value in addition to a list, and add tests plus metadata updates to cover and document this new behavior. Sequence diagram for AddIOCtoIOCCollectionAction run logic with list or single indicatorsequenceDiagram
actor Caller
participant AddIOCtoIOCCollectionAction as Action
Caller->>Action: run(arguments)
activate Action
Action->>Action: indicators = json_argument("indicators", arguments, required=False)
Action->>Action: single_indicator = arguments.get("indicator")
Action->>Action: ioc_collection_id = arguments.get("ioc_collection_id")
Action->>Action: indicator_type = arguments.get("indicator_type")
Action->>Action: valid_for = int(arguments.get("valid_for", 0))
Action->>Action: result_indicators = indicators or [single_indicator]
alt indicator_type is IP address
alt indicators is list or single_indicator is set
Action->>Action: add_IP_action(result_indicators, ioc_collection_id, valid_for)
else invalid indicators configuration
Action->>Action: raise ValueError
end
else other indicator_type
alt indicator_type is supported
Action->>Action: perform_request(result_indicators, ioc_collection_id, mapped_type, valid_for)
else unsupported indicator_type
Action->>Action: error(Improper indicator type)
end
end
deactivate Action
Class diagram for AddIOCtoIOCCollectionAction accepting single indicatorclassDiagram
class InThreatBaseAction {
}
class AddIOCtoIOCCollectionAction {
+perform_request(indicators, ioc_collection_id, indicator_type, valid_for)
+add_IP_action(indicators, ioc_collection_id, valid_for)
+run(arguments)
}
InThreatBaseAction <|-- AddIOCtoIOCCollectionAction
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:
- When neither
indicatorsnorindicatoris provided,result_indicators = indicators or [single_indicator]will produce[None]and for non-IP addresstypes this is passed through toperform_requestwithout validation; consider explicitly checking for the absence of both inputs and raising a clear error instead of sending aNoneindicator. - You currently build
result_indicatorsbefore validating the IP-address-specific constraints; consider moving or duplicating the validation so that you don't construct a[single_indicator]list whensingle_indicatoris falsy, which can help avoid accidental propagation ofNonevalues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When neither `indicators` nor `indicator` is provided, `result_indicators = indicators or [single_indicator]` will produce `[None]` and for non-`IP address` types this is passed through to `perform_request` without validation; consider explicitly checking for the absence of both inputs and raising a clear error instead of sending a `None` indicator.
- You currently build `result_indicators` before validating the IP-address-specific constraints; consider moving or duplicating the validation so that you don't construct a `[single_indicator]` list when `single_indicator` is falsy, which can help avoid accidental propagation of `None` values.
## Individual Comments
### Comment 1
<location path="Sekoia.io/sekoiaio/intelligence_center/add_ioc_to_ioc_collection.py" line_range="61" />
<code_context>
indicator_type = arguments.get("indicator_type")
valid_for = int(arguments.get("valid_for", 0))
+ result_indicators = indicators or [single_indicator]
+
if str(indicator_type) == "IP address":
</code_context>
<issue_to_address>
**issue:** Guard against both `indicators` and `single_indicator` being missing to avoid passing `[None]` downstream.
When both are absent, `result_indicators` becomes `[None]` and is passed to `add_IP_action`/`perform_request`, likely causing confusing downstream errors. Add an explicit guard, for example:
```python
if not indicators and not single_indicator:
raise ValueError("You must provide either 'indicators' or 'indicator'")
result_indicators = indicators or [single_indicator]
```
</issue_to_address>
### Comment 2
<location path="Sekoia.io/sekoiaio/intelligence_center/add_ioc_to_ioc_collection.py" line_range="63-67" />
<code_context>
if str(indicator_type) == "IP address":
- if not isinstance(indicators, list):
- raise ValueError("Indicators should be list type")
+ if not isinstance(indicators, list) and not single_indicator:
+ raise ValueError("Indicators should be list type, or you should provide a single indicator value")
- self.add_IP_action(indicators, ioc_collection_id, valid_for)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align validation error message with the actual accepted shapes and ensure list type is also enforced when a single indicator is supplied as a list.
The current condition (`not isinstance(indicators, list) and not single_indicator`) permits non-list `indicators` whenever `single_indicator` is set, which conflicts with the message and weakens type guarantees. If `indicators` should always be a list when present, you could instead enforce:
```python
if indicators is not None and not isinstance(indicators, list):
raise ValueError("'indicators' must be a list; alternatively, use 'indicator' for a single value")
```
Then rely on the normalization of `result_indicators` to handle cases where both fields are provided.
```suggestion
if str(indicator_type) == "IP address":
if indicators is not None and not isinstance(indicators, list):
raise ValueError("'indicators' must be a list; alternatively, use 'indicator' for a single value")
self.add_IP_action(result_indicators, ioc_collection_id, valid_for)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| indicator_type = arguments.get("indicator_type") | ||
| valid_for = int(arguments.get("valid_for", 0)) | ||
|
|
||
| result_indicators = indicators or [single_indicator] |
Contributor
There was a problem hiding this comment.
issue: Guard against both indicators and single_indicator being missing to avoid passing [None] downstream.
When both are absent, result_indicators becomes [None] and is passed to add_IP_action/perform_request, likely causing confusing downstream errors. Add an explicit guard, for example:
if not indicators and not single_indicator:
raise ValueError("You must provide either 'indicators' or 'indicator'")
result_indicators = indicators or [single_indicator]
Comment on lines
63
to
+67
| if str(indicator_type) == "IP address": | ||
| if not isinstance(indicators, list): | ||
| raise ValueError("Indicators should be list type") | ||
| if not isinstance(indicators, list) and not single_indicator: | ||
| raise ValueError("Indicators should be list type, or you should provide a single indicator value") | ||
|
|
||
| self.add_IP_action(indicators, ioc_collection_id, valid_for) | ||
| self.add_IP_action(result_indicators, ioc_collection_id, valid_for) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Align validation error message with the actual accepted shapes and ensure list type is also enforced when a single indicator is supplied as a list.
The current condition (not isinstance(indicators, list) and not single_indicator) permits non-list indicators whenever single_indicator is set, which conflicts with the message and weakens type guarantees. If indicators should always be a list when present, you could instead enforce:
if indicators is not None and not isinstance(indicators, list):
raise ValueError("'indicators' must be a list; alternatively, use 'indicator' for a single value")Then rely on the normalization of result_indicators to handle cases where both fields are provided.
Suggested change
| if str(indicator_type) == "IP address": | |
| if not isinstance(indicators, list): | |
| raise ValueError("Indicators should be list type") | |
| if not isinstance(indicators, list) and not single_indicator: | |
| raise ValueError("Indicators should be list type, or you should provide a single indicator value") | |
| self.add_IP_action(indicators, ioc_collection_id, valid_for) | |
| self.add_IP_action(result_indicators, ioc_collection_id, valid_for) | |
| if str(indicator_type) == "IP address": | |
| if indicators is not None and not isinstance(indicators, list): | |
| raise ValueError("'indicators' must be a list; alternatively, use 'indicator' for a single value") | |
| self.add_IP_action(result_indicators, ioc_collection_id, valid_for) |
mchupeau-sk
reviewed
Mar 9, 2026
Contributor
mchupeau-sk
left a comment
mchupeau-sk
left a comment
There was a problem hiding this comment.
Ok for me but need to rebase with develop before
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
2 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.
Relates to
Summary by Sourcery
Allow adding a single indicator value to an IOC collection in addition to the existing list-based input and update metadata accordingly.
New Features:
indicatorfield to AddIOCtoIOCCollectionAction alongside the existingindicatorslist parameter.Enhancements:
indicatorsarray or a singleindicatorvalue and validate presence accordingly.Documentation:
indicatorfield for AddIOCtoIOCCollectionAction in the changelog.Tests:
valid_for, and error behavior when no indicators are provided.Chores: