Feat: Expand security test coverage and reorganize auth test suite#390
Open
Harrio-6 wants to merge 13 commits into
Open
Feat: Expand security test coverage and reorganize auth test suite#390Harrio-6 wants to merge 13 commits into
Harrio-6 wants to merge 13 commits into
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR hardens secret leakage prevention by modifying three production code paths to log exception type names instead of full exception messages, then adds comprehensive security tests verifying that OAuth2 state, RBAC decorators, and infrastructure layers fail safely without exposing sensitive data through exception channels. ChangesSecret Leakage Prevention in Error Handling
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/tests/security/test_input_rail.py`:
- Around line 157-160: The test currently checks each caplog.records using
record.getMessage(), which misses exception tracebacks logged by
logger.exception; update the test in test_input_rail.py to also assert that the
secret string "sk-proj-DO_NOT_LEAK_XYZ" is not present in caplog.text (the full
captured log output) in addition to the existing per-record checks, so any
leakage via tracebacks or exception formatting is detected; keep the existing
loop over caplog.records and add a single assertion like `assert
"sk-proj-DO_NOT_LEAK_XYZ" not in caplog.text` referencing caplog.text and
record.getMessage()/caplog.records.
In `@server/tests/security/test_oauth_callback_redaction.py`:
- Around line 78-85: The test helper _build_app currently imports
utils.auth.oauth2_state_cache which throws if REDIS_URL is unset; before
importing state_cache_mod in _build_app (and the other helper that does the
same) set a test default REDIS_URL in the test environment (use the monkeypatch
fixture or os.environ) so the import succeeds; specifically, update the
_build_app helper in server/tests/security/test_oauth_callback_redaction.py to
set REDIS_URL before importing utils.auth.oauth2_state_cache (and mirror the
same change in the second helper that imports oauth2_state_cache).
In `@server/tests/security/test_secret_leak_on_error.py`:
- Around line 71-74: The tests iterate over caplog.records and assert
_LLM_API_KEY_MARKER not in record.getMessage(), but that misses API keys that
only appear in exception tracebacks; update the assertions in
test_secret_leak_on_error.py (the loop using caplog.records and the related
assertions at the other block around lines 100-103) to also assert
_LLM_API_KEY_MARKER not in caplog.text so the entire captured log (including
tracebacks) is checked; keep the existing per-record assertions and add a single
caplog.text check after the loop for comprehensive coverage.
In `@server/utils/db/db_adapters.py`:
- Around line 75-76: The user DB connection path currently logs the full
exception text in connect_to_db_as_user (which may leak secrets); change its
logger.error call to mirror the admin path by logging only the exception
type/name (e.g., use type(e).__name__) and a short context message, and keep the
existing exception raising behavior (do not include the raw exception or
traceback in the log message). Target the logger.error call inside the
connect_to_db_as_user function to make this change.
In `@server/utils/security/command_safety.py`:
- Around line 179-180: The current logger.exception call leaks the original
exception text via traceback; replace logger.exception("[CommandSafety] LLM call
failed: %s", type(e).__name__) with a non-traceback log (e.g., logger.error or
logger.error(..., exc_info=False)) that only records the safe identifier
(type(e).__name__) and never includes exc_info or the full exception message,
then return _fail_verdict(type(e).__name__) as before; update the code around
the exception handler that references logger.exception, the exception variable
e, and _fail_verdict to ensure no traceback or raw exception text is logged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9c42d25-0464-4640-9e9c-ec1aba35e10a
📒 Files selected for processing (8)
server/routes/atlassian/atlassian_routes.pyserver/tests/auth/test_oauth2_state_cache.pyserver/tests/auth/test_rbac_decorators.pyserver/tests/security/test_input_rail.pyserver/tests/security/test_oauth_callback_redaction.pyserver/tests/security/test_secret_leak_on_error.pyserver/utils/db/db_adapters.pyserver/utils/security/command_safety.py
…to Security-tests
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reorganizes the auth/security test suite by removing redundant or overly broad test files and replacing them with focused, targeted security tests. Adds new coverage for OAuth callback token redaction, secret leakage on errors, OAuth state/CSRF validation, and RBAC decorator behavior. Cleans up conftest.py and trims unrelated noise from the DB connection pool tests.
Summary by CodeRabbit
Bug Fixes
Tests