[scanner] fix: add comprehensive error branch tests for missions/share.go#19222
Conversation
…F validation Adds table-driven tests covering SSRF bypass shapes, allowlist enforcement, and env var parsing: - validateSlackWebhookURL: 20 new test cases for SSRF bypass attempts (prefix attack, port injection, userinfo, scheme variations, host manipulation) - isRepoAllowedForShareWithList: edge cases (empty repo, partial matches, slashes, nil/empty allowlist) - resolveAllowedShareRepos: whitespace handling, env var parsing, single/multiple repos All tests validate error messages for better diagnostics and cover security-critical allowlist logic per #18759. Signed-off-by: hive-scanner <hive-scanner@kubestellar.io>
…e.go Adds tests covering previously untested error paths: - ShareToSlack: empty text, text exceeds max size, invalid body - ShareToGitHub: body too large (413), missing required fields, invalid filePath (path traversal), invalid branch, fork failure, fork missing full_name, invalid JSON body - validateSlackWebhookURL: userinfo bypass, explicit port, wrong path prefix, invalid URL parsing Signed-off-by: Andy Anderson <andy@clubanderson.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🐝 Hi @clubanderson! I'm Trusted users — org members and contributors with write access — can mention Automation may take a moment to start, and follow-up happens through workflow activity rather than chat replies. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds unit tests to improve coverage for security- and reliability-critical error branches in missions/share.go, particularly around Slack webhook SSRF validation and GitHub share request validation/upstream failure handling.
Changes:
- Expands
share_test.gowith handler-level tests for Slack sharing, allowlist resolution, repo allowlist checks, and Slack webhook URL validation. - Adds
share_error_test.goto cover additional ShareToSlack / ShareToGitHub error branches (oversized payloads, missing fields, invalid path/branch, fork failures, invalid JSON). - Adds extra Slack webhook URL edge-case tests intended to catch SSRF bypass shapes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pkg/api/handlers/missions/share_test.go | Adds broader handler + validation test coverage, but currently introduces compilation failures and a couple of incorrect expectations. |
| pkg/api/handlers/missions/share_error_test.go | Adds error-branch tests for Slack/GitHub sharing, but currently doesn’t compile due to missing helper + a duplicate test name. |
| import ( | ||
| "os" | ||
| "encoding/json" | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/gofiber/fiber/v2" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) |
| app, _ := setupMissionsTest() | ||
|
|
||
| payload := map[string]string{ | ||
| "webhookUrl": slackMock.URL + "/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXX", | ||
| "text": "Test mission shared", | ||
| } |
| validURL := "https://hooks.slack.com/services/T00/B00/XXX" | ||
| err := validateSlackWebhookURL(validURL) | ||
| assert.NoError(t, err) | ||
| func TestMissions_ShareToSlack_Success(t *testing.T) { |
| {"trailing dot in host", "https://hooks.slack.com./services/T00/B00/XXX", true, "host must be hooks.slack.com"}, | ||
| {"percent encoded @", "https://hooks.slack.com%40attacker.evil/services/T00/B00/XXX", true, ""}, | ||
| {"double slash path", "https://hooks.slack.com//services/T00/B00/XXX", false, ""}, | ||
| {"backslash in path", "https://hooks.slack.com/services\\T00\\B00\\XXX", false, ""}, | ||
| {"path without /services/", "https://hooks.slack.com/T00/B00/XXX", true, "path must begin with /services/"}, | ||
| {"empty path", "https://hooks.slack.com", true, "path must begin with /services/"}, |
|
|
||
| // ---------- ShareToSlack error branches ---------- | ||
|
|
|
|
||
| // ---------- ShareToSlack error branches ---------- | ||
|
|
||
| func TestMissions_ShareToSlack_EmptyText(t *testing.T) { |
| assert.Equal(t, 400, resp.StatusCode) | ||
| } | ||
|
|
||
| func TestMissions_ShareToSlack_WebhookReturnsError(t *testing.T) { |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
❌ Post-Merge Verification: failedCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Fixes #18759
Adds comprehensive unit tests covering previously untested error branches in missions/share.go:
ShareToSlack error paths:
ShareToGitHub error paths:
validateSlackWebhookURL edge cases: