test(eventviewerx): ✅ guard known-field string literals in rules#241
Conversation
|
Claude finished @PrzemyslawKlys's task —— View job Code Review AnalysisReviewing the merged PR for code quality, best practices, potential issues, and test coverage. Todo List:
Review SummaryThis PR adds excellent static analysis tests that enforce coding standards for rule source files. The implementation demonstrates strong understanding of defensive programming principles. ✅ Code Quality & Best PracticesStrengths:
Minor suggestions:
✅ Test Coverage & EffectivenessComprehensive rule enforcement:
Strong implementation details:
✅ Performance ConsiderationsEfficient approach:
Potential optimizations:
✅ Security ConcernsNo security issues identified:
✅ Architecture AlignmentExcellent integration with existing codebase:
🔍 ObservationsTest vs Implementation sync: Example enforcement: // ❌ Blocked by tests
e.GetDataValueOrEmpty("SubjectUserName")
e.TryGetDataValue("TargetSid", out var sid)
// ✅ Enforced pattern
e.GetDataValueOrEmpty(KnownEventField.SubjectUserName)
e.TryGetDataValue(KnownEventField.TargetSid, out var sid)Final AssessmentThis is a well-implemented static analysis test suite that effectively enforces coding standards and prevents common anti-patterns. The code quality is high, with no security concerns and good performance characteristics. The tests provide excellent coverage for their intended purpose of maintaining code consistency across rule implementations. Rating: ✅ Excellent - Ready for production with strong defensive programming practices. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a010f93fa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| public class TestRuleSourceGuards | ||
| { | ||
| private static readonly IReadOnlyList<string> KnownFieldKeys = new[] |
There was a problem hiding this comment.
The new guard hard-codes KnownFieldKeys but omits existing KnownEventField members (Message, NoNameA0, NoNameA1, and TextPayload from Sources/EventViewerX/Definitions/KnownEventField.cs), so string-literal access to those canonical fields is not detected. I verified this gap by searching current rules, which already contain calls like GetValueFromDataDictionary("NoNameA0") and GetValueFromDataDictionary("#text"); this test still passes despite those known-field literals.
Useful? React with 👍 / 👎.
| { | ||
| string knownKeysPattern = string.Join("|", KnownFieldKeys.Select(Regex.Escape)); | ||
| Regex knownFieldLiteralPattern = new( | ||
| $@"\b(?:GetValueFromDataDictionary|GetDataValueOrEmpty|TryGetDataValue|TryGetDataEnum)\s*\([^)]*""(?:{knownKeysPattern})""", |
There was a problem hiding this comment.
The pattern \s*\([^)]*"..." is applied to one line at a time, so it only catches calls where the method name and string literal are on the same line. A multiline form (for example, GetDataValueOrEmpty( on one line and the known key string on the next) will bypass this test entirely, which makes the new guard easy to evade through formatting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds an additional guard test in the EventViewerX test suite to prevent rule source files from using string literals for “known” event field keys when calling EventObject accessor methods (encouraging the KnownEventField enum instead).
Changes:
- Add
KnownFieldKeyslist for known event-field key strings. - Add a new
[Fact]that searches rule source code for accessor calls using those keys as string literals. - Add
System.Linqfor building the dynamic regex pattern.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| string knownKeysPattern = string.Join("|", KnownFieldKeys.Select(Regex.Escape)); | ||
| Regex knownFieldLiteralPattern = new( | ||
| $@"\b(?:GetValueFromDataDictionary|GetDataValueOrEmpty|TryGetDataValue|TryGetDataEnum)\s*\([^)]*""(?:{knownKeysPattern})""", |
There was a problem hiding this comment.
The offender detection only tests each source line individually (FindRuleSourceOffenders uses File.ReadAllLines + pattern.IsMatch(line)), but this regex requires the method call and the string literal to appear on the same line. Multi-line accessor calls won’t be detected (e.g., Sources/EventViewerX/Rules/AzureAD/AADConnectStagingDisabled.cs uses GetValueFromDataDictionary(\n "SubjectUserName", ...)). Consider scanning the full file text (or buffering a statement until ')' / ';') so the guard also catches multi-line invocations, while still reporting an accurate line number for the match.
| $@"\b(?:GetValueFromDataDictionary|GetDataValueOrEmpty|TryGetDataValue|TryGetDataEnum)\s*\([^)]*""(?:{knownKeysPattern})""", | |
| $@"""(?:{knownKeysPattern})""", |
| private static readonly IReadOnlyList<string> KnownFieldKeys = new[] | ||
| { | ||
| "SubjectUserName", | ||
| "SubjectDomainName", | ||
| "SubjectLogonId", | ||
| "TargetUserName", | ||
| "TargetDomainName", | ||
| "TargetSid", | ||
| "WorkstationName", | ||
| "IpAddress", | ||
| "IpPort", | ||
| "LogonType", | ||
| "Status", | ||
| "SubStatus", | ||
| "FailureReason", | ||
| "AuthenticationPackageName", | ||
| "LogonProcessName", | ||
| "LmPackageName", | ||
| "KeyLength", | ||
| "ProcessId", | ||
| "ProcessName", | ||
| "TransmittedServices", | ||
| "TicketOptions", | ||
| "TicketEncryptionType", | ||
| "PreAuthType" | ||
| }; |
There was a problem hiding this comment.
KnownFieldKeys duplicates KnownEventField values but is manually maintained and currently omits enum-backed keys like Message/NoNameA0/NoNameA1 and the TextPayload mapping ("#text"). This can lead to false negatives (e.g., Rules/RuleHelpers.cs uses "NoNameA0" and "#text" as string literals). To avoid drift, build the key set from KnownEventField (Enum.GetValues/Enum.GetNames) and include the special ToEventFieldKey mapping for TextPayload.
No description provided.