Add security rules for hard-coded secrets and error handling in C#, Java, and Kotlin #104
Conversation
|
Sakshis seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis pull request introduces several new security rules across C#, Java, and Kotlin to enhance the detection of hard-coded secrets and improper error handling practices. Specifically, it adds rules to prevent stack trace disclosures in production environments and to identify hard-coded credentials in Java and Kotlin applications. Additionally, it includes corresponding test configurations to validate these rules, ensuring that developers adhere to best practices for security and sensitive data management. Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
rules/csharp/security/stacktrace-disclosure-csharp.yml (1)
13-24: Enhance pattern matching to catch additional casesThe current pattern only checks for direct
IsDevelopment()calls. Consider enhancing it to catch:
- Negated production checks (
!IsProduction())- Environment variable comparisons (
ASPNETCORE_ENVIRONMENT == "Development")utils: $APP.UseDeveloperExceptionPage(...): kind: expression_statement pattern: $APP.UseDeveloperExceptionPage($$$); inside: stopBy: neighbor kind: block not: follows: stopBy: end kind: invocation_expression - pattern: $ENV.IsDevelopment() + patterns: + - $ENV.IsDevelopment() + - !$ENV.IsProduction() + - $ENV == "Development"🧰 Tools
🪛 yamllint (1.35.1)
[warning] 15-15: wrong indentation: expected 8 but found 5
(indentation)
[warning] 21-21: wrong indentation: expected 13 but found 10
(indentation)
[warning] 22-22: wrong indentation: expected 14 but found 12
(indentation)
tests/csharp/stacktrace-disclosure-csharp-test.yml (1)
8-38: Add missing test casesConsider adding these common scenarios to invalid test cases:
- Environment variable comparison
- Custom environment names
- Nested conditions
- | if (Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT") == "Development") { app.UseDeveloperExceptionPage(); } - | if (env.EnvironmentName == "LocalDev") { app.UseDeveloperExceptionPage(); } - | if (isDebugging) if (!isProd) { app.UseDeveloperExceptionPage(); }🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
tests/__snapshots__/stacktrace-disclosure-csharp-snapshot.yml (2)
1-72: Fix formatting inconsistenciesThere are several formatting issues in the snapshots:
- Inconsistent indentation (e.g., lines 31, 68)
- Trailing spaces in code blocks
- Inconsistent newline usage
Consider running a formatter to ensure consistent style across all snapshots.
1-72: Security Implementation ReviewOverall, this security rule implementation effectively addresses CWE-209 by preventing stack trace disclosure in production. However, consider these security-focused improvements:
- Elevate rule severity to ERROR to prevent accidental merges
- Add pattern matching for environment variable comparisons
- Fix the valid test case to properly demonstrate production error handling
- Add test cases for nested conditions and custom environment names
These changes will strengthen the security posture by catching more potential vulnerabilities.
rules/kotlin/security/jwt-hardcode-kotlin.yml (1)
34-34: Consider including HMAC512 in the primary pattern match.The regex pattern
^(HMAC256$|HMAC384)$excludes HMAC512, which is handled separately. Consider consolidating all HMAC algorithms into a single pattern for better maintainability.- regex: ^(HMAC256$|HMAC384)$ + regex: ^(HMAC256$|HMAC384$|HMAC512$)$rules/java/security/hardcoded-secret-in-credentials-java.yml (1)
16-84: Enhance credential pattern matchingThe current pattern might miss some variations of credential creation. Consider:
- Adding patterns for common credential-related method names (e.g.,
createCredentials,getBasicAuth)- Adding validation for obviously fake or test credentials (e.g., "test", "password123")
- has: stopBy: end kind: identifier field: name - regex: "^basic$" + regex: "^(basic|createCredentials|getBasicAuth)$" - has: kind: string_literal pattern: $STRING + not: + regex: "^(test|password|password123|admin|root)$"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
rules/csharp/security/stacktrace-disclosure-csharp.yml(1 hunks)rules/java/security/hardcoded-secret-in-credentials-java.yml(1 hunks)rules/kotlin/security/jwt-hardcode-kotlin.yml(1 hunks)tests/__snapshots__/hardcoded-secret-in-credentials-java-snapshot.yml(1 hunks)tests/__snapshots__/jwt-hardcode-kotlin-snapshot.yml(1 hunks)tests/__snapshots__/stacktrace-disclosure-csharp-snapshot.yml(1 hunks)tests/csharp/stacktrace-disclosure-csharp-test.yml(1 hunks)tests/java/hardcoded-secret-in-credentials-java-test.yml(1 hunks)tests/kotlin/jwt-hardcode-kotlin-test.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/csharp/stacktrace-disclosure-csharp-test.yml
[error] 6-6: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
[warning] 39-39: too many blank lines
(1 > 0) (empty-lines)
rules/csharp/security/stacktrace-disclosure-csharp.yml
[warning] 15-15: wrong indentation: expected 8 but found 5
(indentation)
[warning] 21-21: wrong indentation: expected 13 but found 10
(indentation)
[warning] 22-22: wrong indentation: expected 14 but found 12
(indentation)
[error] 27-27: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (5)
rules/kotlin/security/jwt-hardcode-kotlin.yml (1)
1-15: LGTM! Well-structured rule definition with comprehensive documentation.
The rule configuration includes:
- Clear severity level
- Detailed explanation of the security risk
- Appropriate references to CWE-798 and OWASP A03:2021
tests/kotlin/jwt-hardcode-kotlin-test.yml (1)
4-5: LGTM! Secure handling of system properties.
The valid test cases correctly demonstrate using configuration variables instead of hardcoded values for sensitive system properties.
tests/__snapshots__/jwt-hardcode-kotlin-snapshot.yml (1)
1-124: LGTM! Comprehensive snapshot with detailed labels.
The snapshot file correctly captures:
- Code structure with imports and implementation
- Detailed labels for pattern matching
- Consistent with test cases
rules/java/security/hardcoded-secret-in-credentials-java.yml (1)
85-189: Simplify pattern matching logic
The current pattern matching is complex and might be fragile. Consider:
- Simplifying the nested structure to improve maintainability
- Adding patterns for additional credential creation patterns:
- Builder pattern (
CredentialsBuilder) - Factory methods (
CredentialsFactory) - Static utility methods
- Builder pattern (
tests/__snapshots__/hardcoded-secret-in-credentials-java-snapshot.yml (1)
1-83: LGTM: Snapshot structure is well-defined
The snapshot correctly captures:
- Source code segments
- Style information for syntax highlighting
- Position information for precise error reporting
Summary by CodeRabbit
New Features
Tests
Documentation