Add C# JWT rules: enforce token verification and no hardcoded secrets #195
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 two new security rules for C# JWT handling. The first rule, defined in Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Analyzer as Static Analyzer
participant DecodeRule as JWT Decode Rule
Dev->>Analyzer: Submits C# code with JWT decoding call (missing verification)
Analyzer->>DecodeRule: Analyze AST for verification flag
DecodeRule-->>Analyzer: Return warning if verification is absent
Analyzer-->>Dev: Report JWT decode without verification warning
sequenceDiagram
participant Dev as Developer
participant Analyzer as Static Analyzer
participant HardcodedRule as JWT Hardcoded Secret Rule
Dev->>Analyzer: Submits C# code with a hardcoded secret in JWT method
Analyzer->>HardcodedRule: Analyze AST for hardcoded string usage
HardcodedRule-->>Analyzer: Return warning for hardcoded secret
Analyzer-->>Dev: Report hardcoded secret detection warning
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: 1
🧹 Nitpick comments (17)
rules/csharp/security/jwt-decode-without-verify-csharp.yml (2)
1-13: Metadata and YAML Structure
The rule’s metadata (ID, severity, language, message, and note) is clear and well structured. However, YAMLlint reported trailing spaces on line 8. Please remove the extra whitespace at the end of line 8 to ensure compliance with YAML style guidelines.
16-373: Comprehensive AST Pattern Definitions in the "utils" Block
Theutils:section defines multiple AST pattern matchers to detect various invocations of JWT decoding without verification. The patterns cover method calls such as(IJwtDecoder $D).Decode($X,verify-false,.),($D).Decode(false), and other variants. This comprehensive approach will help catch diverse coding styles.
Please verify that these patterns match all intended use cases in your C# codebase. Consider adding inline comments to explain non‑obvious pattern choices for future maintainability.tests/__snapshots__/jwt-hardcoded-secret-csharp-snapshot.yml (1)
1-469: Snapshot Tests for Hardcoded JWT Secrets
The snapshot file contains detailed examples for various JWT encoding scenarios with hardcoded secrets. The snapshots include multiple labels and secondary source references that help verify both the payload and the secret values.
Overall, the snapshots are comprehensive. Consider reviewing and possibly consolidating duplicate label entries for improved clarity in future maintenance.tests/__snapshots__/jwt-decode-without-verify-csharp-snapshot.yml (1)
1-908: Snapshot Tests for JWT Decoding Without Verification
This snapshot file is extensive and demonstrates a variety of JWT decode invocations with different parameter configurations—such as usingverify: false, passingnullfor certain parameters, and leveraging builder patterns. The snapshots offer a solid baseline for verifying that tokens are not decoded without a proper verification step.
A couple of suggestions:
- Ensure that the variables
token(andkeywhere applicable) are defined in your actual test harness so that these snapshots remain valid.- Consider adding inline comments within the snapshot code blocks to provide context for each distinct decoding pattern.
Overall, these snapshots are comprehensive and align well with the new security rule requirements.tests/csharp/jwt-decode-without-verify-csharp-test.yml (13)
1-20: Review Valid JWT Test Case:
The valid test case is structured correctly usingJwtBuilder.Create()and enforcing signature verification viaMustVerifySignature(). However, ensure that the placeholder variables (keyandtoken) are defined in the test context to avoid runtime errors. Also, consider adding assertions to validate the decoded output rather than only printing to the console.
21-40: Review Invalid Test - JwtTest7:
This test accurately simulates disabled signature verification by callingdecoder.Decode(token, verify: false). To enhance clarity and robustness, consider adding assertions that either check for an expected exception or log a warning when unsafe decoding is attempted. Additionally, a more descriptive test method name could help clarify the intent.
41-54: Review Invalid Test - JwtTest9:
JwtTest9 tests the scenario where anullsecret is passed and signature verification is disabled. Confirm that this test scenario aligns with your security rules by either expecting a specific error or behavior. Adding a comment or assertion about the expected outcome would improve clarity.
71-86: Review Test - JwtTest11:
JwtTest11 explicitly disables signature verification using.Decode(token, verify: false), which correctly simulates unsafe usage. To further strengthen the test, add assertions that validate the resulting behavior (or failure) of the decoding operation.
87-104: Review Test - JwtTest13:
This test initializesValidationParameterswith several validations disabled (including signature validation) but does not actually use them in a decode or validate call. Completing the test by invoking a decode/validation method would fully exercise the security rule.
105-120: Review Test - JwtTest15:
JwtTest15 decodes a token without explicitly specifying a signature verification flag. Clarify whether this scenario is intended to rely on default behavior (and if that default is insecure) by adding either an assertion or explanatory comment.
121-136: Review Test - JwtTest17:
JwtTest17 configuresJwtAuthenticationOptionswithVerifySignatureset to false and simply logs a message. To leverage the new security rules, consider asserting that such a configuration is detected or flagged by your analyzer, ensuring that the test validates the intended security behavior.
137-156: Review Test - JwtTest18:
This test usesTokenValidationParameterswithValidateIssuerSigningKeydisabled and then callsValidateToken. While it effectively simulates an unsafe configuration, the test would be more robust if it included assertions to confirm that the operation either produces a warning or behaves in a predictable manner under the insecure setup.
157-174: Review Test - JwtTest19 (First Instance):
JwtTest19 createsTokenValidationParameterswith signature validation turned off. This test correctly represents an unsafe configuration. Enhancing it with assertions to check for expected outcomes (e.g., a warning or error) could improve its effectiveness.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 167-167: trailing spaces
(trailing-spaces)
193-210: Review Test - JwtTest1 (First Instance):
This test decodes a token with verification disabled by using a custom decoder. While the structure is acceptable, adding assertions to validate the decoded output would enhance the test’s robustness. Also, ensure that any sensitive token data is handled appropriately.
230-245: Review Test - JwtTest2:
This test usesJwtBuilder.Create()to decode a token. To fully validate that unsafe decoding is being caught, include assertions checking the output or behavior against the expected security rule criteria.
246-262: Review Test - JwtTest3:
Similar to JwtTest2, JwtTest3 decodes a token via a JWT builder setup but only prints the output. Enhancing this test with explicit assertions would help enforce the expected behavior and align with your security testing objectives.
167-167: Style Fix - Remove Trailing Whitespace:
Trailing whitespace detected on this line. Please remove any extra spaces to adhere to YAML formatting standards.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 167-167: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
rules/csharp/security/jwt-decode-without-verify-csharp.yml(1 hunks)rules/csharp/security/jwt-hardcoded-secret-csharp.yml(1 hunks)tests/__snapshots__/jwt-decode-without-verify-csharp-snapshot.yml(1 hunks)tests/__snapshots__/jwt-hardcoded-secret-csharp-snapshot.yml(1 hunks)tests/csharp/jwt-decode-without-verify-csharp-test.yml(1 hunks)tests/csharp/jwt-hardcoded-secret-csharp-test.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/csharp/jwt-hardcoded-secret-csharp-test.yml
[error] 8-8: trailing spaces
(trailing-spaces)
tests/csharp/jwt-decode-without-verify-csharp-test.yml
[error] 167-167: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
rules/csharp/security/jwt-decode-without-verify-csharp.yml (1)
714-728: Rule Consolidation and Match Conditions
The finalrule:block aggregates the various match patterns identified earlier. The list of match patterns is logically organized and appears to cover the intended scenarios.
Ensure that the ordering and specificity of these patterns do not conflict with other rules in your suite.tests/csharp/jwt-hardcoded-secret-csharp-test.yml (1)
1-106: JWT Hardcoded Secret Test Cases
This new test file effectively separates valid and invalid JWT secret usage scenarios. The valid test (OkJwtTest6) demonstrates token creation with a secret from an environment variable, while the invalid tests intentionally use hardcoded strings.
Make sure that these tests trigger the security rule detection as expected and that the console outputs are used only for debugging purposes.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 8-8: trailing spaces
(trailing-spaces)
rules/csharp/security/jwt-hardcoded-secret-csharp.yml (2)
1-150: Rule for Hardcoded Secret Detection – Metadata and Utils Configuration
The rule’s metadata (ID, severity, language, message, and note) is clearly defined. The description appropriately highlights the risks of embedding secrets in source code.
Theutils:block provides several AST patterns for detecting hardcoded secrets in JWT encoding/decoding operations (e.g. using literal strings via the$PASSplaceholder). The patterns appear detailed and methodical.
Please ensure that these patterns continue to capture all common hardcoded secret scenarios in your projects.
673-683: Rule Definition Block Consolidation
The finalrule:block here aggregates the various AST matchers defined earlier. It logically groups the conditions using thematches:keyword, and it is consistent with the structure in the other rule file.
Double-check that the defined patterns do not produce false positives when encountering acceptable secret management (e.g. environment variables).tests/csharp/jwt-decode-without-verify-csharp-test.yml (2)
55-70: Review Test - JwtTest10:
This test invokes token decoding without an explicit verification flag. It is unclear whether the default behavior is secure or if it should be flagged by your new rules. Consider clarifying the expected outcome by either adding assertions or commenting on why this scenario is noteworthy.
175-192: Duplicate Test - JwtTest19 (Second Instance):
This test is identical to the previous JwtTest19. Duplicate definitions can lead to maintenance issues. Consider consolidating these two tests into one unless there is an intentional reason for their duplication.
…#195) * removed missing-secure-java * httponly-false-csharp * use-of-md5-digest-utils-java * removing use-of-md5-digest-utils and httponly-false-csharp * jwt-hardcoded-secret-csharp * jwt-decode-without-verify-csharp --------- Co-authored-by: Sakshis <sakshil@abc.com>
Summary by CodeRabbit
New Features
Tests