Enable and test Gotify and Custom Webhook notifications#754
Enable and test Gotify and Custom Webhook notifications#754Wikid82 merged 100 commits intodevelopmentfrom
Conversation
…n-major-updates chore(deps): update actions/download-artifact digest to 70fc10c (feature/beta-release)
Wikid82
added
frontend
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…load validation - Enhanced Notifications component tests to include support for Discord, Gotify, and Webhook provider types. - Updated test cases to validate the correct handling of provider type options and ensure proper payload structure during creation, preview, and testing. - Introduced new tests for Gotify token handling and ensured sensitive information is not exposed in the UI. - Refactored existing tests for clarity and maintainability, including improved assertions and error handling. - Added comprehensive coverage for payload validation scenarios, including malformed requests and security checks against SSRF and oversized payloads.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…in golangci-lint files
…n-major-updates chore(deps): update github/codeql-action digest to 28737ec (feature/beta-release)
… improved compatibility
…cumentation - Updated tools for Doc_Writer, Frontend_Dev, Management, Planning, Playwright_Dev, QA_Security, and Supervisor agents to enhance terminal command execution capabilities and streamline operations. - Removed redundant tools and ensured uniformity in tool listings across agents.
…response handling
…e-non-major-updates
…n-major-updates chore(deps): update non-major-updates (feature/beta-release)
… filling and upload handling
… state after login
…jor-8-github-artifact-actions chore(deps): update github artifact actions to v8 (feature/beta-release) (major)
… event handling in security scan workflow
…on and improve accessibility checks in authentication flows
There was a problem hiding this comment.
Pull request overview
This PR enables Gotify and Custom Webhook notification providers with comprehensive security hardening. The changes focus on write-only token handling, SSRF protections, error message sanitization, and expanded test coverage.
Changes:
- Added support for Gotify and Custom Webhook notification providers with HTTP wrapper delivery
- Implemented write-only token handling and masked API responses for sensitive fields (API keys, invite tokens, Gotify tokens)
- Enhanced security controls including authenticated admin checks, sensitive setting masking, and secure cookie handling
- Increased coverage thresholds from 85% to 87% for both backend and frontend
- Improved Docker socket access documentation and troubleshooting guidance
- Added comprehensive test coverage for new features and security improvements
Reviewed changes
Copilot reviewed 164 out of 167 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/api/notifications.test.ts |
Updated to support gotify/webhook types with token payload contract enforcement |
frontend/src/api/user.ts |
Changed API key response to masked format with metadata |
frontend/src/pages/Account.tsx |
Removed copy button for API keys, now displays masked value only |
backend/internal/api/handlers/settings_handler.go |
Added sensitive setting masking logic and admin-only SMTP config access |
backend/internal/api/handlers/permission_helpers.go |
Added requireAuthenticatedAdmin helper for stricter auth checks |
backend/internal/api/handlers/auth_handler.go |
Improved cookie security with Secure=true by default, exception only for local loopback |
backend/internal/api/handlers/docker_handler.go |
Enhanced error messages with supplemental group guidance |
backend/internal/notifications/router.go |
Added webhook service flag support |
backend/internal/models/notification_provider.go |
Added HasToken computed field for safe token presence indication |
scripts/*-test-coverage.sh |
Increased coverage threshold to 87% |
docs/getting-started.md |
Added Docker socket access guidance |
.github/workflows/quality-checks.yml |
Added auth route protection contract test job |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| func setSecureCookie(c *gin.Context, name, value string, maxAge int) { | ||
| scheme := requestScheme(c) | ||
| secure := scheme == "https" | ||
| secure := true | ||
| sameSite := http.SameSiteStrictMode | ||
| if scheme != "https" { | ||
| sameSite = http.SameSiteLaxMode | ||
| if isLocalRequest(c) { | ||
| secure = false | ||
| } | ||
| } | ||
|
|
||
| if isLocalRequest(c) { | ||
| secure = false | ||
| sameSite = http.SameSiteLaxMode | ||
| } |
There was a problem hiding this comment.
The cookie security logic has been updated to set Secure=true by default, with an exception only for local loopback HTTP requests. This is a security improvement. However, verify that the loopback detection logic (isLocalRequest) correctly identifies all legitimate local development scenarios to prevent breaking local development workflows.
| func isSensitiveSettingKey(key string) bool { | ||
| normalizedKey := strings.ToLower(strings.TrimSpace(key)) | ||
|
|
||
| sensitiveFragments := []string{ | ||
| "password", | ||
| "secret", | ||
| "token", | ||
| "api_key", | ||
| "apikey", | ||
| "webhook", | ||
| } | ||
|
|
||
| for _, fragment := range sensitiveFragments { | ||
| if strings.Contains(normalizedKey, fragment) { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
The settings handler now masks sensitive values using fragment-based detection. The isSensitiveSettingKey function checks for keywords like "password", "secret", "token", etc. This is a good security improvement, but consider using a whitelist approach instead of blacklist for more robust security. Also, the function should be case-insensitive for the comparison, which it already does, but ensure all variations are covered (e.g., "api-key" with hyphens).
| func requireAuthenticatedAdmin(c *gin.Context) bool { | ||
| if _, exists := c.Get("userID"); !exists { | ||
| c.JSON(http.StatusUnauthorized, gin.H{ | ||
| "error": "Authorization header required", | ||
| }) | ||
| return false | ||
| } | ||
|
|
||
| return requireAdmin(c) | ||
| } |
There was a problem hiding this comment.
The requireAuthenticatedAdmin helper has been added and is now used in import handler endpoints. This is a security improvement that ensures unauthenticated requests are rejected with 401 before checking admin role. Verify that all sensitive endpoints use this helper instead of just requireAdmin to prevent information disclosure to unauthenticated users.
| details := unavailableErr.Details() | ||
| if details == "" { | ||
| details = "Cannot connect to Docker. Please ensure Docker is running and the socket is accessible (e.g., /var/run/docker.sock is mounted)." | ||
| } | ||
| log.WithFields(map[string]any{"server_id": util.SanitizeForLog(serverID), "host": util.SanitizeForLog(host), "error": util.SanitizeForLog(err.Error())}).Warn("docker unavailable") | ||
| c.JSON(http.StatusServiceUnavailable, gin.H{ | ||
| "error": "Docker daemon unavailable", | ||
| "details": "Cannot connect to Docker. Please ensure Docker is running and the socket is accessible (e.g., /var/run/docker.sock is mounted).", | ||
| "details": details, | ||
| }) |
There was a problem hiding this comment.
The Docker handler now uses the Details() method from DockerUnavailableError to provide more informative error messages, including supplemental group guidance. Ensure that the Details() method never leaks sensitive information like file paths, environment variables, or internal system details that could aid attackers.
| Type string `json:"type" gorm:"index"` // discord (only supported type in current rollout) | ||
| URL string `json:"url"` // Discord webhook URL (HTTPS format required) | ||
| Token string `json:"-"` // Auth token for providers (e.g., Gotify) - never exposed in API | ||
| HasToken bool `json:"has_token" gorm:"-"` // Computed: indicates whether a token is set (never exposes raw value) |
There was a problem hiding this comment.
The notification provider model now includes a HasToken field with gorm:"-" tag, indicating it's not persisted to the database. This is good for preventing token exposure in API responses. However, ensure that this field is properly populated in the service layer before returning to the API, and that the actual Token field remains write-only with json:"-" tag as intended.
Primary goals: