Merge pull request #368 from warmbly/fix/issue-343-behaviour

feat: fix hosted form creation failing with a not-null violation on allowed_domains

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
 	}

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.

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)
 	}
 }

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)
+	}
+}

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()

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
 }
 

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)
+	}
+}

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 (