diff --git a/.auto/autoresearch-lessons.md b/.auto/autoresearch-lessons.md index cbef9519..160d0090 100644 --- a/.auto/autoresearch-lessons.md +++ b/.auto/autoresearch-lessons.md @@ -119,7 +119,79 @@ and `backend/core` concurrently. the pinned yardstick. **Metric delta**: -25 in one iteration. +## Lesson 9 — iterations 23-24 +**Pattern**: Cross-check every service a plugin's `Apply` reads out of the +container against what that plugin's `Inject()` declares. `Inject()` is the only +thing `App.reconcileLocked` gates on, so anything consumed as a *value* at Apply +time but left undeclared is resolved from a container that may not hold it yet. +**Why it worked**: It found the run's worst defect, invisible to every +mechanical gate: `user` declared only `DBService` while capturing +`contracts.AuthService` to build its route guard, and `cmd/app.go` lists `user` +before `auth`. Because user's dep set is a strict subset of auth's and it sits +earlier in the slice, user *always* mounts first — deterministically, not a +race — so `loginMW` fell back to a `c.Next()` closure and +`/api/v1/user/{change-password,profile,access-tokens}` mounted unguarded. The +same lookups in `admin` read a package global that its own `OnDispose` nils, so +in-flight requests fail open during dispose. +**Conditions**: Any Cordis plugin whose Apply assigns a contract result to a +variable used later (middleware, handler closures). Services bound through +`core.When` late binding are exempt — that is the correct pattern for genuinely +late deps, so do not blanket-declare everything. +**Anti-pattern**: Assuming a checked `x, ok :=` assertion is safe. All three +plugins used the checked form and all three failed *open* — checked syntax, +unchecked semantics. +**Metric delta**: 0 across both iterations (kept under the proven-fix gate), +but this is the run's highest-severity finding. `RouterRegistry` records each +route's `Handlers`/`Middlewares`, which makes "is this route actually guarded?" +directly assertable from the route table — the cheapest available oracle for +security properties here. + +## Lesson 10 — iteration 23 review +**Pattern**: When the remaining metric is dominated by a positional or +taste-based analyzer, say so and refuse to spend iterations on it. +**Why it worked**: `funcorder` was 21 of 54 findings (39%) — pure function +*ordering within a file*. Reordering private helpers to the bottom of a file +moves the number and changes nothing a reader or the machine cares about, which +is Lesson 1's "looks productive while shuffling strings" with a different label. +Skipping it kept the loop honest. Triage also cleared 12 of 13 +`forcetypeassert` (guarded by construction) and 2 of 3 `unparam` (deliberate +constructor symmetry behind one factory switch). +**Conditions**: Whenever one linter dominates a shrinking total, break the count +down per linter *before* picking a hypothesis. +**Anti-pattern**: Treating a large single-linter share as an easy win. Real +headroom at this point is ~10 findings, so a plateau in `debt` no longer means a +stalled loop. +**Metric delta**: 0 spent, ~21 findings deliberately left in place. + +## Lesson 11 — iterations 24-25 +**Pattern**: Run the Guard after every single commit, and confirm which commit a +proof script is actually reverting against. +**Why it worked**: Four `staticcheck ST1023` findings from iteration 24 shipped +straight through `go build ./...` and a green 47-package `go test ./...` — +neither runs the project linter, so only `checks.sh` section 3 catches them. +Separately, `prove_fix.sh` reverts to `HEAD^`; appending the iteration-23 log +commit shifted `HEAD^` to the *fixed* state and reported "PROVE FAILED: tests +still pass without the fix" on a genuinely load-bearing fix. Re-checking against +the explicit pre-fix commit (`git checkout -- `) showed the real +answer. A false negative here is worse than no proof: it reads like the fix was +cosmetic. +**Conditions**: Always. Also note zsh does not word-split unquoted variables, so +`git checkout $FILES` passes one bogus pathspec and silently reverts nothing — +the command still exits 0. +**Anti-pattern**: Batch-verifying at ship time. And any shell loop built on the +bash word-splitting habit in this environment. +**Metric delta**: -0, 1 wrong verdict corrected. + ## Standing notes +- Upstream moves fast in this repo: `origin/main` gained 11 commits mid-run + (Cordis config extension point — `ctx.Config().Bind`, `DeclareConfig()`, + `core.ConfigGatedPlugin`), which raised measured `debt` 54 -> 64 and + `nolint_dirs` 72 -> 73 on its own. Rebase early and re-run the Guard after; + a clean rebase does not mean a green one. +- `cmd.TestNewWaveletAppWithRedisEnabled` needs a live Redis on + `127.0.0.1:6379` and fails without one. Pre-existing on `origin/main`, so + `tests_passed` 46 vs 47 is environmental, not a regression. Confirm against a + scratch `git worktree` of `origin/main` before blaming a change for it. - Repo facts: backend module rooted at `backend/`, gofumpt orders a single import group as `Wavelet/...` before stdlib (uppercase sorts first); new Go files need the Apache license header or `scripts/update_go_license.sh --check` @@ -128,5 +200,6 @@ the pinned yardstick. when only bodies change). - Dead suppressions are tracked by the `nolint_dirs` counter; removing one that is still needed re-raises the original finding, so the metric self-corrects. - 24 were removed in iteration 22; 72 remain, each still doing work. + 24 were removed in iteration 22; 72 remain, each still doing work (73 after + the upstream rebase). diff --git a/.auto/results.tsv b/.auto/results.tsv index 73bced6c..375ee755 100644 --- a/.auto/results.tsv +++ b/.auto/results.tsv @@ -21,3 +21,6 @@ iteration commit metric delta status guard description 20 efa7555 79 0.0 keep pass BUGFIX telegram: LongPoller.Timeout was 10 nanoseconds -> getUpdates timeout=0 -> busy polling; now 10s (proven by reverting the constant) 21 1023fa3 79 0.0 keep pass DISK LEAK: telegram inbound media scratch dirs were never removed (no consumer reads them); cleanup on handler exit. No test possible (needs live download) 22 ad83841 54 -25.0 keep pass dead lint suppressions removed (24); 2 were load-bearing -> restored+narrowed with reasons after guard veto exposed verified contextcheck FPs +23 de938de 54 0.0 keep pass SECURITY/BUGFIX fail-open auth: user+message_gateway consumed contracts.AuthService in Apply but declared only DBService, so reconcile mounted user before auth and loginMW degraded to a pass-through (user change-password/profile/access-tokens unguarded in production, deterministically); declared the dep + added reconcile-level ordering test (PROVED: assertion fails on revert) +24 62b48e9 54 0.0 keep pass SECURITY: all three auth-middleware fallbacks were c.Next() (fail-open). Reachable at runtime in admin: OnDispose->ResetServices() nils the global the per-request guard reads, so in-flight requests pass as authenticated. Added ginutil.AuthUnavailable() + table test driving each registered guard (PROVED: abort assertion fails on revert to 577d795) +25 f58f5a4 54 0.0 keep pass staticcheck ST1023 x4 from iter 24 (redundant gin.HandlerFunc on typed-RHS decls) - caught by GUARD only, go build/go test both stayed green; lesson: run checks.sh after EVERY commit, not just before ship diff --git a/backend/pkg/ginutil/auth.go b/backend/pkg/ginutil/auth.go new file mode 100644 index 00000000..99a0638d --- /dev/null +++ b/backend/pkg/ginutil/auth.go @@ -0,0 +1,24 @@ +// Copyright 2026 Arctel.net +// SPDX-License-Identifier: Apache-2.0 + +package ginutil + +import ( + "Wavelet/pkg/response" + + "github.com/gin-gonic/gin" +) + +// errAuthUnavailable is reported when a route's authentication guard cannot be +// resolved, so the request is rejected instead of reaching the handler. +const errAuthUnavailable = "authServiceUnavailable" + +// AuthUnavailable returns a middleware that denies the request. Plugins use it +// as the fallback when contracts.AuthService cannot be resolved or its middleware +// has an unexpected shape: the alternative is a pass-through closure that serves +// the request as if it were authenticated. +func AuthUnavailable() gin.HandlerFunc { + return func(c *gin.Context) { + response.AbortUnauthorized(c, errAuthUnavailable) + } +} diff --git a/backend/plugins/domain/admin/plugin.go b/backend/plugins/domain/admin/plugin.go index 1772823c..af9b122d 100644 --- a/backend/plugins/domain/admin/plugin.go +++ b/backend/plugins/domain/admin/plugin.go @@ -8,6 +8,7 @@ import ( "Wavelet/core" "Wavelet/core/contracts" "Wavelet/core/extpoints" + "Wavelet/pkg/ginutil" "Wavelet/plugins/domain/admin/handler" "Wavelet/plugins/domain/admin/model" "Wavelet/plugins/domain/admin/service" @@ -142,6 +143,7 @@ func (p *Plugin) Apply(ctx *core.Context) error { }) // 0a. Dynamic Auth Middlewares + denyAuth := ginutil.AuthUnavailable() var loginMW gin.HandlerFunc = func(c *gin.Context) { if authSvc := service.GetAuthService(c.Request.Context()); authSvc != nil { if mw, ok := authSvc.RequireAuthMiddleware().(gin.HandlerFunc); ok { @@ -149,7 +151,7 @@ func (p *Plugin) Apply(ctx *core.Context) error { return } } - c.Next() + denyAuth(c) } var adminMW gin.HandlerFunc = func(c *gin.Context) { if authSvc := service.GetAuthService(c.Request.Context()); authSvc != nil { @@ -158,7 +160,7 @@ func (p *Plugin) Apply(ctx *core.Context) error { return } } - c.Next() + denyAuth(c) } // 0b. Register migrations diff --git a/backend/plugins/domain/auth_dependency_ordering_test.go b/backend/plugins/domain/auth_dependency_ordering_test.go new file mode 100644 index 00000000..5e4df016 --- /dev/null +++ b/backend/plugins/domain/auth_dependency_ordering_test.go @@ -0,0 +1,219 @@ +// Copyright 2026 Arctel.net +// SPDX-License-Identifier: Apache-2.0 + +package domain_test + +import ( + "context" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "Wavelet/core" + "Wavelet/core/contracts" + "Wavelet/core/extpoints" + "Wavelet/plugins/domain/admin" + "Wavelet/plugins/domain/message_gateway" + "Wavelet/plugins/domain/user" +) + +// The kernel only gates a plugin's Apply on the services it DECLARES in Inject, +// so a plugin that consumes contracts.AuthService without declaring it can be +// mounted before auth exists. Those plugins fall back to a pass-through +// middleware, which silently un-guards their routes. + +func sentinelLogin(c *gin.Context) { c.Next() } +func sentinelAdmin(c *gin.Context) { c.Next() } +func sentinelNoToken(c *gin.Context) { c.Next() } + +type stubAuthService struct{ contracts.AuthService } + +func (stubAuthService) RequireAuthMiddleware() any { return gin.HandlerFunc(sentinelLogin) } +func (stubAuthService) RequireAdminMiddleware() any { return gin.HandlerFunc(sentinelAdmin) } +func (stubAuthService) DisallowTokenAuthMiddleware() any { return gin.HandlerFunc(sentinelNoToken) } + +type stubDBService struct{ contracts.DBService } + +// providerPlugin publishes a contract into the container at Apply time, the way +// the real infra and domain plugins do. +type providerPlugin struct { + name string + provide func(*core.Context) error +} + +func (p providerPlugin) Name() string { return p.name } + +func (p providerPlugin) Apply(ctx *core.Context) error { return p.provide(ctx) } + +func dbProvider() core.Plugin { + return providerPlugin{name: "stub-database", provide: func(ctx *core.Context) error { + core.Provide[contracts.DBService](ctx, stubDBService{}) + return nil + }} +} + +func authProvider() core.Plugin { + return providerPlugin{name: "stub-auth", provide: func(ctx *core.Context) error { + core.Provide[contracts.AuthService](ctx, stubAuthService{}) + return nil + }} +} + +func findRoute(routes []extpoints.RouteDefinition, method, path string) (extpoints.RouteDefinition, bool) { + for _, rd := range routes { + if rd.Method == method && rd.Path == path { + return rd, true + } + } + return extpoints.RouteDefinition{}, false +} + +// codePointer resolves the function code pointer of a registered handler so +// identity can be compared without depending on gin internals. +func codePointer(handler any) uintptr { + switch fn := handler.(type) { + case gin.HandlerFunc: + return reflect.ValueOf(fn).Pointer() + case func(*gin.Context): + return reflect.ValueOf(fn).Pointer() + default: + return 0 + } +} + +func assertsMiddleware(t *testing.T, routes []extpoints.RouteDefinition, method, path string, want func(*gin.Context)) { + t.Helper() + + rd, ok := findRoute(routes, method, path) + if !ok { + t.Fatalf("route %s %s was never registered", method, path) + } + wantPtr := codePointer(gin.HandlerFunc(want)) + for _, h := range rd.Handlers { + if codePointer(h) == wantPtr { + return + } + } + for _, m := range rd.Middlewares { + if codePointer(m) == wantPtr { + return + } + } + t.Errorf("route %s %s is not guarded by the auth middleware: handlers=%d middlewares=%d", + method, path, len(rd.Handlers), len(rd.Middlewares)) +} + +// TestRoutesMountedBeforeAuthServiceAreGuarded 回归：cmd/app.go 把 user/message_gateway +// 注册在 auth 之前，若插件未在 Inject 中声明 contracts.AuthService，reconcile 会先 +// Apply 它们，导致鉴权中间件退化为透传闭包，路由完全不受保护。 +func TestRoutesMountedBeforeAuthServiceAreGuarded(t *testing.T) { + gin.SetMode(gin.TestMode) + ctx := core.NewContext(context.Background()) + + // Registration order mirrors cmd/app.go: the auth provider comes last. + app := core.NewApp( + core.WithContext(ctx), + core.WithPlugins( + dbProvider(), + user.New(), + message_gateway.New(), + authProvider(), + ), + ) + require.NoError(t, app.ApplyPlugins()) + + routes := ctx.Router().Routes() + + assertsMiddleware(t, routes, "POST", "/api/v1/user/change-password", sentinelLogin) + assertsMiddleware(t, routes, "PUT", "/api/v1/user/profile", sentinelLogin) + assertsMiddleware(t, routes, "GET", "/api/v1/user/access-tokens", sentinelLogin) + assertsMiddleware(t, routes, "GET", "/api/v1/user/access-tokens", sentinelNoToken) + assertsMiddleware(t, routes, "GET", "/api/v1/message-gateway/channels", sentinelLogin) + assertsMiddleware(t, routes, "GET", "/api/v1/admin/message-gateway/channels", sentinelAdmin) +} + +// TestAuthConsumersDeclareAuthDependency 是同一缺陷的架构面：声明依赖是内核排序的唯一 +// 依据，漏声明会让正确性取决于注册表顺序。 +func TestAuthConsumersDeclareAuthDependency(t *testing.T) { + want := reflect.TypeFor[contracts.AuthService]() + for _, tc := range []struct { + name string + deps []reflect.Type + }{ + {"user", user.New().Inject()}, + {"message_gateway", message_gateway.New().Inject()}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Contains(t, tc.deps, want, + "%s resolves contracts.AuthService in Apply, so it must declare it in Inject", tc.name) + }) + } +} + +// firstGuard returns the outermost middleware registered for the first route +// matching method and path prefix — the auth guard the plugin resolved at Apply. +func firstGuard(t *testing.T, routes []extpoints.RouteDefinition, method, prefix string) gin.HandlerFunc { + t.Helper() + + for _, rd := range routes { + if rd.Method != method || !strings.HasPrefix(rd.Path, prefix) { + continue + } + for _, candidate := range rd.Middlewares { + if mw, ok := candidate.(gin.HandlerFunc); ok { + return mw + } + } + for _, candidate := range rd.Handlers { + if mw, ok := candidate.(gin.HandlerFunc); ok { + return mw + } + } + t.Fatalf("route %s %s has no inspectable guard", method, rd.Path) + } + t.Fatalf("no route registered for %s %s*", method, prefix) + return nil +} + +// TestAuthGuardFailsClosed 回归：鉴权服务无法解析时兜底必须是拒绝。旧实现兜底为 +// c.Next()，所以任何装配缺失——例如 admin 的 OnDispose 调用 service.ResetServices +// 把全局 authService 置 nil——都会让路由以“已登录”的姿态直达业务处理函数。 +func TestAuthGuardFailsClosed(t *testing.T) { + gin.SetMode(gin.TestMode) + + tests := []struct { + name string + apply func(*core.Context) error + method string + prefix string + }{ + {"user", user.New().Apply, http.MethodPost, "/api/v1/user/change-password"}, + {"message_gateway", message_gateway.New().Apply, http.MethodGet, "/api/v1/message-gateway"}, + {"admin", admin.New().Apply, http.MethodGet, "/api/v1/admin"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := core.NewContext(context.Background()) + require.NoError(t, tc.apply(ctx)) + + guard := firstGuard(t, ctx.Router().Routes(), tc.method, tc.prefix) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(tc.method, tc.prefix, nil) + guard(c) + + assert.True(t, c.IsAborted(), + "%s guard must reject the request when contracts.AuthService is unavailable", tc.name) + assert.NotEmpty(t, c.Errors, + "%s guard must record why the request was rejected", tc.name) + }) + } +} diff --git a/backend/plugins/domain/message_gateway/plugin.go b/backend/plugins/domain/message_gateway/plugin.go index af8edef0..fb8e14e0 100644 --- a/backend/plugins/domain/message_gateway/plugin.go +++ b/backend/plugins/domain/message_gateway/plugin.go @@ -8,6 +8,7 @@ import ( "Wavelet/core" "Wavelet/core/contracts" "Wavelet/core/extpoints" + "Wavelet/pkg/ginutil" "Wavelet/pkg/util" "Wavelet/plugins/domain/message_gateway/handler" "Wavelet/plugins/domain/message_gateway/model" @@ -59,6 +60,10 @@ func (p *Plugin) Name() string { func (p *Plugin) Inject() []reflect.Type { return []reflect.Type{ reflect.TypeFor[contracts.DBService](), + // AuthService is captured as a middleware value in Apply, so it cannot + // be late-bound with core.When like the other services below; the + // kernel must mount auth first or the routes get a pass-through guard. + reflect.TypeFor[contracts.AuthService](), } } @@ -130,8 +135,9 @@ func (p *Plugin) Apply(ctx *core.Context) error { }) // 0. Resolve auth service for middleware (via IoC, not direct import) - var loginMW gin.HandlerFunc = func(c *gin.Context) { c.Next() } - var adminMW gin.HandlerFunc = func(c *gin.Context) { c.Next() } + denyAuth := ginutil.AuthUnavailable() + loginMW := denyAuth + adminMW := denyAuth if authSvc, err := core.Inject[contracts.AuthService](ctx); err == nil && authSvc != nil { if mw, ok := authSvc.RequireAuthMiddleware().(gin.HandlerFunc); ok { loginMW = mw diff --git a/backend/plugins/domain/user/plugin.go b/backend/plugins/domain/user/plugin.go index ffb5d2ea..7ba1bcd2 100644 --- a/backend/plugins/domain/user/plugin.go +++ b/backend/plugins/domain/user/plugin.go @@ -8,6 +8,7 @@ import ( "Wavelet/core" "Wavelet/core/contracts" "Wavelet/core/extpoints" + "Wavelet/pkg/ginutil" "context" "embed" "reflect" @@ -56,6 +57,10 @@ func (p *Plugin) Name() string { func (p *Plugin) Inject() []reflect.Type { return []reflect.Type{ reflect.TypeFor[contracts.DBService](), + // Apply resolves AuthService to build the route auth middleware. The + // kernel only gates Apply on declared deps, so leaving this out lets + // user mount before auth and fall back to a pass-through middleware. + reflect.TypeFor[contracts.AuthService](), } } @@ -85,8 +90,9 @@ func (p *Plugin) Apply(ctx *core.Context) error { }) // 0.1 Resolve auth service for middleware (via IoC, not direct import) - var loginMW gin.HandlerFunc = func(c *gin.Context) { c.Next() } - var noTokenMW gin.HandlerFunc = func(c *gin.Context) { c.Next() } + denyAuth := ginutil.AuthUnavailable() + loginMW := denyAuth + noTokenMW := denyAuth if authSvc, err := core.Inject[contracts.AuthService](ctx); err == nil && authSvc != nil { if mw, ok := authSvc.RequireAuthMiddleware().(gin.HandlerFunc); ok { loginMW = mw