From 87a7e3e1edaa7b1481a1e18b990f4fe3c8ebf9d1 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Mon, 7 Sep 2026 04:35:56 -0700 Subject: [PATCH 1/2] feat: make a hosted form creatable again by binding an unset embed allowlist and field list as empty arrays instead of NULL in the forms repository writes, which is what made every New form fail with a not-null violation on forms.allowed_domains since the feature shipped (issue #343), and while proving the flow end to end keep the port on the shared forms host so share links and embeds resolve on a ported install, build the embed snippet on the form's own origin so an organization on a verified custom forms domain does not embed from the shared one, and apply the same empty-array fix to webhook endpoints created without event_types, which answered the raw Postgres error instead of the documented subscribe-to-everything --- internal/app/form/domain.go | 2 +- internal/config/endpoints.go | 10 ++- internal/repository/form_live_test.go | 44 ++++++++++++ internal/repository/pg_form.go | 36 ++++++++-- internal/repository/pg_webhook.go | 8 ++- .../webhook_event_filter_live_test.go | 72 +++++++++++++++++++ web/src/components/app/forms/ShareTab.tsx | 12 +++- 7 files changed, 174 insertions(+), 10 deletions(-) create mode 100644 internal/repository/webhook_event_filter_live_test.go 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..c4b2d197 100644 --- a/internal/config/endpoints.go +++ b/internal/config/endpoints.go @@ -76,11 +76,19 @@ func FormsBaseURL() string { } // 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. 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 ( From 372df39eaaeef8011803913ceb04752ad31c99aa Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Mon, 7 Sep 2026 04:45:21 -0700 Subject: [PATCH 2/2] feat: decide the hosted form URL scheme from the host rather than the port, so an install that terminates TLS on a non-default port (forms.example.com:8443) keeps https in its share links, embeds and base_url instead of being downgraded to http by the port check, with a private-network host now treated as the LAN install it is, one scheme helper shared by FormsBaseURL and FormURLOn so the builder's base_url and a form's share_url can never disagree, and a table test covering every install shape --- internal/config/endpoints.go | 41 ++++++++++--------- internal/config/endpoints_test.go | 66 +++++++++++++++++++++++-------- 2 files changed, 70 insertions(+), 37 deletions(-) diff --git a/internal/config/endpoints.go b/internal/config/endpoints.go index c4b2d197..c5a2e9a5 100644 --- a/internal/config/endpoints.go +++ b/internal/config/endpoints.go @@ -63,16 +63,26 @@ 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 @@ -97,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) } }