diff --git a/internal/app/form/domain.go b/internal/app/form/domain.go index d4cab10d..a01a463c 100644 --- a/internal/app/form/domain.go +++ b/internal/app/form/domain.go @@ -33,7 +33,7 @@ type OrgStore interface { // sitting in a recipient's inbox into a dead link, which is worse than a link // on the shared host. func (s *service) FormsHost(ctx context.Context, orgID uuid.UUID) string { - shared := config.FormsHostname() + shared := config.FormsURLHost() if s.domains == nil { return shared } diff --git a/internal/config/endpoints.go b/internal/config/endpoints.go index c2d3f844..c5a2e9a5 100644 --- a/internal/config/endpoints.go +++ b/internal/config/endpoints.go @@ -63,24 +63,42 @@ func GetInviteURL(token string) string { // pages do not live on the API origin, so there is nothing to fall back to, // and a share link pointing at the wrong process is worse than none. func FormsBaseURL() string { - if host := strings.TrimSpace(os.Getenv("FORMS_DOMAIN")); host != "" { - host = strings.TrimPrefix(strings.TrimPrefix(host, "https://"), "http://") - host = strings.TrimRight(host, "/") - scheme := "https" - if strings.HasPrefix(host, "localhost") || strings.HasPrefix(host, "127.0.0.1") { - scheme = "http" - } - return scheme + "://" + host + host := NormalizeTrackingHost(os.Getenv("FORMS_DOMAIN")) + if host == "" { + return "" } - return "" + return formsScheme(host) + "://" + host +} + +// formsScheme is https except where TLS cannot be terminated: a form page on a +// loopback or private-network host is a development or LAN install. The port is +// deliberately not a signal, because an install can terminate TLS on any port +// and inferring http from one handed an https deployment http:// share links. +func formsScheme(host string) string { + name := hostWithoutPort(NormalizeTrackingHost(host)) + if name == "localhost" || strings.HasSuffix(name, ".localhost") { + return "http" + } + if ip := net.ParseIP(strings.Trim(name, "[]")); ip != nil && (ip.IsLoopback() || ip.IsPrivate()) { + return "http" + } + return "https" } // FormsHostname is the bare host this install serves forms on. It is the -// CNAME target a customer points their own forms subdomain at. +// CNAME target a customer points their own forms subdomain at, so it never +// carries a port. func FormsHostname() string { return hostWithoutPort(NormalizeTrackingHost(FormsBaseURL())) } +// FormsURLHost is the shared host form URLs are built on. Unlike the CNAME +// target it keeps the port, because dropping it points every share link and +// embed on a ported install at nothing. +func FormsURLHost() string { + return NormalizeTrackingHost(FormsBaseURL()) +} + // FormURLOn builds the hosted page URL on a specific host, which is how a // verified custom forms domain replaces the shared one. An empty host falls // back to this install's own forms base. @@ -89,18 +107,7 @@ func FormURLOn(host, publicID string) string { if host == "" { return GetFormURL(publicID) } - scheme := "https" - if name, port, err := net.SplitHostPort(host); err == nil { - if port != "" && port != "443" { - scheme = "http" - } - if name == "localhost" || strings.HasSuffix(name, ".localhost") { - scheme = "http" - } - } else if host == "localhost" || strings.HasSuffix(host, ".localhost") { - scheme = "http" - } - return scheme + "://" + host + "/f/" + url.PathEscape(publicID) + return formsScheme(host) + "://" + host + "/f/" + url.PathEscape(publicID) } // GetFormURL is the hosted page for one form; empty when no base is known. diff --git a/internal/config/endpoints_test.go b/internal/config/endpoints_test.go index ca9c7e4d..dfab96e9 100644 --- a/internal/config/endpoints_test.go +++ b/internal/config/endpoints_test.go @@ -2,22 +2,56 @@ package config import "testing" -// Clients dial whatever GET /v1/auth/config advertises, so every form an -// operator plausibly writes has to normalise to the Phoenix transport path. -func TestWebsocketURLNormalisation(t *testing.T) { - cases := map[string]string{ - "wss://ws.example.com": "wss://ws.example.com/socket/websocket", - "wss://ws.example.com/": "wss://ws.example.com/socket/websocket", - "wss://ws.example.com/socket": "wss://ws.example.com/socket/websocket", - "wss://ws.example.com/socket/": "wss://ws.example.com/socket/websocket", - "wss://ws.example.com/socket/websocket": "wss://ws.example.com/socket/websocket", - "ws://localhost:4000/socket/websocket": "ws://localhost:4000/socket/websocket", - "": "", +// The hosted form URL has to be reachable on every install shape: the shared +// host keeps its port (a share link that drops it points at nothing), and the +// scheme follows the host rather than the port, because an install can +// terminate TLS on any port and a ported https deployment must not be handed +// http:// links (PR #368). +func TestFormURLsFollowTheInstallHost(t *testing.T) { + for _, tc := range []struct { + formsDomain string + wantBase string + wantShare string + wantCNAME string + }{ + {"localhost:8090", "http://localhost:8090", "http://localhost:8090/f/abc", "localhost"}, + {"127.0.0.1:8090", "http://127.0.0.1:8090", "http://127.0.0.1:8090/f/abc", "127.0.0.1"}, + {"192.168.1.5:8090", "http://192.168.1.5:8090", "http://192.168.1.5:8090/f/abc", "192.168.1.5"}, + {"forms.example.com", "https://forms.example.com", "https://forms.example.com/f/abc", "forms.example.com"}, + {"forms.example.com:8443", "https://forms.example.com:8443", "https://forms.example.com:8443/f/abc", "forms.example.com"}, + {"https://Forms.Example.com/", "https://forms.example.com", "https://forms.example.com/f/abc", "forms.example.com"}, + } { + t.Run(tc.formsDomain, func(t *testing.T) { + t.Setenv("FORMS_DOMAIN", tc.formsDomain) + if got := FormsBaseURL(); got != tc.wantBase { + t.Errorf("FormsBaseURL() = %q, want %q", got, tc.wantBase) + } + if got := GetFormURL("abc"); got != tc.wantShare { + t.Errorf("GetFormURL() = %q, want %q", got, tc.wantShare) + } + // What the handler stamps on every form: the shared host, resolved + // through FormsHost, then built into a URL. + if got := FormURLOn(FormsURLHost(), "abc"); got != tc.wantShare { + t.Errorf("FormURLOn(FormsURLHost()) = %q, want %q", got, tc.wantShare) + } + // The CNAME target is a DNS name, so it never carries the port. + if got := FormsHostname(); got != tc.wantCNAME { + t.Errorf("FormsHostname() = %q, want %q", got, tc.wantCNAME) + } + }) } - for in, want := range cases { - t.Setenv("WEBSOCKET_URL", in) - if got := WebsocketURL(); got != want { - t.Errorf("WebsocketURL(%q) = %q, want %q", in, got, want) - } +} + +// A verified custom forms domain replaces the shared host and is always a bare +// name, so its links stay https whatever the install runs on. +func TestFormURLOnCustomDomain(t *testing.T) { + t.Setenv("FORMS_DOMAIN", "localhost:8090") + if got := FormURLOn("forms.acme.com", "abc"); got != "https://forms.acme.com/f/abc" { + t.Errorf("custom domain URL = %q", got) + } + // No host and no configured base is an empty URL, never a relative one. + t.Setenv("FORMS_DOMAIN", "") + if got := FormURLOn("", "abc"); got != "" { + t.Errorf("unconfigured install URL = %q, want empty", got) } } diff --git a/internal/repository/form_live_test.go b/internal/repository/form_live_test.go index 9b87b93f..91148213 100644 --- a/internal/repository/form_live_test.go +++ b/internal/repository/form_live_test.go @@ -135,3 +135,47 @@ func TestLiveFormLifecycle(t *testing.T) { t.Fatalf("delete: %v", xerr) } } + +// Issue #343: the builder's "New form" hands the repository a Form with no +// allowed domains and no fields at all. Both columns reject NULL, so the nil +// slices have to reach Postgres as empty values instead. +func TestLiveFormCreateEmptyLists(t *testing.T) { + handle, pool := liveContactDB(t) + f := newSharedOrgFixture(t, pool) + repo := NewFormRepository(handle) + ctx := context.Background() + t.Cleanup(func() { + if _, err := pool.Exec(context.Background(), `DELETE FROM forms WHERE organization_id = $1`, f.org); err != nil { + t.Errorf("cleanup forms: %v", err) + } + }) + + created, xerr := repo.Create(ctx, f.org, &f.owner, &models.Form{ + PublicID: "livetest-" + uuid.New().String()[:13], + Name: "Empty lists", + Status: models.FormStatusDraft, + SuccessMessage: "Thanks", + }) + if xerr != nil { + t.Fatalf("create: %v", xerr) + } + if created.AllowedDomains == nil || len(created.AllowedDomains) != 0 { + t.Fatalf("allowed domains: %#v", created.AllowedDomains) + } + if created.Fields == nil || len(created.Fields) != 0 { + t.Fatalf("fields: %#v", created.Fields) + } + + // The same nil slices on the way back out (publishing a form the builder + // never gave domains to). + created.AllowedDomains = nil + created.Fields = nil + created.Status = models.FormStatusPublished + updated, xerr := repo.Update(ctx, f.org, created) + if xerr != nil { + t.Fatalf("update: %v", xerr) + } + if len(updated.AllowedDomains) != 0 || len(updated.Fields) != 0 { + t.Fatalf("update round-trip: %#v %#v", updated.AllowedDomains, updated.Fields) + } +} diff --git a/internal/repository/pg_form.go b/internal/repository/pg_form.go index ecfc8736..92b2be6b 100644 --- a/internal/repository/pg_form.go +++ b/internal/repository/pg_form.go @@ -125,6 +125,24 @@ func (r *formRepository) GetByPublicID(ctx context.Context, publicID string) (*m return f, nil } +// formWriteValues renders the columns a Go zero value would corrupt: a form +// with no domains and no fields yet is exactly what "New form" creates, and +// its nil slices would bind as NULL against a NOT NULL allowed_domains and a +// fields column CHECKed to hold a JSON array (issue #343). +func formWriteValues(f *models.Form) (fields, design []byte, domains []string, err error) { + list := f.Fields + if list == nil { + list = []models.FormField{} + } + if fields, err = json.Marshal(list); err != nil { + return nil, nil, nil, err + } + if design, err = json.Marshal(f.Design); err != nil { + return nil, nil, nil, err + } + return fields, design, textArray(f.AllowedDomains), nil +} + func (r *formRepository) Create(ctx context.Context, orgID uuid.UUID, createdBy *uuid.UUID, f *models.Form) (*models.Form, *errx.Error) { var count int if err := r.DB.QueryRow(ctx, `SELECT COUNT(*) FROM forms WHERE organization_id = $1`, orgID).Scan(&count); err != nil { @@ -135,8 +153,11 @@ func (r *formRepository) Create(ctx context.Context, orgID uuid.UUID, createdBy return nil, errx.New(errx.BadRequest, fmt.Sprintf("at most %d forms per organization", models.FormsPerOrgMax)) } - fields, _ := json.Marshal(f.Fields) - design, _ := json.Marshal(f.Design) + fields, design, domains, err := formWriteValues(f) + if err != nil { + db.CaptureError(err, "forms create", nil, "marshal") + return nil, errx.InternalError() + } tx, err := r.DB.Begin(ctx) if err != nil { @@ -152,7 +173,7 @@ func (r *formRepository) Create(ctx context.Context, orgID uuid.UUID, createdBy VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) RETURNING id `, orgID, createdBy, f.PublicID, f.Name, f.Status, fields, design, - f.SuccessMessage, f.RedirectURL, f.CampaignID, f.AllowedDomains, f.CaptchaEnabled).Scan(&id) + f.SuccessMessage, f.RedirectURL, f.CampaignID, domains, f.CaptchaEnabled).Scan(&id) if err != nil { db.CaptureError(err, "forms create", nil, "insert") return nil, errx.InternalError() @@ -168,8 +189,11 @@ func (r *formRepository) Create(ctx context.Context, orgID uuid.UUID, createdBy } func (r *formRepository) Update(ctx context.Context, orgID uuid.UUID, f *models.Form) (*models.Form, *errx.Error) { - fields, _ := json.Marshal(f.Fields) - design, _ := json.Marshal(f.Design) + fields, design, domains, err := formWriteValues(f) + if err != nil { + db.CaptureError(err, "forms update", nil, "marshal") + return nil, errx.InternalError() + } tx, err := r.DB.Begin(ctx) if err != nil { @@ -184,7 +208,7 @@ func (r *formRepository) Update(ctx context.Context, orgID uuid.UUID, f *models. published_at = $12, updated_at = NOW() WHERE organization_id = $1 AND id = $2 `, orgID, f.ID, f.Name, f.Status, fields, design, f.SuccessMessage, - f.RedirectURL, f.CampaignID, f.AllowedDomains, f.CaptchaEnabled, f.PublishedAt) + f.RedirectURL, f.CampaignID, domains, f.CaptchaEnabled, f.PublishedAt) if err != nil { db.CaptureError(err, "forms update", nil, "exec") return nil, errx.InternalError() diff --git a/internal/repository/pg_webhook.go b/internal/repository/pg_webhook.go index 0484e8d8..85baf738 100644 --- a/internal/repository/pg_webhook.go +++ b/internal/repository/pg_webhook.go @@ -105,6 +105,11 @@ func (r *webhookRepository) CreateEndpoint(ctx context.Context, endpoint *models } endpoint.CreatedAt = time.Now().UTC() endpoint.UpdatedAt = endpoint.CreatedAt + // An omitted filter is the documented "every non-firehose event", which + // the matcher reads as an empty array; NULL is not allowed. Normalized on + // the struct too, so the created endpoint reads back the way a later GET + // returns it instead of echoing a null filter. + endpoint.EventTypes = textArray(endpoint.EventTypes) _, err := r.db.Exec(ctx, ` INSERT INTO webhook_endpoints ( @@ -123,6 +128,7 @@ func (r *webhookRepository) CreateEndpoint(ctx context.Context, endpoint *models func (r *webhookRepository) UpdateEndpoint(ctx context.Context, endpoint *models.WebhookEndpoint) error { endpoint.UpdatedAt = time.Now().UTC() + endpoint.EventTypes = textArray(endpoint.EventTypes) cmd, err := r.db.Exec(ctx, ` UPDATE webhook_endpoints SET url = $1, description = $2, event_types = $3, enabled = $4, updated_at = $5 @@ -291,7 +297,7 @@ func (r *webhookRepository) UpsertAppEndpoint(ctx context.Context, orgID, appID DO UPDATE SET url = EXCLUDED.url, secret = EXCLUDED.secret, event_types = EXCLUDED.event_types, enabled = true, auto_disabled_at = NULL, disabled_reason = NULL, updated_at = NOW() - `, orgID, url, "Managed by OAuth app", secret, eventTypes, appID) + `, orgID, url, "Managed by OAuth app", secret, textArray(eventTypes), appID) return err } diff --git a/internal/repository/webhook_event_filter_live_test.go b/internal/repository/webhook_event_filter_live_test.go new file mode 100644 index 00000000..59a77820 --- /dev/null +++ b/internal/repository/webhook_event_filter_live_test.go @@ -0,0 +1,72 @@ +package repository + +import ( + "context" + "testing" + + "github.com/warmbly/warmbly/internal/models" +) + +// An omitted event filter is the documented "every non-firehose event", which +// the matcher reads as an empty array. It used to bind as NULL and fail the +// NOT NULL column, so POST /webhooks without event_types answered with the raw +// Postgres error (the same defect as issue #343 on forms). +// +// WARMBLY_TEST_DB=postgres://warmbly:warmbly@localhost:15432/warmbly_dev?sslmode=disable \ +// go test ./internal/repository/ -run LiveWebhookEndpointEmptyEventFilter -v +func TestLiveWebhookEndpointEmptyEventFilter(t *testing.T) { + _, pool := liveContactDB(t) + f := newSharedOrgFixture(t, pool) + repo := NewWebhookRepository(pool) + ctx := context.Background() + + endpoint := &models.WebhookEndpoint{ + OrganizationID: f.org, + URL: "https://example.com/hooks/issue343", + Description: "no filter", + Enabled: true, + } + if err := repo.CreateEndpoint(ctx, endpoint, "secret", "token"); err != nil { + t.Fatalf("create endpoint without event types: %v", err) + } + t.Cleanup(func() { + if _, err := pool.Exec(context.Background(), `DELETE FROM webhook_endpoints WHERE id = $1`, endpoint.ID); err != nil { + t.Errorf("cleanup endpoint: %v", err) + } + }) + if endpoint.EventTypes == nil { + t.Fatal("the created endpoint still reports a null event filter") + } + + stored, err := repo.GetEndpoint(ctx, f.org, endpoint.ID) + if err != nil || stored == nil { + t.Fatalf("get endpoint: %v", err) + } + if len(stored.EventTypes) != 0 { + t.Fatalf("event types: %#v", stored.EventTypes) + } + + // The empty filter is what makes the endpoint match a standard event. + if _, err := pool.Exec(ctx, `UPDATE webhook_endpoints SET verified_at = NOW() WHERE id = $1`, endpoint.ID); err != nil { + t.Fatalf("verify endpoint: %v", err) + } + matched, err := repo.MatchingEndpoints(ctx, f.org, models.WebhookEventCampaignReplyReceived) + if err != nil { + t.Fatalf("matching endpoints: %v", err) + } + found := false + for _, m := range matched { + if m.ID == endpoint.ID { + found = true + } + } + if !found { + t.Fatal("an endpoint with no filter did not match a standard event") + } + + // Clearing the filter on update must not reintroduce the NULL either. + endpoint.EventTypes = nil + if err := repo.UpdateEndpoint(ctx, endpoint); err != nil { + t.Fatalf("update endpoint without event types: %v", err) + } +} diff --git a/web/src/components/app/forms/ShareTab.tsx b/web/src/components/app/forms/ShareTab.tsx index f54d706d..4040cccb 100644 --- a/web/src/components/app/forms/ShareTab.tsx +++ b/web/src/components/app/forms/ShareTab.tsx @@ -149,7 +149,17 @@ function PersonalizedLinksCard({ form }: { form: Form }) { export default function ShareTab({ form, baseUrl }: { form: Form; baseUrl: string }) { const pageUrl = form.share_url || (baseUrl ? `${baseUrl}/f/${form.public_id}` : ""); - const scriptUrl = baseUrl ? `${baseUrl}/forms.js` : ""; + // The embed loads its iframe from its own origin, so the snippet has to + // come from the same host as the page: an organization on a verified + // custom forms domain would otherwise embed on the shared one. + const scriptOrigin = React.useMemo(() => { + try { + return new URL(pageUrl).origin; + } catch { + return baseUrl; + } + }, [pageUrl, baseUrl]); + const scriptUrl = scriptOrigin ? `${scriptOrigin}/forms.js` : ""; if (!pageUrl) { return (