common: add SNTNetworkFlowRule wire model class#965
Merged
Conversation
github-actions
Bot
added
lang/objc++
Contributor
📝 WalkthroughWalkthroughAdds SNTNetworkFlowRule: a new Objective-C model with an enum state, NSSecureCoding implementation, Bazel build/test targets, and unit tests covering initialization, secure-archiving round trips, proto-blob copy semantics, and decode rejection for invalid state. ChangesSNTNetworkFlowRule Model Class
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/common/BUILD`:
- Around line 696-702: The new test target SNTNetworkFlowRuleTest is declared
but not included in the test_suite that aggregates unit tests; add
"SNTNetworkFlowRuleTest" to the unit_tests test_suite list (the test_suite that
collects other santa_unit_test targets) so it runs with the aggregate unit test
target—update the unit_tests suite declaration to include the
SNTNetworkFlowRuleTest symbol alongside the other test target entries.
In `@Source/common/SNTNetworkFlowRule.mm`:
- Around line 27-35: Validate the state/blob combinations at the start of both
initializers (initWithRuleId:state:protoBlob and the other initializer around
lines 49–55) and reject invalid tuples: disallow
SNTNetworkFlowRuleStateUnspecified, disallow SNTNetworkFlowRuleStateAdd when
protoBlob is nil/empty, and disallow SNTNetworkFlowRuleStateRemove when
protoBlob is non-nil/non-empty; if validation fails, return nil (or
NSParameterAssert/log and return nil) instead of assigning
_ruleId/_state/_protoBlob so malformed tuples never get stored on the instance.
In `@Source/common/SNTNetworkFlowRuleTest.mm`:
- Around line 36-38: The multiline Objective-C initializer for
SNTNetworkFlowRule (the SNTNetworkFlowRule* rule assignment using
initWithRuleId: state: protoBlob:) is misformatted for clang-format; reformat
both occurrences (the one at the top and the one around lines 67–69) to follow
the project's clang-format Objective‑C style: put the '[' and class/alloc on the
first line and align the selector arguments consistently (each selector/argument
pair aligned per style) so the initializer is a single well‑formatted block that
passes lint/clang‑format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1a666f1-893b-4dfa-9e21-522098ea1ab1
📒 Files selected for processing (4)
Source/common/BUILDSource/common/SNTNetworkFlowRule.hSource/common/SNTNetworkFlowRule.mmSource/common/SNTNetworkFlowRuleTest.mm
NSSecureCoding wrapper that carries a serialized NetworkFlowRule.Add proto blob (or just a rule_id for Remove) across XPC and into SNTRuleTable. Mirrors the SNTFileAccessRule pattern; uses the ENCODE_/DECODE_ macros from CoderMacros.h for archival.
mlw
force-pushed
the
mlw/network-rules-common
branch
from
165d5e5 to
0239d30
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Source/common/SNTNetworkFlowRule.mm (1)
27-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce state/blob invariants in both initializers.
The class currently accepts/decodes invalid tuples that violate its own contract (
Unspecified,Addwithout blob,Removewith blob). Reject invalid inputs before assignment (and after decode) so malformed wire objects are never constructed.Suggested fix
`@implementation` SNTNetworkFlowRule +- (BOOL)isValidState:(SNTNetworkFlowRuleState)state protoBlob:(NSData*)protoBlob { + if (state != SNTNetworkFlowRuleStateAdd && state != SNTNetworkFlowRuleStateRemove) return NO; + if (state == SNTNetworkFlowRuleStateAdd && !protoBlob) return NO; + if (state == SNTNetworkFlowRuleStateRemove && protoBlob) return NO; + return YES; +} + - (instancetype)initWithRuleId:(int64_t)ruleId state:(SNTNetworkFlowRuleState)state protoBlob:(NSData*)protoBlob { self = [super init]; - if (self) { - _ruleId = ruleId; - _state = state; - _protoBlob = [protoBlob copy]; - } + if (!self) return nil; + if (![self isValidState:state protoBlob:protoBlob]) return nil; + + _ruleId = ruleId; + _state = state; + _protoBlob = [protoBlob copy]; return self; } @@ - (instancetype)initWithCoder:(NSCoder*)decoder { self = [super init]; - if (self) { - DECODE_SELECTOR(decoder, ruleId, NSNumber, longLongValue); - DECODE_SELECTOR(decoder, state, NSNumber, integerValue); - DECODE(decoder, protoBlob, NSData); - } + if (!self) return nil; + DECODE_SELECTOR(decoder, ruleId, NSNumber, longLongValue); + DECODE_SELECTOR(decoder, state, NSNumber, integerValue); + DECODE(decoder, protoBlob, NSData); + if (![self isValidState:_state protoBlob:_protoBlob]) return nil; return self; }Also applies to: 49-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/common/SNTNetworkFlowRule.mm` around lines 27 - 35, Validate and enforce the state/blob invariants in both initializers: in -initWithRuleId:state:protoBlob: and in the decoder/other initializer that constructs from wire payload, reject any tuple where state == SNTNetworkFlowRuleStateUnspecified or where state == SNTNetworkFlowRuleStateAdd but protoBlob is nil/empty or state == SNTNetworkFlowRuleStateRemove but protoBlob is non-nil/non-empty; implement checks before assigning ivars and return nil (failing initializer) when invariants are violated, otherwise proceed to copy/assign _ruleId, _state, and _protoBlob as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Source/common/SNTNetworkFlowRule.mm`:
- Around line 27-35: Validate and enforce the state/blob invariants in both
initializers: in -initWithRuleId:state:protoBlob: and in the decoder/other
initializer that constructs from wire payload, reject any tuple where state ==
SNTNetworkFlowRuleStateUnspecified or where state == SNTNetworkFlowRuleStateAdd
but protoBlob is nil/empty or state == SNTNetworkFlowRuleStateRemove but
protoBlob is non-nil/non-empty; implement checks before assigning ivars and
return nil (failing initializer) when invariants are violated, otherwise proceed
to copy/assign _ruleId, _state, and _protoBlob as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c2a5248-c3a0-463a-a89a-3730fa9f6f42
📒 Files selected for processing (4)
Source/common/BUILDSource/common/SNTNetworkFlowRule.hSource/common/SNTNetworkFlowRule.mmSource/common/SNTNetworkFlowRuleTest.mm
Replaces the combined initWithRuleId:state:protoBlob: with two named factories — initAddRuleWithId:protoBlob: and initRemoveRuleWithId: — mirroring SNTFileAccessRule's pattern. Invalid combinations (Add-without-blob, Remove-with-blob, Unspecified state) are now unrepresentable through the public API. initWithCoder: also rejects an out-of-range state from a corrupted archive. Adds tests for the new rejection paths.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Source/common/SNTNetworkFlowRule.mm (1)
27-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject decoded state/blob mismatches to preserve wire-model invariants.
-initWithCoder:still accepts invalid tuples (Addwithnilblob,Removewith non-nilblob). That permits malformed archived input to instantiate contract-breaking objects.Suggested fix
- (instancetype)initWithState:(SNTNetworkFlowRuleState)state ruleId:(int64_t)ruleId protoBlob:(NSData*)protoBlob { if (state != SNTNetworkFlowRuleStateAdd && state != SNTNetworkFlowRuleStateRemove) { return nil; } + if ((state == SNTNetworkFlowRuleStateAdd && !protoBlob) || + (state == SNTNetworkFlowRuleStateRemove && protoBlob)) { + return nil; + } self = [super init]; if (self) { _state = state; @@ - (instancetype)initWithCoder:(NSCoder*)decoder { self = [super init]; if (self) { @@ if (_state != SNTNetworkFlowRuleStateAdd && _state != SNTNetworkFlowRuleStateRemove) { return nil; } + if ((_state == SNTNetworkFlowRuleStateAdd && !_protoBlob) || + (_state == SNTNetworkFlowRuleStateRemove && _protoBlob)) { + return nil; + } } return self; }Also applies to: 63-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/common/SNTNetworkFlowRule.mm` around lines 27 - 31, The init methods currently allow invalid state/blob combinations; update -initWithState:ruleId:protoBlob: to validate that SNTNetworkFlowRuleStateAdd always has a non-nil protoBlob and SNTNetworkFlowRuleStateRemove always has a nil protoBlob (return nil if this invariant is violated), and likewise update -initWithCoder: to decode state and protoBlob then reject and return nil when the decoded tuple is invalid; apply the same validation to the other initializer variant referenced in the diff so malformed archived input cannot instantiate contract-breaking objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Source/common/SNTNetworkFlowRule.mm`:
- Around line 27-31: The init methods currently allow invalid state/blob
combinations; update -initWithState:ruleId:protoBlob: to validate that
SNTNetworkFlowRuleStateAdd always has a non-nil protoBlob and
SNTNetworkFlowRuleStateRemove always has a nil protoBlob (return nil if this
invariant is violated), and likewise update -initWithCoder: to decode state and
protoBlob then reject and return nil when the decoded tuple is invalid; apply
the same validation to the other initializer variant referenced in the diff so
malformed archived input cannot instantiate contract-breaking objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e898b57-f480-4465-9112-6cb9aefbf270
📒 Files selected for processing (3)
Source/common/SNTNetworkFlowRule.hSource/common/SNTNetworkFlowRule.mmSource/common/SNTNetworkFlowRuleTest.mm
4 tasks
russellhancox
approved these changes
May 26, 2026
mlw
added a commit
that referenced
this pull request
May 26, 2026
## Summary - Adds a third sub-table `network_flow_rules` (migration v12 → v13) holding raw `NetworkFlowRule.Add` proto blobs keyed by `rule_id`. - Extends `addExecutionRules:fileAccessRules:…` to also take `networkFlowRules:`, so all three rule types from a single `RuleDownloadResponse` apply in one transaction. The 3-arg legacy wrapper is preserved for the compiler/standalone callers. - Adds `networkFlowRulesHash` to `SNTRuleTableRulesHash`, populated via `hashOfHashes`. Xxhash128 over the blobs in `rule_id ASC` order — `rule_id` is field 1 of the proto so it's not hashed separately. - Retrieve and hash loops use indexed-column FMDB access; enterprise blocklists are expected to reach 100k+ rules. ## Dependencies **Stacked on #965** (`mlw/network-rules-common`, currently in review). This PR's base is that branch — please merge #965 first; GitHub will retarget this PR to `main` automatically. The two are intended to stand independently in review. XPC delivery to `santanetd` and the corresponding `SNTDaemonControlController` entry are out of scope here and will follow in a later PR. ## Test plan - [x] `bazel test //Source/santad:SNTRuleTableTest` — 16 new tests covering persist / upsert / remove / remove-nonexistent / CleanAll / CleanNonTransitive / CleanupExecutionRules-preserves / empty-array rejection / network-only non-empty guard / hash stability / hex length / blob sensitivity / order independence / empty-table. - [x] `bazel test //Source/santad:SNTDaemonControlControllerTest //Source/santad:SNTCompilerControllerTest //Source/santad:SNTExecutionControllerTest` — existing tests still pass with the 5-arg signature. - [x] `bazel build //Source/santad:Santad` - [x] `Testing/check_test_suites.sh` clean.
This was referenced May 26, 2026
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.
NSSecureCoding wrapper that carries a serialized NetworkFlowRule.Add proto blob (or just a rule_id for Remove) across XPC and into SNTRuleTable. Mirrors the SNTFileAccessRule pattern.
This PR is only for the new wire wrapper, it is not yet hooked up to anything.