diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7edf1eb0..7513a94d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -100,6 +100,24 @@ sqlc generate ## API changes +### Router configuration and optional admin endpoints + +Keep `router.New`'s signature stable. Pass startup settings by name in +`router.Options`; a new optional setting must not require unrelated callers and +tests to supply another positional argument. + +An optional admin feature owns its subrouter, dependencies and HTTP methods. +Register it once through `Options.AdminRoutes` in the startup wiring, using a +literal path such as `/accounts`. The parent admin router applies bearer +authentication to the entire subtree; the feature must not mount itself beside +that boundary. `/config` remains owned by the shared admin router. No global +registration or `init` side effects are needed. + +When concurrent PRs touch the same startup wiring, stack them in an explicit +merge order and link each feature-only comparison. Rebase the stack together +after prerequisite merges, regenerate Swagger rather than hand-merging it, and +compile the combined result even when GitHub reports it as mergeable. + Any new or modified REST endpoint must have swagger annotations and regenerated docs: diff --git a/cmd/beacon/main.go b/cmd/beacon/main.go index f2a9294c..b518ab1f 100644 --- a/cmd/beacon/main.go +++ b/cmd/beacon/main.go @@ -303,7 +303,11 @@ func main() { // Wrap after wiring cache invalidators and cleanup callbacks to the actual // CachedReader. Only response projections receive the geographic annotation. reader = api.WithLocalBorders(reader, localBorders) - r := router.New(h, reader, []*ingest.Worker{broker1, broker2}, resolved.MaxConnsPerIP, resolved.MaxConnectsPerMinute, cfg.CORS, cfg.Server, cfg.Auth, resolved.RateLimit) + r := router.New(h, reader, []*ingest.Worker{broker1, broker2}, router.Options{ + MaxConnsPerIP: resolved.MaxConnsPerIP, + MaxConnectsPerMinute: resolved.MaxConnectsPerMinute, + CORS: cfg.CORS, Server: cfg.Server, Auth: cfg.Auth, RateLimit: resolved.RateLimit, + }) srv := &http.Server{ Addr: addr, diff --git a/internal/api/handlers/admin.go b/internal/api/handlers/admin.go index b775a515..cf4f0d6a 100644 --- a/internal/api/handlers/admin.go +++ b/internal/api/handlers/admin.go @@ -17,10 +17,13 @@ import ( // AdminRouter mounts operator endpoints. Its caller must wrap the // entire subrouter with BearerAuth, including unknown paths and methods. -func AdminRouter(runtime *mw.RuntimeConfig) http.Handler { +func AdminRouter(runtime *mw.RuntimeConfig, routes map[string]http.Handler) http.Handler { r := chi.NewRouter() r.Get("/config", getAdminConfig(runtime)) r.Put("/config", updateAdminConfig(runtime)) + for path, handler := range routes { + r.Mount(path, handler) + } return r } diff --git a/internal/api/router/admin_config_test.go b/internal/api/router/admin_config_test.go index 43ee7cfb..9acd524c 100644 --- a/internal/api/router/admin_config_test.go +++ b/internal/api/router/admin_config_test.go @@ -32,7 +32,7 @@ func TestAdminConfig(t *testing.T) { want = map[string]any{"allowed_origins": []any{"https://example.test"}, "allowed_methods": []any{"GET"}, "allowed_headers": []any{"Authorization", "X-Operator"}, "allow_credentials": true, "max_age": float64(600)} } - handler := New(nil, nil, []*ingest.Worker{{}, {}}, 5, 1000, cfg, config.ServerConfig{}, config.AuthConfig{APIKey: key}, config.ResolvedRateLimitConfig{}) + handler := New(nil, nil, []*ingest.Worker{{}, {}}, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, CORS: cfg, Auth: config.AuthConfig{APIKey: key}}) if custom { cfg.AllowedOrigins[0] = "https://changed.test" cfg.AllowedMethods[0] = "DELETE" diff --git a/internal/api/router/admin_routes_test.go b/internal/api/router/admin_routes_test.go new file mode 100644 index 00000000..c017236c --- /dev/null +++ b/internal/api/router/admin_routes_test.go @@ -0,0 +1,54 @@ +// Copyright 2026 Beacon Contributors +// SPDX-License-Identifier: AGPL-3.0-or-later + +package router + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/MeshCore-Beacon/beacon-server/internal/config" +) + +func TestAdditionalAdminRoutesStayAuthenticated(t *testing.T) { + for _, key := range []string{"", "fixture-key"} { + calls := 0 + opts := Options{Auth: config.AuthConfig{APIKey: key}, AdminRoutes: map[string]http.Handler{ + "/feature": http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.WriteHeader(http.StatusNoContent) + }), + }} + handler := New(nil, nil, nil, opts) + // Registration happens at startup; later edits to the caller's map must + // not replace the registered handler or create a new unprotected route. + delete(opts.AdminRoutes, "/feature") + for _, path := range []string{"/api/v1/admin/feature", "/api/v1/admin/feature/nested"} { + for _, method := range []string{"GET", "POST", "DELETE"} { + for _, token := range []string{"", "wrong", "fixture-key"} { + request := httptest.NewRequest(method, path, nil) + if token != "" { + request.Header.Set("Authorization", "Bearer "+token) + } + response := httptest.NewRecorder() + before := calls + handler.ServeHTTP(response, request) + want := http.StatusServiceUnavailable + if key != "" { + want = http.StatusUnauthorized + if token == key { + want = http.StatusNoContent + } + } + if response.Code != want || response.Header().Get("Cache-Control") != "no-store" { + t.Fatalf("%s %s: status=%d want=%d", method, path, response.Code, want) + } + if (calls != before) != (want == http.StatusNoContent) { + t.Fatal("authentication boundary bypassed") + } + } + } + } + } +} diff --git a/internal/api/router/auth_test.go b/internal/api/router/auth_test.go index 66c9614e..d8796f79 100644 --- a/internal/api/router/auth_test.go +++ b/internal/api/router/auth_test.go @@ -13,7 +13,7 @@ import ( func TestAdminAuthBoundary(t *testing.T) { for _, key := range []string{"", "synthetic-test-key"} { - handler := New(nil, nil, nil, 5, 1000, config.CORSConfig{}, config.ServerConfig{}, config.AuthConfig{APIKey: key}, config.ResolvedRateLimitConfig{}) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, Auth: config.AuthConfig{APIKey: key}}) for _, path := range []string{"/api/v1/admin", "/api/v1/admin/", "/api/v1/admin/config", "/api/v1/admin//config"} { for _, method := range []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"} { for _, token := range []string{"", "wrong", "synthetic-test-key"} { diff --git a/internal/api/router/options.go b/internal/api/router/options.go new file mode 100644 index 00000000..ec9e1fbf --- /dev/null +++ b/internal/api/router/options.go @@ -0,0 +1,25 @@ +// Copyright 2026 Beacon Contributors +// SPDX-License-Identifier: AGPL-3.0-or-later + +package router + +import ( + "net/http" + + "github.com/MeshCore-Beacon/beacon-server/internal/config" +) + +// Options names the startup settings used by the router. Adding an optional +// route does not change New's signature or require unrelated callers to change. +type Options struct { + MaxConnsPerIP int + MaxConnectsPerMinute int + CORS config.CORSConfig + Server config.ServerConfig + Auth config.AuthConfig + RateLimit config.ResolvedRateLimitConfig + // AdminRoutes mounts operator-only subrouters at literal paths such as + // "/accounts". Each feature owns its dependencies, methods and handlers. + // The router applies authentication to every path and method in this group. + AdminRoutes map[string]http.Handler +} diff --git a/internal/api/router/rate_limit_test.go b/internal/api/router/rate_limit_test.go index 95ab42f2..b0e1cd86 100644 --- a/internal/api/router/rate_limit_test.go +++ b/internal/api/router/rate_limit_test.go @@ -22,7 +22,7 @@ import ( ) func TestDefaultAPIRateLimit(t *testing.T) { - handler := New(nil, nil, nil, 5, 1000, config.CORSConfig{}, config.ServerConfig{}, config.AuthConfig{}, config.Resolve(&config.Config{}).RateLimit) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, RateLimit: config.Resolve(&config.Config{}).RateLimit}) for i := 0; i <= 300; i++ { request := httptest.NewRequest(http.MethodGet, "/api/v1/brokers", nil) request.RemoteAddr = "198.51.100.1:1234" @@ -56,7 +56,7 @@ func TestAPIRateLimitContractAndLogging(t *testing.T) { slog.SetDefault(slog.New(slog.NewJSONHandler(&logs, nil))) t.Cleanup(func() { slog.SetDefault(previous); log.SetOutput(writer); log.SetFlags(flags) }) proxy := config.ServerConfig{TrustedProxies: []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")}} - handler := New(nil, nil, nil, 5, 1000, config.CORSConfig{}, proxy, config.AuthConfig{}, config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 2, Burst: 10}) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, Server: proxy, RateLimit: config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 2, Burst: 10}}) for _, path := range []string{"/api/v1/brokers", "/api/v1/packets?limit=0"} { response := rateRequest(handler, path, "127.0.0.1:1234", "198.51.100.25") if response.Code != http.StatusOK && response.Code != http.StatusBadRequest { @@ -111,7 +111,7 @@ func TestAPIRateLimitClientIdentity(t *testing.T) { if tc.trusted { proxy.TrustedProxies = []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")} } - handler := New(nil, nil, nil, 5, 1000, config.CORSConfig{}, proxy, config.AuthConfig{}, config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 1, Burst: 1}) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, Server: proxy, RateLimit: config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 1, Burst: 1}}) if response := rateRequest(handler, "/api/v1/brokers", tc.firstPeer, tc.firstHeader); response.Code != http.StatusOK { t.Fatalf("first client: %d", response.Code) } @@ -128,7 +128,7 @@ func TestAPIRateLimitClientIdentity(t *testing.T) { func TestAPIRateLimitWindowsAndExclusions(t *testing.T) { synctest.Test(t, func(t *testing.T) { - handler := New(nil, nil, nil, 5, 1000, config.CORSConfig{}, config.ServerConfig{}, config.AuthConfig{}, config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 4, Burst: 2}) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, RateLimit: config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 4, Burst: 2}}) request := func() *httptest.ResponseRecorder { return rateRequest(handler, "/api/v1/brokers", "198.51.100.1:1", "") } @@ -164,7 +164,7 @@ func TestAPIRateLimitWindowsAndExclusions(t *testing.T) { t.Fatal("minute budget did not recover") } }) - handler := New(nil, nil, nil, 5, 1000, config.CORSConfig{}, config.ServerConfig{}, config.AuthConfig{}, config.ResolvedRateLimitConfig{Enabled: false, RequestsPerMinute: 1, Burst: 1}) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, RateLimit: config.ResolvedRateLimitConfig{Enabled: false, RequestsPerMinute: 1, Burst: 1}}) for range 5 { if response := rateRequest(handler, "/api/v1/brokers", "198.51.100.1:1", ""); response.Code != http.StatusOK || response.Header().Get("Retry-After") != "" { t.Fatal("disabled limiter still applied") @@ -173,7 +173,7 @@ func TestAPIRateLimitWindowsAndExclusions(t *testing.T) { } func TestAPIRateLimitConcurrentRequests(t *testing.T) { - handler := New(nil, nil, nil, 5, 1000, config.CORSConfig{}, config.ServerConfig{}, config.AuthConfig{}, config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 5, Burst: 5}) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, RateLimit: config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 5, Burst: 5}}) var accepted, rejected atomic.Int32 var group sync.WaitGroup for range 20 { diff --git a/internal/api/router/router.go b/internal/api/router/router.go index d58e8b71..447d407e 100644 --- a/internal/api/router/router.go +++ b/internal/api/router/router.go @@ -15,7 +15,6 @@ import ( "github.com/MeshCore-Beacon/beacon-server/internal/api" "github.com/MeshCore-Beacon/beacon-server/internal/api/handlers" mw "github.com/MeshCore-Beacon/beacon-server/internal/api/middleware" - "github.com/MeshCore-Beacon/beacon-server/internal/config" "github.com/MeshCore-Beacon/beacon-server/internal/hub" "github.com/MeshCore-Beacon/beacon-server/internal/ingest" "github.com/MeshCore-Beacon/beacon-server/internal/ws" @@ -38,35 +37,35 @@ import ( // /stats → stats subrouter // // Admin endpoints require a configured bearer key. -func New(h *hub.Hub, reader api.Reader, workers []*ingest.Worker, maxConnsPerIP, maxConnectsPerMinute int, corsCfg config.CORSConfig, serverCfg config.ServerConfig, authCfg config.AuthConfig, rateLimitCfg config.ResolvedRateLimitConfig) http.Handler { +func New(h *hub.Hub, reader api.Reader, workers []*ingest.Worker, opts Options) http.Handler { r := chi.NewRouter() // ── CORS ───────────────────────────────────────────────────────────────── - allowedOrigins := corsCfg.AllowedOrigins + allowedOrigins := opts.CORS.AllowedOrigins if len(allowedOrigins) == 0 { allowedOrigins = []string{"*"} } - allowedMethods := corsCfg.AllowedMethods + allowedMethods := opts.CORS.AllowedMethods if len(allowedMethods) == 0 { allowedMethods = []string{"GET", "HEAD", "OPTIONS"} } - allowedHeaders := corsCfg.AllowedHeaders + allowedHeaders := opts.CORS.AllowedHeaders if len(allowedHeaders) == 0 { allowedHeaders = []string{"Accept", "Authorization", "Content-Type"} } - maxAge := corsCfg.MaxAge + maxAge := opts.CORS.MaxAge if maxAge == 0 { maxAge = 300 } // Capture only values used by this router. Do not retain Config, credentials // or caller-owned slices in the admin response. adminConfig := api.AdminConfig{ - Auth: api.AdminAuthConfig{Configured: authCfg.APIKey != ""}, + Auth: api.AdminAuthConfig{Configured: opts.Auth.APIKey != ""}, CORS: api.AdminCORSConfig{ AllowedOrigins: append([]string{}, allowedOrigins...), AllowedMethods: append([]string{}, allowedMethods...), AllowedHeaders: append([]string{}, allowedHeaders...), - AllowCredentials: corsCfg.AllowCredentials, + AllowCredentials: opts.CORS.AllowCredentials, MaxAge: maxAge, }, Ingest: api.AdminIngestConfig{BrokerCount: len(workers)}, @@ -76,7 +75,7 @@ func New(h *hub.Hub, reader api.Reader, workers []*ingest.Worker, maxConnsPerIP, // ── Global middleware ──────────────────────────────────────────────────── r.Use(middleware.RequestID) - r.Use(mw.TrustedProxyIP(serverCfg.TrustedProxies)) + r.Use(mw.TrustedProxyIP(opts.Server.TrustedProxies)) r.Use(mw.RequestLogger) r.Use(middleware.Recoverer) r.Use(middleware.CleanPath) @@ -92,11 +91,11 @@ func New(h *hub.Hub, reader api.Reader, workers []*ingest.Worker, maxConnsPerIP, )) // ── WebSocket ──────────────────────────────────────────────────────────── - r.Get("/ws", ws.Handler(h, reader, maxConnsPerIP, maxConnectsPerMinute)) + r.Get("/ws", ws.Handler(h, reader, opts.MaxConnsPerIP, opts.MaxConnectsPerMinute)) // ── Public REST API (v1) ───────────────────────────────────────────────── r.Route("/api/v1", func(r chi.Router) { - r.Use(mw.RateLimit(rateLimitCfg)) + r.Use(mw.RateLimit(opts.RateLimit)) // Public group — no authentication required. r.Group(func(r chi.Router) { r.Mount("/packets", handlers.PacketsRouter(reader)) @@ -114,7 +113,7 @@ func New(h *hub.Hub, reader api.Reader, workers []*ingest.Worker, maxConnsPerIP, }) // Protect the entire subtree, including its root and unknown paths. - r.Mount("/admin", mw.BearerAuth(authCfg.APIKey, handlers.AdminRouter(runtimeConfig))) + r.Mount("/admin", mw.BearerAuth(opts.Auth.APIKey, handlers.AdminRouter(runtimeConfig, opts.AdminRoutes))) }) return r diff --git a/internal/api/router/router_test.go b/internal/api/router/router_test.go index 88927e29..bbb434dd 100644 --- a/internal/api/router/router_test.go +++ b/internal/api/router/router_test.go @@ -36,7 +36,7 @@ func TestWebSocketLimitUsesTrustedClientIP(t *testing.T) { func checkWebSocketLimit(t *testing.T, cfg config.ServerConfig, secondIP string, wantShed bool) { t.Helper() - server := httptest.NewServer(New(hub.New(), nil, nil, 1, 10, config.CORSConfig{}, cfg, config.AuthConfig{}, config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 1, Burst: 1})) + server := httptest.NewServer(New(hub.New(), nil, nil, Options{MaxConnsPerIP: 1, MaxConnectsPerMinute: 10, Server: cfg, RateLimit: config.ResolvedRateLimitConfig{Enabled: true, RequestsPerMinute: 1, Burst: 1}})) defer server.Close() // Exhaust this client's REST budget before checking its independent WS cap. for _, want := range []int{http.StatusOK, http.StatusTooManyRequests} { diff --git a/internal/api/router/runtime_config_test.go b/internal/api/router/runtime_config_test.go index 17dc67b9..6ac6073e 100644 --- a/internal/api/router/runtime_config_test.go +++ b/internal/api/router/runtime_config_test.go @@ -16,7 +16,7 @@ import ( func TestRuntimeConfigUpdate(t *testing.T) { newRouter := func() http.Handler { - return New(nil, nil, nil, 5, 1000, config.CORSConfig{AllowedOrigins: []string{"https://before.test"}}, config.ServerConfig{}, config.AuthConfig{APIKey: "test-key"}, config.ResolvedRateLimitConfig{}) + return New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1000, CORS: config.CORSConfig{AllowedOrigins: []string{"https://before.test"}}, Auth: config.AuthConfig{APIKey: "test-key"}}) } handler := newRouter() request := func(method, path, body, token, media, origin string) *httptest.ResponseRecorder { diff --git a/internal/api/router/ws_attempt_test.go b/internal/api/router/ws_attempt_test.go index b0f73982..853ecde6 100644 --- a/internal/api/router/ws_attempt_test.go +++ b/internal/api/router/ws_attempt_test.go @@ -29,7 +29,7 @@ func TestWebSocketAttemptClientIdentity(t *testing.T) { if tc.trusted { cfg.TrustedProxies = []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")} } - handler := New(nil, nil, nil, 5, 1, config.CORSConfig{}, cfg, config.AuthConfig{}, config.ResolvedRateLimitConfig{}) + handler := New(nil, nil, nil, Options{MaxConnsPerIP: 5, MaxConnectsPerMinute: 1, Server: cfg}) attempt := func(peer, header string) int { request := httptest.NewRequest(http.MethodGet, "/ws", nil) request.RemoteAddr = peer