amadan

carlos / platform Public

apppolicy: the writers source test sees same-package and raw-string imports (#328)

Review fix round 1: unqualified AppPolicyKey calls inside internal/release
and writer references inside internal/apppolicy outside the writer chain
are now checked; imports are parsed and unquoted before the relevance
filter, so a backtick import path is seen. A fixture module pins both.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 175b7eeaadaf62b8a01ee58ce88159b8a2ce8241 parent da7e6e6
1 file changed, +261 −65
  • internal/apppolicy/writers_test.go +261 −65
diff --git a/internal/apppolicy/writers_test.go b/internal/apppolicy/writers_test.go
index 74c3ad5f1..d6d144331 100644
--- a/internal/apppolicy/writers_test.go
+++ b/internal/apppolicy/writers_test.go
@@ -2,11 +2,13 @@ package apppolicy
import (
"bytes"
+ "fmt"
"go/ast"
"go/parser"
"go/token"
"io/fs"
"os"
+ "path"
"path/filepath"
"slices"
"strconv"
@@ -15,8 +17,12 @@ import (
)
const (
- apppolicyImport = "github.com/carlosframework/platform/internal/apppolicy"
- releaseImport = "github.com/carlosframework/platform/internal/release"
+ modulePath = "github.com/carlosframework/platform"
+ apppolicyImport = modulePath + "/internal/apppolicy"
+ releaseImport = modulePath + "/internal/release"
+ apppolicyPkgDir = "internal/apppolicy"
+ releasePkgDir = "internal/release"
+ releasePolicyKey = "release.AppPolicyKey"
)
// policyWriterSites is the boundary of spec §3, kept by source: the only
@@ -32,108 +38,298 @@ var policyWriterSites = map[string][]string{
// The create-time clear (Task 11).
"apppolicy.Delete": {"internal/console/server.go"},
// The purge, the box's store door and the mutation classifier.
- "release.AppPolicyKey": {"internal/release/trash.go", "internal/edge/storedoor.go", "internal/hostops/mutation_classifier.go"},
+ releasePolicyKey: {"internal/release/trash.go", "internal/edge/storedoor.go", "internal/hostops/mutation_classifier.go"},
}
+// inPackageChain is, inside internal/apppolicy, the functions whose bodies
+// may name a restricted function: the writers call each other (PutIfMatch
+// -> PutIfMatchETag -> Sign, PutWire) and Key builds the key. Any other
+// function there that names one would be a writer of its own.
+var inPackageChain = []string{"PutIfMatch", "PutIfMatchETag", "PutWire", "Sign", "Delete", "Key"}
+
// TestPolicyWritersAreTheDoorThePurgeAndCreate walks every non-test .go
// file of the module and fails naming file:line of any reference to a
// policy writer, or to release.AppPolicyKey, outside its allowed files.
func TestPolicyWritersAreTheDoorThePurgeAndCreate(t *testing.T) {
root := moduleRoot(t)
+ violations, seen, err := scanPolicyWriters(root)
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, v := range violations {
+ t.Error(v)
+ }
+ // The box's store door builds the key today: a walk that finds no
+ // reference there is broken, not clean.
+ if seen[releasePolicyKey] == 0 {
+ t.Fatalf("no %s reference found under %s: the scan is broken", releasePolicyKey, root)
+ }
+}
+
+// TestPolicyWritersScanSeesEveryForm runs the scan over a fixture module:
+// unqualified calls inside the defining package, a raw-string import, an
+// aliased import and a dot import are all seen, and the permitted forms
+// are not reported.
+func TestPolicyWritersScanSeesEveryForm(t *testing.T) {
+ root := t.TempDir()
+ files := map[string]string{
+ "go.mod": "module " + modulePath + "\n\ngo 1.26.0\n",
+ // The definition itself is not a reference.
+ "internal/release/release.go": `package release
+func AppPolicyKey(acct, app string) string { return acct + app }
+`,
+ // The purge may build the key, unqualified.
+ "internal/release/trash.go": `package release
+func purge() string { return AppPolicyKey("bab", "shop") }
+`,
+ // Any other file in package release may not; a local of the same
+ // name, a field and a selector are not references to it.
+ "internal/release/other.go": `package release
+type rec struct{ AppPolicyKey string }
+func other(r rec) string {
+ AppPolicyKey := "x"
+ _ = r.AppPolicyKey
+ _ = rec{AppPolicyKey: "y"}
+ return AppPolicyKey
+}
+func sneaky() string { return AppPolicyKey("bab", "shop") }
+`,
+ "internal/release/other_test.go": `package release
+func inTest() string { return AppPolicyKey("bab", "shop") }
+`,
+ // Inside apppolicy the writer chain and Key are permitted; a new
+ // function there that names a writer is not.
+ "internal/apppolicy/apppolicy.go": `package apppolicy
+import "github.com/carlosframework/platform/internal/release"
+func Key(sqid, app string) string { return release.AppPolicyKey(sqid, app) }
+func Sign() []byte { return nil }
+func PutWire(b []byte) {}
+func PutIfMatchETag() { PutWire(Sign()) }
+func PutIfMatch() { PutIfMatchETag() }
+func Delete() { _ = Key("bab", "shop") }
+func Resign() { PutWire(Sign()) }
+`,
+ // A raw-string import is still an import.
+ "internal/door/raw.go": "package door\nimport `" + apppolicyImport + "`\nfunc clear() { apppolicy.Delete() }\n",
+ "internal/door/alias.go": `package door
+import ap "` + apppolicyImport + `"
+func write() { ap.PutIfMatch() }
+func read() string { return ap.Key("bab", "shop") }
+`,
+ "internal/door/dot.go": `package door
+import . "` + releaseImport + `"
+`,
+ // The permitted callers.
+ "internal/console/server.go": `package console
+import "` + apppolicyImport + `"
+func create() { apppolicy.Delete(); _ = apppolicy.Key("bab", "shop") }
+`,
+ "internal/edge/storedoor.go": `package edge
+import "` + releaseImport + `"
+var _ = release.AppPolicyKey("bab", "shop")
+`,
+ }
+ for name, src := range files {
+ p := filepath.Join(root, filepath.FromSlash(name))
+ if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(p, []byte(src), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ }
+ violations, _, err := scanPolicyWriters(root)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := []string{
+ "internal/apppolicy/apppolicy.go:9:17: apppolicy.PutWire",
+ "internal/apppolicy/apppolicy.go:9:25: apppolicy.Sign",
+ "internal/door/alias.go:3:16: apppolicy.PutIfMatch",
+ "internal/door/dot.go:2:8: dot import",
+ "internal/door/raw.go:3:16: apppolicy.Delete",
+ "internal/release/other.go:9:31: release.AppPolicyKey",
+ }
+ if len(violations) != len(want) {
+ t.Errorf("got %d violations, want %d:\n%s", len(violations), len(want), strings.Join(violations, "\n"))
+ }
+ for _, w := range want {
+ if !slices.ContainsFunc(violations, func(v string) bool { return strings.HasPrefix(v, w) }) {
+ t.Errorf("missing violation %q in:\n%s", w, strings.Join(violations, "\n"))
+ }
+ }
+}
+
+// scanPolicyWriters walks the module at root and returns one line per
+// reference to a restricted name outside its allowed files, and how many
+// references of each name it saw, allowed or not.
+func scanPolicyWriters(root string) (violations []string, seen map[string]int, err error) {
+ seen = map[string]int{}
fset := token.NewFileSet()
- seen := map[string]int{} // references found, allowed or not, by name
- err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
+ err = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error {
if err != nil {
return err
}
- rel, _ := filepath.Rel(root, path)
+ rel, _ := filepath.Rel(root, p)
rel = filepath.ToSlash(rel)
if d.IsDir() {
- if path == root {
+ if p == root {
return nil
}
name := d.Name()
if strings.HasPrefix(name, ".") || strings.HasPrefix(name, "_") || name == "testdata" || name == "vendor" || name == "node_modules" {
return filepath.SkipDir
}
- if _, err := os.Stat(filepath.Join(path, "go.mod")); err == nil {
+ if _, err := os.Stat(filepath.Join(p, "go.mod")); err == nil {
return filepath.SkipDir // another module
}
- if rel == "internal/apppolicy" {
- return filepath.SkipDir
- }
return nil
}
if !strings.HasSuffix(rel, ".go") || strings.HasSuffix(rel, "_test.go") {
return nil
}
- src, err := os.ReadFile(path)
+ src, err := os.ReadFile(p)
if err != nil {
return err
}
- if !bytes.Contains(src, []byte(apppolicyImport+`"`)) && !bytes.Contains(src, []byte(releaseImport+`"`)) {
+ dir := path.Dir(rel)
+ // Imports first, unquoted, so a raw-string import path is seen.
+ hdr, err := parser.ParseFile(token.NewFileSet(), rel, src, parser.ImportsOnly)
+ if err != nil {
+ return err
+ }
+ relevant := dir == apppolicyPkgDir || dir == releasePkgDir
+ for _, imp := range hdr.Imports {
+ if ip, _ := strconv.Unquote(imp.Path.Value); ip == apppolicyImport || ip == releaseImport {
+ relevant = true
+ }
+ }
+ if !relevant {
return nil
}
- f, err := parser.ParseFile(fset, rel, src, parser.SkipObjectResolution)
+ f, err := parser.ParseFile(fset, rel, src, 0)
if err != nil {
return err
}
- local := map[string]string{} // file-local package name -> "apppolicy" or "release"
- for _, imp := range f.Imports {
- path, _ := strconv.Unquote(imp.Path.Value)
- var pkg string
- switch path {
- case apppolicyImport:
- pkg = "apppolicy"
- case releaseImport:
- pkg = "release"
- default:
- continue
- }
- name := pkg
- if imp.Name != nil {
- name = imp.Name.Name
- }
- if name == "." {
- t.Errorf("%s: dot import of %s hides policy writer calls from this check", fset.Position(imp.Pos()), path)
- continue
+ violations = append(violations, scanFile(fset, f, rel, seen)...)
+ return nil
+ })
+ return violations, seen, err
+}
+
+func scanFile(fset *token.FileSet, f *ast.File, rel string, seen map[string]int) []string {
+ var out []string
+ report := func(pos token.Pos, name, fn string) {
+ seen[name]++
+ if path.Dir(rel) == apppolicyPkgDir {
+ if !slices.Contains(inPackageChain, fn) {
+ out = append(out, fmt.Sprintf("%s: %s in %s, outside the writer chain %v", fset.Position(pos), name, fn, inPackageChain))
}
- local[name] = pkg
+ return
}
- ast.Inspect(f, func(n ast.Node) bool {
- sel, ok := n.(*ast.SelectorExpr)
- if !ok {
- return true
- }
- x, ok := sel.X.(*ast.Ident)
- if !ok {
- return true
- }
- pkg, ok := local[x.Name]
- if !ok {
- return true
- }
- name := pkg + "." + sel.Sel.Name
- allowed, restricted := policyWriterSites[name]
- if !restricted {
- return true
- }
- seen[name]++
- if !slices.Contains(allowed, rel) {
- t.Errorf("%s: %s outside its allowed files %v", fset.Position(sel.Pos()), name, allowed)
+ if allowed := policyWriterSites[name]; !slices.Contains(allowed, rel) {
+ out = append(out, fmt.Sprintf("%s: %s outside its allowed files %v", fset.Position(pos), name, allowed))
+ }
+ }
+ local := map[string]string{} // file-local package name -> "apppolicy" or "release"
+ for _, imp := range f.Imports {
+ ip, _ := strconv.Unquote(imp.Path.Value)
+ var pkg string
+ switch ip {
+ case apppolicyImport:
+ pkg = "apppolicy"
+ case releaseImport:
+ pkg = "release"
+ default:
+ continue
+ }
+ name := pkg
+ if imp.Name != nil {
+ name = imp.Name.Name
+ }
+ if name == "." {
+ out = append(out, fmt.Sprintf("%s: dot import of %s hides policy writer references from this check", fset.Position(imp.Pos()), ip))
+ continue
+ }
+ local[name] = pkg
+ }
+ // Unqualified names refer to the file's own package.
+ var home string
+ switch path.Dir(rel) {
+ case apppolicyPkgDir:
+ home = "apppolicy"
+ case releasePkgDir:
+ home = "release"
+ }
+ for _, decl := range f.Decls {
+ fn := ""
+ if fd, ok := decl.(*ast.FuncDecl); ok && fd.Recv == nil {
+ fn = fd.Name.Name
+ }
+ var walk func(ast.Node) bool
+ walk = func(n ast.Node) bool {
+ switch n := n.(type) {
+ case *ast.FuncDecl:
+ // The definition's own name is not a reference.
+ for _, c := range []ast.Node{n.Recv, n.Type, n.Body} {
+ if c != nil && !isNilNode(c) {
+ ast.Inspect(c, walk)
+ }
+ }
+ return false
+ case *ast.SelectorExpr:
+ if x, ok := n.X.(*ast.Ident); ok && x.Obj == nil {
+ if pkg, ok := local[x.Name]; ok {
+ if name := pkg + "." + n.Sel.Name; policyWriterSites[name] != nil {
+ report(n.Pos(), name, fn)
+ }
+ }
+ }
+ ast.Inspect(n.X, walk) // never the selected field or method name
+ return false
+ case *ast.KeyValueExpr:
+ if _, ok := n.Key.(*ast.Ident); ok {
+ ast.Inspect(n.Value, walk) // a composite literal's field name
+ return false
+ }
+ case *ast.Field:
+ if n.Type != nil {
+ ast.Inspect(n.Type, walk) // field and parameter names are declarations
+ }
+ return false
+ case *ast.Ident:
+ // Unresolved in this file means package scope (or universe);
+ // resolved to a top-level func of this file is the same.
+ if home == "" || !(n.Obj == nil || isTopFunc(n.Obj)) {
+ return false
+ }
+ if name := home + "." + n.Name; policyWriterSites[name] != nil {
+ report(n.Pos(), name, fn)
+ }
}
return true
- })
- return nil
- })
- if err != nil {
- t.Fatal(err)
+ }
+ ast.Inspect(decl, walk)
}
- // The box's store door builds the key today: a walk that finds no
- // reference there is broken, not clean.
- if seen["release.AppPolicyKey"] == 0 {
- t.Fatalf("no release.AppPolicyKey reference found under %s: the scan is broken", root)
+ return out
+}
+
+func isTopFunc(o *ast.Object) bool {
+ fd, ok := o.Decl.(*ast.FuncDecl)
+ return o.Kind == ast.Fun && ok && fd.Recv == nil
+}
+
+// isNilNode reports a typed nil (a FuncDecl without receiver or body).
+func isNilNode(n ast.Node) bool {
+ switch v := n.(type) {
+ case *ast.FieldList:
+ return v == nil
+ case *ast.BlockStmt:
+ return v == nil
+ case *ast.FuncType:
+ return v == nil
}
+ return false
}
func moduleRoot(t *testing.T) string {
@@ -144,7 +340,7 @@ func moduleRoot(t *testing.T) string {
}
for {
if b, err := os.ReadFile(filepath.Join(dir, "go.mod")); err == nil &&
- bytes.HasPrefix(bytes.TrimSpace(b), []byte("module github.com/carlosframework/platform\n")) {
+ bytes.HasPrefix(bytes.TrimSpace(b), []byte("module "+modulePath+"\n")) {
return dir
}
parent := filepath.Dir(dir)