fix: address DeepSource audit findings - #193
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 108 files, which is 8 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (109)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAuthentication validation was consolidated, password-reset and admin-user routes were separated by HTTP method, server utilities were cleaned up, and CI workflows now upload server coverage to DeepSource. ChangesAuthentication and Route Handler Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 7 minor |
| ErrorProne | 3 high |
| Security | 2 high |
| CodeStyle | 4 minor |
| Complexity | 2 medium |
🟢 Metrics 231 complexity · 6 duplication
Metric Results Complexity 231 Duplication 6
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene  |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jul 22, 2026 12:22p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![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 (1)
server/pssm_gremlin_server/auth.py (1)
474-494: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract repeated token validation logic.
The token validation and user fetching logic is virtually identical for both the Bearer and Cookie authentication methods. Consider extracting this into a helper function to reduce duplication.
♻️ Proposed helper extraction
def _authenticate_token(token: str | None, auth_method: str) -> dict[str, Any] | None: if not token: return None payload = validate_token(token) if payload is None: return None user = db.get_user(payload["uid"]) if ( user is not None and _is_account_blocked(user) is None and payload.get("ver", 0) == user.get("token_version", 0) ): g.auth_method = auth_method return user return None if user := _authenticate_token(_extract_bearer_token(), "bearer"): return user if user := _authenticate_token(request.cookies.get("auth_token"), "cookie"): return user🤖 Prompt for 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. In `@server/pssm_gremlin_server/auth.py` around lines 474 - 494, Extract the duplicated token validation, user lookup, account-blocking, token-version, and auth-method assignment logic from the Bearer and Cookie branches into a shared _authenticate_token helper. Update both authentication paths to call it with their respective token and method values, preserving Bearer-first precedence and the existing user return behavior.
🤖 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/pssm_gremlin_server/routes.py`:
- Around line 751-753: Update auth_reset_password to increment the user’s
token_version when completing the password reset, alongside the db.update_user
password-hash update. Ensure the updated version is persisted so existing bearer
tokens and authentication cookies are invalidated while the reset flow continues
to issue the new password successfully.
---
Nitpick comments:
In `@server/pssm_gremlin_server/auth.py`:
- Around line 474-494: Extract the duplicated token validation, user lookup,
account-blocking, token-version, and auth-method assignment logic from the
Bearer and Cookie branches into a shared _authenticate_token helper. Update both
authentication paths to call it with their respective token and method values,
preserving Bearer-first precedence and the existing user return behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 803e3dec-1546-404a-9827-a920f5953520
📒 Files selected for processing (3)
server/pssm_gremlin_server/auth.pyserver/pssm_gremlin_server/pssm_gremlin.pyserver/pssm_gremlin_server/routes.py
Comment on lines
+751
to
+753
| @app.route("/PSSM_GREMLIN/reset_password", methods=["POST"]) | ||
| def auth_reset_password(): | ||
| """Set a new password using a password-reset token.""" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if update_user handles token_version automatically.
rg -A 20 'def update_user\b' server/Repository: YaoYinYing/REvoDesign
Length of output: 1663
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- auth.py outline ---'
ast-grep outline server/pssm_gremlin_server/auth.py --view expanded | sed -n '1,220p'
echo
echo '--- routes.py outline ---'
ast-grep outline server/pssm_gremlin_server/routes.py --view expanded | sed -n '1,220p'
echo
echo '--- auth.py relevant slice ---'
grep -n -A80 -B20 'def update_user\b' server/pssm_gremlin_server/auth.py | sed -n '1,180p'
echo
echo '--- routes.py relevant slice ---'
grep -n -A80 -B20 'def auth_reset_password\b' server/pssm_gremlin_server/routes.py | sed -n '1,180p'Repository: YaoYinYing/REvoDesign
Length of output: 13781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -n -A60 -B20 'def generate_token\b\|def validate_token\b\|token_version' server/pssm_gremlin_server/auth.py | sed -n '1,220p'Repository: YaoYinYing/REvoDesign
Length of output: 11088
Invalidate sessions on password reset. db.update_user(...) only updates the password hash; it does not bump token_version, so existing bearer tokens and auth cookies remain valid. Increment token_version here as well.
🤖 Prompt for 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.
In `@server/pssm_gremlin_server/routes.py` around lines 751 - 753, Update
auth_reset_password to increment the user’s token_version when completing the
password reset, alongside the db.update_user password-hash update. Ensure the
updated version is persisted so existing bearer tokens and authentication
cookies are invalidated while the reset flow continues to issue the new password
successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyproject.toml (1)
229-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd spaces after commas in the list.
As per coding guidelines, repository formatting checks should be run before committing. The list items are missing spaces after the commas.
🎨 Proposed format
-exclude_dirs = ["build","dist","tests","server/tests","scripts"] +exclude_dirs = ["build", "dist", "tests", "server/tests", "scripts"]🤖 Prompt for 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. In `@pyproject.toml` at line 229, Update the exclude_dirs list in pyproject.toml to include a space after each comma while preserving the existing directory entries and order.Source: Coding guidelines
🤖 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 @.github/workflows/server-test.yml:
- Around line 37-38: Disable credential persistence in the actions/checkout
`with` blocks by adding `persist-credentials: false` at both
.github/workflows/server-test.yml lines 37-38 and
.github/workflows/unit_tests_tag.yml lines 110-111; keep the existing checkout
references unchanged.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 229: Update the exclude_dirs list in pyproject.toml to include a space
after each comma while preserving the existing directory entries and order.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 258b45f2-49c1-4159-a2aa-a77c7224b40f
📒 Files selected for processing (5)
.deepsource.toml.github/workflows/server-test.yml.github/workflows/unit_tests_tag.ymlpyproject.tomlserver/pssm_gremlin_server/pssm_gremlin.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server/pssm_gremlin_server/pssm_gremlin.py
Comment on lines
+37
to
+38
| with: | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable credential persistence in checkout actions.
The actions/checkout step defaults to persisting the repository credentials in the local Git configuration. This creates a risk of credential exposure if subsequent steps are compromised. Set persist-credentials: false to mitigate this risk.
.github/workflows/server-test.yml#L37-L38: Addpersist-credentials: falseto thewithblock..github/workflows/unit_tests_tag.yml#L110-L111: Addpersist-credentials: falseto thewithblock.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 35-38: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 2 files
.github/workflows/server-test.yml#L37-L38(this comment).github/workflows/unit_tests_tag.yml#L110-L111
🤖 Prompt for 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.
In @.github/workflows/server-test.yml around lines 37 - 38, Disable credential
persistence in the actions/checkout `with` blocks by adding
`persist-credentials: false` at both .github/workflows/server-test.yml lines
37-38 and .github/workflows/unit_tests_tag.yml lines 110-111; keep the existing
checkout references unchanged.
Source: Linters/SAST tools
YaoYinYing
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
+ Coverage 73.55% 74.08% +0.53%
==========================================
Files 122 122
Lines 15221 15592 +371
==========================================
+ Hits 11196 11552 +356
- Misses 4025 4040 +15
🚀 New features to boost your workflow:
|
- ensure_ui_file: restore fetch-on-missing path, cache to writable dir - fetch_gist_file: add timeout parameter (default 10s) - self_upgrade: HMAC-verify downloaded assets against manifest.json - fetch_tags: silent degrade on network failure (no popup) - make_window: re-fetch on loadUi failure instead of os.remove+recurse - refresh_remote_json: cache successful fetches to disk - _load_extras_table: shared loader with notify_on_error flag - Makefile: upload-gists generates manifest.json automatically - validate_package_data: check canonical Gist sources, not vendored copies - Remove TODO.md entry for package manager bootstrapping (addressed) - Update CHANGELOG and package-manager docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- conftest: move platformdirs patch before all REvoDesign imports so import-time bootstrap resolves against the mock user-data dir instead of ~/Library/Application Support. Guarded with # isort: split. - current_font: guard setFont against CURRENT_FONT or DEFAULT_FONT being None when the plugin is not fully initialised (tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Addresses the current live DeepSource Python audit results for default-branch run
9ea7977c-250b-4dbd-abd2-a5b02ae9e62e/ checkQ2hlY2s6eGp5cnJvbnk=.Full-scope snapshot pulled from the DeepSource API:
SECURITY=453,ANTI_PATTERN=59,STYLE=3,BUG_RISK=2,PERFORMANCE=2.server/tests/**being analyzed as production:BAN-B101=444plus test chmod/import/comment noise.Fixed production-code findings:
PYL-E0602CRITICAL: importAnyinroutes.pyfor admin update annotations.PYL-E0102MAJOR: remove the shadowing_env_intimport inpssm_gremlin.py.PY-S6007MAJOR: split mixed GET/POST handlers for reset-password and admin users routes.else/return, avoid built-inopen()findings by usingPath.open(), and replace the route side-effect import withimportlib.import_module().Fixed audit configuration:
server/tests/**to.deepsource.tomltest/exclude patterns.server/teststo Banditexclude_dirsso local security scanning has the same test boundary.Deferred:
PY-R1000) and the route-module cycle (PYL-R0401) need a separate refactor PR because they are architectural, not safe one-line audit cleanup.Validation:
make cleanconda run -n REvoDesignTestFlight make kw-test PYTEST_KW='"reset_password or admin_users or admin_create_user or api_key or auth_me"'passed: 8 passed, 837 deselected. This expression selected OpenKinetics due sharedapi_keywording, so server tests were run separately.conda run -n REvoDesignTestFlight python -m pytest -q server/tests/test_auth.py::test_reset_password_get_renders_form server/tests/test_auth.py::test_reset_password_get_rejects_missing_token server/tests/test_auth.py::test_reset_password_get_rejects_invalid_token server/tests/test_auth.py::test_reset_password_post_sets_new_password server/tests/test_auth.py::test_upload_rejects_binary_content server/tests/test_auth.py::test_upload_rejects_invalid_fasta_content server/tests/test_admin.py::test_admin_can_list_users server/tests/test_admin.py::test_non_admin_cannot_list_users server/tests/test_admin.py::test_admin_create_user_with_affiliationpassed: 9 passed.conda run -n REvoDesignTestFlight python -m py_compile server/pssm_gremlin_server/auth.py server/pssm_gremlin_server/pssm_gremlin.py server/pssm_gremlin_server/routes.pypassed in the first batch.conda run -n REvoDesignTestFlight python -m py_compile server/pssm_gremlin_server/pssm_gremlin.pypassed after the second batch.python3TOML parse check for.deepsource.tomlandpyproject.tomlpassed.git diff --checkpassed.Note:
python -m ruff check ...could not run becauseruffis not installed inREvoDesignTestFlight.Summary by CodeRabbit
New Features
Bug Fixes
Quality Improvements