Add security rules for insecure cryptography and hardcoded credentials in Python and Ruby #125
Conversation
WalkthroughThis pull request introduces three new security rules across Python and Ruby languages, focusing on detecting insecure cryptographic practices and hardcoded credentials. The changes include a rule for identifying the use of the insecure RC4 cipher algorithm in Python, a rule for detecting hardcoded OpenAI API keys, and a rule for finding hardcoded HTTP authentication credentials in Ruby controllers. Each rule is accompanied by corresponding test cases and snapshot files to validate the detection mechanisms. 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: 4
🧹 Nitpick comments (10)
tests/ruby/hardcoded-http-auth-in-controller-ruby-test.yml (2)
3-12: Consider adding more edge cases to strengthen test coverage.The valid test cases cover non-string variables, but consider adding tests for:
- Empty string variables
- Environment variables
- Configuration objects
- Different authentication options (only, except)
18-18: Add newline at end of file.Add a newline character at the end of the file to comply with YAML standards.
puts "do more stuff" end +🧰 Tools
🪛 yamllint (1.35.1)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
rules/ruby/security/hardcoded-http-auth-in-controller-ruby.yml (3)
4-8: Enhance error message with specific remediation steps.The current message could be more actionable. Consider adding:
- Specific examples of secure alternatives
- Reference to Rails credentials system
- Link to Rails-specific security documentation
Detected hardcoded password used in basic authentication in a controller class. Including this password in version control could expose this credential. Consider refactoring to use environment variables or - configuration files + configuration files. Use Rails.application.credentials or environment + variables (e.g., ENV['AUTH_PASSWORD']) instead of hardcoded strings.
15-55: Consider strengthening the pattern matching.The current pattern matching might miss some edge cases. Consider:
- Adding checks for string interpolation (
#{var})- Detecting Base64 encoded strings
- Checking for common password variable names
Would you like me to provide an enhanced pattern matching configuration that covers these cases?
9-12: Enhance security references and documentation.The current references are good but could be improved by adding:
- Link to Rails-specific security guidelines
- Reference to CWE-259 (Use of Hard-coded Password)
- Examples of secure implementations
[CWE-798] Use of Hard-coded Credentials. + [CWE-259] Use of Hard-coded Password. [REFERENCES] - https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html + - https://guides.rubyonrails.org/security.html#custom-credentialsrules/python/security/insecure-cipher-algorithm-rc4-python.yml (2)
11-15: Consider adding more security references.While the current references are good, consider adding:
- NIST recommendations against RC4
- CVEs related to RC4 vulnerabilities
- Links to implementation guides for recommended alternatives
79-85: Fix file formatting issues.The file has excessive blank lines at the end and is missing a newline at EOF.
- - - - - - - +🧰 Tools
🪛 yamllint (1.35.1)
[warning] 84-84: too many blank lines
(6 > 2) (empty-lines)
[error] 85-85: no new line character at the end of file
(new-line-at-end-of-file)
[error] 85-85: trailing spaces
(trailing-spaces)
rules/python/security/openai-hardcoded-secret-python.yml (2)
20-21: Fix YAML indentationThe indentation is inconsistent with the rest of the file.
- stopBy: end - kind: string + stopBy: end + kind: string🧰 Tools
🪛 yamllint (1.35.1)
[warning] 20-20: wrong indentation: expected 12 but found 9
(indentation)
23-24: Add newline at end of fileAdd a newline character at the end of the file to follow YAML best practices.
rule: all: - - matches: match_api_key + - matches: match_api_key +🧰 Tools
🪛 yamllint (1.35.1)
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
tests/python/openai-hardcoded-secret-python-test.yml (1)
1-8: Add more test cases for comprehensive coverageThe current test cases are limited. Consider adding:
- Different variable names (e.g.,
secret_key,key)- Different string formats (single quotes, f-strings)
- Keys in different contexts (function parameters, class attributes)
- Invalid format variations (wrong prefix, wrong length)
Would you like me to provide examples of additional test cases?
🧰 Tools
🪛 Gitleaks (8.21.2)
7-7: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
8-8: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
rules/python/security/insecure-cipher-algorithm-rc4-python.yml(1 hunks)rules/python/security/openai-hardcoded-secret-python.yml(1 hunks)rules/ruby/security/hardcoded-http-auth-in-controller-ruby.yml(1 hunks)tests/__snapshots__/hardcoded-http-auth-in-controller-ruby-snapshot.yml(1 hunks)tests/__snapshots__/insecure-cipher-algorithm-rc4-python-snapshot.yml(1 hunks)tests/__snapshots__/openai-hardcoded-secret-python-snapshot.yml(1 hunks)tests/python/insecure-cipher-algorithm-rc4-python-test.yml(1 hunks)tests/python/openai-hardcoded-secret-python-test.yml(1 hunks)tests/ruby/hardcoded-http-auth-in-controller-ruby-test.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/hardcoded-http-auth-in-controller-ruby-snapshot.yml
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/ruby/hardcoded-http-auth-in-controller-ruby-test.yml
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
rules/python/security/insecure-cipher-algorithm-rc4-python.yml
[warning] 84-84: too many blank lines
(6 > 2) (empty-lines)
[error] 85-85: no new line character at the end of file
(new-line-at-end-of-file)
[error] 85-85: trailing spaces
(trailing-spaces)
rules/python/security/openai-hardcoded-secret-python.yml
[warning] 20-20: wrong indentation: expected 12 but found 9
(indentation)
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
tests/python/insecure-cipher-algorithm-rc4-python-test.yml
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 26-26: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Gitleaks (8.21.2)
tests/python/openai-hardcoded-secret-python-test.yml
7-7: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
8-8: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
tests/__snapshots__/openai-hardcoded-secret-python-snapshot.yml
4-4: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
5-5: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
7-7: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
11-11: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
🔇 Additional comments (5)
rules/python/security/insecure-cipher-algorithm-rc4-python.yml (2)
4-10: LGTM! Clear and informative security warning message.
The message effectively communicates:
- The security risk of using RC4
- Specific secure alternatives (ChaCha20, XChaCha20, Salsa20, AES)
- Modern security considerations (block size, authentication modes)
73-78: LGTM! Comprehensive pattern matching for RC4 detection.
The rule effectively catches:
- Direct calls to ARC4.new()
- Aliased imports and usage
- Both Crypto and Cryptodome library variants
tests/python/insecure-cipher-algorithm-rc4-python-test.yml (1)
13-26: LGTM! Comprehensive test coverage for RC4 detection.
The invalid test cases effectively cover:
- Different import patterns (Crypto/Cryptodome)
- Aliased imports
- Direct calls with/without parameters
🧰 Tools
🪛 yamllint (1.35.1)
[error] 26-26: no new line character at the end of file
(new-line-at-end-of-file)
tests/__snapshots__/insecure-cipher-algorithm-rc4-python-snapshot.yml (1)
1-157: LGTM! Well-structured and comprehensive test snapshots.
The snapshots effectively capture:
- All RC4 usage patterns
- Precise source locations
- Both direct and aliased imports
- Different parameter variations
tests/__snapshots__/openai-hardcoded-secret-python-snapshot.yml (1)
7-14: Remember to update label positions
If you update the API keys to use placeholders, remember to adjust the start and end positions in the label definitions to match the new string lengths.
✅ Verification successful
Label positions are correctly aligned with the string length
The current label positions (9-60) define a range of 51 characters, which exactly matches the length of the API key string. No adjustments are needed as the positions are accurate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that label positions match the new placeholder keys
echo "New key length (excluding quotes): $(echo -n "sk-test123TEST123test123TEST123test123TEST123test12" | wc -c)"
echo "Current label range: 9-60 (length: $((60-9)))"
echo "New label range should be adjusted if different"
Length of output: 348
🧰 Tools
🪛 Gitleaks (8.21.2)
7-7: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
11-11: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
| - | | ||
| cipher = AES.new(key, AES.MODE_EAX, nonce=nonce) | ||
| plaintext = cipher.decrypt(ciphertext) | ||
| try: | ||
| cipher.verify(tag) | ||
| print("The message is authentic:", plaintext) | ||
| except ValueError: | ||
| print("Key incorrect or message corrupted") |
There was a problem hiding this comment.
Fix indentation in the valid test case.
The try-except block has incorrect indentation which could affect YAML parsing.
cipher = AES.new(key, AES.MODE_EAX, nonce=nonce)
plaintext = cipher.decrypt(ciphertext)
try:
- cipher.verify(tag)
- print("The message is authentic:", plaintext)
+ cipher.verify(tag)
+ print("The message is authentic:", plaintext)
except ValueError:
- print("Key incorrect or message corrupted")
+ print("Key incorrect or message corrupted")Committable suggestion skipped: line range outside the PR's diff.
| kind: string_content | ||
| regex: \b(sk-[[:alnum:]]{20}T3BlbkFJ[[:alnum:]]{20})\b |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the API key pattern more flexible
The current regex pattern \b(sk-[[:alnum:]]{20}T3BlbkFJ[[:alnum:]]{20})\b might be too strict:
- It expects exactly 20 characters before and after "T3BlbkFJ"
- It requires the literal string "T3BlbkFJ"
OpenAI API keys might vary in format over time. Consider using a more flexible pattern:
- regex: \b(sk-[[:alnum:]]{20}T3BlbkFJ[[:alnum:]]{20})\b
+ regex: \b(sk-[[:alnum:]]{48})\b📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kind: string_content | |
| regex: \b(sk-[[:alnum:]]{20}T3BlbkFJ[[:alnum:]]{20})\b | |
| kind: string_content | |
| regex: \b(sk-[[:alnum:]]{48})\b |
| api_key="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" | ||
| f = "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" |
There was a problem hiding this comment.
Replace potentially real API keys with placeholders
The test cases appear to contain real API keys. Even if they're invalid, it's best practice to use obvious placeholder values in test files.
- api_key="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"
- f = "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"
+ api_key="sk-test123TEST123test123TEST123test123TEST123test12"
+ f = "sk-test456TEST456test456TEST456test456TEST456test45"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| api_key="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" | |
| f = "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" | |
| api_key="sk-test123TEST123test123TEST123test123TEST123test12" | |
| f = "sk-test456TEST456test456TEST456test456TEST456test45" |
🧰 Tools
🪛 Gitleaks (8.21.2)
7-7: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
8-8: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
| api_key="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" | ||
| f = "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" |
There was a problem hiding this comment.
Replace potentially real API keys with placeholders
The snapshot file contains the same potentially real API keys as the test file. Update these to match the placeholder values used in the test file.
- api_key="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"
- f = "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"
+ api_key="sk-test123TEST123test123TEST123test123TEST123test12"
+ f = "sk-test456TEST456test456TEST456test456TEST456test45"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| api_key="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" | |
| f = "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" | |
| api_key="sk-test123TEST123test123TEST123test123TEST123test12" | |
| f = "sk-test456TEST456test456TEST456test456TEST456test45" |
🧰 Tools
🪛 Gitleaks (8.21.2)
4-4: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
5-5: Found an OpenAI API Key, posing a risk of unauthorized access to AI services and data manipulation.
(openai-api-key)
Summary by CodeRabbit
New Features
Bug Fixes
Tests