From d0e10f2d46d721a47e58707c99d5c487233997ec Mon Sep 17 00:00:00 2001 From: Igor Serganov <41109559+iSerganov@users.noreply.github.com> Date: Mon, 2 Nov 2026 12:35:24 -0700 Subject: [PATCH] util: read .json files as HuJSON in UnmarshalByExt (#3634) dns.extra_records_path and derp.paths files ending in .json were decoded with encoding/json, so a comment or trailing comma in otherwise valid JSON made headscale refuse to load the file. Policy files already accept HuJSON regardless of extension. HuJSON is a superset of JSON, so standardizing .json input the same way as .hujson keeps every valid JSON file decoding exactly as before. Add tests for comments and trailing commas in extra records, in generic .json decoding and in ACL policies, and for comment markers inside string values. Fixes #3627 --- CHANGELOG.md | 1 + config-example.yaml | 6 +- docs/ref/derp.md | 3 +- docs/ref/dns.md | 2 + hscontrol/derp/derp_test.go | 3 + hscontrol/dns/extrarecords_test.go | 92 +++++++++++++++++++++++++++++ hscontrol/policy/v2/types_test.go | 95 ++++++++++++++++++++++++++++++ hscontrol/util/file.go | 11 ++-- hscontrol/util/util_test.go | 24 +++++--- 9 files changed, 219 insertions(+), 18 deletions(-) create mode 100644 hscontrol/dns/extrarecords_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cca5bb76..decaf7dc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -203,6 +203,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - Nodes stay ephemeral after key deletion. [#3640](https://github.com/juanfont/headscale/issues/3640), [#3619](https://github.com/juanfont/headscale/pull/3619) - User renames and OIDC updates refresh node users and clients without restarting Headscale. [#3471](https://github.com/juanfont/headscale/issues/3471), [#3619](https://github.com/juanfont/headscale/pull/3619) - Routers receive their own approved routes regardless of policy. [#3626](https://github.com/juanfont/headscale/issues/3626), [#3619](https://github.com/juanfont/headscale/pull/3619) +- `.json` files in `dns.extra_records_path` and `derp.paths` may contain comments and trailing commas, like `.hujson` files [#3627](https://github.com/juanfont/headscale/issues/3627) ## 0.29.7 (2026-10-30) diff --git a/config-example.yaml b/config-example.yaml index 578565101..9a788f44c 100644 --- a/config-example.yaml +++ b/config-example.yaml @@ -115,7 +115,8 @@ derp: - https://controlplane.tailscale.com/derpmap/default # Locally available DERP map files. The extension picks the format: .yaml/.yml, - # or Tailscale JSON as .json or .hujson. + # or Tailscale JSON as .json or .hujson. Both JSON extensions accept comments + # and trailing commas. # # This option is mostly interesting for people hosting their own DERP servers: # https://tailscale.com/docs/reference/derp-servers/custom-derp-servers @@ -351,7 +352,8 @@ dns: # - { name: "prometheus.myvpn.example.com", type: "A", value: "100.64.0.3" } # # Alternatively, extra DNS records can be loaded from a file. The extension - # picks the format: .json, .hujson, .yaml or .yml. + # picks the format: .json, .hujson, .yaml or .yml. Both JSON extensions + # accept comments and trailing commas. # Headscale processes this file on each change. # extra_records_path: /var/lib/headscale/extra-records.json diff --git a/docs/ref/derp.md b/docs/ref/derp.md index c53c3c1ff..166b60818 100644 --- a/docs/ref/derp.md +++ b/docs/ref/derp.md @@ -55,7 +55,8 @@ derp: The DERP map offered to clients can be customized with a [dedicated YAML-configuration file](https://github.com/juanfont/headscale/blob/main/derp-example.yaml). The file extension picks the format: `.yaml` -or `.yml`, or the JSON format Tailscale serves DERP maps in as `.json` or `.hujson`. This allows to modify previously +or `.yml`, or the JSON format Tailscale serves DERP maps in as `.json` or `.hujson`. Both JSON extensions are read as +[HuJSON](https://github.com/tailscale/hujson), so comments and trailing commas are allowed. This allows to modify previously loaded DERP maps fetched via URL or to offer your own, custom DERP servers to nodes. === "Remove specific DERP regions" diff --git a/docs/ref/dns.md b/docs/ref/dns.md index af080a42e..f175b16ad 100644 --- a/docs/ref/dns.md +++ b/docs/ref/dns.md @@ -68,6 +68,8 @@ hostname and port combination "http://hostname-in-magic-dns.myvpn.example.com:30 - The `dns.extra_records_path` option in the [configuration file](configuration.md) needs to reference the file containing extra DNS records. Its extension picks the format: `.json`, `.hujson`, `.yaml` or `.yml`. + A `.json` file is read as [HuJSON](https://github.com/tailscale/hujson), so it may contain comments and + trailing commas. - Be sure to "sort keys" and produce a stable output in case you generate the JSON file with a script. Headscale uses a checksum to detect changes to the file and a stable output avoids unnecessary processing. diff --git a/hscontrol/derp/derp_test.go b/hscontrol/derp/derp_test.go index 07094c077..79c160cdd 100644 --- a/hscontrol/derp/derp_test.go +++ b/hscontrol/derp/derp_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -399,7 +400,9 @@ func TestLoadDERPMapFromPath(t *testing.T) { content: "{regions: {999: {regionid: 999, regioncode: test, nodes: [{name: 999a, regionid: 999, hostname: derp.test, derpport: 443, insecurefortests: true}]}}}", }, {name: "tailcfg json", file: "derp.json", content: string(tailcfgJSON)}, + {name: "tailcfg json with comments and trailing comma", file: "derp.json", content: "// test map\n" + strings.TrimSuffix(string(tailcfgJSON), "}") + ",}"}, {name: "tailcfg hujson", file: "derp.hujson", content: "// test map\n" + string(tailcfgJSON)}, + {name: "tailcfg json with unterminated comment", file: "derp.json", content: string(tailcfgJSON) + " /*", wantErr: true}, {name: "json as yaml decodes to nothing", file: "derp.yaml", content: string(tailcfgJSON), wantErr: true}, {name: "empty", file: "derp.yaml", content: "", wantErr: true}, {name: "no extension", file: "derp", content: yamlMap, wantErr: true}, diff --git a/hscontrol/dns/extrarecords_test.go b/hscontrol/dns/extrarecords_test.go new file mode 100644 index 000000000..d25e698d0 --- /dev/null +++ b/hscontrol/dns/extrarecords_test.go @@ -0,0 +1,92 @@ +package dns + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" +) + +func TestNewExtraRecordsManagerReadsFormats(t *testing.T) { + want := []tailcfg.DNSRecord{ + {Name: "grafana.example.com", Type: "A", Value: "100.64.0.3"}, + {Name: "prometheus.example.com", Type: "AAAA", Value: "fd7a:115c:a1e0::3"}, + } + + tests := []struct { + name string + file string + content string + wantErr bool + }{ + { + name: "json", + file: "extra-records.json", + content: `[ + {"name": "grafana.example.com", "type": "A", "value": "100.64.0.3"}, + {"name": "prometheus.example.com", "type": "AAAA", "value": "fd7a:115c:a1e0::3"} + ]`, + }, + { + name: "json with comments between entries", + file: "extra-records.json", + content: `[ + // monitoring dashboards + {"name": "grafana.example.com", "type": "A", "value": "100.64.0.3"}, + /* metrics + backend */ + {"name": "prometheus.example.com", "type": "AAAA", "value": "fd7a:115c:a1e0::3"} // v6 only + ]`, + }, + { + name: "json with trailing commas", + file: "extra-records.json", + content: `[ + {"name": "grafana.example.com", "type": "A", "value": "100.64.0.3",}, + {"name": "prometheus.example.com", "type": "AAAA", "value": "fd7a:115c:a1e0::3"}, + ]`, + }, + { + name: "hujson with comments", + file: "extra-records.hujson", + content: `[ + // monitoring dashboards + {"name": "grafana.example.com", "type": "A", "value": "100.64.0.3"}, + {"name": "prometheus.example.com", "type": "AAAA", "value": "fd7a:115c:a1e0::3"}, + ]`, + }, + { + name: "json with unterminated comment", + file: "extra-records.json", + content: `[{"name": "grafana.example.com", "type": "A", "value": "100.64.0.3"}] /*`, + wantErr: true, + }, + { + name: "json with object instead of list", + file: "extra-records.json", + content: `{"name": "grafana.example.com", "type": "A", "value": "100.64.0.3"}`, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), tt.file) + require.NoError(t, os.WriteFile(path, []byte(tt.content), 0o600)) + + er, err := NewExtraRecordsManager(path) + if tt.wantErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + t.Cleanup(er.Close) + + assert.ElementsMatch(t, want, er.Records()) + }) + } +} diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 747b641cc..8c5ecd7a3 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -2379,6 +2379,101 @@ func TestUnmarshalPolicy(t *testing.T) { } } +func TestUnmarshalPolicyCommentsMatchPlainJSON(t *testing.T) { + const plain = `{ + "groups": {"group:admins": ["admin@example.com"]}, + "hosts": {"db": "100.64.0.10"}, + "acls": [{"action": "accept", "src": ["group:admins"], "dst": ["db:5432"]}] + }` + + want, err := unmarshalPolicy([]byte(plain)) + require.NoError(t, err) + + tests := []struct { + name string + input string + wantErr string + }{ + { + name: "plain json", + input: plain, + }, + { + name: "line comments", + input: `{ + // people allowed to reach the database + "groups": {"group:admins": ["admin@example.com"]}, // on-call rota + "hosts": {"db": "100.64.0.10"}, + // postgres only + "acls": [{"action": "accept", "src": ["group:admins"], "dst": ["db:5432"]}] + }`, + }, + { + name: "block comments", + input: `/* production policy */ + { + "groups": {"group:admins": [/* lead */ "admin@example.com"]}, + "hosts": {"db": "100.64.0.10"}, + /* + postgres only + */ + "acls": [{"action": "accept", "src": ["group:admins"], "dst": ["db:5432"]}] + }`, + }, + { + name: "trailing commas", + input: `{ + "groups": {"group:admins": ["admin@example.com",],}, + "hosts": {"db": "100.64.0.10",}, + "acls": [{"action": "accept", "src": ["group:admins",], "dst": ["db:5432",],},], + }`, + }, + { + name: "comments and trailing commas mixed", + input: `{ + "groups": { + "group:admins": [ + "admin@example.com", // lead + ], + }, + /* inventory */ + "hosts": {"db": "100.64.0.10",}, + "acls": [ + {"action": "accept", "src": ["group:admins"], "dst": ["db:5432"]}, // postgres + ], + }`, + }, + { + name: "unterminated block comment", + input: `{"acls": [] /* never closed`, + wantErr: "parsing HuJSON", + }, + } + + cmps := append(util.Comparers, + cmp.Comparer(func(x, y Prefix) bool { + return x == y + }), + cmpopts.IgnoreUnexported(Policy{}), + ) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := unmarshalPolicy([]byte(tt.input)) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + return + } + + require.NoError(t, err) + + if diff := cmp.Diff(want, got, cmps...); diff != "" { + t.Errorf("unmarshalPolicy mismatch (-plain +commented):\n%s", diff) + } + }) + } +} + func gp(s string) *Group { return new(Group(s)) } func up(s string) *Username { return new(Username(s)) } func hp(s string) *Host { return new(Host(s)) } diff --git a/hscontrol/util/file.go b/hscontrol/util/file.go index 8b9091896..c2c81b606 100644 --- a/hscontrol/util/file.go +++ b/hscontrol/util/file.go @@ -35,9 +35,10 @@ var ErrInvalidFileMode = errors.New("not an octal file mode between 0 and 0777") var ErrUnknownFileFormat = errors.New("unknown file format, want .json, .hujson, .yaml or .yml") // UnmarshalByExt decodes data into a T in the format name's extension picks: -// .json, .hujson (JSON with comments and trailing commas), or .yaml/.yml. -// The extension decides, not the content: YAML parses JSON syntax, so sniffing -// cannot tell the two apart. YAML keys are the lowercased Go field names. +// .json or .hujson, both read as HuJSON (JSON with comments and trailing +// commas), or .yaml/.yml. The extension decides, not the content: YAML parses +// JSON syntax, so sniffing cannot tell the two apart. YAML keys are the +// lowercased Go field names. func UnmarshalByExt[T any](name string, data []byte) (T, error) { var ( v T @@ -45,9 +46,7 @@ func UnmarshalByExt[T any](name string, data []byte) (T, error) { ) switch ext := strings.ToLower(filepath.Ext(name)); ext { - case ".json": - err = json.Unmarshal(data, &v) - case ".hujson": + case ".json", ".hujson": data, err = hujson.Standardize(data) if err == nil { err = json.Unmarshal(data, &v) diff --git a/hscontrol/util/util_test.go b/hscontrol/util/util_test.go index df2d28b28..48906d399 100644 --- a/hscontrol/util/util_test.go +++ b/hscontrol/util/util_test.go @@ -901,22 +901,28 @@ func TestUnmarshalByExt(t *testing.T) { Port int `json:"port"` } - want := rec{Name: "a", Port: 1} + a1 := rec{Name: "a", Port: 1} tests := []struct { name string file string data string + want rec wantFormat bool // want ErrUnknownFileFormat wantErr bool }{ - {name: "json", file: "x.json", data: `{"name": "a", "port": 1}`}, - {name: "extension case is ignored", file: "x.JSON", data: `{"name": "a", "port": 1}`}, - {name: "hujson", file: "x.hujson", data: "{\n // comment\n \"name\": \"a\",\n \"port\": 1,\n}"}, - {name: "yaml", file: "x.yaml", data: "name: a\nport: 1\n"}, - {name: "yml", file: "x.yml", data: "name: a\nport: 1\n"}, - // The extension is binding: a .json file does not get HuJSON leniency. - {name: "comments in json", file: "x.json", data: "{\n // comment\n \"name\": \"a\"\n}", wantErr: true}, + {name: "json", file: "x.json", data: `{"name": "a", "port": 1}`, want: a1}, + {name: "extension case is ignored", file: "x.JSON", data: `{"name": "a", "port": 1}`, want: a1}, + {name: "json with line comments", file: "x.json", data: "{\n // comment\n \"name\": \"a\", // trailing\n \"port\": 1\n}", want: a1}, + {name: "json with block comments", file: "x.json", data: "/* header */ {\"name\": /* inline */ \"a\", \"port\": 1}", want: a1}, + {name: "json with trailing commas", file: "x.json", data: `{"name": "a", "port": 1,}`, want: a1}, + {name: "json comment markers inside strings are kept", file: "x.json", data: `{"name": "https://a/*b*/", "port": 1}`, want: rec{Name: "https://a/*b*/", Port: 1}}, + {name: "hujson", file: "x.hujson", data: "{\n // comment\n \"name\": \"a\",\n \"port\": 1,\n}", want: a1}, + {name: "yaml", file: "x.yaml", data: "name: a\nport: 1\n", want: a1}, + {name: "yml", file: "x.yml", data: "name: a\nport: 1\n", want: a1}, + {name: "json with unterminated block comment", file: "x.json", data: `{"name": "a", "port": 1} /* open`, wantErr: true}, + {name: "json with missing value", file: "x.json", data: `{"name": "a", "port": }`, wantErr: true}, + {name: "json with wrong value type", file: "x.json", data: `{"name": "a", "port": "1"}`, wantErr: true}, {name: "unknown extension", file: "x.txt", data: `{"name": "a", "port": 1}`, wantFormat: true}, {name: "no extension", file: "x", data: `{"name": "a", "port": 1}`, wantFormat: true}, } @@ -944,7 +950,7 @@ func TestUnmarshalByExt(t *testing.T) { t.Fatalf("UnmarshalByExt(%q) error = %v", tt.file, err) } - if diff := cmp.Diff(want, got); diff != "" { + if diff := cmp.Diff(tt.want, got); diff != "" { t.Errorf("UnmarshalByExt(%q) mismatch (-want +got):\n%s", tt.file, diff) } })