From 34c43c8a9d9427df6446a940d7bc988da71bba8a Mon Sep 17 00:00:00 2001 From: Kuzz007 Date: Thu, 30 Jul 2026 01:16:12 +0300 Subject: [PATCH] fix: address the automated review round on PR #6154 - Replace parseGeodataFile's full proto.Unmarshal with a protowire-based scan that reads only each entry's Code, skipping every Domain/CIDR payload without allocating it -- the actual bulk of a real geoip.dat/geosite.dat. Also caps the file read at 256 MiB. - Hold geodataMu across the full scan-and-maybe-parse in GetGeodataCategories instead of releasing it around the parse, so concurrent cache misses (e.g. several browser tabs) can't all independently re-parse every file; clone the cached slices before returning them so a caller mutating its result can't corrupt the cache. - Gate useGeodataCategories on the rule editor's own `open` state instead of firing on every visit to the Routing tab. - formatGeodataSuggestion now compares filenames with strings.EqualFold, matching scanGeodataFiles' own case-insensitive match -- a file that IS the default one on a case-insensitive filesystem (e.g. Windows) no longer gets the long ext: form. - Fix a real bug the review's hypothesis led to: Select mode="tags" only commits the search text on Enter/comma, so clicking Save right after typing (a blur, not an Enter) silently dropped the value entirely, with no domain/ip key at all in the saved rule. Wrap it in a small TagsAutocomplete that also commits on blur. Same autocomplete now applies to sourceIP, which accepts geoip:/ext: too. - Guard useGeodataCategories' fetch per-field with Array.isArray instead of a single top-level `?? EMPTY_CATEGORIES`, since parseMsg returns the original unvalidated obj (not null) on a schema mismatch. - Test fixes: exact slices.Equal instead of slices.Contains-only assertions, t.Run subtests, a cache-hit-skips-reparse test (via a test-only parse counter), a returns-independent-slices test, a file-size-cap test, and four new frontend tests covering the tags round-trip including the blur-commit regression above. - GeodataCategories now goes through the same generated-example path as every other response type (StructAllow + example: tags + responseSchema in endpoints.ts) instead of a hand-written response string. The existing hand-written GeodataCategoriesSchema in schemas/routing.ts is unrelated to this and is left alone -- CLAUDE.md is explicit that Zod schemas under src/schemas/ are the source of truth and only the generated example/openapi path comes from Go example: tags. - Drop the two PR-illustration screenshots from media/ -- nothing in the repo referenced them; they only ever needed to exist in the PR description itself. Not changed: leaving geodataFileKind's leak into generated/{types,zod}.ts as-is. internal/web/service's openapigen request has no AliasAllow at all, so every non-struct type in the package already leaks this way (e.g. staticEgressResolver, transportBits predate this PR) -- scoping an AliasAllow for the whole package is a real cleanup but a separate, wider change than this PR's own footprint, and needs checking nothing already depends on those existing generated aliases first. Co-Authored-By: Claude Sonnet 5 --- frontend/public/openapi.json | 34 +++- .../src/api/queries/useGeodataCategories.ts | 19 +- frontend/src/generated/examples.ts | 10 ++ frontend/src/generated/schemas.ts | 30 ++++ frontend/src/generated/types.ts | 5 + frontend/src/generated/zod.ts | 6 + frontend/src/pages/api-docs/endpoints.ts | 2 +- .../src/pages/xray/routing/RuleFormModal.tsx | 68 +++++-- .../src/test/rule-form-geodata-tags.test.tsx | 117 ++++++++++++ internal/web/service/xray_setting_geodata.go | 167 +++++++++++++----- .../web/service/xray_setting_geodata_test.go | 144 +++++++++++---- tools/openapigen/main.go | 1 + 12 files changed, 507 insertions(+), 96 deletions(-) create mode 100644 frontend/src/test/rule-form-geodata-tags.test.tsx diff --git a/frontend/public/openapi.json b/frontend/public/openapi.json index ae34f7704..02460031e 100644 --- a/frontend/public/openapi.json +++ b/frontend/public/openapi.json @@ -1378,6 +1378,36 @@ ], "type": "object" }, + "GeodataCategories": { + "description": "GeodataCategories lists every geosite/geoip category found in the .dat\nfiles currently present in the Xray bin folder, already formatted as\nready-to-use xray-core routing-rule values (see formatGeodataSuggestion).\nReturned by XraySettingService.GetGeodataCategories and served as\nGET /panel/api/xray/getGeodataCategories for the routing rule editor's\nDomain/IP autocomplete.", + "properties": { + "domain": { + "example": [ + "geosite:cn", + "geosite:youtube" + ], + "items": { + "type": "string" + }, + "type": "array" + }, + "ip": { + "example": [ + "geoip:cn", + "geoip:private" + ], + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "domain", + "ip" + ], + "type": "object" + }, "HistoryOfSeeders": { "description": "HistoryOfSeeders tracks which database seeders have been executed to prevent re-running.", "properties": { @@ -10473,7 +10503,9 @@ "msg": { "type": "string" }, - "obj": {} + "obj": { + "$ref": "#/components/schemas/GeodataCategories" + } } }, "example": { diff --git a/frontend/src/api/queries/useGeodataCategories.ts b/frontend/src/api/queries/useGeodataCategories.ts index ffb5aeaf3..b1cfca727 100644 --- a/frontend/src/api/queries/useGeodataCategories.ts +++ b/frontend/src/api/queries/useGeodataCategories.ts @@ -11,7 +11,15 @@ async function fetchGeodataCategories(): Promise { const msg = await HttpUtil.get('/panel/api/xray/getGeodataCategories', undefined, { silent: true }); if (!msg?.success) throw new Error(msg?.msg || 'Failed to fetch geodata categories'); const validated = parseMsg(msg, GeodataCategoriesSchema, 'xray/getGeodataCategories'); - return validated.obj ?? EMPTY_CATEGORIES; + // parseMsg falls back to the original, unvalidated obj on a schema mismatch + // (see zodValidate.ts) rather than clearing it, so each field is guarded + // independently here -- same reasoning as useInboundOptions's Array.isArray + // check, just applied per-field since this response is an object of two + // arrays rather than one top-level array. + return { + domain: Array.isArray(validated.obj?.domain) ? validated.obj.domain : EMPTY_CATEGORIES.domain, + ip: Array.isArray(validated.obj?.ip) ? validated.obj.ip : EMPTY_CATEGORIES.ip, + }; } // Deliberately not staleTime: Infinity like useInboundOptions: geodata .dat @@ -20,9 +28,16 @@ async function fetchGeodataCategories(): Promise { // global default staleTime lets a long-open tab pick up newly downloaded // categories on refocus, at near-zero backend cost thanks to the // mtime/size cache in GetGeodataCategories. -export function useGeodataCategories() { +// +// enabled defaults to true but is meant to be passed as `open` from the rule +// editor modal: the underlying scan/parse is the expensive part of this +// feature (see GetGeodataCategories), so it should run when the editor is +// actually opened, not on every visit to the Routing tab that merely mounts +// this modal closed. +export function useGeodataCategories(enabled = true) { return useQuery({ queryKey: keys.xray.geodataCategories(), queryFn: fetchGeodataCategories, + enabled, }); } diff --git a/frontend/src/generated/examples.ts b/frontend/src/generated/examples.ts index fa4bb8e94..aecf37b45 100644 --- a/frontend/src/generated/examples.ts +++ b/frontend/src/generated/examples.ts @@ -315,6 +315,16 @@ export const EXAMPLES: Record = { "masterId": 0, "path": "" }, + "GeodataCategories": { + "domain": [ + "geosite:cn", + "geosite:youtube" + ], + "ip": [ + "geoip:cn", + "geoip:private" + ] + }, "HistoryOfSeeders": { "id": 0, "seederName": "" diff --git a/frontend/src/generated/schemas.ts b/frontend/src/generated/schemas.ts index 00da739f9..4ddd9b786 100644 --- a/frontend/src/generated/schemas.ts +++ b/frontend/src/generated/schemas.ts @@ -1352,6 +1352,36 @@ export const SCHEMAS: Record = { ], "type": "object" }, + "GeodataCategories": { + "description": "GeodataCategories lists every geosite/geoip category found in the .dat\nfiles currently present in the Xray bin folder, already formatted as\nready-to-use xray-core routing-rule values (see formatGeodataSuggestion).\nReturned by XraySettingService.GetGeodataCategories and served as\nGET /panel/api/xray/getGeodataCategories for the routing rule editor's\nDomain/IP autocomplete.", + "properties": { + "domain": { + "example": [ + "geosite:cn", + "geosite:youtube" + ], + "items": { + "type": "string" + }, + "type": "array" + }, + "ip": { + "example": [ + "geoip:cn", + "geoip:private" + ], + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "domain", + "ip" + ], + "type": "object" + }, "HistoryOfSeeders": { "description": "HistoryOfSeeders tracks which database seeders have been executed to prevent re-running.", "properties": { diff --git a/frontend/src/generated/types.ts b/frontend/src/generated/types.ts index e8a1dbe83..11dc4e3ed 100644 --- a/frontend/src/generated/types.ts +++ b/frontend/src/generated/types.ts @@ -331,6 +331,11 @@ export interface FallbackParentInfo { path?: string; } +export interface GeodataCategories { + domain: string[]; + ip: string[]; +} + export interface HistoryOfSeeders { id: number; seederName: string; diff --git a/frontend/src/generated/zod.ts b/frontend/src/generated/zod.ts index ac218b67a..e6e6952ce 100644 --- a/frontend/src/generated/zod.ts +++ b/frontend/src/generated/zod.ts @@ -357,6 +357,12 @@ export const FallbackParentInfoSchema = z.object({ }); export type FallbackParentInfo = z.infer; +export const GeodataCategoriesSchema = z.object({ + domain: z.array(z.string()), + ip: z.array(z.string()), +}); +export type GeodataCategories = z.infer; + export const HistoryOfSeedersSchema = z.object({ id: z.number().int(), seederName: z.string(), diff --git a/frontend/src/pages/api-docs/endpoints.ts b/frontend/src/pages/api-docs/endpoints.ts index eaa01d05e..dc9023842 100644 --- a/frontend/src/pages/api-docs/endpoints.ts +++ b/frontend/src/pages/api-docs/endpoints.ts @@ -1281,7 +1281,7 @@ export const sections: readonly Section[] = [ method: 'GET', path: '/panel/api/xray/getGeodataCategories', summary: 'Return every geosite/geoip category found in the .dat files currently present in the Xray bin folder (including custom files added via the Geodata auto-update feature), formatted as ready-to-use routing rule values, e.g. "geosite:youtube" or "ext:geosite_roscom.dat:some-code".', - response: '{\n "success": true,\n "obj": {\n "domain": ["geosite:cn", "geosite:youtube"],\n "ip": ["geoip:cn", "geoip:private"]\n }\n}', + responseSchema: 'GeodataCategories', }, { method: 'POST', diff --git a/frontend/src/pages/xray/routing/RuleFormModal.tsx b/frontend/src/pages/xray/routing/RuleFormModal.tsx index 95954ac70..d240ead4f 100644 --- a/frontend/src/pages/xray/routing/RuleFormModal.tsx +++ b/frontend/src/pages/xray/routing/RuleFormModal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { Button, Form, Input, Modal, Select, Space, Switch, Tooltip } from 'antd'; import { PlusOutlined, MinusOutlined, QuestionCircleOutlined } from '@ant-design/icons'; @@ -82,6 +82,51 @@ function filterBySubstring(input: string, option?: { value?: string }): boolean return typeof option?.value === 'string' && option.value.toLowerCase().includes(input.toLowerCase()); } +interface TagsAutocompleteProps { + id?: string; + value?: string[]; + onChange?: (value: string[]) => void; + onBlur?: () => void; + options: { value: string; label: string }[]; + placeholder?: string; +} + +// A plain Select mode="tags" only commits the text being typed into a tag on +// Enter or a tokenSeparator character -- clicking Save directly (a blur, not +// an Enter) silently drops it, with no `domain`/`ip` key at all ending up in +// the saved rule. This wraps it with a controlled searchValue that also gets +// committed as a tag on blur, so free-text entry behaves like the old Input +// it replaced. +function TagsAutocomplete({ id, value, onChange, onBlur, options, placeholder }: TagsAutocompleteProps) { + const [searchValue, setSearchValue] = useState(''); + + function commitSearchValue(next: string[]) { + const trimmed = searchValue.trim(); + setSearchValue(''); + if (!trimmed || next.includes(trimmed)) return next; + return [...next, trimmed]; + } + + return ( + + - + el.getAttribute('title') ?? el.textContent ?? '', + ); +} + +describe('RuleFormModal domain/ip tags autocomplete', () => { + it('renders a comma-separated existing value as tags and preserves it unchanged on save', () => { + const onConfirm = vi.fn(); + renderWithProviders( + , + ); + + expect(selectedTags('domain')).toEqual(['google.com', 'geosite:cn']); + + fireEvent.click(screen.getByRole('button', { name: 'Save Changes' })); + + expect(onConfirm).toHaveBeenCalledTimes(1); + expect(onConfirm.mock.calls[0][0]).toMatchObject({ domain: ['google.com', 'geosite:cn'] }); + }); + + it('adds a typed value to the tag list on Enter and includes it on save', () => { + const onConfirm = vi.fn(); + renderWithProviders( + , + ); + + const input = domainInput(); + fireEvent.change(input, { target: { value: 'example.com' } }); + fireEvent.keyDown(input, { key: 'Enter', code: 'Enter', keyCode: 13, which: 13 }); + + fireEvent.click(screen.getByRole('button', { name: 'Create' })); + expect(onConfirm.mock.calls[0][0]).toMatchObject({ domain: ['example.com'] }); + }); + + it('commits a typed value on blur even without pressing Enter or a comma', () => { + // Regression test: a plain Select mode="tags" only commits the search + // text on Enter/tokenSeparator. Clicking Save directly is a blur, not an + // Enter -- without TagsAutocomplete's onBlur commit, the typed value was + // silently dropped and the rule saved with no `domain` key at all. + const onConfirm = vi.fn(); + renderWithProviders( + , + ); + + const input = domainInput(); + fireEvent.change(input, { target: { value: 'blurred.com' } }); + // A real click on Save blurs the still-focused input first (native + // browser focus handling); jsdom's fireEvent.click doesn't replicate + // that side effect, so blur is fired explicitly to match what a real + // click does immediately before Save's own handler runs. + fireEvent.blur(input); + fireEvent.click(screen.getByRole('button', { name: 'Create' })); + + expect(onConfirm).toHaveBeenCalledTimes(1); + expect(onConfirm.mock.calls[0][0]).toMatchObject({ domain: ['blurred.com'] }); + }); + + it('omits domain entirely when left empty', () => { + const onConfirm = vi.fn(); + renderWithProviders( + , + ); + + fireEvent.click(screen.getByRole('button', { name: 'Create' })); + + expect(onConfirm).toHaveBeenCalledTimes(1); + expect(onConfirm.mock.calls[0][0]).not.toHaveProperty('domain'); + }); +}); diff --git a/internal/web/service/xray_setting_geodata.go b/internal/web/service/xray_setting_geodata.go index 9f69cf14f..e5648303c 100644 --- a/internal/web/service/xray_setting_geodata.go +++ b/internal/web/service/xray_setting_geodata.go @@ -1,20 +1,35 @@ package service import ( + "fmt" + "io" "os" "path/filepath" "slices" "sort" "strings" + "sync/atomic" "time" "github.com/mhsanaei/3x-ui/v3/internal/config" "github.com/mhsanaei/3x-ui/v3/internal/logger" "github.com/xtls/xray-core/common/geodata" - "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/encoding/protowire" ) +// maxGeodataFileSize bounds how much of one .dat file parseGeodataFile will +// read from disk. Real Loyalsoldier-published geoip.dat/geosite.dat files +// are a few MB to a few tens of MB; this leaves generous headroom while +// still refusing an unbounded/corrupt file instead of reading it whole. +const maxGeodataFileSize = 256 << 20 // 256 MiB + +// geodataParseCalls counts parseGeodataFile invocations. Not read by any +// non-test code; exists so tests can assert that GetGeodataCategories' +// cache actually skips re-parsing on an unchanged fingerprint, rather than +// only checking the (identical either way) returned value. +var geodataParseCalls atomic.Int64 + // GeodataCategories lists every geosite/geoip category found in the .dat // files currently present in the Xray bin folder, already formatted as // ready-to-use xray-core routing-rule values (see formatGeodataSuggestion). @@ -22,8 +37,8 @@ import ( // GET /panel/api/xray/getGeodataCategories for the routing rule editor's // Domain/IP autocomplete. type GeodataCategories struct { - Domain []string `json:"domain"` - IP []string `json:"ip"` + Domain []string `json:"domain" example:"[\"geosite:cn\",\"geosite:youtube\"]"` + IP []string `json:"ip" example:"[\"geoip:cn\",\"geoip:private\"]"` } // geodataFileKind distinguishes a geosite-shaped .dat file (parsed as a @@ -73,26 +88,36 @@ type geodataCategoryCache struct { // file actually changes -- e.g. because xray-core's own geodata auto-update // downloaded a new one. A file that fails to parse (e.g. an interrupted // download) is skipped with a logged warning; it never fails the request. +// +// The mutex is held for the full scan-and-maybe-parse instead of being +// released around buildGeodataCategories: the work is bounded and +// idempotent (a full-file parse, not an unbounded/blocking operation), so +// serializing it is simpler and cheaper than the alternative of N +// concurrent cache misses (e.g. several browser tabs open at once) each +// independently re-parsing every .dat file before any of them get to store +// a result. func (s *XraySettingService) GetGeodataCategories() GeodataCategories { dir := config.GetBinFolderPath() entries := scanGeodataFiles(dir) fingerprint := geodataFingerprintOf(entries) s.geodataMu.Lock() + defer s.geodataMu.Unlock() + if s.geodataCache != nil && slices.Equal(s.geodataCache.fingerprint, fingerprint) { - result := s.geodataCache.result - s.geodataMu.Unlock() - return result + return cloneGeodataCategories(s.geodataCache.result) } - s.geodataMu.Unlock() result := buildGeodataCategories(entries) - - s.geodataMu.Lock() s.geodataCache = &geodataCategoryCache{fingerprint: fingerprint, result: result} - s.geodataMu.Unlock() + return cloneGeodataCategories(result) +} - return result +// cloneGeodataCategories returns a copy whose slices don't alias the +// cache's own, so a caller mutating (e.g. sorting, appending to) its result +// can never corrupt what other goroutines read from the shared cache. +func cloneGeodataCategories(c GeodataCategories) GeodataCategories { + return GeodataCategories{Domain: slices.Clone(c.Domain), IP: slices.Clone(c.IP)} } // scanGeodataFiles lists dir for files matched by name: geosite*.dat parses @@ -195,47 +220,101 @@ func buildGeodataCategories(entries []geodataFileEntry) GeodataCategories { return result } -// parseGeodataFile fully unmarshals one geosite*/geoip*.dat file and returns -// every category Code it contains. xray-core's own loaders (loadSite/loadIP -// in common/geodata/geodat_loader.go) stream a single named category out of -// a file via an unexported, custom varint-prefixed scanner; enumerating -// *every* category instead needs a full-file proto.Unmarshal into the -// package's exported GeoSiteList/GeoIPList message types. +// parseGeodataFile returns every category Code found in one geosite*/geoip*.dat +// file. xray-core's own loaders (loadSite/loadIP in +// common/geodata/geodat_loader.go) stream a single named category out of a +// file via an unexported, custom varint-prefixed scanner; enumerating +// *every* category here instead walks the raw protobuf wire format directly +// via codesFromGeodataList rather than a full proto.Unmarshal, since the +// only field ever read is each entry's Code -- a full unmarshal would also +// materialize every Domain/CIDR message the file contains (the bulk of a +// real geoip.dat/geosite.dat's size) just to throw it away unused. func parseGeodataFile(entry geodataFileEntry) ([]string, error) { - data, err := os.ReadFile(entry.path) + geodataParseCalls.Add(1) + + f, err := os.Open(entry.path) if err != nil { return nil, err } + defer f.Close() + + data, err := io.ReadAll(io.LimitReader(f, maxGeodataFileSize+1)) + if err != nil { + return nil, err + } + if len(data) > maxGeodataFileSize { + return nil, fmt.Errorf("file exceeds %d byte limit", maxGeodataFileSize) + } switch entry.kind { - case geositeFile: - var list geodata.GeoSiteList - if err := proto.Unmarshal(data, &list); err != nil { - return nil, err - } - codes := make([]string, 0, len(list.GetEntry())) - for _, site := range list.GetEntry() { - if code := site.GetCode(); code != "" { - codes = append(codes, code) - } - } - return codes, nil - case geoipFile: - var list geodata.GeoIPList - if err := proto.Unmarshal(data, &list); err != nil { - return nil, err - } - codes := make([]string, 0, len(list.GetEntry())) - for _, ip := range list.GetEntry() { - if code := ip.GetCode(); code != "" { - codes = append(codes, code) - } - } - return codes, nil + case geositeFile, geoipFile: + return codesFromGeodataList(data) } return nil, nil } +// codesFromGeodataList extracts every entry's Code from a serialized +// GeoSiteList or GeoIPList without unmarshaling into either message type. +// Both list messages put their repeated "entry" on field 1, and both +// GeoSite and GeoIP put "code" on field 1 of that entry (see +// common/geodata/geodat.proto) -- so every other field, including the +// repeated Domain/CIDR payloads that make up nearly all of a real file's +// size, is skipped via protowire.ConsumeFieldValue without ever being +// decoded into an allocated message. +func codesFromGeodataList(data []byte) ([]string, error) { + var codes []string + for len(data) > 0 { + num, typ, n := protowire.ConsumeTag(data) + if n < 0 { + return nil, protowire.ParseError(n) + } + data = data[n:] + if num != 1 || typ != protowire.BytesType { + m := protowire.ConsumeFieldValue(num, typ, data) + if m < 0 { + return nil, protowire.ParseError(m) + } + data = data[m:] + continue + } + entry, m := protowire.ConsumeBytes(data) + if m < 0 { + return nil, protowire.ParseError(m) + } + data = data[m:] + if code, ok := geodataEntryCode(entry); ok && code != "" { + codes = append(codes, code) + } + } + return codes, nil +} + +// geodataEntryCode returns field 1 (GeoSite.code / GeoIP.code, both plain +// proto3 strings) of one serialized entry submessage. +func geodataEntryCode(data []byte) (string, bool) { + for len(data) > 0 { + num, typ, n := protowire.ConsumeTag(data) + if n < 0 { + return "", false + } + data = data[n:] + if num != 1 || typ != protowire.BytesType { + m := protowire.ConsumeFieldValue(num, typ, data) + if m < 0 { + return "", false + } + data = data[m:] + continue + } + value, m := protowire.ConsumeBytes(data) + if m < 0 { + return "", false + } + return string(value), true + } + return "", false +} + // formatGeodataSuggestion builds the exact rule value xray-core's rule // parser (common/geodata/rule_parser.go) accepts for one category code from // one file. The default file gets the short "geosite:"/"geoip:" form; every @@ -248,11 +327,11 @@ func formatGeodataSuggestion(entry geodataFileEntry, code string) string { lowerCode := strings.ToLower(code) switch entry.kind { case geositeFile: - if entry.name == geodata.DefaultGeoSiteDat { + if strings.EqualFold(entry.name, geodata.DefaultGeoSiteDat) { return "geosite:" + lowerCode } case geoipFile: - if entry.name == geodata.DefaultGeoIPDat { + if strings.EqualFold(entry.name, geodata.DefaultGeoIPDat) { return "geoip:" + lowerCode } } diff --git a/internal/web/service/xray_setting_geodata_test.go b/internal/web/service/xray_setting_geodata_test.go index 46931af4c..c12b8be19 100644 --- a/internal/web/service/xray_setting_geodata_test.go +++ b/internal/web/service/xray_setting_geodata_test.go @@ -119,6 +119,28 @@ func TestParseGeodataFile(t *testing.T) { } } +func TestParseGeodataFileRejectsOversizedFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "geosite.dat") + // Sparse-write a file one byte past the cap without actually holding + // maxGeodataFileSize+1 bytes of valid protobuf in memory for the test. + f, err := os.Create(path) + if err != nil { + t.Fatalf("create fixture: %v", err) + } + if err := f.Truncate(maxGeodataFileSize + 1); err != nil { + t.Fatalf("truncate fixture to %d bytes: %v", maxGeodataFileSize+1, err) + } + if err := f.Close(); err != nil { + t.Fatalf("close fixture: %v", err) + } + + _, err = parseGeodataFile(geodataFileEntry{name: "geosite.dat", path: path, kind: geositeFile}) + if err == nil { + t.Fatal("expected parseGeodataFile to reject a file over the size limit, got nil error") + } +} + func TestFormatGeodataSuggestion(t *testing.T) { tests := []struct { name string @@ -130,12 +152,20 @@ func TestFormatGeodataSuggestion(t *testing.T) { {name: "geoip.dat", kind: geoipFile, code: "PRIVATE", want: "geoip:private"}, {name: "geosite_roscom.dat", kind: geositeFile, code: "SOME-CODE", want: "ext:geosite_roscom.dat:some-code"}, {name: "geoip_rosip.dat", kind: geoipFile, code: "RU", want: "ext:geoip_rosip.dat:ru"}, + // Default filenames are matched case-insensitively, same as + // scanGeodataFiles -- a file that IS the default one on a + // case-insensitive filesystem (e.g. Windows) must still get the + // short geosite:/geoip: form, not ext:. + {name: "GEOSITE.DAT", kind: geositeFile, code: "CN", want: "geosite:cn"}, + {name: "GeoIP.dat", kind: geoipFile, code: "PRIVATE", want: "geoip:private"}, } for _, tt := range tests { - entry := geodataFileEntry{name: tt.name, kind: tt.kind} - if got := formatGeodataSuggestion(entry, tt.code); got != tt.want { - t.Errorf("formatGeodataSuggestion(%q, %q) = %q, want %q", tt.name, tt.code, got, tt.want) - } + t.Run(tt.name, func(t *testing.T) { + entry := geodataFileEntry{name: tt.name, kind: tt.kind} + if got := formatGeodataSuggestion(entry, tt.code); got != tt.want { + t.Errorf("formatGeodataSuggestion(%q, %q) = %q, want %q", tt.name, tt.code, got, tt.want) + } + }) } } @@ -144,29 +174,34 @@ func TestGeodataFingerprintOf(t *testing.T) { {name: "geoip.dat", size: 100, modTime: time.Unix(1, 0)}, {name: "geosite.dat", size: 200, modTime: time.Unix(2, 0)}, } - b := []geodataFileEntry{ // same content, different order - {name: "geosite.dat", size: 200, modTime: time.Unix(2, 0)}, - {name: "geoip.dat", size: 100, modTime: time.Unix(1, 0)}, - } - if !slices.Equal(geodataFingerprintOf(a), geodataFingerprintOf(b)) { - t.Fatal("fingerprints should be equal regardless of input order") - } - c := []geodataFileEntry{ - {name: "geoip.dat", size: 999, modTime: time.Unix(1, 0)}, // size changed - {name: "geosite.dat", size: 200, modTime: time.Unix(2, 0)}, - } - if slices.Equal(geodataFingerprintOf(a), geodataFingerprintOf(c)) { - t.Fatal("fingerprints should differ when a file's size changes") - } + t.Run("order independent", func(t *testing.T) { + b := []geodataFileEntry{ // same content, different order + {name: "geosite.dat", size: 200, modTime: time.Unix(2, 0)}, + {name: "geoip.dat", size: 100, modTime: time.Unix(1, 0)}, + } + if !slices.Equal(geodataFingerprintOf(a), geodataFingerprintOf(b)) { + t.Fatal("fingerprints should be equal regardless of input order") + } + }) + + t.Run("size change invalidates", func(t *testing.T) { + c := []geodataFileEntry{ + {name: "geoip.dat", size: 999, modTime: time.Unix(1, 0)}, // size changed + {name: "geosite.dat", size: 200, modTime: time.Unix(2, 0)}, + } + if slices.Equal(geodataFingerprintOf(a), geodataFingerprintOf(c)) { + t.Fatal("fingerprints should differ when a file's size changes") + } + }) } func TestGetGeodataCategories_SkipsMalformedFile(t *testing.T) { dir := t.TempDir() writeGeoSiteFixture(t, dir, "geosite.dat", "CN") // An unterminated varint (continuation bit set on every byte) is - // guaranteed to fail proto.Unmarshal, unlike an arbitrary text string - // which might accidentally parse as protobuf garbage. + // guaranteed to fail parsing, unlike an arbitrary text string which + // might accidentally parse as protobuf garbage. if err := os.WriteFile(filepath.Join(dir, "geosite_broken.dat"), []byte{0xFF, 0xFF, 0xFF}, 0o644); err != nil { t.Fatalf("write broken fixture: %v", err) } @@ -174,8 +209,13 @@ func TestGetGeodataCategories_SkipsMalformedFile(t *testing.T) { entries := scanGeodataFiles(dir) result := buildGeodataCategories(entries) - if !slices.Contains(result.Domain, "geosite:cn") { - t.Fatalf("expected the valid file's category to survive, got %v", result.Domain) + // Exact equality, not just Contains: a regression that made the broken + // file emit junk suggestions alongside the good one must fail this. + if !slices.Equal(result.Domain, []string{"geosite:cn"}) { + t.Fatalf("result.Domain = %v, want [geosite:cn]", result.Domain) + } + if len(result.IP) != 0 { + t.Fatalf("result.IP = %v, want empty", result.IP) } } @@ -193,20 +233,62 @@ func TestGetGeodataCategories_EndToEnd(t *testing.T) { svc := &XraySettingService{} result := svc.GetGeodataCategories() - if !slices.Contains(result.Domain, "geosite:cn") { - t.Errorf("Domain = %v, want to contain geosite:cn", result.Domain) + wantDomain := []string{"ext:geosite_roscom.dat:some-code", "geosite:cn"} + if !slices.Equal(result.Domain, wantDomain) { + t.Errorf("Domain = %v, want %v", result.Domain, wantDomain) } - if !slices.Contains(result.Domain, "ext:geosite_roscom.dat:some-code") { - t.Errorf("Domain = %v, want to contain ext:geosite_roscom.dat:some-code", result.Domain) - } - if !slices.Contains(result.IP, "geoip:private") { - t.Errorf("IP = %v, want to contain geoip:private", result.IP) + if !slices.Equal(result.IP, []string{"geoip:private"}) { + t.Errorf("IP = %v, want [geoip:private]", result.IP) } // Cache must reflect a file that appears after the first call. writeGeoIPFixture(t, dir, "geoip_rosip.dat", "RU") result = svc.GetGeodataCategories() - if !slices.Contains(result.IP, "ext:geoip_rosip.dat:ru") { - t.Errorf("IP after adding a new file = %v, want to contain ext:geoip_rosip.dat:ru", result.IP) + wantIP := []string{"ext:geoip_rosip.dat:ru", "geoip:private"} + if !slices.Equal(result.IP, wantIP) { + t.Errorf("IP after adding a new file = %v, want %v", result.IP, wantIP) + } +} + +func TestGetGeodataCategories_CacheHitSkipsReparse(t *testing.T) { + dir := t.TempDir() + t.Setenv("XUI_BIN_FOLDER", dir) + writeGeoSiteFixture(t, dir, "geosite.dat", "CN") + writeGeoIPFixture(t, dir, "geoip.dat", "PRIVATE") + + svc := &XraySettingService{} + + before := geodataParseCalls.Load() + first := svc.GetGeodataCategories() + afterFirst := geodataParseCalls.Load() + if afterFirst == before { + t.Fatal("expected the first call (a cache miss) to parse at least one file") + } + + second := svc.GetGeodataCategories() + afterSecond := geodataParseCalls.Load() + if afterSecond != afterFirst { + t.Fatalf("expected a second call with an unchanged fingerprint to skip re-parsing entirely, but the parse count went from %d to %d", afterFirst, afterSecond) + } + if !slices.Equal(first.Domain, second.Domain) || !slices.Equal(first.IP, second.IP) { + t.Fatalf("cached result differs from the original: first=%+v second=%+v", first, second) + } +} + +func TestGetGeodataCategories_ReturnsIndependentSlices(t *testing.T) { + // The cache must never hand out its own backing array: a caller that + // mutates its result (e.g. sorts or appends to it) must not corrupt what + // other callers read from the shared cache on their next call. + dir := t.TempDir() + t.Setenv("XUI_BIN_FOLDER", dir) + writeGeoSiteFixture(t, dir, "geosite.dat", "CN", "YOUTUBE") + + svc := &XraySettingService{} + first := svc.GetGeodataCategories() + first.Domain[0] = "tampered" + + second := svc.GetGeodataCategories() + if slices.Contains(second.Domain, "tampered") { + t.Fatalf("mutating one caller's result leaked into the cache: %v", second.Domain) } } diff --git a/tools/openapigen/main.go b/tools/openapigen/main.go index 84165ba4d..5983f9816 100644 --- a/tools/openapigen/main.go +++ b/tools/openapigen/main.go @@ -82,6 +82,7 @@ func run(root, outDir string) error { "NodeView", "ProbeResultUI", "RealityScanResult", + "GeodataCategories", ), }, {