test: exercise StackRox AI review on process signals - #4001
robbycochran wants to merge 24 commits into
Conversation
robbycochran
added
the
stackrox-ai-review
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![stackrox-openshell-workflow[bot]](https://avatars.githubusercontent.com/u/40638982?s=60&v=4){kind=small}
|
|
||
| std::string ProcessSignalPublisher::ProcessKey(const storage::ProcessSignal& signal) { | ||
| std::stringstream key; | ||
| key << signal.name() << " " << signal.exec_file_path(); |
There was a problem hiding this comment.
The new rate-limiting key is significantly less specific than the one it replaces. It no longer includes the container ID or process arguments. This means that processes with the same name and path but in different containers, or with different arguments, will be treated as identical for rate-limiting purposes. This can lead to the silent dropping of important process signals, causing a loss of security visibility.
|
|
||
| namespace collector { | ||
|
|
||
| std::string ProcessSignalPublisher::ProcessKey(const storage::ProcessSignal& signal) { |
There was a problem hiding this comment.
The new rate-limiting key is less specific than the one it replaces. The old key included the container ID and process arguments. By only using the process name and exec path, the rate limiter may now incorrectly drop signals for distinct processes, such as the same command run with different arguments or in different containers. This could lead to a loss of security visibility.
| on: | ||
| # Demonstration only. Production should use pull_request_target after the | ||
| # trusted caller workflow and review skill are on the default branch. | ||
| pull_request: |
There was a problem hiding this comment.
Using pull_request is the correct, secure trigger for this workflow. The comment on lines 4-5 suggests using pull_request_target in production, which would create a major security vulnerability. Workflows triggered by pull_request_target can expose secrets to untrusted code from PRs. This pattern should not be adopted, and the comment should be updated to reflect the security implications.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4001 +/- ##
==========================================
+ Coverage 27.43% 27.48% +0.05%
==========================================
Files 94 96 +2
Lines 5406 5403 -3
Branches 2535 2529 -6
==========================================
+ Hits 1483 1485 +2
+ Misses 3198 3192 -6
- Partials 725 726 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
|
||
| namespace collector { | ||
|
|
||
| std::string ProcessSignalPublisher::ProcessKey(const storage::ProcessSignal& signal) { |
There was a problem hiding this comment.
The new rate-limiting key is generated from only the process name and its executable path. The previous implementation also included the container ID and process arguments. This change significantly weakens the uniqueness of the key, causing processes with the same name and path to be incorrectly grouped together for rate-limiting, even if they are in different containers or have different arguments. This could cause legitimate process signals to be dropped, which can mask suspicious activity.
|
|
||
| std::string ProcessSignalPublisher::ProcessKey(const storage::ProcessSignal& signal) { | ||
| std::stringstream key; | ||
| key << signal.name() << " " << signal.exec_file_path(); |
There was a problem hiding this comment.
The rate-limiting key is now generated from only the process name and executable path. The previous implementation also included the container ID and process arguments. This change in the key composition can lead to incorrect rate-limiting behavior. For instance, processes with the same name and path but running in different containers or with different arguments will now be treated as the same by the rate limiter. This could cause important security signals from one process to be dropped because of noise from another, effectively reducing process visibility.
2 participants
Summary
stackrox-ai-reviewThis PR is intentionally for review testing and must not be merged. The workflow uses
pull_requestso the pinned Harness feature revision can be exercised before any production-default-branch rollout.Validation
git diff --check@coderabbitai ignore