From 947e8c5946fb4e065a24e47e72bbfa78426635bd Mon Sep 17 00:00:00 2001 From: Nick Mills-Barrett Date: Thu, 29 Oct 2026 13:27:46 +0000 Subject: [PATCH] bridgelint: check avatar hashes, media info, GIF capabilities and dropped decode errors --- bridgelint/README.md | 11 + bridgelint/rules/analyzers.go | 2 +- bridgelint/rules/ep.go | 79 ++- bridgelint/rules/mc.go | 532 ++++++++++++++++++ bridgelint/rules/report.go | 5 + bridgelint/rules/rules_test.go | 6 +- .../rules/testdata/src/epdecode/epdecode.go | 80 +++ .../go/mautrix/bridgev2/bridgev2.go | 8 + .../src/maunium.net/go/mautrix/event/event.go | 44 ++ bridgelint/rules/testdata/src/mc/mc.go | 155 +++++ bridgelint/rules/testdata/src/mcgif/mcgif.go | 14 + bridgelint/rules/testdata/src/mcrej/mcrej.go | 7 + 12 files changed, 940 insertions(+), 3 deletions(-) create mode 100644 bridgelint/rules/mc.go create mode 100644 bridgelint/rules/testdata/src/epdecode/epdecode.go create mode 100644 bridgelint/rules/testdata/src/mc/mc.go create mode 100644 bridgelint/rules/testdata/src/mcgif/mcgif.go create mode 100644 bridgelint/rules/testdata/src/mcrej/mcrej.go diff --git a/bridgelint/README.md b/bridgelint/README.md index cd93044..f0a4414 100644 --- a/bridgelint/README.md +++ b/bridgelint/README.md @@ -259,6 +259,7 @@ as deliberate for LP002, and sets `com.beeper.can_retry` for clients. | EP001 | error | Errors matched by exact string: `err.Error() == "..."`, `switch err.Error()`, `strings.EqualFold(err.Error(), ...)`. Comparisons with `""` and substring matches aren't flagged, since some servers only give text. | | EP002 | warning | In library packages (ones that don't import bridgev2, other than `main`), an `http.Response` status formatted into a plain error, so callers can't tell 429 from 403 from 5xx. Forks like fhttp count when their `Response` has net/http's fields. | | EP003 | info | `errors.Join` returned from a Matrix handler or used as a user-facing message. | +| EP004 | warning | In a function with no results, `if json.Unmarshal(...) != nil { return }`, the same with `err := ...; err != nil`, or a `json.Decoder.Decode`/`proto.Unmarshal` check, where the branch is only a bare `return`. A payload of an unexpected shape then disappears with no log. `continue` isn't flagged, since loops often skip entries of other shapes on purpose. | ### General reliability (GR) @@ -274,6 +275,16 @@ as deliberate for LP002, and sets `com.beeper.can_retry` for clients. | GR011 | warning | `GetCapabilities` changes `RoomFeatures` fields but never the `ID`, so clients cache the wrong features. | | GR013 | warning | `LogoutRemote` that never disconnects or stops the client, when the pinned mautrix doesn't disconnect after `LogoutRemote` itself (v0.32.0 and earlier). The stopping call is recognised by name, a name heuristic. | +### Matrix content (MC) + +What clients need from bridged events and room features to render them correctly. + +| Rule | Severity | Flags | +|---|---|---| +| MC001 | error | A `bridgev2.Avatar` built in the function with a non-constant `MXC` and no `Hash`, either as a literal or by assigning `MXC` to an avatar literal that has no hash. bridgev2 skips the update when the new hash equals the stored one and an MXC is already set, so every avatar change after the first is lost. Avatars passed in or returned by helpers aren't checked. | +| MC002 | warning | Image capabilities that accept `image/gif` (a `FileFeatureMap` literal's `m.image` entry, following `MimeTypes` to a package-level map) when nothing in the package uses `event.CapMsgGIF`. Clients only enable their GIF picker with `fi.mau.gif`. | +| MC003 | warning | Image, video or audio `MessageEventContent` built in the function, with a URL or file, that gets no `Info` on any path to the end of the function. Content handed to another function is assumed to get its info there. Clients use `info.mimetype` to pick a renderer and to name cached files. | + ### Deliberately not included These were considered and left out because they can't be checked without false positives: diff --git a/bridgelint/rules/analyzers.go b/bridgelint/rules/analyzers.go index e7dd698..c66f709 100644 --- a/bridgelint/rules/analyzers.go +++ b/bridgelint/rules/analyzers.go @@ -3,4 +3,4 @@ package rules import "golang.org/x/tools/go/analysis" // Analyzers are the bridgelint rule groups, in the order they're documented. -var Analyzers = []*analysis.Analyzer{MSSAnalyzer, BSAnalyzer, LPAnalyzer, DMAnalyzer, EPAnalyzer, GRAnalyzer, BLAnalyzer} +var Analyzers = []*analysis.Analyzer{MSSAnalyzer, BSAnalyzer, LPAnalyzer, DMAnalyzer, EPAnalyzer, GRAnalyzer, MCAnalyzer, BLAnalyzer} diff --git a/bridgelint/rules/ep.go b/bridgelint/rules/ep.go index 2e33b7f..a572e81 100644 --- a/bridgelint/rules/ep.go +++ b/bridgelint/rules/ep.go @@ -10,7 +10,7 @@ import ( var EPAnalyzer = &analysis.Analyzer{ Name: "ep", - Doc: "error plumbing rules (EP001-EP003): errors must be matched by type and carry typed statuses", + Doc: "error plumbing rules (EP001-EP004): errors must be matched by type, carry typed statuses and not vanish", Run: runEP, } @@ -34,6 +34,10 @@ func runEP(pass *analysis.Pass) (any, error) { } ast.Inspect(f, func(n ast.Node) bool { switch x := n.(type) { + case *ast.FuncDecl, *ast.FuncLit: + if sig := funcType(info, x); sig != nil && sig.Results().Len() == 0 { + checkDroppedDecode(pass, funcBody(x)) + } case *ast.BinaryExpr: if (x.Op == token.EQL || x.Op == token.NEQ) && (isErrText(x.X) && !isEmptyString(info, x.Y) || isErrText(x.Y) && !isEmptyString(info, x.X)) { report(pass, x, EP001, "error matched by comparing err.Error(); use errors.Is/errors.As or a typed code accessor") @@ -90,3 +94,76 @@ func containsErrorsJoin(pass *analysis.Pass, e ast.Expr) bool { }) return found } + +func isDecodeCall(info *types.Info, e ast.Expr) bool { + call, ok := unparen(e).(*ast.CallExpr) + if !ok { + return false + } + fn, ok := calleeObj(info, call).(*types.Func) + if !ok { + return false + } + if sig, _ := fn.Type().(*types.Signature); sig != nil && sig.Recv() != nil { + return fn.Name() == "Decode" && isNamed(sig.Recv().Type(), "encoding/json", "Decoder") + } + return isPkgFunc(fn, "encoding/json", "Unmarshal") || isPkgFunc(fn, "google.golang.org/protobuf/proto", "Unmarshal") +} + +// checkDroppedDecode implements EP004: in a function that can't return the error, a failed decode that +// only returns leaves no trace, so a payload shape the code didn't expect looks like a message that never +// arrived. Only bare returns are flagged: continue usually skips entries of other shapes on purpose. +func checkDroppedDecode(pass *analysis.Pass, body *ast.BlockStmt) { + info := pass.TypesInfo + inspectFunc(body, func(n ast.Node) bool { + ifs, ok := n.(*ast.IfStmt) + if !ok || ifs.Else != nil || len(ifs.Body.List) != 1 { + return true + } + if ret, ok := ifs.Body.List[0].(*ast.ReturnStmt); !ok || len(ret.Results) != 0 { + return true + } + var errVar types.Object + if as, ok := ifs.Init.(*ast.AssignStmt); ok && len(as.Lhs) == 1 && len(as.Rhs) == 1 && isDecodeCall(info, as.Rhs[0]) { + if id, ok := as.Lhs[0].(*ast.Ident); ok { + errVar = info.ObjectOf(id) + } + } + var decodeCheck ast.Expr + var visit func(e ast.Expr) + visit = func(e ast.Expr) { + be, ok := unparen(e).(*ast.BinaryExpr) + if !ok { + return + } + switch be.Op { + case token.LOR: + visit(be.X) + visit(be.Y) + case token.NEQ: + if !isNilIdent(info, be.Y) { + return + } + if isDecodeCall(info, be.X) { + decodeCheck = be + } else if id, ok := unparen(be.X).(*ast.Ident); ok && errVar != nil && info.ObjectOf(id) == errVar { + decodeCheck = be + } + } + } + visit(ifs.Cond) + if decodeCheck != nil { + report(pass, decodeCheck, EP004, "decode error dropped with a bare return; log it or pass it to an error handler, or a payload the code didn't expect vanishes without a trace") + } + return true + }) +} + +func isNilIdent(info *types.Info, e ast.Expr) bool { + id, ok := unparen(e).(*ast.Ident) + if !ok { + return false + } + _, isNil := info.Uses[id].(*types.Nil) + return isNil +} diff --git a/bridgelint/rules/mc.go b/bridgelint/rules/mc.go new file mode 100644 index 0000000..ee4b749 --- /dev/null +++ b/bridgelint/rules/mc.go @@ -0,0 +1,532 @@ +package rules + +import ( + "go/ast" + "go/constant" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/cfg" +) + +var MCAnalyzer = &analysis.Analyzer{ + Name: "mc", + Doc: "Matrix content rules (MC001-MC003): avatar hashes, media info and capabilities that clients depend on", + Run: runMC, +} + +func runMC(pass *analysis.Pass) (any, error) { + if err := ConfigError(pass); err != nil { + return nil, err + } + for _, f := range pass.Files { + if isTestFile(pass, f) { + continue + } + ast.Inspect(f, func(n ast.Node) bool { + var body *ast.BlockStmt + switch fn := n.(type) { + case *ast.FuncDecl: + body = fn.Body + case *ast.FuncLit: + body = fn.Body + } + if body != nil { + checkAvatarHash(pass, body) + checkMediaInfo(pass, body) + } + return true + }) + } + checkGIFCapability(pass) + return nil, nil +} + +// compositeOf returns the composite literal e builds, looking through & and parentheses. +func compositeOf(e ast.Expr) *ast.CompositeLit { + e = unparen(e) + if u, ok := e.(*ast.UnaryExpr); ok && u.Op == token.AND { + e = unparen(u.X) + } + lit, _ := e.(*ast.CompositeLit) + return lit +} + +func litField(lit *ast.CompositeLit, name string) (*ast.KeyValueExpr, bool) { + for _, elt := range lit.Elts { + if kv, ok := elt.(*ast.KeyValueExpr); ok { + if id, ok := kv.Key.(*ast.Ident); ok && id.Name == name { + return kv, true + } + } + } + return nil, false +} + +func isConstValue(info *types.Info, e ast.Expr, kind constant.Kind, match func(constant.Value) bool) bool { + tv, ok := info.Types[e] + return ok && tv.Value != nil && tv.Value.Kind() == kind && match(tv.Value) +} + +func isStringConst(info *types.Info, e ast.Expr) bool { + return isConstValue(info, e, constant.String, func(constant.Value) bool { return true }) +} + +// literalTargets maps each composite literal assigned or declared in body to the expression it's stored in. +func literalTargets(body ast.Node) map[*ast.CompositeLit]string { + targets := map[*ast.CompositeLit]string{} + ast.Inspect(body, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.AssignStmt: + if len(x.Lhs) == len(x.Rhs) { + for i, rhs := range x.Rhs { + if lit := compositeOf(rhs); lit != nil { + targets[lit] = types.ExprString(x.Lhs[i]) + } + } + } + case *ast.ValueSpec: + for i, name := range x.Names { + if i < len(x.Values) { + if lit := compositeOf(x.Values[i]); lit != nil { + targets[lit] = name.Name + } + } + } + } + return true + }) + return targets +} + +// checkAvatarHash implements MC001: bridgev2 compares the hash of a pre-uploaded avatar with the stored one +// and skips the update when they match and an MXC is already set, so an MXC without a hash makes every +// avatar change after the first a no-op. +func checkAvatarHash(pass *analysis.Pass, body *ast.BlockStmt) { + info := pass.TypesInfo + isAvatar := func(e ast.Expr) bool { return isNamed(info.TypeOf(e), pathBridgev2, "Avatar") } + targets := literalTargets(body) + hashAssigned := map[string]bool{} + type built struct { + lit *ast.CompositeLit + hasHash bool + } + litFor := map[string]built{} + var mxcAssigns []*ast.SelectorExpr + var noHash []*ast.KeyValueExpr + ast.Inspect(body, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.FuncLit: + return false + case *ast.AssignStmt: + for i, lhs := range x.Lhs { + sel, ok := unparen(lhs).(*ast.SelectorExpr) + if !ok || !isAvatar(sel.X) { + continue + } + switch sel.Sel.Name { + case "Hash": + hashAssigned[types.ExprString(sel.X)] = true + case "MXC": + if len(x.Lhs) == len(x.Rhs) && isStringConst(info, x.Rhs[i]) { + continue + } + mxcAssigns = append(mxcAssigns, sel) + } + } + case *ast.CompositeLit: + if !isAvatar(x) { + return true + } + _, hasHash := litField(x, "Hash") + if target, ok := targets[x]; ok { + litFor[target] = built{x, hasHash} + } + mxc, hasMXC := litField(x, "MXC") + // A constant MXC is a fixed avatar, which never needs a later update to get through. + if !hasMXC || hasHash || isStringConst(info, mxc.Value) { + return true + } + if rm, ok := litField(x, "Remove"); ok && isConstValue(info, rm.Value, constant.Bool, constant.BoolVal) { + return true + } + noHash = append(noHash, mxc) + } + return true + }) + const msg = "avatar has an MXC but no Hash; bridgev2 skips the update when the hash is unchanged, so later avatar changes never reach Matrix. Set Hash, e.g. sha256 of the remote avatar ID or URL" + for _, kv := range noHash { + var target string + for lit, t := range targets { + if v, ok := litField(lit, "MXC"); ok && v == kv { + target = t + } + } + if target != "" && hashAssigned[target] { + continue + } + report(pass, kv, MC001, msg) + } + for _, sel := range mxcAssigns { + target := types.ExprString(sel.X) + b, ok := litFor[target] + // Only avatars built in this function: one passed in or returned by a helper may already have a hash. + if !ok || b.hasHash || hashAssigned[target] { + continue + } + report(pass, sel, MC001, msg) + } +} + +var inlineMediaTypes = map[string]bool{"m.image": true, "m.video": true, "m.audio": true} + +func isInlineMediaType(info *types.Info, e ast.Expr) bool { + return isConstValue(info, e, constant.String, func(v constant.Value) bool { return inlineMediaTypes[constant.StringVal(v)] }) +} + +func isMessageContent(info *types.Info, e ast.Expr) bool { + return isNamed(info.TypeOf(e), pathEvent, "MessageEventContent") +} + +// checkMediaInfo implements MC003: image, video and audio content built in a function that never gets an +// Info on any path to the end of the function. Clients use info.mimetype to pick a renderer and name cached +// files, so a GIF without it shows as a still photo and iOS saves voice notes as unplayable .bin files. +func checkMediaInfo(pass *analysis.Pass, body *ast.BlockStmt) { + info := pass.TypesInfo + targets := literalTargets(body) + // Locals that start as content literals without Info, keyed by variable. + fresh := map[types.Object]*ast.CompositeLit{} + for lit, target := range targets { + if !isMessageContent(info, lit) { + continue + } + if _, ok := litField(lit, "Info"); ok { + continue + } + if obj := localVar(info, body, target, lit); obj != nil { + fresh[obj] = lit + } + } + type source struct { + node ast.Node + obj types.Object // nil for a literal that isn't stored in a local + lit *ast.CompositeLit + } + var sources []source + hasURL := map[types.Object]bool{} + ast.Inspect(body, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.FuncLit: + return false + case *ast.AssignStmt: + for i, lhs := range x.Lhs { + sel, ok := unparen(lhs).(*ast.SelectorExpr) + if !ok || !isMessageContent(info, sel.X) { + continue + } + id, ok := unparen(sel.X).(*ast.Ident) + if !ok { + continue + } + obj := info.ObjectOf(id) + if _, ok := fresh[obj]; !ok { + continue + } + switch sel.Sel.Name { + case "URL", "File": + hasURL[obj] = true + case "MsgType": + if len(x.Lhs) == len(x.Rhs) && isInlineMediaType(info, x.Rhs[i]) { + sources = append(sources, source{node: x, obj: obj}) + } + } + } + case *ast.CompositeLit: + if !isMessageContent(info, x) { + return true + } + mt, ok := litField(x, "MsgType") + if !ok || !isInlineMediaType(info, mt.Value) { + return true + } + if _, ok := litField(x, "Info"); ok { + return true + } + _, url := litField(x, "URL") + _, file := litField(x, "File") + var obj types.Object + if target, ok := targets[x]; ok { + obj = localVar(info, body, target, x) + } + if obj != nil { + if url || file { + hasURL[obj] = true + } + sources = append(sources, source{node: x, obj: obj, lit: x}) + } else if url || file { + sources = append(sources, source{node: x, lit: x}) + } + } + return true + }) + if len(sources) == 0 { + return + } + var g *cfg.CFG + for _, src := range sources { + if src.obj != nil && !hasURL[src.obj] { + continue + } + if src.obj == nil && !anonymousContentDropsInfo(pass, body, src.lit) { + continue + } + if g == nil { + g = cfg.New(body, func(*ast.CallExpr) bool { return true }) + } + if src.obj != nil && infoReachable(pass, g, src.node, src.obj) { + continue + } + report(pass, src.node, MC003, "image, video or audio content without Info on any path; clients need info.mimetype (and w/h for images and video) to render it and name cached files") + } +} + +// localVar resolves target to a variable declared in body, or nil. +func localVar(info *types.Info, body *ast.BlockStmt, target string, lit *ast.CompositeLit) types.Object { + var obj types.Object + ast.Inspect(body, func(n ast.Node) bool { + if obj != nil { + return false + } + id, ok := n.(*ast.Ident) + if ok && id.Name == target { + if o, ok := info.Defs[id].(*types.Var); ok && o.Pos() >= body.Pos() && o.Pos() < body.End() { + obj = o + } else if o, ok := info.Uses[id].(*types.Var); ok && o.Pos() >= body.Pos() && o.Pos() < body.End() && id.Pos() < lit.Pos() { + obj = o + } + } + return true + }) + return obj +} + +// anonymousContentDropsInfo reports whether a content literal that isn't stored in a local can't get an +// Info later: it isn't a call argument and nothing in the function assigns any content's Info. +func anonymousContentDropsInfo(pass *analysis.Pass, body *ast.BlockStmt, lit *ast.CompositeLit) bool { + info := pass.TypesInfo + ok := true + ast.Inspect(body, func(n ast.Node) bool { + if !ok { + return false + } + switch x := n.(type) { + case *ast.CallExpr: + for _, arg := range x.Args { + if compositeOf(arg) == lit { + ok = false + } + } + case *ast.AssignStmt: + for _, lhs := range x.Lhs { + if sel, isSel := unparen(lhs).(*ast.SelectorExpr); isSel && sel.Sel.Name == "Info" && isMessageContent(info, sel.X) { + ok = false + } + } + } + return ok + }) + return ok +} + +// infoReachable reports whether any statement reachable from src sets obj's Info, hands obj (or its +// Info) to a call, or replaces obj. +func infoReachable(pass *analysis.Pass, g *cfg.CFG, src ast.Node, obj types.Object) bool { + info := pass.TypesInfo + isObj := func(e ast.Expr) bool { + e = unparen(e) + if u, ok := e.(*ast.UnaryExpr); ok && u.Op == token.AND { + e = unparen(u.X) + } + if sel, ok := e.(*ast.SelectorExpr); ok && sel.Sel.Name == "Info" { + e = unparen(sel.X) + } + id, ok := e.(*ast.Ident) + return ok && info.ObjectOf(id) == obj + } + sets := func(n ast.Node) bool { + found := false + ast.Inspect(n, func(m ast.Node) bool { + if found { + return false + } + switch x := m.(type) { + case *ast.AssignStmt: + for _, lhs := range x.Lhs { + lhs = unparen(lhs) + if id, ok := lhs.(*ast.Ident); ok && info.ObjectOf(id) == obj && !(x.Pos() <= src.Pos() && src.End() <= x.End()) { + found = true + } + for { + sel, ok := lhs.(*ast.SelectorExpr) + if !ok { + break + } + if sel.Sel.Name == "Info" && isObj(sel.X) { + found = true + } + lhs = unparen(sel.X) + } + } + case *ast.CallExpr: + for _, arg := range x.Args { + if isObj(arg) { + found = true + } + } + } + return !found + }) + return found + } + var start *cfg.Block + startIdx := -1 + for _, b := range g.Blocks { + for i, n := range b.Nodes { + if n.Pos() <= src.Pos() && src.End() <= n.End() { + start, startIdx = b, i + } + } + if start != nil { + break + } + } + if start == nil { + return true + } + for _, n := range start.Nodes[startIdx:] { + if sets(n) { + return true + } + } + seen := map[*cfg.Block]bool{start: true} + queue := append([]*cfg.Block(nil), start.Succs...) + for len(queue) > 0 { + b := queue[0] + queue = queue[1:] + if seen[b] { + continue + } + seen[b] = true + for _, n := range b.Nodes { + if sets(n) { + return true + } + } + queue = append(queue, b.Succs...) + } + return false +} + +// checkGIFCapability implements MC002: Beeper clients only enable their GIF picker when the room features +// list fi.mau.gif, so a bridge that accepts image/gif images but never declares it hides a working feature. +func checkGIFCapability(pass *analysis.Pass) { + info := pass.TypesInfo + declaresGIF := false + var gifEntries []ast.Node + for _, f := range pass.Files { + if isTestFile(pass, f) { + continue + } + ast.Inspect(f, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.Ident: + if c, ok := info.Uses[x].(*types.Const); ok && c.Pkg() != nil && c.Pkg().Path() == pathEvent && c.Name() == "CapMsgGIF" { + declaresGIF = true + } + case *ast.BasicLit: + if isConstValue(info, x, constant.String, func(v constant.Value) bool { return constant.StringVal(v) == "fi.mau.gif" }) { + declaresGIF = true + } + case *ast.CompositeLit: + if !isNamed(info.TypeOf(x), pathEvent, "FileFeatureMap") { + return true + } + for _, elt := range x.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok || !isConstValue(info, kv.Key, constant.String, func(v constant.Value) bool { return constant.StringVal(v) == "m.image" }) { + continue + } + if entry := gifMimeEntry(pass, kv.Value); entry != nil { + gifEntries = append(gifEntries, entry) + } + } + } + return true + }) + } + if declaresGIF { + return + } + for _, e := range gifEntries { + report(pass, e, MC002, "image capabilities accept image/gif but no room features declare event.CapMsgGIF (fi.mau.gif), so clients keep their GIF picker disabled") + } +} + +// gifMimeEntry returns the "image/gif" entry of a supported image FileFeatures value, following +// MimeTypes to a package-level map variable. +func gifMimeEntry(pass *analysis.Pass, e ast.Expr) ast.Node { + info := pass.TypesInfo + lit := compositeOf(e) + if lit == nil { + return nil + } + mt, ok := litField(lit, "MimeTypes") + if !ok { + return nil + } + mimes := compositeOf(mt.Value) + if mimes == nil { + if id, ok := unparen(mt.Value).(*ast.Ident); ok { + if v, ok := info.Uses[id].(*types.Var); ok && v.Parent() == pass.Pkg.Scope() { + mimes = pkgVarLiteral(pass, v) + } + } + } + if mimes == nil { + return nil + } + for _, elt := range mimes.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok || !isConstValue(info, kv.Key, constant.String, func(v constant.Value) bool { return constant.StringVal(v) == "image/gif" }) { + continue + } + if isConstValue(info, kv.Value, constant.Int, func(v constant.Value) bool { + n, exact := constant.Int64Val(v) + return exact && n > 0 + }) { + return kv + } + } + return nil +} + +func pkgVarLiteral(pass *analysis.Pass, v *types.Var) *ast.CompositeLit { + for _, f := range pass.Files { + for _, d := range f.Decls { + gd, ok := d.(*ast.GenDecl) + if !ok || gd.Tok != token.VAR { + continue + } + for _, spec := range gd.Specs { + vs := spec.(*ast.ValueSpec) + for i, name := range vs.Names { + if pass.TypesInfo.Defs[name] == v && i < len(vs.Values) { + return compositeOf(vs.Values[i]) + } + } + } + } + } + return nil +} diff --git a/bridgelint/rules/report.go b/bridgelint/rules/report.go index 5537ae9..b963572 100644 --- a/bridgelint/rules/report.go +++ b/bridgelint/rules/report.go @@ -73,6 +73,7 @@ var ( EP001 = rule("EP001", SeverityError, "matching errors by string") EP002 = rule("EP002", SeverityWarning, "library error without a typed status") EP003 = rule("EP003", SeverityInfo, "errors.Join in a user-facing path") + EP004 = rule("EP004", SeverityWarning, "decode error dropped without a trace") GR001 = rule("GR001", SeverityWarning, "response body not closed") GR002 = rule("GR002", SeverityWarning, "HTTP request without context or timeout") @@ -83,6 +84,10 @@ var ( GR009 = rule("GR009", SeverityWarning, "package-level map or slice written at runtime, shared across logins") GR011 = rule("GR011", SeverityWarning, "RoomFeatures.ID is constant while capabilities vary") GR013 = rule("GR013", SeverityWarning, "LogoutRemote doesn't stop the client") + + MC001 = rule("MC001", SeverityError, "pre-uploaded avatar without a hash") + MC002 = rule("MC002", SeverityWarning, "GIFs accepted but fi.mau.gif not declared") + MC003 = rule("MC003", SeverityWarning, "image, video or audio content without info") ) // Category is ":" so drivers can recover both from a Diagnostic. diff --git a/bridgelint/rules/rules_test.go b/bridgelint/rules/rules_test.go index 671a9e6..095d134 100644 --- a/bridgelint/rules/rules_test.go +++ b/bridgelint/rules/rules_test.go @@ -118,13 +118,17 @@ func TestDM(t *testing.T) { } func TestEP(t *testing.T) { - analysistest.Run(t, analysistest.TestData(), EPAnalyzer, "ep", "epnetlib", "epmain") + analysistest.Run(t, analysistest.TestData(), EPAnalyzer, "ep", "epnetlib", "epmain", "epdecode") } func TestGR(t *testing.T) { analysistest.Run(t, analysistest.TestData(), GRAnalyzer, "gr") } +func TestMC(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), MCAnalyzer, "mc", "mcgif", "mcrej") +} + func TestBL(t *testing.T) { analysistest.Run(t, analysistest.TestData(), BLAnalyzer, "bl") } diff --git a/bridgelint/rules/testdata/src/epdecode/epdecode.go b/bridgelint/rules/testdata/src/epdecode/epdecode.go new file mode 100644 index 0000000..4789b2d --- /dev/null +++ b/bridgelint/rules/testdata/src/epdecode/epdecode.go @@ -0,0 +1,80 @@ +package epdecode + +import ( + "encoding/json" + "io" +) + +type Message struct{ ID string } + +type Handler interface { + HandleMessage(m Message) + HandleError(err error) +} + +func dispatch(h Handler, b []byte) { + var out Message + if json.Unmarshal(b, &out) != nil || out.ID == "" { // want `EP004` + return + } + h.HandleMessage(out) +} + +func dispatchBound(h Handler, b []byte) { + var out Message + if err := json.Unmarshal(b, &out); err != nil { // want `EP004` + return + } + h.HandleMessage(out) +} + +func dispatchStream(h Handler, r io.Reader) { + var out Message + if err := json.NewDecoder(r).Decode(&out); err != nil { // want `EP004` + return + } + h.HandleMessage(out) +} + +func dispatchReported(h Handler, b []byte) { + var out Message + if err := json.Unmarshal(b, &out); err != nil { + h.HandleError(err) + return + } + h.HandleMessage(out) +} + +func decode(b []byte) (Message, error) { + var out Message + if err := json.Unmarshal(b, &out); err != nil { + return out, err + } + return out, nil +} + +func decodeOK(b []byte) bool { + var out Message + if json.Unmarshal(b, &out) != nil { + return false + } + return true +} + +func filterRows(h Handler, rows [][]byte) { + for _, row := range rows { + var out Message + if json.Unmarshal(row, &out) != nil { + continue + } + h.HandleMessage(out) + } +} + +func fallback(h Handler, b []byte) { + var out Message + if json.Unmarshal(b, &out) != nil { + out = Message{ID: "unknown"} + } + h.HandleMessage(out) +} diff --git a/bridgelint/rules/testdata/src/maunium.net/go/mautrix/bridgev2/bridgev2.go b/bridgelint/rules/testdata/src/maunium.net/go/mautrix/bridgev2/bridgev2.go index 955384b..c8b18f6 100644 --- a/bridgelint/rules/testdata/src/maunium.net/go/mautrix/bridgev2/bridgev2.go +++ b/bridgelint/rules/testdata/src/maunium.net/go/mautrix/bridgev2/bridgev2.go @@ -130,3 +130,11 @@ type LoginInputDataField struct { type Bridge struct{} func (br *Bridge) QueueRemoteEvent(login *UserLogin, evt any) {} + +type Avatar struct { + ID string + Get func() ([]byte, error) + Remove bool + MXC string + Hash [32]byte +} diff --git a/bridgelint/rules/testdata/src/maunium.net/go/mautrix/event/event.go b/bridgelint/rules/testdata/src/maunium.net/go/mautrix/event/event.go index 7d0b8bb..9dff957 100644 --- a/bridgelint/rules/testdata/src/maunium.net/go/mautrix/event/event.go +++ b/bridgelint/rules/testdata/src/maunium.net/go/mautrix/event/event.go @@ -23,4 +23,48 @@ type RoomFeatures struct { ID string MaxTextSize int Edit int + File FileFeatureMap +} + +type MessageType string + +const ( + MsgText MessageType = "m.text" + MsgImage MessageType = "m.image" + MsgVideo MessageType = "m.video" + MsgAudio MessageType = "m.audio" + MsgFile MessageType = "m.file" +) + +type CapabilityMsgType = MessageType + +const CapMsgGIF CapabilityMsgType = "fi.mau.gif" + +type CapabilitySupportLevel int + +const ( + CapLevelRejected CapabilitySupportLevel = -2 + CapLevelUnsupported CapabilitySupportLevel = 0 + CapLevelFullySupported CapabilitySupportLevel = 2 +) + +type FileFeatures struct { + MimeTypes map[string]CapabilitySupportLevel +} + +type FileFeatureMap map[CapabilityMsgType]*FileFeatures + +type FileInfo struct { + MimeType string + Width int +} + +type EncryptedFileInfo struct{} + +type MessageEventContent struct { + MsgType MessageType + Body string + URL string + File *EncryptedFileInfo + Info *FileInfo } diff --git a/bridgelint/rules/testdata/src/mc/mc.go b/bridgelint/rules/testdata/src/mc/mc.go new file mode 100644 index 0000000..152f16a --- /dev/null +++ b/bridgelint/rules/testdata/src/mc/mc.go @@ -0,0 +1,155 @@ +package mc + +import ( + "crypto/sha256" + + "maunium.net/go/mautrix/bridgev2" + "maunium.net/go/mautrix/event" +) + +var imageMimes = map[string]event.CapabilitySupportLevel{ + "image/png": event.CapLevelFullySupported, + "image/gif": event.CapLevelFullySupported, // want `MC002` +} + +var caps = &event.RoomFeatures{ + File: event.FileFeatureMap{ + event.MsgImage: {MimeTypes: imageMimes}, + event.MsgVideo: {MimeTypes: map[string]event.CapabilitySupportLevel{"image/gif": event.CapLevelFullySupported}}, + }, +} + +func directURI(url string) (string, error) { return "mxc://" + url, nil } + +func avatarLiteral(mxc string) *bridgev2.Avatar { + return &bridgev2.Avatar{ID: mxc, MXC: mxc} // want `MC001` +} + +func avatarLiteralHashed(mxc string) *bridgev2.Avatar { + return &bridgev2.Avatar{ID: mxc, MXC: mxc, Hash: sha256.Sum256([]byte(mxc))} +} + +const defaultAvatar = "mxc://example.com/default" + +func avatarConstant() *bridgev2.Avatar { + return &bridgev2.Avatar{ID: defaultAvatar, MXC: defaultAvatar} +} + +func avatarRemoved(mxc string) *bridgev2.Avatar { + return &bridgev2.Avatar{MXC: mxc, Remove: true} +} + +func avatarHashedLater(mxc string) *bridgev2.Avatar { + a := &bridgev2.Avatar{ID: mxc, MXC: mxc} + a.Hash = sha256.Sum256([]byte(mxc)) + return a +} + +func avatarAssigned(url string, direct bool) *bridgev2.Avatar { + avatar := &bridgev2.Avatar{ID: url, Get: func() ([]byte, error) { return nil, nil }} + if direct { + var err error + avatar.MXC, err = directURI(url) // want `MC001` + _ = err + } + return avatar +} + +func avatarAssignedHashed(url string) *bridgev2.Avatar { + avatar := &bridgev2.Avatar{ID: url} + avatar.MXC, _ = directURI(url) + avatar.Hash = sha256.Sum256([]byte(url)) + return avatar +} + +type userInfo struct{ Avatar *bridgev2.Avatar } + +func avatarField(url string) *userInfo { + info := &userInfo{} + info.Avatar = &bridgev2.Avatar{ID: url} + info.Avatar.MXC, _ = directURI(url) // want `MC001` + return info +} + +func avatarFromHelper(url string) *bridgev2.Avatar { + avatar := avatarLiteralHashed(url) + avatar.MXC, _ = directURI(url) + return avatar +} + +func avatarCleared(a *bridgev2.Avatar) { + b := &bridgev2.Avatar{ID: "x"} + b.MXC = "" + _ = b +} + +type attachment struct { + Type, URL, Mime string +} + +func mediaSwitch(att *attachment) *event.MessageEventContent { + content := &event.MessageEventContent{} + switch att.Type { + case "image": + content.MsgType, content.Body = event.MsgImage, "image" // want `MC003` + case "video": + content.MsgType, content.Body = event.MsgVideo, "video" // want `MC003` + case "file": + content.MsgType, content.Body = event.MsgFile, "file" + content.Info = &event.FileInfo{MimeType: att.Mime} + default: + return nil + } + content.URL, _ = directURI(att.URL) + return content +} + +func mediaConditionalInfo(att *attachment) *event.MessageEventContent { + content := &event.MessageEventContent{MsgType: event.MsgImage, URL: att.URL} + if att.Mime != "" { + content.Info = &event.FileInfo{MimeType: att.Mime} + } + return content +} + +func mediaInfoField(att *attachment) *event.MessageEventContent { + content := &event.MessageEventContent{MsgType: event.MsgAudio, URL: att.URL, Info: &event.FileInfo{}} + content.Info.MimeType = att.Mime + return content +} + +func fillInfo(c *event.MessageEventContent) {} + +func mediaHelperFills(att *attachment) *event.MessageEventContent { + content := &event.MessageEventContent{MsgType: event.MsgVideo, URL: att.URL} + fillInfo(content) + return content +} + +func mediaLiteral(att *attachment) *event.MessageEventContent { + return &event.MessageEventContent{MsgType: event.MsgImage, URL: att.URL} // want `MC003` +} + +func mediaLiteralArg(att *attachment) { + fillInfo(&event.MessageEventContent{MsgType: event.MsgImage, URL: att.URL}) +} + +func mediaNoURL(att *attachment) *event.MessageEventContent { + return &event.MessageEventContent{MsgType: event.MsgImage, Body: "image unavailable"} +} + +func textContent() *event.MessageEventContent { + content := &event.MessageEventContent{MsgType: event.MsgText, URL: "x"} + return content +} + +func mediaParam(content *event.MessageEventContent) { + content.MsgType = event.MsgImage + content.URL = "x" +} + +func mediaReplaced(att *attachment, other *event.MessageEventContent) *event.MessageEventContent { + content := &event.MessageEventContent{MsgType: event.MsgImage, URL: att.URL} + content = other + return content +} diff --git a/bridgelint/rules/testdata/src/mcgif/mcgif.go b/bridgelint/rules/testdata/src/mcgif/mcgif.go new file mode 100644 index 0000000..1555369 --- /dev/null +++ b/bridgelint/rules/testdata/src/mcgif/mcgif.go @@ -0,0 +1,14 @@ +package mcgif + +import "maunium.net/go/mautrix/event" + +var caps = &event.RoomFeatures{ + File: event.FileFeatureMap{ + event.MsgImage: {MimeTypes: map[string]event.CapabilitySupportLevel{"image/gif": event.CapLevelFullySupported}}, + event.CapMsgGIF: {MimeTypes: map[string]event.CapabilitySupportLevel{"image/gif": event.CapLevelFullySupported}}, + }, +} + +var rejected = event.FileFeatureMap{ + event.MsgImage: {MimeTypes: map[string]event.CapabilitySupportLevel{"image/gif": event.CapLevelRejected}}, +} diff --git a/bridgelint/rules/testdata/src/mcrej/mcrej.go b/bridgelint/rules/testdata/src/mcrej/mcrej.go new file mode 100644 index 0000000..980f173 --- /dev/null +++ b/bridgelint/rules/testdata/src/mcrej/mcrej.go @@ -0,0 +1,7 @@ +package mcrej + +import "maunium.net/go/mautrix/event" + +var rejected = event.FileFeatureMap{ + event.MsgImage: {MimeTypes: map[string]event.CapabilitySupportLevel{"image/gif": event.CapLevelRejected, "image/png": event.CapLevelFullySupported}}, +}