server: add pending jobs to job_status_reports#153
Conversation
job_status_reports
Summary of ChangesHello @hsjoberg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how background job statuses and notification tracking are managed. It introduces new 'Pending' and 'Timeout' states for jobs, coupled with a server-side cronjob to automatically mark unresponsive pending jobs as timed out. The system for tracking notification spacing has been streamlined by removing a dedicated table and instead leveraging existing job status and heartbeat records. Client-side APIs have been updated to align with these changes, ensuring that all job status updates are correlated via a unique Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors job status reporting by introducing a 'Pending' state and a timeout mechanism, simplifying the design by removing the notification_tracking table and consolidating logic into job_status_reports. Critically, the implementation of UnifiedPush support introduces a significant security vulnerability, allowing authenticated users to leak the server's ntfy_auth_token and perform SSRF by registering arbitrary URLs as push tokens. This vulnerability must be addressed before merging. Additionally, consider a performance improvement for database transactions when recording pending jobs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors job status and notification tracking by consolidating logic into the job_status_reports table and introducing a Pending status with a cron job for timeouts. However, a critical security flaw was identified in the UnifiedPush implementation where untrusted user input is used as a network destination, leading to SSRF and the leakage of the ntfy_auth_token secret. This must be addressed by validating push token URLs and ensuring sensitive tokens are only sent to trusted endpoints.
| send_unified_notification( | ||
| &http_client_clone, | ||
| &target.push_token, | ||
| &data_string, | ||
| &ntfy_auth, | ||
| ) |
There was a problem hiding this comment.
The send_unified_notification function uses the user-provided target.push_token as a destination URL without validation. An authenticated user can register a malicious URL (e.g., an internal service or an attacker-controlled server) to trigger an SSRF. Furthermore, the server's ntfy_auth_token is sent to this URL in the Authorization header, leading to the leakage of server-side credentials.
There was a problem hiding this comment.
This is not true unless something changed. The client registers their push token on sign up and we do a regex check if it's a ntfy URL iirc
There was a problem hiding this comment.
Yeah I didn't touch such code
There was a problem hiding this comment.
@gemini-code-assist it's being followed up on another PR
There was a problem hiding this comment.
Thanks for the update. I'll close this thread then, as the issue is being addressed in another PR.
client/src/lib/api.ts
Outdated
| export const reportJobStatus = (payload: ReportJobCompletionPayload & { k1: string }) => | ||
| payload.k1 | ||
| ? post<ReportJobCompletionPayload & { k1: string }, DefaultSuccessPayload>( | ||
| "/report_job_status", | ||
| payload, | ||
| ) | ||
| : Promise.resolve(err(new Error("Missing notification k1 for report_job_status"))); |
There was a problem hiding this comment.
The payload.k1 check appears to be redundant. The function's type signature ReportJobCompletionPayload & { k1: string } already ensures that k1 is a non-optional string. Furthermore, the main call site in pushNotifications.ts within handleTaskCompletion already includes a guard if (!k1) to prevent this function from being called with a falsy k1.
Relying on the type system and caller-side validation simplifies this function and avoids defensive code. Consider removing the ternary operator.
export const reportJobStatus = (payload: ReportJobCompletionPayload & { k1: string }) =>
post<ReportJobCompletionPayload & { k1: string }, DefaultSuccessPayload>(
"/report_job_status",
payload,
);There was a problem hiding this comment.
Yeah I think this review is valid
| let mut tx = self.app_state.db_pool.begin().await?; | ||
| JobStatusRepository::create_with_k1_and_prune( | ||
| &mut tx, | ||
| &dispatch.pubkey, | ||
| &dispatch.notification_k1, | ||
| &report_type, | ||
| &ReportStatus::Pending, | ||
| None, | ||
| ) | ||
| .await?; | ||
| tx.commit().await?; |
There was a problem hiding this comment.
Initiating and committing a new database transaction for each dispatch within this loop can be inefficient, especially during broadcast notifications with many recipients. This approach incurs significant overhead from repeated, small transactions.
For better performance and scalability, consider processing all dispatches within a single transaction. This would reduce the number of round-trips to the database. If failure isolation for each dispatch is a strict requirement, the current implementation is valid, but it's worth noting the performance trade-off.
| ) | ||
| : Promise.resolve(err(new Error("Missing notification k1 for report_job_status"))); | ||
|
|
||
| export const submitInvoice = (payload: SubmitInvoicePayload & { k1: string }) => |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed refactoring of job status and notification tracking. Dropping the notification_tracking table and consolidating the logic into job_status_reports simplifies the data model and state management. The introduction of a Pending state and a cron job to handle timeouts is a robust way to manage jobs that rely on client responses. The changes are consistent across the backend and client, and the new test suite is comprehensive. I have a few suggestions to improve performance and maintainability.
| let last_sent = match notification_data { | ||
| NotificationData::Maintenance(_) => { | ||
| sqlx::query_scalar::<_, Option<DateTime<Utc>>>( | ||
| "SELECT MAX(created_at) | ||
| FROM job_status_reports | ||
| WHERE pubkey = $1 AND report_type = 'Maintenance'", | ||
| ) | ||
| .bind(pubkey) | ||
| .fetch_one(self.pool) | ||
| .await? | ||
| } | ||
| NotificationData::BackupTrigger(_) => { | ||
| sqlx::query_scalar::<_, Option<DateTime<Utc>>>( | ||
| "SELECT MAX(created_at) | ||
| FROM job_status_reports | ||
| WHERE pubkey = $1 AND report_type = 'Backup'", | ||
| ) | ||
| .bind(pubkey) | ||
| .fetch_one(self.pool) | ||
| .await? | ||
| } | ||
| NotificationData::Heartbeat(_) => { | ||
| sqlx::query_scalar::<_, Option<DateTime<Utc>>>( | ||
| "SELECT MAX(sent_at) | ||
| FROM heartbeat_notifications | ||
| WHERE pubkey = $1", | ||
| ) | ||
| .bind(pubkey) | ||
| .fetch_one(self.pool) | ||
| .await? | ||
| } | ||
| NotificationData::LightningInvoiceRequest(_) => None, | ||
| }; |
There was a problem hiding this comment.
The match statement has duplicated code for Maintenance and BackupTrigger notifications. This can be simplified to reduce redundancy and improve maintainability by combining the match arms.
let last_sent = match notification_data {
NotificationData::Maintenance(_) | NotificationData::BackupTrigger(_) => {
let report_type = if matches!(notification_data, NotificationData::Maintenance(_)) {
"Maintenance"
} else {
"Backup"
};
sqlx::query_scalar::<_, Option<DateTime<Utc>>>(
"SELECT MAX(created_at)
FROM job_status_reports
WHERE pubkey = $1 AND report_type = $2",
)
.bind(pubkey)
.bind(report_type)
.fetch_one(self.pool)
.await?
}
NotificationData::Heartbeat(_) => {
sqlx::query_scalar::<_, Option<DateTime<Utc>>>(
"SELECT MAX(sent_at)
FROM heartbeat_notifications
WHERE pubkey = $1",
)
.bind(pubkey)
.fetch_one(self.pool)
.await?
}
NotificationData::LightningInvoiceRequest(_) => None,
};865350a to
f9a9e3f
Compare
This PR changes the behavior of
job_status_reportsand drops thenotification_trackingtable.We create a pending entry (status
Pending) once we send out the silent push notification, and once the client responds we update the entry.We also set up a cronjob that runs every hour to clear up any pending jobs and set their status to
Timeout(n.b: if the device didn't wake up)Closes #149.