-
-
Notifications
You must be signed in to change notification settings - Fork 46
feat: notification system for container and image updates #730
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: kmendell <[email protected]>
Co-authored-by: kmendell <[email protected]>
Co-authored-by: kmendell <[email protected]>
Co-authored-by: kmendell <[email protected]>
@copilot fix the codeql errors |
- Add webhook URL validation to prevent SSRF attacks - Validate Discord webhook URLs are HTTPS and point to Discord domains - Add email address validation using net/mail package - Improve TLS configuration with MinVersion and explicit InsecureSkipVerify=false - Ensure proper certificate verification for SMTP TLS connections Co-authored-by: kmendell <[email protected]>
Fixed CodeQL security issues in commit fc43077: Security improvements:
All changes ensure proper input validation and secure network communication. |
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.
22 files reviewed, 12 comments
notificationService: notificationService, | ||
} | ||
|
||
notifications := group.Group("/environments/:id/notifications") |
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.
logic: The :id
parameter from the environment route is never used in any handler methods, creating inconsistency between the API design and implementation
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/api/notification_handler.go
Line: 21:21
Comment:
**logic:** The `:id` parameter from the environment route is never used in any handler methods, creating inconsistency between the API design and implementation
How can I resolve this? If you propose a fix, please make it concise.
} | ||
|
||
func (h *NotificationHandler) GetAllSettings(c *gin.Context) { | ||
settings, err := h.notificationService.GetAllSettings(c.Request.Context()) |
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.
style: Consider extracting environment ID from URL path if notifications should be environment-scoped: envID := c.Param("id")
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/api/notification_handler.go
Line: 33:33
Comment:
**style:** Consider extracting environment ID from URL path if notifications should be environment-scoped: `envID := c.Param("id")`
How can I resolve this? If you propose a fix, please make it concise.
|
||
type NotificationSettings struct { | ||
ID uint `json:"id" gorm:"primaryKey"` | ||
Provider string `json:"provider" gorm:"not null;index"` |
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.
style: Provider field should use NotificationProvider type instead of string for better type safety and validation
Context Used: Rule from dashboard
- GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/models/notification.go
Line: 24:24
Comment:
**style:** Provider field should use NotificationProvider type instead of string for better type safety and validation
**Context Used:** Rule from `dashboard` - GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... ([source](https://app.greptile.com/review/custom-context?memory=214b40a8-9695-4738-986d-5949b5d65ff1))
How can I resolve this? If you propose a fix, please make it concise.
if decrypted, err := utils.Decrypt(emailConfig.SMTPPassword); err == nil { | ||
emailConfig.SMTPPassword = decrypted | ||
} |
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.
style: same pattern as webhook URL - silently ignoring password decryption errors could mask configuration problems
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/services/notification_service.go
Line: 265:267
Comment:
**style:** same pattern as webhook URL - silently ignoring password decryption errors could mask configuration problems
How can I resolve this? If you propose a fix, please make it concise.
} | ||
|
||
// Authenticate if credentials are provided | ||
if config.SMTPUsername != "" || config.SMTPPassword != "" { |
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.
logic: Authentication logic allows empty username with non-empty password, which may not be intended behavior for most SMTP servers
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/utils/notifications/email_util.go
Line: 74:74
Comment:
**logic:** Authentication logic allows empty username with non-empty password, which may not be intended behavior for most SMTP servers
How can I resolve this? If you propose a fix, please make it concise.
// sendHelloCommand sends the HELO/EHLO command to the SMTP server | ||
func sendHelloCommand(client *smtp.Client) error { | ||
// Try to get hostname, use "localhost" as fallback | ||
hostname := "localhost" |
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.
style: Using hardcoded 'localhost' as hostname may cause issues with some SMTP servers that validate HELO hostname
Context Used: Rule from dashboard
- GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/utils/notifications/email_util.go
Line: 87:87
Comment:
**style:** Using hardcoded 'localhost' as hostname may cause issues with some SMTP servers that validate HELO hostname
**Context Used:** Rule from `dashboard` - GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... ([source](https://app.greptile.com/review/custom-context?memory=214b40a8-9695-4738-986d-5949b5d65ff1))
How can I resolve this? If you propose a fix, please make it concise.
} | ||
|
||
// Write the email content | ||
_, err = io.Copy(w, strings.NewReader(message)) |
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.
style: Using io.Copy
with strings.NewReader
is less efficient than directly writing the string with w.Write([]byte(message))
Context Used: Rule from dashboard
- GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/utils/notifications/email_util.go
Line: 130:130
Comment:
**style:** Using `io.Copy` with `strings.NewReader` is less efficient than directly writing the string with `w.Write([]byte(message))`
**Context Used:** Rule from `dashboard` - GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... ([source](https://app.greptile.com/review/custom-context?memory=214b40a8-9695-4738-986d-5949b5d65ff1))
How can I resolve this? If you propose a fix, please make it concise.
s = strings.ReplaceAll(s, "\n", "") | ||
|
||
// Allow only: letters, numbers, dot, slash, dash, colon, at, underscore | ||
safe := make([]rune, 0, len(s)) |
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.
style: Pre-allocating slice with len(s)
capacity may be wasteful if many characters are filtered out
Context Used: Rule from dashboard
- GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/utils/notifications/email_util.go
Line: 159:159
Comment:
**style:** Pre-allocating slice with `len(s)` capacity may be wasteful if many characters are filtered out
**Context Used:** Rule from `dashboard` - GoLang Best Practices
Follow idiomatic Go patterns and conventions
Handle errors explicitly, don’t ... ([source](https://app.greptile.com/review/custom-context?memory=214b40a8-9695-4738-986d-5949b5d65ff1))
How can I resolve this? If you propose a fix, please make it concise.
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Notification System for Container Updates
✅ All tasks completed!
Implementation of Discord and Email notifications when containers have updates available:
Security Improvements (Latest Commit)
SSRF Prevention
Input Validation
net/mail
packageTLS Security
Security Features Summary
Original prompt
Fixes #147
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.
Greptile Overview
Updated On: 2025-10-17 19:17:57 UTC
Greptile Summary
This PR implements a comprehensive notification system that sends Discord webhook and email notifications when container image updates are detected. The implementation includes complete backend infrastructure with database models, migrations for both PostgreSQL and SQLite, a notification service with SSRF protection and email validation, API handlers for CRUD operations, and integration with the existing image update service. The frontend adds a new notifications settings page with configuration forms for both Discord and Email providers, complete with test functionality and proper internationalization support.
The system uses encrypted storage for sensitive data like webhook URLs and SMTP passwords, implements security hardening measures including webhook URL validation to prevent SSRF attacks, email address validation using Go's
net/mail
package, and enforces TLS 1.2+ for SMTP connections. The architecture follows the existing codebase patterns with proper dependency injection, service separation, and environment scoping (though notification handlers don't currently use environment context).Important Files Changed
Changed Files
Confidence score: 4/5