-
Notifications
You must be signed in to change notification settings - Fork 0
RC-153 chore improve security #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances security and reliability by preventing credential exposure in job queues, improving error handling for disconnected compute resources, and ensuring proper cleanup of sensitive data. The changes address credential leakage risks by regenerating temporary credentials within workers rather than passing them through Redis/Celery.
- Credential handling now uses just-in-time regeneration in workers instead of passing credentials through the job queue
- Job status reporting updated from
SERVER_ERRORtoHPC_DISCONNECTEDfor better clarity when compute resources are unreachable - GitHub tokens are now encrypted at the database level with corresponding migration
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/app/service_job/controller.py | Validates storage credentials before job submission and passes only storage ID and user email instead of actual credentials |
| backend/app/service_job/tasks.py | Regenerates temporary credentials in worker, adds credential cleanup on job completion, and handles HPC_DISCONNECTED status |
| backend/app/service_job/models.py | Renames SERVER_ERROR to HPC_DISCONNECTED in job status enum |
| backend/app/utils/executor/ssh.py | Improves SSH tunnel cleanup with better error handling and adds HPC_DISCONNECTED to monitored statuses |
| backend/app/service_credential/models/personal.py | Changes GitHub token field from CharField to EncryptedTextField |
| backend/migrations/models/5_20260105205918_update.py | Updates database comments to replace SERVER_ERROR with HPC_DISCONNECTED |
| backend/migrations/models/6_20260105224743_update.py | Migrates GitHub token column from VARCHAR(200) to TEXT for encrypted storage |
Comments suppressed due to low confidence (1)
backend/app/service_job/tasks.py:1
- Using positional arguments for this many parameters (6 total) makes the function call fragile and hard to maintain. Consider using keyword arguments in apply_async:
submit_job.apply_async(kwargs={'job_id': str(job.id), 'is_web_job': analysis.allow_access, ...})for better readability and to prevent argument order mismatches.
from celery import shared_task
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat: handle server disconnect * fix: add github token to encrypt * feat: remove credentails from params in celery * feat: increase max_length token Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request introduces several important security and reliability improvements to job submission and credential handling, as well as updates to job status management. The main focus is on preventing sensitive credentials from being exposed via the job queue, enhancing error handling for disconnected compute resources, and improving process cleanup. Below are the most significant changes:
Security & Credential Handling:
tokenfield in theGithubmodel is now stored as an encrypted text field, and a migration is included to update its database type. [1] [2]Job Status & Error Handling:
SERVER_ERRORstatus is replaced withHPC_DISCONNECTEDin job models and database comments, reflecting more accurate error states when compute resources are unreachable. [1] [2] [3]HPC_DISCONNECTEDstatus, and process cleanup for credentials is performed on job completion or failure, with improved logging and error handling. [1] [2] [3]Process Management & Logging:
Database & Migration Updates:
tokenfield type for consistency and security. [1] [2]These changes collectively harden the system against credential leaks, improve error reporting, and ensure that job lifecycle management is robust and secure.