Conversation
Adds DELETE /admin/submissions/{submission_id} protected by a shared
ADMIN_API_SECRET. Kernelboard verifies admin identity via its whitelist
before proxying to this endpoint, so no duplicated admin list needed here.
63272f2 to
38c7fa0
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This pull request adds an admin submission deletion endpoint intended for use by the kernelboard web UI. The endpoint uses a shared secret authentication mechanism (ADMIN_API_SECRET) rather than the standard admin token authentication used by other admin endpoints. The PR description indicates that kernelboard verifies admin identity via its whitelist before proxying requests to this endpoint.
Changes:
- Adds
DELETE /admin/submissions/{submission_id}endpoint withX-Admin-Secretheader authentication - Reuses existing
db.get_submission_by_id()anddb.delete_submission()database methods - Implements 404 check before deletion (unlike the existing admin delete endpoint)
Comments suppressed due to low confidence (1)
src/kernelbot/api/main.py:872
- This new endpoint lacks test coverage. The test file
tests/test_admin_api.pyincludes comprehensive tests for other admin endpoints including the duplicateDELETE /admin/submissions/{submission_id}at line 578. Tests should be added for this new endpoint covering: 1) Missing X-Admin-Secret header (403), 2) Invalid X-Admin-Secret (403), 3) Valid secret with non-existent submission (404), 4) Valid secret with existing submission (successful deletion). The test fixture would need to be updated to set theADMIN_API_SECRETenvironment variable.
x_admin_secret: Optional[str] = Header(None, alias="X-Admin-Secret"),
db_context=Depends(get_db),
) -> dict:
"""Admin-only: delete any submission by ID, regardless of ownership.
Protected by a shared secret between kernelboard and kernelbot.
Kernelboard verifies admin identity (whitelist) before calling this.
"""
await simple_rate_limit()
if not env.ADMIN_API_SECRET or x_admin_secret != env.ADMIN_API_SECRET:
raise HTTPException(status_code=403, detail="Admin access required")
try:
with db_context as db:
submission = db.get_submission_by_id(submission_id)
if submission is None:
raise HTTPException(status_code=404, detail="Submission not found")
db.delete_submission(submission_id)
return {"message": f"Submission {submission_id} deleted successfully"}
except HTTPException:
raise
except Exception as e:
raise HTTPException(
status_code=500, detail=f"Error deleting submission: {e}"
) from e
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise HTTPException(status_code=404, detail="Submission not found") | ||
|
|
||
| db.delete_submission(submission_id) | ||
| return {"message": f"Submission {submission_id} deleted successfully"} |
There was a problem hiding this comment.
The response format is inconsistent with the user delete endpoint. The user delete endpoint at line 833 returns {"status": "ok", "submission_id": submission_id} while this admin endpoint returns {"message": f"Submission {submission_id} deleted successfully"}. For consistency and API design coherence, both endpoints should use the same response format since they perform the same operation.
| return {"message": f"Submission {submission_id} deleted successfully"} | |
| return {"status": "ok", "submission_id": submission_id} |
| x_admin_secret: Optional[str] = Header(None, alias="X-Admin-Secret"), | ||
| db_context=Depends(get_db), | ||
| ) -> dict: | ||
| """Admin-only: delete any submission by ID, regardless of ownership. | ||
|
|
||
| Protected by a shared secret between kernelboard and kernelbot. |
There was a problem hiding this comment.
This endpoint duplicates the existing DELETE /admin/submissions/{submission_id} endpoint defined at line 578. FastAPI will use the first registered route, making this new endpoint unreachable. The existing endpoint uses the standard require_admin dependency (with Authorization Bearer token), while this one uses a different authentication method (X-Admin-Secret header). Either the existing endpoint should be removed/replaced, or this new endpoint should use a different path (e.g., /admin/submissions/{submission_id}/delete-alt or /kernelboard/admin/submissions/{submission_id}).
| raise HTTPException(status_code=500, detail=f"Error deleting submission: {e}") from e | ||
|
|
||
|
|
||
| @app.delete("/admin/submissions/{submission_id}") |
There was a problem hiding this comment.
The ADMIN_API_SECRET should be accessed through the env module following the codebase convention, not directly via os.environ.get(). Add env.ADMIN_API_SECRET = os.getenv("ADMIN_API_SECRET", "") to src/kernelbot/env.py (similar to line 19 for ADMIN_TOKEN), then use env.ADMIN_API_SECRET here instead of ADMIN_API_SECRET. This maintains consistency with how other environment variables like ADMIN_TOKEN are handled throughout the codebase.
| if not env.ADMIN_API_SECRET or x_admin_secret != env.ADMIN_API_SECRET: | ||
| raise HTTPException(status_code=403, detail="Admin access required") | ||
|
|
||
| try: |
There was a problem hiding this comment.
The secret comparison on line 856 is vulnerable to timing attacks. An attacker could use timing measurements to determine the correct secret character by character. Use secrets.compare_digest() for constant-time comparison of secrets to prevent timing attacks. This should be: if not ADMIN_API_SECRET or not secrets.compare_digest(x_admin_secret or "", ADMIN_API_SECRET):
|
No kernelbot changes needed — the existing |
Summary
DELETE /admin/submissions/{submission_id}endpoint protected byADMIN_API_SECRETenv var (shared secret with kernelboard)db.get_submission_by_id()anddb.delete_submission()logicContext
Currently admin submission deletion is only possible via Discord (
/admin delete-submission). This enables the same action from the kernelboard web UI. The admin check lives in kernelboard (hardcoded Discord ID whitelist), and this endpoint trusts that check via a shared secret — keeping the admin list in one place.Note: popcorn-cli already has user-level
DELETE /user/submissions/{id}(owner-only). Extending admin delete to CLI would be a natural follow-up.Deployment
Both kernelboard and kernelbot need the same
ADMIN_API_SECRETenv var set.Test plan
X-Admin-Secretheader is missing or wrongDELETE /user/submissions/{id}still works unchanged