fix(security): authenticate shutdown endpoints with a per-session token#1305
fix(security): authenticate shutdown endpoints with a per-session token#1305Dotify71 wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a per-startup shutdown token written to a temp file; backend and sync microservice require and validate X-Shutdown-Token headers; Windows frontend reads the temp token and includes it on shutdown POSTs. ChangesShutdown Endpoint Token Authentication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src-tauri/src/main.rs (1)
89-97: ⚡ Quick winLog non-success status codes for easier debugging
Currently only success responses are logged. Logging 4xx/5xx responses would help diagnose authentication or other issues:
♻️ Proposed refactor to improve diagnostics
match client.post(*url).headers(headers.clone()).send() { Ok(resp) => { let status = resp.status(); if status.is_success() { println!("[{}] Shutdown OK ({})", name, status); + } else { + eprintln!("[{}] Shutdown failed ({})", name, status); } } - Err(_err) => {} + Err(err) => { + eprintln!("[{}] Shutdown request error: {}", name, err); + } }🤖 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 `@frontend/src-tauri/src/main.rs` around lines 89 - 97, The shutdown response handling only logs successful statuses and drops errors; update the Ok(resp) branch that inspects resp.status() (variables resp and status, and identifier name) to log non-success HTTP statuses (4xx/5xx) along with the status code and any useful response body or error text for diagnostics, and replace the empty Err(_err) arm with a logged error including the error details so both non-success Ok and Err paths produce actionable logs.
🤖 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 `@backend/app/config/settings.py`:
- Around line 49-50: The shutdown token file is created with default
(world-readable) permissions; update the write logic that uses
SHUTDOWN_TOKEN_FILE and SHUTDOWN_TOKEN so the file is created with restrictive
permissions (owner read/write only). Implement this by opening/creating the file
with secure mode (e.g., use os.open with flags and mode 0o600, or set
os.fchmod/os.chmod to 0o600 immediately after creation) and ensure the token is
written and the descriptor closed; avoid a window where the file is briefly
world-readable by setting permissions atomically at creation time.
In `@frontend/src-tauri/src/main.rs`:
- Around line 70-73: The code silently treats a missing/empty shutdown token by
using unwrap_or_default on the result of reading token_path which causes missing
header errors; change the logic around token_path and token so that
read_to_string errors are detected and logged (include the error from
std::fs::read_to_string when it Errs) and also emit a warning when the token
string is empty before attempting shutdown; update the block that constructs
token (and any caller that uses token) to log a clear warning including
token_path and the read error or an "empty token" message so backend 422s are
diagnosable.
In `@sync-microservice/app/config/settings.py`:
- Around line 38-44: The current settings.py block reads SHUTDOWN_TOKEN_FILE
once and falls back to generating SHUTDOWN_TOKEN independently, causing
mismatched tokens if the backend writes the file later; change this to attempt
repeated reads of SHUTDOWN_TOKEN_FILE with a short retry loop (e.g., sleep + max
timeout) before generating a fallback token so the sync service uses the backend
token when it becomes available; update the logic around SHUTDOWN_TOKEN_FILE and
SHUTDOWN_TOKEN to retry for a configurable period and only generate
secrets.token_hex(32) after the retry period elapses.
---
Nitpick comments:
In `@frontend/src-tauri/src/main.rs`:
- Around line 89-97: The shutdown response handling only logs successful
statuses and drops errors; update the Ok(resp) branch that inspects
resp.status() (variables resp and status, and identifier name) to log
non-success HTTP statuses (4xx/5xx) along with the status code and any useful
response body or error text for diagnostics, and replace the empty Err(_err) arm
with a logged error including the error details so both non-success Ok and Err
paths produce actionable logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 720a4107-cad4-43e8-b22e-8b39fd99b609
📒 Files selected for processing (5)
backend/app/config/settings.pybackend/app/routes/shutdown.pyfrontend/src-tauri/src/main.rssync-microservice/app/config/settings.pysync-microservice/app/routes/shutdown.py
Dotify71
force-pushed
the
fix/authenticated-shutdown-endpoint
branch
from
3997f8b to
2df4123
Compare
The /shutdown endpoints on both the backend (port 52123) and sync microservice (port 52124) were unauthenticated, allowing any local process to send a POST request and terminate PictoPy without user interaction. Fix: - Generate a cryptographically random 256-bit token (secrets.token_hex) on every backend startup and write it to a temporary file (pictopy_shutdown.token in the OS temp directory). - Both shutdown endpoints now require an X-Shutdown-Token header whose value is compared against the session token using hmac.compare_digest to prevent timing-based attacks. Requests with a missing or incorrect token receive 403 Forbidden. - The sync microservice reads the same token file written by the backend, so both services share one token without additional coordination. - The Tauri frontend (Windows path) is updated to read the token file at shutdown time and attach it as an X-Shutdown-Token header, preserving the existing Windows close behaviour. Closes AOSSIE-Org#1241
Dotify71
force-pushed
the
fix/authenticated-shutdown-endpoint
branch
from
2df4123 to
bc4968a
Compare
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
|
Hi maintainers, |
|
@coderabbitai, please review the pull request. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== backend/app/config/settings.py ==="
cat backend/app/config/settings.py
echo ""
echo "=== backend/app/routes/shutdown.py ==="
cat backend/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.py
echo ""
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== frontend/src-tauri/src/main.rs ==="
cat frontend/src-tauri/src/main.rsLength of output: 21134
Review: fix(security): authenticate shutdown endpoints with a per-session tokenThe PR correctly addresses the core vulnerability ( 🔴 High — Blocking sleep at module import time (
|
- Moved blocking sleep out of settings.py into FastAPI lifespan hook - Removed silent broken-state fallback token; now raises RuntimeError - Added error handling and chmod to token write in backend - Fixed 422 vs 401 response code on missing token - Ensured token file is cleaned up after shutdown - Fixed Rust kill_process visibility and error logging
|
@coderabbitai review: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== backend/app/config/settings.py ==="
cat backend/app/config/settings.py
echo ""
echo "=== backend/app/routes/shutdown.py ==="
cat backend/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.py
echo ""
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.py 2>/dev/null || echo "[FILE NOT FOUND]"
echo ""
echo "=== frontend/src-tauri/src/main.rs ==="
cat frontend/src-tauri/src/main.rsLength of output: 25461
🔴 Critical —
|
| # | Finding | Status |
|---|---|---|
| 1 | Blocking sleep at module import | ✅ Moved to lifespan hook |
| 2 | Silent broken-state / fallback token | ✅ Raises RuntimeError |
| 3 | No error handling on token-file write | ✅ try/except OSError + sys.exit(1) |
| 4 | 0o600 not enforced on pre-existing files |
✅ os.chmod added |
| 5 | Missing token returns 422 instead of 401 | ✅ 401/403 split correctly |
| 6 | Rust HTTP errors silently discarded | ✅ eprintln! added |
| 7 | pub fn kill_process / signature mismatch |
✅ fn on both platforms, Unix returns Result |
| 8 | Token file never cleaned up | ✅ os.remove in _delayed_shutdown |
Once the two issues above are addressed (particularly the critical SHUTDOWN_TOKEN import fix), the PR should be in good shape. 🐇
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sync-microservice/app/config/settings.py (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused imports.
The imports
secrets,time, andwarningsare not used in this file. They appear to be remnants from the previous implementation that performed token polling at module level. As per coding guidelines, unused imports violate PEP 8.🧹 Proposed fix
import os -import secrets import tempfile -import time as _time -import warnings from platformdirs import user_data_dirAlso applies to: 4-5
🤖 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 `@sync-microservice/app/config/settings.py` at line 2, Remove the unused imports from the module: delete the import statements for secrets, time, and warnings in settings.py (they are no longer used after removing token polling at module level); keep only the imports that are referenced elsewhere in the file so the module conforms to PEP8 and avoids unused-import warnings.Source: Coding guidelines
🧹 Nitpick comments (1)
sync-microservice/app/core/lifespan.py (1)
31-31: 💤 Low valueMove import to module level.
Importing
settingsinside the function is not idiomatic Python. Unless there's a circular import issue (none is evident), move it to the top of the file with other imports.♻️ Proposed refactor
from contextlib import asynccontextmanager from fastapi import FastAPI import time +import app.config.settings as settings from app.database.folders import ( db_check_database_connection, )Then remove the import from line 31:
logger.info("Starting PictoPy Sync Microservice...") # Wait for shutdown token from backend (up to 5 seconds) - import app.config.settings as settings logger.info("Waiting for shutdown token from backend...")🤖 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 `@sync-microservice/app/core/lifespan.py` at line 31, Move the local import "import app.config.settings as settings" out of the function and into the module-level imports at the top of sync-microservice/app/core/lifespan.py alongside the other imports, then remove the in-function import statement; this keeps settings available as the module-level name "settings" for any functions/classes in the file (e.g., the lifespan function) and avoids non-idiomatic runtime imports.
🤖 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 `@sync-microservice/app/core/lifespan.py`:
- Around line 36-43: The current loop only catches FileNotFoundError when
reading settings.SHUTDOWN_TOKEN_FILE, leaving other IO errors unhandled; update
the read block around open(...) / token = f.read().strip() to also catch
PermissionError and OSError (or a broad Exception) and log the failure using
logger.error with the exception details, while preserving the existing behavior
of setting settings.SHUTDOWN_TOKEN and calling logger.info when successful;
reference the settings.SHUTDOWN_TOKEN_FILE, settings.SHUTDOWN_TOKEN and
logger.info/logger.error so reviewers can find and update the open/read/except
handling accordingly.
---
Outside diff comments:
In `@sync-microservice/app/config/settings.py`:
- Line 2: Remove the unused imports from the module: delete the import
statements for secrets, time, and warnings in settings.py (they are no longer
used after removing token polling at module level); keep only the imports that
are referenced elsewhere in the file so the module conforms to PEP8 and avoids
unused-import warnings.
---
Nitpick comments:
In `@sync-microservice/app/core/lifespan.py`:
- Line 31: Move the local import "import app.config.settings as settings" out of
the function and into the module-level imports at the top of
sync-microservice/app/core/lifespan.py alongside the other imports, then remove
the in-function import statement; this keeps settings available as the
module-level name "settings" for any functions/classes in the file (e.g., the
lifespan function) and avoids non-idiomatic runtime imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8925eed0-8fdc-45db-b9f4-3840bd2a8aa8
📒 Files selected for processing (6)
backend/app/config/settings.pybackend/app/routes/shutdown.pyfrontend/src-tauri/src/main.rssync-microservice/app/config/settings.pysync-microservice/app/core/lifespan.pysync-microservice/app/routes/shutdown.py
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/app/config/settings.py
- backend/app/routes/shutdown.py
- sync-microservice/app/routes/shutdown.py
- frontend/src-tauri/src/main.rs
- Removed unused imports in settings.py left over from old token polling - Moved settings import to module level in lifespan.py - Added generic Exception catch in the token read loop for broader error logging
|
@coderabbitai review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.py
echo ""
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.pyLength of output: 9009
🔴 Still open —
|
- Fixed SHUTDOWN_TOKEN being captured as empty string at import time in sync shutdown route - Replaced blocking time.sleep() calls with await asyncio.sleep() in async lifespan
|
@coderabbitai review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.pyLength of output: 7275
All 10 findings across all three rounds of review are now addressed. The PR is in great shape — well done on the thorough follow-through! 🐇 🎉 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.py
echo ""
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.pyLength of output: 9036
One tiny nit (non-blocking): All 10 findings from across the full review are now resolved. The PR looks solid — the security fix is correct, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== backend/app/config/settings.py ==="
cat backend/app/config/settings.py
echo ""
echo "=== backend/app/routes/shutdown.py ==="
cat backend/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.py
echo ""
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.py
echo ""
echo "=== frontend/src-tauri/src/main.rs ==="
cat frontend/src-tauri/src/main.rsLength of output: 25496
✅ Full finding status (all 10 rounds)
One lingering nit (non-blocking): The PR is in excellent shape. 🐇 🎉 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== backend/app/config/settings.py ==="
cat backend/app/config/settings.py
echo ""
echo "=== backend/app/routes/shutdown.py ==="
cat backend/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.py
echo ""
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== frontend/src-tauri/src/main.rs ==="
cat frontend/src-tauri/src/main.rsLength of output: 25496
Final review summary
The one remaining cosmetic nit ( The PR is in great shape. 🐇 🎉 |
|
@Dotify71 One more thing before I do a proper review: please try to avoid leaving messy comments in the codebase. Comments are definitely appreciated, but it’s best to keep them concise and clean. Single-line comments are preferred, and at most two lines in rare cases. |
- Removed local `from app.config.settings import SHUTDOWN_TOKEN_FILE` inside `_delayed_shutdown()` - Used module-level `settings.SHUTDOWN_TOKEN_FILE` for consistency across both backends
- Shortened long multi-line comments explaining the token exchange logic into single-line comments as requested by maintainer
|
@coderabbitai review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== backend/app/config/settings.py ==="
cat backend/app/config/settings.py
echo ""
echo "=== backend/app/routes/shutdown.py ==="
cat backend/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.py
echo ""
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== frontend/src-tauri/src/main.rs ==="
cat frontend/src-tauri/src/main.rsLength of output: 23671
✅ Full final review summary
The comment cleanup is also well done — all verbose multi-line blocks are gone, replaced with concise single-line comments throughout. All 11 findings are fully resolved, and no new issues were introduced. The PR is in great shape. 🐇 🎉 |
|
@rohan-pandeyy Good call! I went through the codebase and stripped down all the verbose multi-line explanations about the token exchange logic. They've all been replaced with concise, single-line comments. Everything is pushed and ready for your review! |
|
Hi @rahulharpal1603, just following up here! I've made sure this PR is fully up to date, and all the requested changes and merge conflicts (if any) have been completely resolved. Could you please take a look and review when you have a moment? Thank you! |
|
@Dotify71, before merging I’d like to see a few additional test results around shutdown behavior and failure modes:
@coderabbitai, can you help cover any additional scenarios for the codebase that required additional testing? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find all test files in the repository
echo "=== Test files ==="
fd -e py -g '*test*' --type f
echo ""
fd -e py -g 'test_*' --type f
echo ""
fd -e py --type f | xargs grep -l "def test_" 2>/dev/null | head -30
echo ""
echo "=== backend test directory ==="
find . -type d -name "test*" | head -20
echo ""
echo "=== backend/tests (if exists) ==="
ls -la backend/tests/ 2>/dev/null || echo "[not found]"
ls -la sync-microservice/tests/ 2>/dev/null || echo "[sync tests not found]"
echo ""
echo "=== shutdown-related test files ==="
fd -g '*shutdown*' --type fLength of output: 2011 🏁 Script executed: #!/bin/bash
echo "=== backend/tests/conftest.py ==="
cat backend/tests/conftest.py
echo ""
echo "=== backend/app/routes/shutdown.py ==="
cat backend/app/routes/shutdown.py
echo ""
echo "=== sync-microservice/app/routes/shutdown.py ==="
cat sync-microservice/app/routes/shutdown.py
echo ""
echo "=== backend/app/config/settings.py ==="
cat backend/app/config/settings.py
echo ""
echo "=== sync-microservice/app/config/settings.py ==="
cat sync-microservice/app/config/settings.py
echo ""
echo "=== sync-microservice/app/core/lifespan.py ==="
cat sync-microservice/app/core/lifespan.py
echo ""
echo "=== One existing test for reference ==="
head -80 backend/tests/test_folders.pyLength of output: 19657
Answers to the manual testing questions (from code analysis)
Proposed
|
2 participants
Summary
Fixes #1241.
The
/shutdownendpoints on both the backend (port 52123) and sync microservice (port 52124) were completely unauthenticated. Any process on the local machine could POST to these endpoints and forcefully terminate PictoPy — a classic local denial-of-service vector.As discussed with @rohan-pandeyy, completely disabling the shutdown routes is not an option since the Tauri frontend relies on them on Windows. The goal instead is to ensure only PictoPy itself can trigger these endpoints.
Approach
A per-session shared secret is generated once by the backend at startup using Python's
secretsmodule (256-bit hex token) and written to a temp file (pictopy_shutdown.token). Both services and the Tauri frontend read this same file.Files Changed
backend/app/config/settings.pySHUTDOWN_TOKENviasecrets.token_hex(32)and write to temp filebackend/app/routes/shutdown.pyX-Shutdown-Tokenheader; validate withhmac.compare_digest; return 403 on mismatchsync-microservice/app/config/settings.pysync-microservice/app/routes/shutdown.pyfrontend/src-tauri/src/main.rsX-Shutdown-Tokenheader to every shutdown POSTSecurity Details
hmac.compare_digestprevents timing side-channel attacks.How to Test
Summary by CodeRabbit
Security Improvements
Frontend
Backend
Reliability