Fix nested flow logging - #812
Conversation
📝 WalkthroughWalkthroughThis PR refactors network flow logging from per-flow serialization to a batched process-flows model. Forward declarations are updated from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Source/santad/Logs/EndpointSecurity/Serializers/BasicString.mm (1)
1072-1079:window_startandwindow_endare unused in this serializer.These parameters are required by the virtual interface, but this implementation silently ignores them. If the BasicString format intentionally omits time-window data this is fine, but consider whether consumers expect time-window information in the basic string output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/santad/Logs/EndpointSecurity/Serializers/BasicString.mm` around lines 1072 - 1079, The SerializeNetworkFlows implementation in BasicString::SerializeNetworkFlows currently ignores the window_start and window_end parameters; either explicitly acknowledge them or include their data. Fix by updating BasicString::SerializeNetworkFlows: if the basic string should not include time windows, mark the parameters as intentionally unused (e.g., (void)window_start; (void)window_end;) and add a clarifying comment; otherwise format and append the time-window values (using an existing helper or by converting window_start/window_end to a string) before or after the santanetd::FormatNetworkFlowsBasicString output so CreateDefaultString -> santanetd::FormatNetworkFlowsBasicString -> FinalizeString produce the expected output. Ensure changes are inside SerializeNetworkFlows to keep signature unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Source/santad/Logs/EndpointSecurity/Serializers/BasicString.mm`:
- Around line 1072-1079: The SerializeNetworkFlows implementation in
BasicString::SerializeNetworkFlows currently ignores the window_start and
window_end parameters; either explicitly acknowledge them or include their data.
Fix by updating BasicString::SerializeNetworkFlows: if the basic string should
not include time windows, mark the parameters as intentionally unused (e.g.,
(void)window_start; (void)window_end;) and add a clarifying comment; otherwise
format and append the time-window values (using an existing helper or by
converting window_start/window_end to a string) before or after the
santanetd::FormatNetworkFlowsBasicString output so CreateDefaultString ->
santanetd::FormatNetworkFlowsBasicString -> FinalizeString produce the expected
output. Ensure changes are inside SerializeNetworkFlows to keep signature
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
Source/common/SNTConfigurator.hSource/santad/BUILDSource/santad/Logs/EndpointSecurity/Logger.hSource/santad/Logs/EndpointSecurity/Logger.mmSource/santad/Logs/EndpointSecurity/Serializers/BasicString.hSource/santad/Logs/EndpointSecurity/Serializers/BasicString.mmSource/santad/Logs/EndpointSecurity/Serializers/Empty.hSource/santad/Logs/EndpointSecurity/Serializers/Empty.mmSource/santad/Logs/EndpointSecurity/Serializers/Protobuf.hSource/santad/Logs/EndpointSecurity/Serializers/Protobuf.mmSource/santad/Logs/EndpointSecurity/Serializers/Serializer.hSource/santad/Logs/EndpointSecurity/Serializers/Serializer.mmSource/santad/SNTNetworkExtensionQueue.mmstubs/santanetd/src/santanetd/SNDNetworkFlowsSerializer.hstubs/santanetd/src/santanetd/SNDNetworkFlowsSerializer.mm
Fixes issue with flow logging where per-process flows were not being collapsed into the nested proto structure.
Part of SNT-261