fix(server): harden security, fix review issues from PR #189 - #190
Conversation
- Host header spoofing: _public_base_url() uses SERVER_BASE_URL exclusively - Role demotion now clears is_admin flag - Cancelled tasks treated as terminal (prevent late-write resurrection) - Admin digest checks _send_email() return value, unmarks on failure - Exception handlers guard _pack_failed_results_archive with _task_is_deleted() - Celery apply_async failure now marks task failed + 503 response - Resend-verification returns generic response (prevents account enumeration) - Batch-enable sets email_verified=True - _ensure_columns() backfills deleted, registration_status, user_status, role - Rate limiter prunes expired IP entries periodically - normalize_email() guards against non-string input - _inject_admin_password targets user DB not task DB - Missing columns added to SQLAlchemy _users_table metadata - Server test env decoupled from root tests/conftest.py - CI: actions/setup-python pinned SHA → @v6 - Add RESULT_RETENTION_DAYS env var (default 30) - Add Codecov coverage tracking for server subproject (flags: server) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR hardens authentication, task lifecycle handling, rate limiting, and Celery submission. It adds result-retention configuration, improves Docker-aware test setup, and enables server coverage reporting in CI. ChangesGREMLIN server hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jul 15, 2026 1:57p.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.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 22 |
| Duplication | 2 |
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23ffdcc4f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| # Backfill legacy rows that predate the column — ensures no NULLs | ||
| # slip through for columns added after initial schema creation. | ||
| if "deleted" not in existing: |
There was a problem hiding this comment.
On databases that already ran the previous migration, these columns are already present in existing but legacy rows can still contain NULL, so this new backfill is skipped exactly where it is needed. Those rows remain broken after upgrade: for example list_users() filters deleted == False, excluding NULL rows, and UserResponse expects non-null registration_status/user_status/role, so admin user listing can still hide users or fail validation.
Useful? React with 👍 / 👎.
| if not _task_is_deleted(md5sum): | ||
| _pack_failed_results_archive(task, error_message) |
There was a problem hiding this comment.
When a user cancels a running task, cancel_task() deletes the artifacts and records status='cancelled', but a terminated Docker run commonly reaches this exception path afterward. Since this new guard only checks deleted statuses, the worker still calls _pack_failed_results_archive() for a cancelled task, recreating the result directory/zip that cancellation just removed while the DB guard prevents the status from changing away from cancelled.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
server/pssm_gremlin_server/ratelimit.py (1)
34-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
time.monotonic()instead oftime.time()for rate limiting.System clock adjustments (e.g., via NTP syncs) can cause
time.time()to jump backwards or forwards. A backwards jump could unexpectedly reset user rate limits, while also causingnow - _last_cleanupto become negative and inadvertently stall the pruning step for an extended period.Using
time.monotonic()is a robust best practice for in-memory interval tracking and rate limiters since it strictly advances and is unaffected by system clock updates.♻️ Proposed refactor
- _last_cleanup: float = 0.0 + _last_cleanup: float = time.monotonic() def _prune_expired(now: float, cutoff: float) -> None: """Drop per-IP entries whose most recent timestamp is expired.""" empty = [ip for ip, ts in state.items() if not ts or ts[-1] <= cutoff] for ip in empty: del state[ip] def decorator(f: Callable) -> Callable: `@wraps`(f) def decorated(*args: Any, **kwargs: Any) -> Any: nonlocal _last_cleanup ip = request.remote_addr or "unknown" - now = time.time() + now = time.monotonic() cutoff = now - window_seconds🤖 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/ratelimit.py` around lines 34 - 48, Replace the time.time() call in the rate-limiting decorator with time.monotonic(), ensuring now, cutoff, timestamp comparisons, and _last_cleanup interval checks all use the same monotonic clock.
🤖 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/auth.py`:
- Around line 175-192: Add a legacy-data backfill in the schema migration logic
alongside the existing checks for deleted, registration_status, user_status, and
role: when verification_resend_count is absent from existing, update users rows
where it is NULL to 0, preserving the column’s non-null default and constraint.
- Around line 920-934: Move the exception handling inside the recipients loop so
each _send_email attempt is isolated and processing continues for all admins
without resetting a prior successful any_sent value. Catch the specific expected
exception rather than a blind Exception, log the failure with appropriate
context, and preserve the existing unmark_users_notified behavior based on
whether any send succeeded.
In `@server/pssm_gremlin_server/pssm_gremlin.py`:
- Around line 900-936: The exception handlers around _task_is_deleted must also
skip _pack_failed_results_archive and failure updates for cancelled tasks; use
the existing TaskDatabase.TERMINAL_STATUSES check or a shared predicate that
recognizes all terminal statuses. Apply this consistently to each handler, and
extract the duplicated failure-handling logic into one helper only if it
preserves the current status, timing, archive, and logging behavior.
In `@server/pssm_gremlin_server/routes.py`:
- Around line 286-295: Update the apply_async exception handler in the task
submission flow to call _pack_failed_results_archive for md5sum after marking
the task failed, matching the existing binary-upload and invalid-FASTA failure
paths so clients receive a terminal archive.
In `@server/tests/conftest.py`:
- Around line 32-43: The duplicated has_docker_daemon implementations in
server/tests/conftest.py lines 32-43 and server/tests/test_server.py lines 31-43
incorrectly treat failed docker info commands as success. Add check=True to
subprocess.run and catch only (subprocess.SubprocessError, OSError) in both
sites; preferably centralize the function in shared test utilities or reuse the
conftest implementation from test_server.py.
---
Nitpick comments:
In `@server/pssm_gremlin_server/ratelimit.py`:
- Around line 34-48: Replace the time.time() call in the rate-limiting decorator
with time.monotonic(), ensuring now, cutoff, timestamp comparisons, and
_last_cleanup interval checks all use the same monotonic clock.
🪄 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: 2ed34e6c-79f1-4957-8150-34a14fa2dfb4
📒 Files selected for processing (13)
.github/workflows/server-test.yml.gitignoreCHANGELOG.mdserver/.env.exampleserver/pssm_gremlin_server/auth.pyserver/pssm_gremlin_server/db.pyserver/pssm_gremlin_server/pssm_gremlin.pyserver/pssm_gremlin_server/ratelimit.pyserver/pssm_gremlin_server/routes.pyserver/pssm_gremlin_server/schemas.pyserver/pyproject.tomlserver/tests/conftest.pyserver/tests/test_server.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
==========================================
- Coverage 73.56% 73.50% -0.07%
==========================================
Files 122 122
Lines 15221 15221
==========================================
- Hits 11198 11188 -10
- Misses 4023 4033 +10 🚀 New features to boost your workflow:
|
- CI: install pytest-order, pytest-dependency; override root addopts - CI: run Codecov upload only on test success (not always()) - ratelimit: use time.monotonic() for clock-jump resilience - auth: backfill verification_resend_count NULL -> 0 in _ensure_columns() - auth: move try/except inside admin digest recipient loop - pssm_gremlin: rename _is_deleted_status -> _is_terminal_status (includes cancelled) - pssm_gremlin: exception handlers check terminal status not just deleted - routes: pack failed results archive on Celery submission failure - tests: has_docker_daemon uses check=True for accurate Docker detection Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Backfills were gated on 'col not in existing', but databases that already ran a prior migration still have NULLs in legacy rows. Backfills now run unconditionally (WHERE col IS NULL — idempotent), except admin_notified which must remain gated to avoid re-marking everyone on restart. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/pssm_gremlin_server/pssm_gremlin.py (1)
881-894: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate log messages to reflect the new
cancelledstate.Since the terminal status checks (
_task_is_terminaland_is_terminal_status) now includecancelledtasks, consider updating these log messages from "was deleted" to "was deleted or cancelled" to prevent confusion during troubleshooting.♻️ Proposed log message updates
if _task_is_terminal(md5sum): - logging.info("Task %s was deleted during execution; skipping result packing and finalization.", md5sum) + logging.info("Task %s was deleted or cancelled during execution; skipping result packing and finalization.", md5sum) return final_stage = stage_state["current"] or _RUNNING_TRACE_STEPS[-1][0] task_store.update_task(md5sum, status="packing results", run_stage=final_stage) refreshed_task = task_store.get_task(md5sum) or task if _is_terminal_status(refreshed_task.get("status")): - logging.info("Task %s was deleted before archive packing; skipping artifact packaging.", md5sum) + logging.info("Task %s was deleted or cancelled before archive packing; skipping artifact packaging.", md5sum) return _pack_results_archive(refreshed_task) refreshed_task = task_store.get_task(md5sum) or refreshed_task if _is_terminal_status(refreshed_task.get("status")): - logging.info("Task %s was deleted during archive packing; skipping final status update.", md5sum) + logging.info("Task %s was deleted or cancelled during archive packing; skipping final status update.", md5sum) return🤖 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/pssm_gremlin.py` around lines 881 - 894, Update the three terminal-task log messages in the finalization flow around _task_is_terminal and _is_terminal_status to say “was deleted or cancelled” instead of only “was deleted.” Preserve the existing checks, control flow, and message context for skipping result packing, artifact packaging, and final status updates.
🤖 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.
Nitpick comments:
In `@server/pssm_gremlin_server/pssm_gremlin.py`:
- Around line 881-894: Update the three terminal-task log messages in the
finalization flow around _task_is_terminal and _is_terminal_status to say “was
deleted or cancelled” instead of only “was deleted.” Preserve the existing
checks, control flow, and message context for skipping result packing, artifact
packaging, and final status updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15618718-d71f-465c-b8a0-6ad51f37baa6
📒 Files selected for processing (7)
.github/workflows/server-test.ymlserver/pssm_gremlin_server/auth.pyserver/pssm_gremlin_server/pssm_gremlin.pyserver/pssm_gremlin_server/ratelimit.pyserver/pssm_gremlin_server/routes.pyserver/tests/conftest.pyserver/tests/test_server.py
🚧 Files skipped from review as they are similar to previous changes (4)
- server/pssm_gremlin_server/ratelimit.py
- .github/workflows/server-test.yml
- server/pssm_gremlin_server/auth.py
- server/pssm_gremlin_server/routes.py
Codecov needs flag-to-path mapping to match uploaded coverage with PR changes. Without this, the server CI upload shows 'No Files covered by tests were changed.' Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Addresses review discussions from CodeRabbit, Codex, and DeepSource on PR #189.
Security
_public_base_url()now always usesSERVER_BASE_URL, never the requestHostheaderadminrole now also clearsis_adminflagCorrectness
_send_email()return value, unmarks users on failure_pack_failed_results_archivewith_task_is_deleted()apply_asyncfailure marks task as failed + 503email_verified = True_ensure_columns()Robustness
normalize_email()guards against non-string input_inject_admin_passwordtargets user DB not task DBInfrastructure
actions/setup-pythonpinned SHA →@v6RESULT_RETENTION_DAYSenv var (default 30)flags: server)🤖 Generated with Claude Code
Summary by CodeRabbit
0to disable auto-cleanup)..envexample.