feat(server): token auth, registration, admin panel, email redesign, and security hardening - #189
Conversation
…on workflow - Replace Flask-HTTPAuth with itsdangerous-based Bearer token auth - Add UserDatabase (standalone SQLite, separate from task tracker) - Add /api/auth/login, /register, /verify-email, /me endpoints - Gate registration behind ENABLE_REGISTER env var - Add SMTP email verification (stdlib smtplib, no new dependency) - Migrate legacy users.txt entries into SQLite on startup - Bootstrap default admin user when DB is empty - Extract all JS/CSS from HTML templates into static/ files - Add login.html and register.html pages - Remove Flask-HTTPAuth dependency Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Read USERS_FILE only when the user DB is empty (first run) - Skip the file on subsequent startups — it is not a live credential store - Add explicit warning that imported users must change passwords immediately - Clarify one-time seed behaviour in comments and .env.example Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Remove USERS_FILE / users.template.txt entirely - Bootstrap: create one admin on first run if user DB is empty - Delete migrate_users_file dead code from auth.py Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Replace HTTP Basic Auth with Bearer-token authentication (itsdangerous) - Add SQLite-backed user store with hashed passwords (pbkdf2:sha256) - Add self-registration workflow gated by ENABLE_REGISTER + SMTP - Add long-lived API keys with restricted privileges (X-API-Key header) - Add profile page with password change and API key management - Add rate limiting on login (5/min/IP) and registration (3/hr/IP) - Add Redis password support via REDIS_PASSWORD env var - Split pssm_gremlin.py (~1500 lines) into db.py, routes.py, ratelimit.py - Fix AUTH_SECRET_KEY duplication across gunicorn workers - Fix cancelled task result directory cleanup - Remove root group from Docker socket access - Update docs and CHANGELOG Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Move _env_bool to auth.py as single source of truth (was duplicated with different semantics in pssm_gremlin.py — auth.py's version now handles both true/false explicitly and raises on invalid input). - Shrink FileNotFoundError/OSError branch in dashboard route 8→4 lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The refactored pssm_gremlin.py imports from sibling packages (pssm_gremlin.db, pssm_gremlin.auth), which requires the pssm_gremlin package to be importable. The test's spec_from_file_location loader didn't add server/ to sys.path — it worked before because the old pssm_gremlin.py had zero sibling imports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ALLOWED_EMAIL_DOMAINS env var restricts registration to listed domains - Email +suffix stripped before storage (user+tag@domain → user@domain) - Domain check enforced on self-registration; admins bypass for manual creation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Replace _basic_auth_header with _bearer_headers (POSTs to login endpoint) - Add _module_bearer_headers for Flask test-client tests (creates user in DB and generates token locally — no HTTP server needed) - Update _wait_for_server_ready/_wait_for_task/_wait_for_failed_task signatures from auth tuple to headers dict - DockerServerStack: bootstrap via DEFAULT_ADMIN_PASSWORD instead of users.txt/USERS_FILE - Remove unused import base64 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The root group was removed prematurely — it is a temporary workaround for a socket permission issue that hasn't been fully resolved yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Empty env vars from docker compose ${VAR:-} now treated as unset,
preventing SQLite paths from resolving to CWD.
- AUTH_SECRET_KEY guard changed from setdefault to explicit empty-check
so docker compose empty-string pass-through is handled correctly.
- Gunicorn --preload ensures consistent token-signing key across workers;
tokens from one worker now validate on all others.
- Package shadowing fix: WORKDIR moved from pssm_gremlin/ to server/
so the directory-package isn't shadowed by the file-module.
- Browser auth: login sets HttpOnly cookie; load_current_user checks it
for page navigations that can't carry the Authorization header.
- Logout: POST /api/auth/logout clears the HttpOnly cookie (JS can't).
- Login/register pages redirect to dashboard when already authenticated.
- Error responses: browser requests get styled HTML error pages with
contextual actions; API requests continue to receive JSON.
- Frontend redesign: card-based profile layout, section headings with
accent bars, logout button, copy-to-clipboard for API keys, full-width
login button, collapsed developer section, lab-instrument focus states.
- API key display: no longer hidden immediately after generation by the
refreshApiKeyStatus race.
- Restart script generates and displays admin password on first boot.
- Docs: server/README.md, docs/dev-guide/server.md, CHANGELOG.md
updated with all changes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The native <input type=file> looked like a text field and didn't
clearly signal its purpose. Replaced with a hidden input + visible
<label> button ("Choose FASTA file") that displays the selected
filename when a file is chosen.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace <label for=...> pattern with explicit <button> + JS fileInput.click() — more reliable across browsers for hidden file inputs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Browsers block .click() on <input type=file hidden> as a security measure. Use the visually-hidden pattern (1px, clipped, positioned) instead — the input is invisible but still considered 'present' by the browser, so programmatic click() opens the file dialog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-upload CSS revert - dashboard.js: download uses authFetch + blob URL instead of window.location - dashboard.js: logout calls POST /api/auth/logout before navigating - routes.py: add missing url_for import (fixes 500 on unauthenticated redirect) - error.html: extract inline CSS/JS to error-page.css and error-page.js - create-task.css: revert .file-input to main-branch simple full-width style - create-task.js: remove dead fileButton/FileNameDisplay code - base.css: remove backdrop-filter (known Chrome hit-testing bug), add .sr-only - routes.py: add /test-click diagnostic endpoint (no auth, bare HTML) - add back auth_token cookie set in login response Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- HTML: use .sr-only (existing) instead of non-existent .visually-hidden - HTML: add drag-and-drop hint text - CSS: add .file-upload-row, .file-name, .drop-highlight classes - CSS: add dark-theme variants for new classes - JS: full rewrite — button click, file input change, drag-and-drop - JS: setSelectedFile() uses DataTransfer API for programmatic FileList - JS: drop zone is entire .input-zone card, document-level drag prevention Drag-and-drop works around a Chrome-specific bug where native <input type=file> click does not open the file picker dialog. Safari file picker works correctly in all cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- routes.py: remove /PSSM_GREMLIN/test-click diagnostic endpoint - CHANGELOG.md: update file-upload entry to reflect drag-and-drop fix Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- docs/dev-guide/server.md: add drag-and-drop note, add Code Structure table listing pssm_gremlin.py, routes.py, auth.py, db.py, ratelimit.py - server/README.md: add drag-and-drop + sequence editor usage notes under Create task page section Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d batch management - Add 7 new user columns: affiliation, terms_agreed, registration_status, user_status, approved_by, approved_at, deleted - Registration flow: affiliation field, terms checkbox, 2-day verification token via /user_verify?c= endpoint - Admin User Control page with two sub-tabs: Tab A — user table with approve/reject/ban/enable actions + batch operations Tab B — manual add-user form (auto-derives username from email) - Batch enable/disable/delete with soft-delete for audit trail - [User Control] button in dashboard header for admin users - 12 new tests for admin user management, registration, and batch ops - Fix pre-existing test assertions for create_task page header changes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Admin-created users (Tab B) only know their email — the username is auto-derived from the email local-part. Login now checks both username and email lookups so users can sign in with either. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace auto-derived username (from email local-part) with an explicit [Username] field in Tab B. Prevents accidental collisions and makes the created username visible to the admin before submission. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Forgot password link on login page with inline email form - POST /api/auth/forgot-password sends reset link (1-hour expiry) - GET /PSSM_GREMLIN/reset_password?c=<token> renders new-password form - POST /PSSM_GREMLIN/reset_password applies the new password - Rate-limited: 3 requests/hour/IP - Does not reveal whether an email is registered Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each user row now has a [Modify] button that opens an inline edit form: - Email, affiliation, registration status, and user status are pre-filled - Password field is optional — leave blank to keep current password - Save/Cancel buttons replace the row content in-place - PUT endpoint updated to accept email, affiliation, and password changes - Email uniqueness validated on update - Row restored on cancel, re-attaches checkbox listener Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace ad-hoc payload validation in route handlers with Pydantic models at the API boundary. All inbound request payloads are validated through typed models before reaching business logic; response serialisation uses UserResponse to guarantee sensitive fields never leak. - New server/pssm_gremlin/schemas.py: LoginRequest, RegisterRequest, AdminCreateUserRequest, AdminUpdateUserRequest, BatchUserRequest, ForgotPasswordRequest, ResetPasswordRequest, ChangePasswordRequest, UserResponse - routes.py: _parse_body() helper replaces manual str(payload.get(...)) patterns across all nine auth/admin route handlers - Removes _normalize_email from routes.py (moved to schemas.py) - Adds pydantic>=2.0.0 to server/docker/server/requirements.txt Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Rename server/pssm_gremlin → server/pssm_gremlin_server package - Update all internal imports and gunicorn/celery module paths - Add server/pyproject.toml for pip install -e 'server/[test]' - Move tests/server/test_pssm_gremlin.py → server/tests/test_server.py - Add .github/workflows/server-test.yml for standalone server CI - Update server/docker-compose.yml, Dockerfiles for new package name - Update server/README.md with dev install instructions - Update CHANGELOG.md Co-Authored-By: Claude Code <noreply@anthropic.com>
…test deps - docs/dev-guide/server.md: update code structure for pssm_gremlin_server package, add schemas.py and new modules, add admin API + forgot-password endpoints, add testing section with pip install and pytest commands - server/README.md: add Pydantic validation note to Security section - CHANGELOG.md: add entries for admin user control panel, forgot/reset password, email verification (2-day), login-by-email, and server-test dependency removal from REvoDesign test suite - Makefile: remove Celery, Flask, Flask-HTTPAuth, SQLAlchemy, Docker SDK from prepare-test — server tests are now self-contained Co-Authored-By: Claude Code <noreply@anthropic.com>
…checks - _is_account_blocked() rejects deleted/banned/pending users at token validation and login time (backward-compat: NULL user_status = active) - _sanitize_headers_for_log() drops Authorization, Cookie, X-API-Key before storage in task DB and worker logs - _is_admin_user() checks DB is_admin in addition to ADMIN_USERS env var - Dockerfile default CMD now includes --preload for token consistency - Flask MAX_CONTENT_LENGTH set to 16 MiB to prevent upload DoS - Test helper creates users with user_status=active Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… from docker compose - _is_account_blocked() now rejects unverified self-registered users even if an admin set user_status=active without approving registration. Admin-created users (registration_status=approved) skip this check. - docker-compose: remove root group (0) from group_add. The setup script auto-detects DOCKER_GID; the fallback to 0 silently granted root-equivalent socket access on misconfigured hosts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Split g.auth_method into 'bearer' (Authorization header) vs 'cookie' so state-changing endpoints can reject cookie-only authenticated requests. Cookie is CSRF-prone; Bearer header cannot be set cross- origin (browser same-origin policy). - require_bearer_auth() applied to: password change, API key gen/revoke, admin create/update/delete, admin batch operations. - Admin approval (registration_status=approved) now auto-sets email_verified=True, closing the path where an unverified self- registered account becomes active via PUT /admin/users/<id>. - Simplified _is_account_blocked: check email_verified is False for all users (no registration_status bypass — admin create/approve always calls verify_email). - docs: removed misleading 'non-root prevents privilege escalation' claim about Docker socket access. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add require_bearer_auth() to upload, cancel, delete, and batch-delete routes — they were still accepting cookie-only auth via plain @login_required. - Update docker-compose comment: 'Least privilege' is misleading when Docker socket group access is host-root-equivalent. - Update README CSRF description: cookie is read-only now, state-changing endpoints require Bearer token in Authorization header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add registration_ip column (TEXT, nullable) to users table + auto-migration - Capture client IP on registration (X-Forwarded-For > X-Real-IP > remote_addr) - Return registration_ip in UserResponse API model - Display IP in admin user-control table (replaces hardcoded em-dash) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n registration - CLIENT_IP_HEADERS: comma-separated headers for client IP (default: X-Forwarded-For, X-Real-IP). Cloudflare: CF-Connecting-IP, ... - CLIENT_COUNTRY_HEADER: single header for country code (e.g. CF-IPCountry) - Both parsed once at import time (not per-request) - Unified _client_ip() used by both task metadata and registration - registration_country column added to users table - Country displayed alongside IP in admin user-control table Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…es in docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion error Move backup after cmd_down so compose env vars are set, then use docker compose run --entrypoint /bin/cp so the container user (who owns SERVER_DIR) writes the backup instead of the host caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two users uploading the same filename (e.g. seqs.fasta) would
overwrite each other in UPLOAD_FOLDER. Save to a temp name first,
then rename to {md5sum}.fasta which is owner-scoped and unique.
The result-dir copy is already safe (per-task directory), and
task["filename"] is kept as the original for display/output naming.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Failed tasks no longer pack their result dir into a zip, but the result directory survives on disk. Relax the status gate in both get_results and download_results to accept "failed" in addition to "finished", and zip the result dir on-the-fly when the pre-packed zip is missing. Useful for debugging failed GREMLIN runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a small (?) indicator next to the "Failed" status pill when the task has a non-empty error message. Uses native title tooltip for simplicity — hover to see the full error text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sk download, and error tooltip Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y overwrite, email template safety Bug 1 (empty approval email): use refreshed user data after DB update so the email address is guaranteed current. Switch _email_html from .format() to str.replace() so curly braces in the body HTML cannot be mis-parsed as format specifiers. Bug 2 (data corruption after profile edit): the inline edit form in user-control.js used escapeAttr() for email and affiliation input values, which strips everything except [a-zA-Z0-9_-] — destroying @, dots, and spaces. Switched to escapeHtml() which properly encodes HTML entities without altering the visible value. Bug 3 (approved_by/approved_at overwrite): every admin edit (affiliation, role, etc.) was setting approved_by and approved_at to the editing admin. Now only set when registration_status is actually changed to "approved". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR replaces Basic Auth with SQLite-backed bearer-token authentication, adds account and task-management routes, introduces redesigned server pages, refactors packaging and Docker startup, expands task failure handling, and adds changelog validation plus server and integration test coverage. ChangesGREMLIN server modernization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 9:30a.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.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 3 medium |
| CodeStyle | 97 minor |
🟢 Metrics 1311 complexity · 78 duplication
Metric Results Complexity 1311 Duplication 78
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fece3f8c87
ℹ️ 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".
| """ | ||
| # ponytail: request.host_url already has scheme+host from the Host header | ||
| try: | ||
| return request.host_url.rstrip("/") |
There was a problem hiding this comment.
Build email links from a trusted origin
When email verification or password-reset endpoints run in a request context, this returns the raw Host-derived request.host_url and ignores SERVER_BASE_URL. If a deployment does not reject unknown Host headers, an attacker can request a reset/registration with a spoofed Host and the emailed URL will contain the one-time token on the attacker-controlled origin; use the configured public base URL or validate trusted hosts before constructing these links.
Useful? React with 👍 / 👎.
| update_fields["role"] = req.role | ||
| if req.role == "admin": | ||
| update_fields["is_admin"] = True |
There was a problem hiding this comment.
Clear admin flag when demoting users
When an existing admin is edited to role: "user" or "guest", this branch only changes the role and leaves is_admin set to true. Because the admin gates still accept is_admin, the supposedly demoted account keeps admin API/page access; set is_admin to false whenever the new role is not admin.
Useful? React with 👍 / 👎.
| if not sent: | ||
| return jsonify({"error": "Failed to send verification email. Contact an administrator."}), 500 | ||
|
|
||
| db.update_user(user["id"], verification_resend_count=count + 1, verification_resend_at=time.time()) |
There was a problem hiding this comment.
Add resend counters to table metadata
This resend path writes verification_resend_count and verification_resend_at, but those columns are only added via raw ALTER TABLE and are absent from _users_table, so SQLAlchemy updates are made with unknown column names and will fail at runtime; the same metadata also prevents future reads from returning the counters. Define these columns on _users_table before updating them.
Useful? React with 👍 / 👎.
| ) | ||
| try: | ||
| for email in recipients: | ||
| _send_email( |
There was a problem hiding this comment.
Honor failed admin-digest sends
The digest marks users notified before sending, but the return value of _send_email is ignored; _send_email returns False on SMTP/Resend failures instead of raising, so a transient mail outage records the registrations as notified and they will not appear in the next digest. Check the boolean result and unmark or retry when any recipient send fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/pssm_gremlin_server/__init__.py (1)
1-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd missing
__future__import.As per coding guidelines, every Python file must use Python 3.10+ syntax with
from __future__ import annotations.🛠️ Proposed fix
# Copyright (c) 2026 The REvoDesign Developers. # Distributed under the terms of the GNU General Public License v3.0. # SPDX-License-Identifier: GPL-3.0-only + +from __future__ import annotations🤖 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/__init__.py` around lines 1 - 4, Add `from __future__ import annotations` at the top of the module, immediately after the copyright and license header, so the file follows the project’s Python 3.10+ annotation syntax guideline.Source: Coding guidelines
🟡 Minor comments (5)
dev/tools/check_changelog_duplicates.py-1-2 (1)
1-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required copyright block.
As per coding guidelines, every Python file must start with the GPL-3.0-only copyright block.
📝 Proposed fix to add the copyright block
#!/usr/bin/env python3 +# Copyright (c) 2026 The REvoDesign Developers. +# Distributed under the terms of the GNU General Public License v3.0. +# SPDX-License-Identifier: GPL-3.0-only + """Check CHANGELOG.md for duplicate section headers within a version block.🤖 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 `@dev/tools/check_changelog_duplicates.py` around lines 1 - 2, Add the repository-required GPL-3.0-only copyright header at the beginning of the Python file, before the existing shebang or module docstring as prescribed by project conventions. Preserve the current check-changelog implementation and module documentation unchanged.Source: Coding guidelines
.github/workflows/server-test.yml-31-38 (1)
31-38: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDisable checkout credential persistence. Add
persist-credentials: falseto theactions/checkoutstep so the GitHub token isn’t left available to later steps.🤖 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 31 - 38, Add persist-credentials: false to the actions/checkout step named Checkout Repository, leaving the existing repository checkout configuration and Python setup unchanged.Sources: Coding guidelines, Linters/SAST tools
server/pssm_gremlin_server/static/js/dashboard.js-167-176 (1)
167-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBatch-select checkbox has no accessible name.
The
titleattribute on the wrapping<label>isn't reliably announced by screen readers, and the checkbox has no visible text oraria-label.♿ Proposed fix
-'<label class="task-select-wrap" title="Select task for batch delete"><input class="task-select" type="checkbox" data-action="toggle-select" data-md5="' + escapeHtml(task.md5) + '" ' + (selected ? "checked" : "") + '></label>' +'<label class="task-select-wrap" title="Select task for batch delete"><input class="task-select" type="checkbox" aria-label="Select task ' + escapeHtml(task.md5) + ' for batch delete" data-action="toggle-select" data-md5="' + escapeHtml(task.md5) + '" ' + (selected ? "checked" : "") + '></label>'🤖 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/static/js/dashboard.js` around lines 167 - 176, Update the batch-select checkbox markup in the task-card rendering flow to provide an accessible name directly on the checkbox, such as an appropriate aria-label describing selection of the task. Keep the existing selection behavior, data attributes, and visual layout unchanged.server/pssm_gremlin_server/static/js/register.js-70-72 (1)
70-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDead conditional — both branches return the same value.
result.data.email_sent ? "block" : "block"always resolves to"block"regardless ofemail_sent, so the flag has no effect. This looks like the "false" branch was meant to differ (e.g. hide the row, or show an alternate message when the email failed to send).🛠️ Proposed fix
- if (resendRow) { - resendRow.style.display = result.data.email_sent ? "block" : "block"; - } + if (resendRow) { + resendRow.style.display = "block"; + }Or, if the intent was different behavior on send failure, restore that branch (e.g. show a distinct message when
email_sentisfalse).🤖 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/static/js/register.js` around lines 70 - 72, Update the resendRow display assignment in the registration flow to remove the dead conditional and implement the intended false-case behavior: hide the row or show the appropriate alternate state when result.data.email_sent is false, while preserving the visible state for successful sends.server/pssm_gremlin_server/templates/email/base.html-26-34 (1)
26-34: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winEscape user-controlled fields before building the email HTML.
_email_html()does a raw string replace, and the callers interpolateuser["username"]andaffiliationdirectly intohtml_body, so a crafted profile value can inject HTML into verification, reset, and admin digest emails.🤖 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/templates/email/base.html` around lines 26 - 34, Update _email_html() and its callers to HTML-escape user-controlled values before interpolating them into html_body, including user["username"] and affiliation. Ensure verification, reset, and admin digest email paths all pass escaped content while preserving the existing template structure and formatting.
🧹 Nitpick comments (6)
server/pssm_gremlin_server/static/css/create-task.css (1)
143-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace deprecated
word-breakvalue.The
break-wordvalue for theword-breakproperty is deprecated. Useoverflow-wrap: break-word(orword-wrap: break-word) instead to achieve the same effect without triggering CSS validation warnings.♻️ Proposed fix
white-space: pre-wrap; - word-break: break-word; + overflow-wrap: break-word; }🤖 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/static/css/create-task.css` around lines 143 - 145, Replace the deprecated word-break: break-word declaration in the affected CSS rule with overflow-wrap: break-word, while preserving the existing white-space behavior.Source: Linters/SAST tools
server/pssm_gremlin_server/static/css/auth-page.css (1)
204-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate dark-theme
.text-inputrule already defined inbase.css.This block repeats the same
background/border-color/colorvalues as the dark-theme override for.text-inputinserver/pssm_gremlin_server/static/css/base.css(lines 305-310). Sincebase.cssis loaded before this file on every auth page, this override is redundant and creates a drift risk if one copy is edited without the other.♻️ Suggested consolidation
-html[data-theme="dark"] .text-input { - background: `#17252c`; - border-color: `#324751`; - color: `#d8e4e9`; -} -Remove this block and rely on the shared rule in
base.css(adjust there if auth pages ever need a different dark value).🤖 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/static/css/auth-page.css` around lines 204 - 208, Remove the duplicate dark-theme html[data-theme="dark"] .text-input rule from auth-page.css and rely on the shared definition in base.css, preserving the existing dark-theme values through that shared rule.server/pssm_gremlin_server/static/js/create-task.js (2)
146-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate system dark-mode sync logic across two files. Both files independently re-implement the same
matchMedia"auto theme" listener instead of using a shared helper fromtheme.js(which both already load).
server/pssm_gremlin_server/static/js/create-task.js#L146-L156: extract this block into aT.watchSystemTheme()(or similar) helper intheme.jsand call it here.server/pssm_gremlin_server/static/js/dashboard.js#L423-L431: replace the identical block with a call to the same shared helper.🤖 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/static/js/create-task.js` around lines 146 - 156, Extract the duplicated system theme synchronization logic into a shared T.watchSystemTheme() helper in theme.js, preserving the auto-mode check and matchMedia change-listener compatibility. Replace the inline blocks in server/pssm_gremlin_server/static/js/create-task.js lines 146-156 and server/pssm_gremlin_server/static/js/dashboard.js lines 423-431 with calls to that helper.
179-190: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
A.authFetchfor the upload request.This handler manually attaches the
Authorizationheader and calls rawfetch, bypassing the centralized 401 handling inauth-api.js. If the token is stale/expired at submit time, the user just sees "Upload failed (HTTP 401)" instead of being redirected to log back in, unlike every other authenticated call in the dashboard.♻️ Proposed fix
- var token = A.getToken(); - var headers = {}; - if (token) headers["Authorization"] = "Bearer " + token; - var response = await fetch("/PSSM_GREMLIN/api/post", { method: "POST", body: formData, headers: headers }); + var response = await A.authFetch("/PSSM_GREMLIN/api/post", { method: "POST", body: formData });🤖 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/static/js/create-task.js` around lines 179 - 190, Update the upload request in the task submission handler to use A.authFetch instead of manually calling A.getToken, constructing the Authorization header, and invoking raw fetch. Preserve the existing POST endpoint and FormData body so centralized authentication and 401 handling are applied.server/pssm_gremlin_server/static/js/user-control.js (2)
164-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray/unclear comment.
"
// ponytail: attach listener per-row..." reads like leftover placeholder text rather than a meaningful note.🤖 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/static/js/user-control.js` at line 164, Remove the stray `ponytail` comment above the per-row checkbox listener; leave the listener implementation unchanged.
262-300: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePassword
minlengthisn't enforced client-side for inline edits.The
minlength="8"attribute on line 266 has no effect since the input isn't inside a<form>and the Save handler never checkspw.lengthbefore submitting. Not a security hole if the server validates, but the UI hint is misleading.🤖 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/static/js/user-control.js` around lines 262 - 300, The inline edit Save handler’s password field constraint is not enforced before submission. In the handler attached to the “.edit-save” control, validate a non-empty `pw` value is at least 8 characters before building or sending the PUT request; show an appropriate alert and return early when it is too short, while preserving blank-password behavior for keeping the current password.
🤖 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 854-916: Update the digest delivery loop in the surrounding
notification function to inspect the boolean result from each _send_email()
call. Treat any False result as delivery failure, unmark all user_ids via
db.unmark_users_notified(user_ids), and preserve the existing exception cleanup
and successful return behavior so failed recipients are retried.
- Around line 734-736: Update the password-reset token flow around the
serializer call and the reset-password handler to include a persisted per-user
reset nonce/version, validate it against the current user record, and atomically
rotate or consume it after a successful reset. Ensure tokens become invalid
immediately after use while preserving the existing user and expiration
validation behavior.
- Around line 646-658: Update _public_base_url to stop using request.host_url or
any Host-header-derived value; construct links exclusively from the configured
SERVER_BASE_URL. Validate SERVER_BASE_URL during production startup and fail
startup when it is missing or invalid, while retaining an appropriate
development fallback only outside production.
- Around line 268-277: The validate_api_key method currently scans and hashes
every stored API key, causing request cost to grow with user count. Add a
deterministic indexed lookup value for each API key, derive that value from the
presented key, query only the matching row, then run check_password_hash on that
row before returning it; update API-key creation/storage and the database schema
consistently, while preserving invalid-key and no-match None behavior.
- Around line 86-108: Update _ensure_columns() to backfill existing users whose
deleted, registration_status, user_status, or role values are NULL, using the
same defaults defined in _users_table. Ensure legacy rows receive false,
"email_sent", "pending", and "user" respectively before list_users() or
UserResponse.model_validate() processes them.
In `@server/pssm_gremlin_server/db.py`:
- Around line 138-140: Update the guarded task-status transition logic near
_is_deleted_status to treat cancelled as terminal alongside the existing deleted
statuses. Ensure late worker writes such as failed, packing results, or finished
cannot overwrite a row already marked cancelled, while preserving current
behavior for other statuses.
In `@server/pssm_gremlin_server/pssm_gremlin.py`:
- Around line 899-933: Update each exception handler in the task execution flow
to check _task_is_deleted(md5sum) before calling _pack_failed_results_archive;
return immediately when the task was deleted, preventing failed-task artifacts
and late updates from being recreated. Apply this to the DockerException and
broad Exception handlers, as well as the preceding failure handler shown.
- Around line 197-216: Update the empty-database bootstrap around _default_pass
and _user_db.create_user so DEFAULT_ADMIN_PASSWORD is required rather than
silently falling back to an unrecoverable random value. Abort or skip admin
creation with a clear error when the environment variable is absent, and only
create the default admin when an explicitly supplied password is available.
In `@server/pssm_gremlin_server/ratelimit.py`:
- Around line 32-56: Update the rate-limiter state management inside decorated
so IP entries with no timestamps newer than cutoff are removed from state,
preventing inactive addresses from accumulating indefinitely. Preserve the
existing request-limit and retry behavior for active entries, and ensure cleanup
occurs while holding _lock.
In `@server/pssm_gremlin_server/routes.py`:
- Around line 1187-1210: Update the batch enable branch in the user iteration to
call verify_email(uid) for each successfully enabled user, matching the
single-user approval flow. Invoke it alongside db.update_user only when
req.action is "enable", while leaving disable, delete, and skipped-user behavior
unchanged.
- Around line 279-287: Wrap the run_gremlin_task.apply_async submission and
task_store.update_task call so Celery broker failures are handled before the
request exits. On submission failure, atomically mark the existing md5sum task
as failed with the error details, or roll back the task row and related
artifacts, ensuring no pending task remains without a Celery job.
- Around line 139-140: Synchronize the authoritative administrator state in the
role-change handlers near the shown admin check and the additional occurrences
at lines 639-645 and 1139-1145. When changing a user to or from the
administrator role, update both role and is_admin atomically, or consistently
derive authorization from one field, so require_admin and task authorization
produce the same result.
- Around line 842-853: Update the verification-resend endpoint around the user
lookup and status checks to always return the same generic response regardless
of whether the email is unknown, deleted, banned, already verified, or eligible.
Preserve internal resend behavior only for eligible accounts, and remove
distinguishable status-specific responses while retaining the existing response
contract for successful requests.
- Around line 1116-1117: Import Any in server/pssm_gremlin_server/routes.py so
the update_fields annotation is defined and Ruff F821 passes. Then run the
repository’s pre-commit checks and formatting workflow, staging any resulting
formatting changes with the edit.
In `@server/pssm_gremlin_server/schemas.py`:
- Around line 65-68: Guard all email validators using mode="before", including
_norm_email, against non-string raw values before calling normalize_email, or
move normalization to mode="after". Ensure invalid types become Pydantic
ValidationError instances so _parse_body() can handle them, and apply the same
fix to every email validator in schemas.py.
In `@server/tests/test_server.py`:
- Around line 1992-1994: Update the test server setup around self.db_path and
_inject_admin_password so the admin password is written to the user database
rather than the task database: define a separate user_db_path, export it through
USER_DB_PATH alongside DB_PATH, and pass user_db_path to _inject_admin_password
in all affected setup paths.
---
Outside diff comments:
In `@server/pssm_gremlin_server/__init__.py`:
- Around line 1-4: Add `from __future__ import annotations` at the top of the
module, immediately after the copyright and license header, so the file follows
the project’s Python 3.10+ annotation syntax guideline.
---
Minor comments:
In @.github/workflows/server-test.yml:
- Around line 31-38: Add persist-credentials: false to the actions/checkout step
named Checkout Repository, leaving the existing repository checkout
configuration and Python setup unchanged.
In `@dev/tools/check_changelog_duplicates.py`:
- Around line 1-2: Add the repository-required GPL-3.0-only copyright header at
the beginning of the Python file, before the existing shebang or module
docstring as prescribed by project conventions. Preserve the current
check-changelog implementation and module documentation unchanged.
In `@server/pssm_gremlin_server/static/js/dashboard.js`:
- Around line 167-176: Update the batch-select checkbox markup in the task-card
rendering flow to provide an accessible name directly on the checkbox, such as
an appropriate aria-label describing selection of the task. Keep the existing
selection behavior, data attributes, and visual layout unchanged.
In `@server/pssm_gremlin_server/static/js/register.js`:
- Around line 70-72: Update the resendRow display assignment in the registration
flow to remove the dead conditional and implement the intended false-case
behavior: hide the row or show the appropriate alternate state when
result.data.email_sent is false, while preserving the visible state for
successful sends.
In `@server/pssm_gremlin_server/templates/email/base.html`:
- Around line 26-34: Update _email_html() and its callers to HTML-escape
user-controlled values before interpolating them into html_body, including
user["username"] and affiliation. Ensure verification, reset, and admin digest
email paths all pass escaped content while preserving the existing template
structure and formatting.
---
Nitpick comments:
In `@server/pssm_gremlin_server/static/css/auth-page.css`:
- Around line 204-208: Remove the duplicate dark-theme html[data-theme="dark"]
.text-input rule from auth-page.css and rely on the shared definition in
base.css, preserving the existing dark-theme values through that shared rule.
In `@server/pssm_gremlin_server/static/css/create-task.css`:
- Around line 143-145: Replace the deprecated word-break: break-word declaration
in the affected CSS rule with overflow-wrap: break-word, while preserving the
existing white-space behavior.
In `@server/pssm_gremlin_server/static/js/create-task.js`:
- Around line 146-156: Extract the duplicated system theme synchronization logic
into a shared T.watchSystemTheme() helper in theme.js, preserving the auto-mode
check and matchMedia change-listener compatibility. Replace the inline blocks in
server/pssm_gremlin_server/static/js/create-task.js lines 146-156 and
server/pssm_gremlin_server/static/js/dashboard.js lines 423-431 with calls to
that helper.
- Around line 179-190: Update the upload request in the task submission handler
to use A.authFetch instead of manually calling A.getToken, constructing the
Authorization header, and invoking raw fetch. Preserve the existing POST
endpoint and FormData body so centralized authentication and 401 handling are
applied.
In `@server/pssm_gremlin_server/static/js/user-control.js`:
- Line 164: Remove the stray `ponytail` comment above the per-row checkbox
listener; leave the listener implementation unchanged.
- Around line 262-300: The inline edit Save handler’s password field constraint
is not enforced before submission. In the handler attached to the “.edit-save”
control, validate a non-empty `pw` value is at least 8 characters before
building or sending the PUT request; show an appropriate alert and return early
when it is too short, while preserving blank-password behavior for keeping the
current password.
🪄 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: 160247eb-56ef-46b6-8c43-261ad494a697
⛔ Files ignored due to path filters (2)
server/pssm_gremlin_server/templates/images/logo.icois excluded by!**/*.icoserver/pssm_gremlin_server/templates/images/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (57)
.github/workflows/server-test.yml.gitignore.pre-commit-config.yamlCHANGELOG.mdCLAUDE.mdMakefiledev/tools/check_changelog_duplicates.pydocs/dev-guide/server.mdprompt/third-party/codex-torvalds.cn.mdserver/.env.exampleserver/README.mdserver/docker-compose.ymlserver/docker/requirements.txtserver/docker/runner/Dockerfileserver/docker/server/Dockerfileserver/docker/server/requirements.txtserver/pssm_gremlin/templates/create_task.htmlserver/pssm_gremlin/templates/pssm_gremlin_dashboard.htmlserver/pssm_gremlin/users.template.txtserver/pssm_gremlin_server/__init__.pyserver/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/pssm_gremlin_server/static/css/auth-page.cssserver/pssm_gremlin_server/static/css/base.cssserver/pssm_gremlin_server/static/css/create-task.cssserver/pssm_gremlin_server/static/css/dashboard.cssserver/pssm_gremlin_server/static/css/error-page.cssserver/pssm_gremlin_server/static/css/user-control.cssserver/pssm_gremlin_server/static/js/auth-api.jsserver/pssm_gremlin_server/static/js/create-task.jsserver/pssm_gremlin_server/static/js/dashboard.jsserver/pssm_gremlin_server/static/js/error-page.jsserver/pssm_gremlin_server/static/js/login.jsserver/pssm_gremlin_server/static/js/profile.jsserver/pssm_gremlin_server/static/js/register.jsserver/pssm_gremlin_server/static/js/theme.jsserver/pssm_gremlin_server/static/js/user-control.jsserver/pssm_gremlin_server/templates/create_task.htmlserver/pssm_gremlin_server/templates/email/base.htmlserver/pssm_gremlin_server/templates/error.htmlserver/pssm_gremlin_server/templates/login.htmlserver/pssm_gremlin_server/templates/profile.htmlserver/pssm_gremlin_server/templates/pssm_gremlin_dashboard.htmlserver/pssm_gremlin_server/templates/register.htmlserver/pssm_gremlin_server/templates/reset-password.htmlserver/pssm_gremlin_server/templates/terms.htmlserver/pssm_gremlin_server/templates/user_control.htmlserver/pssm_gremlin_server/templates/verify-email.htmlserver/pyproject.tomlserver/run/restart_pssm_flask.shserver/tests/conftest.pyserver/tests/test_server.pytests/dev_tools/test_check_changelog_duplicates.py
💤 Files with no reviewable changes (6)
- server/pssm_gremlin/users.template.txt
- prompt/third-party/codex-torvalds.cn.md
- server/docker/requirements.txt
- server/docker/server/requirements.txt
- server/pssm_gremlin/templates/pssm_gremlin_dashboard.html
- server/pssm_gremlin/templates/create_task.html
| _users_table = sa.Table( | ||
| "users", | ||
| _metadata, | ||
| sa.Column("id", sa.Integer, primary_key=True), | ||
| sa.Column("username", sa.String(128), nullable=False, unique=True, index=True), | ||
| sa.Column("email", sa.String(256), nullable=False, unique=True), | ||
| sa.Column("password_hash", sa.String(256), nullable=False), | ||
| sa.Column("email_verified", sa.Boolean, nullable=False, default=False), | ||
| sa.Column("is_admin", sa.Boolean, nullable=False, default=False), | ||
| sa.Column("created_at", sa.Float, nullable=False), | ||
| sa.Column("api_key_hash", sa.String(256), nullable=True), | ||
| sa.Column("affiliation", sa.String(256), nullable=True), | ||
| sa.Column("terms_agreed", sa.Boolean, nullable=False, default=False), | ||
| sa.Column("registration_status", sa.String(32), nullable=False, default="email_sent"), | ||
| sa.Column("user_status", sa.String(32), nullable=False, default="pending"), | ||
| sa.Column("approved_by", sa.Integer, nullable=True), | ||
| sa.Column("approved_at", sa.Float, nullable=True), | ||
| sa.Column("deleted", sa.Boolean, nullable=False, default=False), | ||
| sa.Column("role", sa.String(32), nullable=False, default="user"), | ||
| sa.Column("admin_notified", sa.Boolean, nullable=False, default=False), | ||
| sa.Column("registration_ip", sa.String(45), nullable=True), | ||
| sa.Column("registration_country", sa.String(8), nullable=True), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth.py symbols ==\n'
ast-grep outline server/pssm_gremlin_server/auth.py --view expanded || true
printf '\n== search for resend_count/resend_at/deleted IS NULL/UserResponse ==\n'
rg -n "verification_resend_count|verification_resend_at|deleted IS NULL|UserResponse|update_user\\(|list_users\\(" server/pssm_gremlin_server -S || true
printf '\n== migration files mentioning users table ==\n'
rg -n "verification_resend_count|verification_resend_at|registration_status|user_status|deleted|approved_by|approved_at|api_key_hash|terms_agreed|admin_notified|registration_ip|registration_country" server -g '*.py' -S || trueRepository: YaoYinYing/REvoDesign
Length of output: 26529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read focused sections once file locations are known.
sed -n '1,240p' server/pssm_gremlin_server/auth.pyRepository: YaoYinYing/REvoDesign
Length of output: 9261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- auth.py (1-260) ---'
nl -ba server/pssm_gremlin_server/auth.py | sed -n '1,260p'Repository: YaoYinYing/REvoDesign
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
ls -1Repository: YaoYinYing/REvoDesign
Length of output: 375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
def show(path, start, end):
print(f"\n--- {path} {start}-{end} ---")
lines = Path(path).read_text().splitlines()
for i in range(start - 1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
show("server/pssm_gremlin_server/auth.py", 140, 175)
show("server/pssm_gremlin_server/auth.py", 218, 245)
show("server/pssm_gremlin_server/auth.py", 300, 315)
show("server/pssm_gremlin_server/schemas.py", 150, 175)
PYRepository: YaoYinYing/REvoDesign
Length of output: 4948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
("server/pssm_gremlin_server/auth.py", 140, 175),
("server/pssm_gremlin_server/auth.py", 218, 245),
("server/pssm_gremlin_server/auth.py", 300, 315),
("server/pssm_gremlin_server/schemas.py", 150, 175),
]:
print(f"\n--- {path} {start}-{end} ---")
lines = Path(path).read_text().splitlines()
for i in range(start - 1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: YaoYinYing/REvoDesign
Length of output: 4948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = "server/pssm_gremlin_server/schemas.py"
lines = Path(path).read_text().splitlines()
for start, end in [(150, 180), (1, 120)]:
print(f"\n--- {path} {start}-{end} ---")
for i in range(start - 1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: YaoYinYing/REvoDesign
Length of output: 5705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "class UserResponse|role:|registration_status:|user_status:|deleted:" server/pssm_gremlin_server/schemas.py -n -SRepository: YaoYinYing/REvoDesign
Length of output: 496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "legacy|NULL|deleted IS NULL|registration_status.*None|user_status.*None|role.*None|model_validate\\(u\\)" server/tests server/pssm_gremlin_server -SRepository: YaoYinYing/REvoDesign
Length of output: 1153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("server/pssm_gremlin_server/auth.py")
lines = path.read_text().splitlines()
for start, end in [(385, 405), (145, 172)]:
print(f"\n--- {path} {start}-{end} ---")
for i in range(start - 1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: YaoYinYing/REvoDesign
Length of output: 2747
Backfill legacy users in _ensure_columns() (server/pssm_gremlin_server/auth.py:146-172). Upgraded DBs still leave deleted, registration_status, user_status, and role as NULL; that hides old accounts from list_users() and can make UserResponse.model_validate() fail on the admin users endpoint.
🤖 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 86 - 108, Update
_ensure_columns() to backfill existing users whose deleted, registration_status,
user_status, or role values are NULL, using the same defaults defined in
_users_table. Ensure legacy rows receive false, "email_sent", "pending", and
"user" respectively before list_users() or UserResponse.model_validate()
processes them.
| def validate_api_key(self, key: str) -> dict[str, Any] | None: | ||
| """Return the user dict if *key* matches a stored API key, or ``None``.""" | ||
| if not key or not key.startswith("revodesign_"): | ||
| return None | ||
| users = sa.select(_users_table).where(_users_table.c.api_key_hash.isnot(None)) | ||
| with self.engine.connect() as conn: | ||
| for row in conn.execute(users).mappings(): | ||
| if check_password_hash(row["api_key_hash"], key): | ||
| return dict(row) | ||
| return None |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and search for API key-related definitions/usages.
wc -l server/pssm_gremlin_server/auth.py
sed -n '1,380p' server/pssm_gremlin_server/auth.py
printf '\n--- SEARCH api_key_hash / API key usage ---\n'
rg -n "api_key_hash|validate_api_key|api key|api_key" server/pssm_gremlin_server -SRepository: YaoYinYing/REvoDesign
Length of output: 21897
Avoid scanning every API-key row on each request. validate_api_key() does a linear check_password_hash() over all users with an API key, so auth cost grows with account count and can become an availability bottleneck. Store an indexed deterministic lookup value (for example, a key ID or HMAC digest) and only verify the matching row.
🤖 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 268 - 277, The
validate_api_key method currently scans and hashes every stored API key, causing
request cost to grow with user count. Add a deterministic indexed lookup value
for each API key, derive that value from the presented key, query only the
matching row, then run check_password_hash on that row before returning it;
update API-key creation/storage and the database schema consistently, while
preserving invalid-key and no-match None behavior.
| def _public_base_url() -> str: | ||
| """Return the public-facing base URL for email links. | ||
|
|
||
| Uses the current request's ``Host`` header so email links point to the | ||
| same domain the user is accessing. Falls back to ``SERVER_BASE_URL`` | ||
| env var, then ``http://localhost:8080`` (dev). | ||
| """ | ||
| # ponytail: request.host_url already has scheme+host from the Host header | ||
| try: | ||
| return request.host_url.rstrip("/") | ||
| except RuntimeError: | ||
| pass # outside request context (tests, scripts) | ||
| return _env_str("SERVER_BASE_URL", "http://localhost:8080").rstrip("/") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not build password-reset or verification links from the request Host header.
An attacker-controlled Host header can place valid account tokens into links pointing at an attacker domain. Generate email links exclusively from a validated, configured SERVER_BASE_URL; reject startup when it is absent in production.
🤖 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 646 - 658, Update
_public_base_url to stop using request.host_url or any Host-header-derived
value; construct links exclusively from the configured SERVER_BASE_URL. Validate
SERVER_BASE_URL during production startup and fail startup when it is missing or
invalid, while retaining an appropriate development fallback only outside
production.
| token = _serializer.dumps({"uid": user["id"], "purpose": "reset-password"}) | ||
| base_url = _public_base_url() | ||
| reset_url = f"{base_url}/PSSM_GREMLIN/reset_password?c={token}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make password-reset tokens single-use.
The token only contains uid and remains valid for one hour after a successful reset, allowing anyone retaining it to repeatedly replace the password. Persist a per-user reset nonce/version and consume or rotate it atomically after use.
Also applies to: 919-927
🤖 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 734 - 736, Update the
password-reset token flow around the serializer call and the reset-password
handler to include a persisted per-user reset nonce/version, validate it against
the current user record, and atomically rotate or consume it after a successful
reset. Ensure tokens become invalid immediately after use while preserving the
existing user and expiration validation behavior.
| # Mark first, send second — prevents duplicate digests when multiple | ||
| # gunicorn workers or celery processes race. Unmark on failure so the | ||
| # users appear in the next digest. | ||
| user_ids = [u["id"] for u in new_users] | ||
| db.mark_users_notified(user_ids) | ||
|
|
||
| base_url = _env_str("SERVER_BASE_URL", "http://localhost:8080").rstrip("/") | ||
| rows = [] | ||
| for u in new_users: | ||
| created = datetime.fromtimestamp(u["created_at"]).strftime("%Y-%m-%d %H:%M") if u.get("created_at") else "?" | ||
| rows.append(f" {u['username']:<20} {u['email']:<32} {u.get('affiliation', '-') or '-':<24} {created}") | ||
|
|
||
| text = ( | ||
| f"{len(new_users)} new registration(s) pending approval:\n\n" | ||
| f" {'Username':<20} {'Email':<32} {'Affiliation':<24} {'Registered'}\n" | ||
| f" {'-' * 20:<20} {'-' * 32:<32} {'-' * 24:<24} {'-' * 16}\n" | ||
| + "\n".join(rows) | ||
| + f"\n\n Review: {base_url}/PSSM_GREMLIN/user_control\n\n" | ||
| f"— REvoDesign GREMLIN Server\n" | ||
| ) | ||
| # Build HTML table rows | ||
| html_rows = [] | ||
| for u in new_users: | ||
| created = datetime.fromtimestamp(u["created_at"]).strftime("%Y-%m-%d %H:%M") if u.get("created_at") else "?" | ||
| affil = u.get("affiliation") or "-" | ||
| html_rows.append( | ||
| f"<tr>" | ||
| f'<td style="padding:8px 12px;border-bottom:1px solid #e5e7eb;">{u["username"]}</td>' | ||
| f'<td style="padding:8px 12px;border-bottom:1px solid #e5e7eb;">{u["email"]}</td>' | ||
| f'<td style="padding:8px 12px;border-bottom:1px solid #e5e7eb;">{affil}</td>' | ||
| f'<td style="padding:8px 12px;border-bottom:1px solid #e5e7eb;">{created}</td>' | ||
| f"</tr>" | ||
| ) | ||
| html_body = ( | ||
| f"<p>{len(new_users)} new registration(s) pending approval:</p>" | ||
| f'<table style="width:100%;border-collapse:collapse;margin:16px 0;' | ||
| f'font-size:14px;" cellpadding="0" cellspacing="0">' | ||
| f'<thead><tr style="background-color:#f3f4f6;text-align:left;">' | ||
| f'<th style="padding:8px 12px;">Username</th>' | ||
| f'<th style="padding:8px 12px;">Email</th>' | ||
| f'<th style="padding:8px 12px;">Affiliation</th>' | ||
| f'<th style="padding:8px 12px;">Registered</th>' | ||
| f"</tr></thead><tbody>" | ||
| + "".join(html_rows) | ||
| + "</tbody></table>" | ||
| f'<p style="margin:24px 0;">' | ||
| f'<a href="{base_url}/PSSM_GREMLIN/user_control" style="display:inline-block;' | ||
| f"padding:12px 24px;background-color:#1a1a2e;color:#ffffff;" | ||
| f"text-decoration:none;border-radius:6px;font-weight:600;\">" | ||
| f"Review Registrations</a></p>" | ||
| ) | ||
| try: | ||
| for email in recipients: | ||
| _send_email( | ||
| to=email, | ||
| subject=f"{len(new_users)} new registration(s) — REvoDesign GREMLIN", | ||
| text=text, | ||
| html=_email_html(html_body), | ||
| ) | ||
| except Exception: | ||
| db.unmark_users_notified(user_ids) | ||
| raise | ||
| return True |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retry digest recipients when _send_email() returns False.
Users are marked notified before delivery, but _send_email() reports SMTP/Resend failures by returning False, not raising. The current except therefore does not restore their flags, permanently dropping failed digest notifications.
🤖 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 854 - 916, Update the digest
delivery loop in the surrounding notification function to inspect the boolean
result from each _send_email() call. Treat any False result as delivery failure,
unmark all user_ids via db.unmark_users_notified(user_ids), and preserve the
existing exception cleanup and successful return behavior so failed recipients
are retried.
| db = _get_user_db() | ||
| user = db.get_user_by_email(email) | ||
| if user is None: | ||
| return jsonify({"error": "No account found with this email address"}), 404 | ||
|
|
||
| if user.get("deleted"): | ||
| return jsonify({"error": "Account has been deleted"}), 403 | ||
| if user.get("user_status") == "banned": | ||
| return jsonify({"error": "Account has been suspended"}), 403 | ||
|
|
||
| if user.get("email_verified"): | ||
| return jsonify({"message": "This email is already verified. You can log in."}), 200 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve account non-enumeration in verification resends.
This unauthenticated endpoint returns distinguishable responses for unknown, deleted, banned, verified, and unverified accounts. Attackers can enumerate registered emails and account status. Return the same generic response for every case while performing eligible resends internally.
🤖 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 842 - 853, Update the
verification-resend endpoint around the user lookup and status checks to always
return the same generic response regardless of whether the email is unknown,
deleted, banned, already verified, or eligible. Preserve internal resend
behavior only for eligible accounts, and remove distinguishable status-specific
responses while retaining the existing response contract for successful
requests.
| # Build update dict from set fields only (all optional) | ||
| update_fields: dict[str, Any] = {} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Import Any so the configured pre-commit checks pass.
Ruff reports F821 for the annotation on Line 1117.
Proposed fix
import time
+from typing import AnyAs per coding guidelines, run the repository's pre-commit checks and formatting workflow before committing; formatting changes must be staged with the edits.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 1117-1117: Undefined name Any
(F821)
🤖 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 1116 - 1117, Import Any in
server/pssm_gremlin_server/routes.py so the update_fields annotation is defined
and Ruff F821 passes. Then run the repository’s pre-commit checks and formatting
workflow, staging any resulting formatting changes with the edit.
Sources: Coding guidelines, Linters/SAST tools
| if req.action == "enable": | ||
| updates = { | ||
| "user_status": "active", | ||
| "registration_status": "approved", | ||
| "deleted": False, | ||
| "approved_by": admin_id, | ||
| "approved_at": now, | ||
| } | ||
| elif req.action == "disable": | ||
| updates = {"user_status": "banned", "approved_by": admin_id, "approved_at": now} | ||
| else: # delete | ||
| updates = {"deleted": True} | ||
|
|
||
| count = 0 | ||
| for uid in req.user_ids: | ||
| user = db.get_user(uid) | ||
| if user is None: | ||
| continue | ||
| if uid == admin_id and req.action in {"disable", "delete"}: | ||
| continue # don't let an admin lock themselves out | ||
| if user.get("is_admin") and req.action == "disable": | ||
| continue # don't disable other admins | ||
| db.update_user(uid, **updates) | ||
| count += 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify email when batch-enabling users.
The enable operation sets accounts to active and approved, but _is_account_blocked() still rejects them while email_verified=False. Call verify_email(uid) during enable, matching single-user approval behavior.
🤖 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 1187 - 1210, Update the
batch enable branch in the user iteration to call verify_email(uid) for each
successfully enabled user, matching the single-user approval flow. Invoke it
alongside db.update_user only when req.action is "enable", while leaving
disable, delete, and skipped-user behavior unchanged.
| @field_validator("email", mode="before") | ||
| @classmethod | ||
| def _norm_email(cls, v: str) -> str: | ||
| return normalize_email(v) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== schemas.py outline ==\n'
ast-grep outline server/pssm_gremlin_server/schemas.py --view expanded || true
printf '\n== relevant lines ==\n'
nl -ba server/pssm_gremlin_server/schemas.py | sed -n '1,220p'
printf '\n== parse body/search ==\n'
rg -n "_parse_body|normalize_email|field_validator\\(\"email\"|field_validator\\(\".*email" server/pssm_gremlin_server -nRepository: YaoYinYing/REvoDesign
Length of output: 2634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("server/pssm_gremlin_server/schemas.py")
print(path.read_text())
PYRepository: YaoYinYing/REvoDesign
Length of output: 5508
🌐 Web query:
Pydantic v2 field_validator before exception AttributeError validation error TypeError converted
💡 Result:
In Pydantic V2, validation functions (including @field_validator with mode='before') do not automatically convert generic Python exceptions like TypeError or AttributeError into Pydantic ValidationError instances [1][2]. Unlike some other validation libraries, Pydantic V2 explicitly allows these errors to propagate (bubble up) and terminate the program, rather than wrapping them [1][3][2]. To ensure that custom validation logic or operations that might raise TypeError or AttributeError are correctly reported as Pydantic validation errors, you must explicitly catch these exceptions within your validator function and re-raise them as ValueError (or PydanticCustomError) [3]. Recommended Pattern: def validator_function(v): try: # Code that might raise TypeError or AttributeError return some_operation(v) except (TypeError, AttributeError) as e: # Re-raise as ValueError to be captured by Pydantic raise ValueError(f'Validation failed due to internal error: {e}') If you do not catch these errors, they will bypass Pydantic's error handling and cause your application to crash or behave unexpectedly, as they are not treated as formal validation failures [3]. Additionally, if you encounter a TypeError during class initialization (e.g., related to validator function signatures or unexpected keyword arguments), this is often a sign of a version mismatch between pydantic and pydantic-core [4]. Ensure that both packages are updated to compatible versions [4].
Citations:
- 1: https://pydantic.dev/docs/validation/2.9/concepts/validators/
- 2: https://pydantic.dev/docs/validation/2.2/usage/validators/
- 3: Update PlainValidator docstring and migration docs for TypeError handling pydantic/pydantic#11953
- 4: Using
@field_validatorthere is an TypeError: no_info_before_validator_function() got an unexpected keyword argument 'json_schema_input_schema' pydantic/pydantic#11159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== _parse_body references ==\n'
rg -n "_parse_body|RegisterRequest|AdminCreateUserRequest|ForgotPasswordRequest|normalize_email\\(" server/pssm_gremlin_server -n -A 4 -B 4
printf '\n== schemas.py numbered excerpt ==\n'
python3 - <<'PY'
from pathlib import Path
for i, line in enumerate(Path("server/pssm_gremlin_server/schemas.py").read_text().splitlines(), 1):
if 1 <= i <= 160:
print(f"{i:4}: {line}")
PYRepository: YaoYinYing/REvoDesign
Length of output: 19433
Guard these mode="before" email validators. normalize_email() calls strip() on raw input, so non-string values like {"email": 123} can raise AttributeError before Pydantic turns them into a ValidationError, and _parse_body() only catches ValidationError. Type-check first or move normalization to mode="after"; the same applies to the other email validators here.
🤖 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/schemas.py` around lines 65 - 68, Guard all email
validators using mode="before", including _norm_email, against non-string raw
values before calling normalize_email, or move normalization to mode="after".
Ensure invalid types become Pydantic ValidationError instances so _parse_body()
can handle them, and apply the same fix to every email validator in schemas.py.
| self.username = "admin" | ||
| self.password = password | ||
| self.users_file.write_text(f"{self.username}:{self.password}\n", encoding="utf-8") | ||
| self.db_path = self.state_dir / "pssm_gremlin.sqlite3" | ||
| self.db_path.touch() | ||
| self.db_path = self.state_dir / "pssm_gremlin_server.sqlite3" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Inject the admin password into the user database, not the task database.
self.db_path is passed as DB_PATH, which configures TaskDatabase. UserDatabase defaults to SERVER_DIR/users.sqlite3, so _inject_admin_password(self.db_path, ...) targets a database without a users table. Add a separate user_db_path, export it as USER_DB_PATH, and inject that file.
Also applies to: 2005-2008, 2038-2040
🤖 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/tests/test_server.py` around lines 1992 - 1994, Update the test server
setup around self.db_path and _inject_admin_password so the admin password is
written to the user database rather than the task database: define a separate
user_db_path, export it through USER_DB_PATH alongside DB_PATH, and pass
user_db_path to _inject_admin_password in all affected setup paths.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
==========================================
+ Coverage 73.55% 73.56% +0.01%
==========================================
Files 122 122
Lines 15221 15221
==========================================
+ Hits 11196 11198 +2
+ Misses 4025 4023 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
* fix(server): harden security, fix review issues from PR #189 - 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> * fix(server): address CI failure and CodeRabbit review comments - 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> * fix(server): make column backfills idempotent (run every startup) 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> * ci: add codecov.yml with server flag path mapping 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> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Major GREMLIN server upgrade: token-based authentication (replaces HTTP Basic Auth), self-service registration with email verification, admin user management panel, role system, HTML email templates, and comprehensive security hardening.
Highlights
Authentication & Registration
itsdangerous.URLSafeTimedSerializer(zero new deps)admin,user,guest— guests restricted to cookie-only web loginX-API-Keyheader with restricted privileges@presenceAdmin Tools
/PSSM_GREMLIN/user_control:Email
templates/email/base.htmlpip install "server/[resend]"), with SMTP stdlib fallbackhello@(not noreply@) per deliverability best practiceSecurity
_safe_join,_path_is_within)[a-f0-9]{32})HttpOnly+SameSite=Lax+Secureon HTTPSrunuser)Data Integrity
_ensure_columnspattern with backfill for existing users{owner_scoped_md5}.fasta— no cross-user collisionsCLIENT_IP_HEADERS,CLIENT_COUNTRY_HEADER)fcntl.flock+ mark-before-sendBug Fixes (this PR)
escapeAttr()was stripping@,., spaces from email/affiliation in user edit form → data corruptionapproved_by/approved_atoverwritten on every admin edit (not just approval)Testing
server/tests/with Bearer-token auth.github/workflows/server-test.yml🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation