From dc0867c3edd8724ab523e5cba3ecda78c358cde5 Mon Sep 17 00:00:00 2001 From: Jamil Date: Tue, 3 Jun 2025 12:40:02 -0700 Subject: [PATCH] fix(portal): prevent resource addresses like **test.com (#9384) These aren't allowed on the clients, causing an error. Fixes: #9054 --------- Signed-off-by: Jamil Co-authored-by: Thomas Eizinger --- .../domain/resources/resource/changeset.ex | 24 +- .../resources/resource/changeset_test.exs | 220 +++++++++--------- 2 files changed, 138 insertions(+), 106 deletions(-) diff --git a/elixir/apps/domain/lib/domain/resources/resource/changeset.ex b/elixir/apps/domain/lib/domain/resources/resource/changeset.ex index f0addda09..a4c9be324 100644 --- a/elixir/apps/domain/lib/domain/resources/resource/changeset.ex +++ b/elixir/apps/domain/lib/domain/resources/resource/changeset.ex @@ -123,7 +123,13 @@ defmodule Domain.Resources.Resource.Changeset do defp validate_dns_address(changeset) do changeset |> validate_length(:address, min: 1, max: 253) - # Reject IPs (IPv4 and IPv6) + |> validate_not_an_ip_address() + |> validate_contains_only_valid_dns_characters() + |> validate_dns_parts() + end + + defp validate_not_an_ip_address(changeset) do + changeset |> validate_change(:address, fn field, address -> cond do String.match?(address, ~r/^(\d+\.){3}\d+(\/\d+)?$/) -> @@ -139,12 +145,20 @@ defmodule Domain.Resources.Resource.Changeset do [] end end) + end + + defp validate_contains_only_valid_dns_characters(changeset) do + changeset |> validate_format( :address, - ~r/^[a-zA-Z0-9\p{L}\-*?]+(?:\.[a-zA-Z0-9\p{L}\-*?]+)*$/u, + ~r/^(?:[a-zA-Z0-9\p{L}*?](?:[a-zA-Z0-9\p{L}\-*?]*[a-zA-Z0-9\p{L}*?])?)(?:\.(?:[a-zA-Z0-9\p{L}*?](?:[a-zA-Z0-9\p{L}\-*?]*[a-zA-Z0-9\p{L}*?])?))*$/u, message: "must be a valid hostname (letters, digits, hyphens, dots; wildcards *, ?, ** allowed)" ) + end + + defp validate_dns_parts(changeset) do + changeset |> validate_change(:address, fn field, dns_address -> parts = String.split(dns_address, ".") @@ -155,6 +169,9 @@ defmodule Domain.Resources.Resource.Changeset do end cond do + Enum.any?(parts, &(String.length(&1) > 63)) -> + [{field, "each label must not exceed 63 characters"}] + String.contains?(tld, ["*", "?"]) -> [{field, "TLD cannot contain wildcards"}] @@ -164,6 +181,9 @@ defmodule Domain.Resources.Resource.Changeset do "#{tld} cannot be used as a TLD. Try adding a DNS alias to /etc/hosts on the Gateway(s) instead"} ] + Enum.any?(domain_parts, fn part -> String.contains?(part, "**") and part != "**" end) -> + [{field, "wildcard pattern must not contain ** in the middle of a label"}] + Enum.all?(parts, &(&1 == "*")) -> [{field, "wildcard pattern must include a valid domain"}] diff --git a/elixir/apps/domain/test/domain/resources/resource/changeset_test.exs b/elixir/apps/domain/test/domain/resources/resource/changeset_test.exs index 6a638b0a6..4e6da33cd 100644 --- a/elixir/apps/domain/test/domain/resources/resource/changeset_test.exs +++ b/elixir/apps/domain/test/domain/resources/resource/changeset_test.exs @@ -2,6 +2,91 @@ defmodule Domain.Resources.Resource.ChangesetTest do use Domain.DataCase, async: true import Domain.Resources.Resource.Changeset + @valid_dns_addresses [ + "**.foo.google.com", + "?.foo.google.com", + "web-*.foo.google.com", + "web-?.foo.google.com", + "web-*.google.com", + "web-?.google.com", + "sub.**.example.com", + "sub.**.foo.bar.google.com", + "**.google.com", + "?.google.com", + "*.*.*.*.google.com", + "?.?.?.?.google.com", + "*.google", + "?.google", + "google", + "example.com", + "example.weird", + "1234567890.com", + "#{String.duplicate("a", 63)}.com", + "такі.справи", + "subdomain.subdomain2.example.space", + "single.label", + "a-b-c.d-e-f.g-h-i", + "xn--fssq61j.com", + "*.xn--fssq61j.com", + "**.xn--fssq61j.com", + "sub.**.xn--fssq61j.com", + "a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.com", + "sub.domain", + "sub-domain.com", + "sub--domain.com" + ] + + @invalid_dns_addresses [ + "2600::1/32", + "2600::1", + "1.1.1.1/32", + "1.1.1.1", + ".example.com", + "example..com", + "**example.com", + "example. com.**", + "example.com.**", + "foo.**bar.com", + "**", + "**.", + "*.", + "*.*", + "?", + "?.", + "example.com.", + "exa&mple.com", + "", + "http://example.com/", + "//example.com/", + "example.com/", + ".example.com", + "example.", + "example.com:80", + "-example.com", + "example-.com", + "example_com", + "example..com", + "too.long.#{String.duplicate("a", 256)}", + "a.#{String.duplicate("a", 64)}.com", + "example.com/", + "example.com?", + "example.com*", + "*.com.", + "**.com.", + "foo.**", + "foo**.com", + "**.example.com.", + "example..**", + "**example.com", + "***.foo.com", + "**.**test.com", + "**.foo**test.com", + "foo.**bar", + "**test", + "*.com", + "?.com" + ] + setup do account = Fixtures.Accounts.create_account() actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account) @@ -30,7 +115,9 @@ defmodule Domain.Resources.Resource.ChangesetTest do }) assert changeset.changes[:address] == cidr - assert changeset.valid? + + assert changeset.valid?, + "Expected '#{string}' to be valid, got: #{inspect(changeset.errors)}" end [ @@ -48,7 +135,8 @@ defmodule Domain.Resources.Resource.ChangesetTest do "2607:f8b0:4012:0::200e/128:80" ] |> Enum.each(fn string -> - refute create(%{type: :cidr, address: string}).valid? + refute create(%{type: :cidr, address: string}).valid?, + "Expected '#{string}' to be invalid" end) end @@ -68,7 +156,9 @@ defmodule Domain.Resources.Resource.ChangesetTest do }) assert changeset.changes[:address] == ip - assert changeset.valid? + + assert changeset.valid?, + "Expected '#{string}' to be valid, got: #{inspect(changeset.errors)}" end [ @@ -83,33 +173,13 @@ defmodule Domain.Resources.Resource.ChangesetTest do "[2607:f8b0:4012:0::200e]:80" ] |> Enum.each(fn string -> - refute create(%{type: :ip, address: string}).valid? + refute create(%{type: :ip, address: string}).valid?, + "Expected '#{string}' to be invalid" end) end test "accepts valid DNS addresses" do - for valid_address <- [ - "**.foo.google.com", - "?.foo.google.com", - "web-*.foo.google.com", - "web-?.foo.google.com", - "web-*.google.com", - "web-?.google.com", - "**.*.?.foo.foo.com", - "**.google.com", - "?.google.com", - "*.*.*.*.google.com", - "?.?.?.?.google.com", - "*.google", - "?.google", - "google", - "example.com", - "example.weird", - "1234567890.com", - "#{String.duplicate("a", 63)}.com", - "такі.справи", - "subdomain.subdomain2.example.space" - ] do + for valid_address <- @valid_dns_addresses do changeset = create(%{ name: "foo", @@ -118,33 +188,14 @@ defmodule Domain.Resources.Resource.ChangesetTest do address_description: valid_address }) - assert changeset.valid? + assert changeset.valid?, + "Expected '#{valid_address}' to be valid, got: #{inspect(changeset.errors)}" end - [ - "1.1.1.1/32", - "1.1.1.1", - ".example.com", - "example..com", - "**", - "**.", - "*.", - "*.*", - "?", - "?.", - "example.com.", - "exa&mple.com", - "", - "http://example.com/", - "//example.com/", - "example.com/", - ".example.com", - "example.", - "example.com:80" - ] - |> Enum.each(fn invalid_address -> - refute create(%{type: :dns, address: invalid_address}).valid? - end) + for invalid_address <- @invalid_dns_addresses do + changeset = create(%{type: :dns, address: invalid_address}) + refute changeset.valid?, "Expected '#{invalid_address}' to be invalid" + end end end @@ -170,7 +221,9 @@ defmodule Domain.Resources.Resource.ChangesetTest do ) assert changeset.changes.address == cidr - assert changeset.valid? + + assert changeset.valid?, + "Expected '#{string}' to be valid, got: #{inspect(changeset.errors)}" end [ @@ -189,7 +242,7 @@ defmodule Domain.Resources.Resource.ChangesetTest do ] |> Enum.each(fn invalid_cidr -> {changeset, _} = update(resource, %{type: :cidr, address: invalid_cidr}, subject) - refute changeset.valid? + refute changeset.valid?, "Expected '#{invalid_cidr}' to be invalid" end) end @@ -213,7 +266,9 @@ defmodule Domain.Resources.Resource.ChangesetTest do ) assert changeset.changes.address == ip - assert changeset.valid? + + assert changeset.valid?, + "Expected '#{string}' to be valid, got: #{inspect(changeset.errors)}" end [ @@ -229,33 +284,12 @@ defmodule Domain.Resources.Resource.ChangesetTest do ] |> Enum.each(fn invalid_ip -> {changeset, _} = update(resource, %{type: :ip, address: invalid_ip}, subject) - refute changeset.valid? + refute changeset.valid?, "Expected '#{invalid_ip}' to be invalid" end) end test "accepts valid DNS addresses", %{resource: resource, subject: subject} do - for valid_address <- [ - "**.foo.google.com", - "?.foo.google.com", - "web-*.foo.google.com", - "web-?.foo.google.com", - "web-*.google.com", - "web-?.google.com", - "**.*.?.foo.foo.com", - "**.google.com", - "?.google.com", - "*.*.*.*.google.com", - "?.?.?.?.google.com", - "*.google", - "?.google", - "google", - "example.com", - "example.weird", - "1234567890.com", - "#{String.duplicate("a", 63)}.com", - "такі.справи", - "subdomain.subdomain2.example.space" - ] do + for valid_address <- @valid_dns_addresses do {changeset, _} = update( resource, @@ -268,36 +302,14 @@ defmodule Domain.Resources.Resource.ChangesetTest do subject ) - assert changeset.valid? + assert changeset.valid?, + "Expected '#{valid_address}' to be valid, got: #{inspect(changeset.errors)}" end - [ - "2600::1/32", - "2600::1", - "1.1.1.1/32", - "1.1.1.1", - ".example.com", - "example..com", - "**", - "**.", - "*.", - "*.*", - "?", - "?.", - "example.com.", - "exa&mple.com", - "", - "http://example.com/", - "//example.com/", - "example.com/", - ".example.com", - "example.", - "example.com:80" - ] - |> Enum.each(fn invalid_address -> + for invalid_address <- @invalid_dns_addresses do {changeset, _} = update(resource, %{type: :dns, address: invalid_address}, subject) - refute changeset.valid? - end) + refute changeset.valid?, "Expected '#{invalid_address}' to be invalid" + end end end