Skip to content

Commit

Permalink
add validate webhhook address
Browse files Browse the repository at this point in the history
Signed-off-by: lengrongfu <[email protected]>
  • Loading branch information
lengrongfu committed Aug 8, 2023
1 parent 39ec1e4 commit 69cdb3a
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 9 deletions.
3 changes: 3 additions & 0 deletions api/v2.0/swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7914,6 +7914,9 @@ definitions:
job_parameters:
type: string
description: the job parameters of purge job.
job_id:
type: string
description: the id of the running job
schedule:
$ref: '#/definitions/ScheduleObj'
job_status:
Expand Down
2 changes: 2 additions & 0 deletions src/server/v2.0/handler/model/jobservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type ExecHistory struct {
ID int64 `json:"id"`
Name string `json:"job_name"`
Kind string `json:"job_kind"`
JobID string `json:"job_id"`
Parameters string `json:"job_parameters"`
Status string `json:"job_status"`
UUID string `json:"-"`
Expand All @@ -45,6 +46,7 @@ func (h *ExecHistory) ToSwagger() *models.ExecHistory {
ID: h.ID,
JobName: h.Name,
JobKind: h.Kind,
JobID: h.JobID,

Check warning on line 49 in src/server/v2.0/handler/model/jobservice.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/model/jobservice.go#L49

Added line #L49 was not covered by tests
JobParameters: h.Parameters,
Deleted: h.Deleted,
JobStatus: h.Status,
Expand Down
10 changes: 10 additions & 0 deletions src/server/v2.0/handler/purge.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,20 @@ func (p *purgeAPI) GetPurgeHistory(ctx context.Context, params purge.GetPurgeHis
if err != nil {
return p.SendError(ctx, err)
}
tasks, err := p.taskCtl.List(ctx, q.New(q.KeyWords{
"ExecutionID": exec.ID,
}))
if err != nil {
return p.SendError(ctx, err)
}
if len(tasks) == 0 {
return p.SendError(ctx, errors.New(nil).WithCode(errors.NotFoundCode).WithMessage("garbage collection %d log is not found", exec.ID))
}

Check warning on line 182 in src/server/v2.0/handler/purge.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/purge.go#L174-L182

Added lines #L174 - L182 were not covered by tests
hs = append(hs, &model.ExecHistory{
ID: exec.ID,
Name: job.PurgeAuditVendorType,
Kind: exec.Trigger,
JobID: tasks[0].JobID,

Check warning on line 187 in src/server/v2.0/handler/purge.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/purge.go#L187

Added line #L187 was not covered by tests
Parameters: string(extraAttrsString),
Schedule: &model.ScheduleParam{
Type: exec.Trigger,
Expand Down
19 changes: 19 additions & 0 deletions src/server/v2.0/handler/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ package handler
import (
"context"
"fmt"
"net/http"
"strings"
"time"

"github.com/go-openapi/runtime/middleware"
"github.com/go-openapi/strfmt"
Expand Down Expand Up @@ -414,6 +416,10 @@ func (n *webhookAPI) validateTargets(policy *policy_model.Policy) (bool, error)
// Prevent SSRF security issue #3755
target.Address = url.Scheme + "://" + url.Host + url.Path

if err := validateAddress(target.Address); err != nil {
return false, errors.New(err).WithCode(errors.BadRequestCode)
}

Check warning on line 421 in src/server/v2.0/handler/webhook.go

View check run for this annotation

Codecov / codecov/patch

src/server/v2.0/handler/webhook.go#L420-L421

Added lines #L420 - L421 were not covered by tests

if !isNotifyTypeSupported(target.Type) {
return false, errors.New(nil).WithMessage("unsupported target type %s with policy %s", target.Type, policy.Name).WithCode(errors.BadRequestCode)
}
Expand Down Expand Up @@ -475,6 +481,19 @@ func (n *webhookAPI) constructPolicyWithTriggerTime(ctx context.Context, policie
return res, nil
}

// validateAddress validate the address is connectable
func validateAddress(address string) error {
request, _ := http.NewRequest("GET", address, nil)
client := http.Client{
Timeout: time.Second * 3,
}
resp, err := client.Do(request)
if err != nil {
return errors.New(err).WithCode(errors.BadRequestCode)
}
return resp.Body.Close()
}

func isEventTypeSupported(eventType string) bool {
for _, t := range notification.GetSupportedEventTypes() {
if t.String() == eventType {
Expand Down
32 changes: 30 additions & 2 deletions src/server/v2.0/handler/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ package handler
import (
"fmt"
"io"
"net/http"
"net/http/httptest"
"testing"
"time"

Expand Down Expand Up @@ -92,7 +94,12 @@ func (suite *WebhookTestSuite) TestCreateWebhookPolicyOfProject() {

{
// valid policy should got 200
resp, err := suite.PostJSON(url, &models.WebhookPolicy{EventTypes: []string{"PUSH_ARTIFACT"}, Targets: []*models.WebhookTargetObject{{Type: "http", Address: "http://127.0.0.1"}}})
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Fprintln(w, "Hello, World!")
})
server := httptest.NewServer(handler)
defer server.Close()
resp, err := suite.PostJSON(url, &models.WebhookPolicy{EventTypes: []string{"PUSH_ARTIFACT"}, Targets: []*models.WebhookTargetObject{{Type: "http", Address: server.URL}}})
suite.NoError(err)
suite.Equal(201, resp.StatusCode)
}
Expand Down Expand Up @@ -121,7 +128,12 @@ func (suite *WebhookTestSuite) TestUpdateWebhookPolicyOfProject() {

{
// valid policy should got 200
resp, err := suite.PutJSON(url, &models.WebhookPolicy{EventTypes: []string{"PUSH_ARTIFACT"}, Targets: []*models.WebhookTargetObject{{Type: "http", Address: "http://127.0.0.1"}}})
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Fprintln(w, "Hello, World!")
})
server := httptest.NewServer(handler)
defer server.Close()
resp, err := suite.PutJSON(url, &models.WebhookPolicy{EventTypes: []string{"PUSH_ARTIFACT"}, Targets: []*models.WebhookTargetObject{{Type: "http", Address: server.URL}}})
suite.NoError(err)
suite.Equal(200, resp.StatusCode)
}
Expand Down Expand Up @@ -232,6 +244,22 @@ func (suite *WebhookTestSuite) TestGetSupportedEventTypes() {
suite.Len(body.NotifyType, len(notification.GetSupportedNotifyTypes()))
}

func (suite *WebhookTestSuite) Test_validateAddress() {
err := validateAddress("http://123:8080")
suite.Error(err)

err = validateAddress("https://1.2.3.4")
suite.Error(err)

handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Fprintln(w, "Hello, World!")
})
server := httptest.NewServer(handler)
defer server.Close()
err = validateAddress(server.URL)
suite.NoError(err)
}

func TestWebhookTestSuite(t *testing.T) {
suite.Run(t, &WebhookTestSuite{})
}
8 changes: 1 addition & 7 deletions tests/apitests/python/test_webhook_crud.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,8 @@ def testDelRepo(self):
type = "slack",
auth_header = "aaa"
)
target_2 = v2_swagger_client.WebhookTargetObject(
address = "https://202.10.12.13",
skip_cert_verify = False,
type = "http",
auth_header = "aaa"
)
#This need to be removed once issue #13378 fixed.
policy_id, policy_name = self.webhook.create_webhook(self.project_id, [target_1, target_2], **self.USER_CLIENT)
policy_id, policy_name = self.webhook.create_webhook(self.project_id, [target_1], **self.USER_CLIENT)
target_1 = v2_swagger_client.WebhookTargetObject(
address = "https://hooks.slack.com/services/new",
skip_cert_verify = True,
Expand Down

0 comments on commit 69cdb3a

Please sign in to comment.