fix: sanitize Google-sourced text in every printer #24
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| package architecture | ||
|
|
||
| import ( | ||
| "go/ast" | ||
| "go/parser" | ||
| "go/token" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestPrintedDTOTextIsSanitized(t *testing.T) { | ||
| t.Parallel() | ||
| root := repoRoot(t) | ||
| textFields := dtoTextFields(t, root) | ||
| wrappedSites := 0 | ||
|
|
||
| for _, kind := range []string{"cmd", "rwcmd"} { | ||
| for _, pkg := range packageDirs(t, kind) { | ||
| dir := filepath.Join(root, "internal", kind, pkg) | ||
| for _, source := range nonTestSources(t, dir) { | ||
| ast.Inspect(source.file, func(node ast.Node) bool { | ||
| call, ok := node.(*ast.CallExpr) | ||
| if !ok || !isOutputCall(call) { | ||
| return true | ||
| } | ||
| wrapped := false | ||
| for _, arg := range call.Args { | ||
| inspectPrintedArgument(arg, textFields, false, func(sel *ast.SelectorExpr, sanitized bool) { | ||
| if sanitized { | ||
| wrapped = true | ||
| return | ||
| } | ||
| t.Errorf("%s:%d: printed DTO text %s must be wrapped in sanitize.Output or sanitize.Filename", relativePath(root, source.path), source.fset.Position(sel.Pos()).Line, sel.Sel.Name) | ||
| }) | ||
| } | ||
| if wrapped { | ||
| wrappedSites++ | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if wrappedSites < 20 { | ||
| t.Fatalf("saw %d sanitized DTO print sites, want at least 20", wrappedSites) | ||
| } | ||
| } | ||
|
|
||
| // dtoTextFields returns the names of DTO text sources: exported string and | ||
| // []string struct fields in internal/api, plus the argument-less Get* string | ||
| // methods declared on those DTOs (GetDisplayName and friends), so text reached | ||
| // through a getter is held to the same rule as a field. Formatting helpers | ||
| // such as FormatTimeRange are not getters and render only dates. | ||
| func dtoTextFields(t *testing.T, root string) map[string]bool { | ||
| t.Helper() | ||
| fields := map[string]bool{} | ||
| for _, pkg := range packageDirs(t, "api") { | ||
| for _, file := range parseNonTestFiles(t, filepath.Join(root, "internal", "api", pkg)) { | ||
| ast.Inspect(file, func(node ast.Node) bool { | ||
| if fn, ok := node.(*ast.FuncDecl); ok { | ||
| if fn.Recv != nil && strings.HasPrefix(fn.Name.Name, "Get") && isStringGetter(fn.Type) { | ||
| fields[fn.Name.Name] = true | ||
| } | ||
| return false | ||
| } | ||
| structType, ok := node.(*ast.StructType) | ||
| if !ok { | ||
| return true | ||
| } | ||
| for _, field := range structType.Fields.List { | ||
| if !isStringOrStringSlice(field.Type) { | ||
| continue | ||
| } | ||
| for _, name := range field.Names { | ||
| if name.IsExported() { | ||
| fields[name.Name] = true | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // These fields are identifiers or machine-readable values, not prose. | ||
| for _, name := range []string{"ID", "ResourceName", "ThreadID", "MessageID", "MimeType", "Type", "Status", "ETag", "TimeZone", "Date", "DateTime"} { | ||
| delete(fields, name) | ||
| } | ||
| for name := range fields { | ||
| if strings.HasSuffix(name, "ID") || strings.HasSuffix(name, "IDs") || strings.HasSuffix(name, "URL") || strings.HasSuffix(name, "Link") { | ||
| delete(fields, name) | ||
| } | ||
| } | ||
| return fields | ||
| } | ||
|
|
||
| func isStringGetter(fn *ast.FuncType) bool { | ||
| if fn.Params != nil && len(fn.Params.List) > 0 { | ||
| return false | ||
| } | ||
| if fn.Results == nil || len(fn.Results.List) != 1 { | ||
| return false | ||
| } | ||
| return isIdent(fn.Results.List[0].Type, "string") | ||
| } | ||
|
|
||
| func isStringOrStringSlice(expr ast.Expr) bool { | ||
| if ident, ok := expr.(*ast.Ident); ok { | ||
| return ident.Name == "string" | ||
| } | ||
| array, ok := expr.(*ast.ArrayType) | ||
| if !ok || array.Len != nil { | ||
| return false | ||
| } | ||
| ident, ok := array.Elt.(*ast.Ident) | ||
| return ok && ident.Name == "string" | ||
| } | ||
|
|
||
| func isOutputCall(call *ast.CallExpr) bool { | ||
| if ident, ok := call.Fun.(*ast.Ident); ok { | ||
| return ident.Name == "append" && len(call.Args) > 0 && isIdent(call.Args[0], "rows") | ||
| } | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok { | ||
| return false | ||
|
There was a problem hiding this comment. 🟡 Medium (harness-engineering:harness-enforcement-reviewer): The new detector (isOutputCall/inspectPrintedArgument) only matches direct struct-field selectors (e.g. event.Summary) against the DTO text-field set. It does not recognize text reached through a getter method (e.g. contact.GetDisplayName(), contact.GetPrimaryEmail(), contact.GetPrimaryPhone(), contact.GetOrganization() in internal/cmd/contacts/output.go), since the call's selector name never matches a raw field name in the collected set. Those exact call sites are correctly wrapped in sanitize.Output today, but if a future edit drops the wrapper around a getter-derived value, this architecture test — the enforcement mechanism golden-principles.md item 11 cites — would not catch the regression. Worth extending the detector to also flag calls whose selector matches 'Get' for any field in the DTO text-field set. Reply to this thread when addressed.
Contributor
Author
There was a problem hiding this comment. Addressed in cce4479: the text-source set now includes every exported argument-less string getter declared on the DTOs, so contact.GetDisplayName() and friends are held to the same rule. Verified by unwrapping one getter site and watching the test fail.
Contributor
Author
There was a problem hiding this comment. Follow-up in 774ac60: the getter rule is limited to Get* methods; the first cut also caught Event.FormatTimeRange, which renders only dates. |
||
| } | ||
| methods := map[string]bool{"Printf": true, "Println": true, "Success": true, "Info": true, "Error": true} | ||
| if methods[sel.Sel.Name] { | ||
| return true | ||
| } | ||
| fmtFunctions := map[string]bool{"Print": true, "Fprint": true, "Fprintf": true, "Fprintln": true, "Sprintf": true, "Sprint": true} | ||
| return fmtFunctions[sel.Sel.Name] && isIdent(sel.X, "fmt") | ||
| } | ||
|
|
||
| func inspectPrintedArgument(expr ast.Expr, fields map[string]bool, sanitized bool, visit func(*ast.SelectorExpr, bool)) { | ||
| ast.Inspect(expr, func(node ast.Node) bool { | ||
| if call, ok := node.(*ast.CallExpr); ok && isSanitizerCall(call) { | ||
| for _, arg := range call.Args { | ||
| inspectPrintedArgument(arg, fields, true, visit) | ||
| } | ||
| return false | ||
| } | ||
| if sel, ok := node.(*ast.SelectorExpr); ok && fields[sel.Sel.Name] { | ||
| visit(sel, sanitized) | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
|
|
||
| func isSanitizerCall(call *ast.CallExpr) bool { | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| return ok && isIdent(sel.X, "sanitize") && (sel.Sel.Name == "Output" || sel.Sel.Name == "Filename") | ||
| } | ||
|
|
||
| func isIdent(expr ast.Expr, name string) bool { | ||
| ident, ok := expr.(*ast.Ident) | ||
| return ok && ident.Name == name | ||
| } | ||
|
|
||
| // parsedSource keeps a parsed file together with the path and file set needed | ||
| // to report positions in it. | ||
| type parsedSource struct { | ||
| path string | ||
| fset *token.FileSet | ||
| file *ast.File | ||
| } | ||
|
|
||
| func nonTestSources(t *testing.T, dir string) []parsedSource { | ||
| t.Helper() | ||
| entries, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| t.Fatalf("read %s: %v", dir, err) | ||
| } | ||
| var sources []parsedSource | ||
| for _, entry := range entries { | ||
| name := entry.Name() | ||
| if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { | ||
| continue | ||
| } | ||
| path := filepath.Join(dir, name) | ||
| fset := token.NewFileSet() | ||
| file, err := parser.ParseFile(fset, path, nil, 0) | ||
| if err != nil { | ||
| t.Fatalf("parse %s: %v", name, err) | ||
| } | ||
| sources = append(sources, parsedSource{path: path, fset: fset, file: file}) | ||
| } | ||
| return sources | ||
| } | ||
|
|
||
| func relativePath(root, path string) string { | ||
| rel, err := filepath.Rel(root, path) | ||
| if err != nil { | ||
| return path | ||
| } | ||
| return rel | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Low (documentation:docs-reviewer): The updated support-package list adds both
sanitizeandview, but the PR description only introducesinternal/sanitize; there is no mention of a new or previously-undocumentedviewpackage. Confirminternal/viewexists and that its addition here is intentional rather than an unrelated/accidental edit, since an incorrect package name in this list would mislead readers about the codebase's structure.Reply to this thread when addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal/view exists (the Success/Info/Error output helper used by init and profiles) and was missing from the list; adding it alongside sanitize is intentional.