Merge pull request #158 from awslabs/cli-improvements

Report construction bug fix + terminal UI improvements

Author: victornicolet

analysis/config/config.go

@@ -705,8 +705,8 @@ func (c Config) ExceedsMaxDepth(d int) bool {
 
 // TargetInfo is the information needed to build the target
 type TargetInfo struct {
-	// Files in the target
-	Files []string
+	// Patterns in the target
+	Patterns []string
 	// Platform of the target
 	Platform string
 	// UseProgramTransforms for the target
@@ -724,7 +724,7 @@ func (c Config) GetTargetMap() map[string]TargetInfo {
 			reflectValueCallInstances = append(reflectValueCallInstances, compileRegexes(r))
 		}
 		targets[targetSpec.Name] = TargetInfo{
-			Files:                     targetSpec.Files,
+			Patterns:                  targetSpec.Files,
 			Platform:                  targetSpec.Platform,
 			UseProgramTransforms:      targetSpec.UseProgramTransforms,
 			ReflectValueCallInstances: reflectValueCallInstances,

analysis/config/logging.go

@@ -15,6 +15,7 @@
 package config
 
 import (
+	"fmt"
 	"io"
 	"log"
 	"os"
@@ -263,3 +264,23 @@ func (l *LogGroup) LogsDebug() bool {
 func (l *LogGroup) LogsTrace() bool {
 	return l.Level >= TraceLevel
 }
+
+// Infoboxf prints info-level logging in a box.
+func (l *LogGroup) Infoboxf(format string, v ...any) {
+	// Example:
+	// ╭───────────────────────────────╮
+	// │  this is the message to print │
+	// ╰───────────────────────────────╯
+	contents := fmt.Sprintf(format, v...)
+	contentsLines := strings.Split(contents, "\n")
+	n := 0
+	for _, line := range contentsLines {
+		n = max(n, len(line))
+	}
+	bar := strings.Repeat("─", n+2)
+	l.Info("╭" + bar + "╮\n")
+	for _, content := range contentsLines {
+		l.Info("│ " + content + " │\n")
+	}
+	l.Info("╰" + bar + "╯\n")
+}

analysis/config/report.go

@@ -20,6 +20,7 @@ import (
 	"sync"
 
 	"github.com/awslabs/ar-go-tools/internal/formatutil"
+	"github.com/awslabs/ar-go-tools/internal/funcutil"
 )
 
 // Errors contains errors generated during the different steps of the analysis, keyed by some string
@@ -84,11 +85,20 @@ func (r *ReportInfo) Merge(other *ReportInfo) {
 	for sev, count := range other.CountBySeverity {
 		r.IncrementSevCount(sev, count)
 	}
-	for tag, reportGroup := range other.Reports {
-		if thisReportGroup, ok := r.Reports[tag]; ok {
-			thisReportGroup.Details = append(thisReportGroup.Details, r.Reports[tag].Details...)
+	for otherTag, otherReportGroup := range other.Reports {
+		if thisReportGroup, ok := r.Reports[otherTag]; ok {
+			newDetails := thisReportGroup.Details
+			for _, otherDetail := range otherReportGroup.Details {
+				if !funcutil.Contains(thisReportGroup.Details, otherDetail) {
+					newDetails = append(newDetails, otherDetail)
+				}
+			}
+			// Note: reports with same tags should have same severity always,
+			// because the config enforces tag uniqueness, and 1 tag - 1 severity
+			thisReportGroup.Details = newDetails
+			r.Reports[otherTag] = thisReportGroup
 		} else {
-			r.Reports[tag] = reportGroup
+			r.Reports[otherTag] = otherReportGroup
 		}
 	}
 }
@@ -112,7 +122,10 @@ func (r *ReportInfo) addEntry(tag string, entry ReportEntry) {
 		return
 	}
 	group := r.Reports[tag]
-	newDetails := append(group.Details, entry.ContentFile)
+	newDetails := group.Details
+	if !funcutil.Contains(group.Details, entry.ContentFile) {
+		newDetails = append(group.Details, entry.ContentFile)
+	}
 	r.Reports[tag] = ReportGroup{
 		Tool:     entry.Tool,
 		Severity: entry.Severity,

analysis/config/report_test.go

@@ -0,0 +1,45 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	"testing"
+
+	"github.com/awslabs/ar-go-tools/internal/funcutil"
+)
+
+func TestMerge(t *testing.T) {
+	report1 := NewReport()
+	report2 := NewReport()
+	report1.addEntry("tag1", ReportEntry{
+		Tool:        SyntacticTool,
+		ContentFile: "example.json",
+		Severity:    High,
+	})
+	report2.addEntry("tag1", ReportEntry{
+		Tool:        SyntacticTool,
+		ContentFile: "ex2.json",
+		Severity:    High,
+	})
+	report2.addEntry("tag2", ReportEntry{
+		Tool:        SyntacticTool,
+		ContentFile: "example3.json",
+		Severity:    Low,
+	})
+	report1.Merge(report2)
+	if !funcutil.Contains(report1.Reports["tag1"].Details, "ex2.json") {
+		t.Fatalf("report1 should contain report2's ex2.json file after merge in tag1")
+	}
+}

analysis/dataflow/function_summary_graph_nodes.go

@@ -161,43 +161,83 @@ func NodeSummary(g GraphNode) string {
 	return ""
 }
 
+// NodeTermLabel returns a string to be printed in terminals for quick identification of a dataflow node
+// in a trace.
+// The returned string prints as a 7-character long string in a terminal.
+func NodeTermLabel(g GraphNode) string {
+	switch g.(type) {
+	case *ParamNode:
+		return formatutil.BgCyan("•PARAM ")
+	case *CallNode:
+		return formatutil.BgMagenta("• CALL ")
+	case *CallNodeArg:
+		return formatutil.BgBlue("• ARG  ")
+	case *ReturnValNode:
+		return formatutil.BgGreen("• RETV ")
+	case *ClosureNode:
+		return formatutil.BgWhite("•CLOSU ")
+	case *BoundLabelNode:
+		return formatutil.BgRed("• BNDL ") // red because it usually yields false positives
+	case *SyntheticNode:
+		return formatutil.BgLightGreen("• SYNT ")
+	case *BoundVarNode:
+		return formatutil.BgLightRed("• BNDV ")
+	case *FreeVarNode:
+		return formatutil.BgOrange("•FREEV ")
+	case *AccessGlobalNode:
+		return formatutil.BgYellow("• GLOB ")
+	}
+	return "  ???  "
+}
+
 // TermNodeSummary returns a string summary of the node, highlighting with terminal colors
 // green indicates a relative index/argument name,
 // magenta is used for function names,
 // italic is used for types.
 func TermNodeSummary(g GraphNode) string {
+	nodeCode := NodeTermLabel(g)
 	switch x := g.(type) {
 	case *ParamNode:
-		return fmt.Sprintf("Parameter %s (type %s) of %s",
+		return fmt.Sprintf("%s Parameter %s (type %s) of %s",
+			nodeCode,
 			formatutil.Green(x.ssaNode.Name()),
-			formatutil.Italic(x.Type().String()),
+			formatutil.Italic(formatutil.FormatLastSplit(x.Type().String(), ".", formatutil.Blue)),
 			formatutil.Magenta(fmt.Sprintf("%q", x.parent.Parent.Name())))
 	case *CallNode:
-		return fmt.Sprintf("Result of call to %s (type %s)",
+		return fmt.Sprintf("%s Result of call to %s (type %s)",
+			nodeCode,
 			formatutil.Magenta(x.Callee().Name()),
-			formatutil.Italic(x.Type().String()))
+			formatutil.Italic(formatutil.FormatLastSplit(x.Type().String(), ".", formatutil.Blue)))
 	case *CallNodeArg:
-		return fmt.Sprintf("Argument #%s (type %s) in call to %s",
+		return fmt.Sprintf("%s Argument #%s (type %s) in call to %s",
+			nodeCode,
 			formatutil.Green(x.Index()),
-			formatutil.Italic(x.Type().String()),
+			formatutil.Italic(formatutil.FormatLastSplit(x.Type().String(), ".", formatutil.Blue)),
 			formatutil.Magenta(x.ParentNode().Callee().Name()))
 	case *ReturnValNode:
-		return fmt.Sprintf("Return value %s (type %s) of %s",
+		return fmt.Sprintf("%s Return value %s (type %s) of %s",
+			nodeCode,
 			formatutil.Green(x.Index()),
-			formatutil.Italic(x.Type().String()),
+			formatutil.Italic(formatutil.FormatLastSplit(x.Type().String(), ".", formatutil.Blue)),
 			formatutil.Magenta(x.ParentName()))
 	case *ClosureNode:
-		return "Closure"
+		return formatutil.BgCyan(nodeCode) + " Closure node (entire function)"
 	case *BoundLabelNode:
-		return fmt.Sprintf("Bound label of type %s", formatutil.Italic(x.targetInfo.Type().String()))
+		return fmt.Sprintf("%s Bound label of type %s",
+			nodeCode,
+			formatutil.Italic(
+				formatutil.FormatLastSplit(x.targetInfo.Type().String(), ".", formatutil.Blue)))
 	case *SyntheticNode:
-		return "Synthetic node"
+		return nodeCode + " Synthetic node"
 	case *BoundVarNode:
-		return "Bound variable"
+		return fmt.Sprintf("%s Bound variable %s", nodeCode, x.String())
 	case *FreeVarNode:
-		return fmt.Sprintf("Free variable #%s of %q", formatutil.Green(x.fvPos), x.ssaNode.Parent().String())
+		return fmt.Sprintf("%s Free variable #%s of %q",
+			nodeCode,
+			formatutil.Green(x.fvPos),
+			x.ssaNode.Parent().String())
 	case *AccessGlobalNode:
-		return fmt.Sprintf("Global variable %s", formatutil.Red(x.Global.String()))
+		return fmt.Sprintf("%s Global variable %s", nodeCode, formatutil.Red(x.Global.String()))
 	}
 	return ""
 }

analysis/dataflow/inter_procedural.go

@@ -319,7 +319,7 @@ func (g *InterProceduralFlowGraph) RunVisitorOnEntryPoints(visitor Visitor, spec
 		summary.ForAllNodes(scanEntryPoints(g, spec, entryPoints))
 	}
 
-	g.AnalyzerState.Logger.Infof("--- # of analysis entry points: %d ---\n", len(entryPoints))
+	g.AnalyzerState.Logger.Infoboxf(" %d analysis entry points", len(entryPoints))
 	if g.AnalyzerState.Logger.LogsDebug() {
 		for _, entryPoint := range entryPoints {
 			g.AnalyzerState.Logger.Debugf("Entry: %s", entryPoint.Node)

analysis/syntactic/structinit/structinit.go

@@ -110,7 +110,8 @@ func Analyze(state *ptr.State, reqs AnalysisReqs) (AnalysisResult, error) {
 		funcutil.Map(specs, func(ss config.StructInitSpec) string { return ss.Tag }), ","))
 	if len(specs) == 0 {
 		// If there is no specs here, it's like because of the tags being filtered out by structInitSpecs
-		state.Logger.Infof("No struct-init specs matching configuration; check the tags if you expected a result")
+		state.Logger.Infof(
+			"No struct-init specs matching configuration; check the tags if you expected a result")
 		return AnalysisResult{}, nil
 	}
 	res := AnalysisResult{InitInfos: make(map[*types.Named]InitInfo)}
@@ -196,7 +197,8 @@ func newState(spec config.StructInitSpec, st *ptr.State) (*state, error) {
 			for i := 0; i < structTyp.NumFields(); i++ {
 				f := structTyp.Field(i)
 				if fieldSpec.Field == "" {
-					return nil, fmt.Errorf("field name in fields-set spec should not be empty: %+v", fieldSpec)
+					return nil,
+						fmt.Errorf("field name in fields-set spec should not be empty: %+v", fieldSpec)
 				}
 				if fieldSpec.Field == f.Name() {
 					field = f
@@ -205,12 +207,21 @@ func newState(spec config.StructInitSpec, st *ptr.State) (*state, error) {
 			}
 
 			if field == nil {
-				return nil, fmt.Errorf("failed to find field %v in struct %v from spec: %+v", fieldSpec.Field, structTyp, spec)
+				return nil,
+					fmt.Errorf(
+						"failed to find field %v in struct %v from spec: %+v",
+						fieldSpec.Field,
+						structTyp,
+						spec)
 			}
 			if fieldSpec.Value.Const != "" {
 				c, ok := findNamedConst(st.Program, fieldSpec.Value)
 				if !ok {
-					return nil, fmt.Errorf("failed to find a named constant in the program for %v in spec: %+v", fieldSpec.Value, spec)
+					return nil,
+						fmt.Errorf(
+							"failed to find a named constant in the program for %v in spec: %+v",
+							fieldSpec.Value,
+							spec)
 				}
 
 				fieldVal[alloc.typ.named][field] = c.Value
@@ -219,7 +230,10 @@ func newState(spec config.StructInitSpec, st *ptr.State) (*state, error) {
 			if fieldSpec.Value.Method != "" {
 				f, ok := findMethod(st.Program, fieldSpec.Value)
 				if !ok {
-					return nil, fmt.Errorf("failed to find a function in the program for %v in spec: %+v", fieldSpec.Value, spec)
+					return nil,
+						fmt.Errorf("failed to find a function in the program for %v in spec: %+v",
+							fieldSpec.Value,
+							spec)
 				}
 
 				fieldVal[alloc.typ.named][field] = f

analysis/taint/report.go

@@ -77,15 +77,24 @@ func reportCoverage(coverage map[string]bool, coverageWriter io.StringWriter) {
 
 // logTaintFlow logs a taint flow on the state's logger.
 func logTaintFlow(s *dataflow.State, source dataflow.NodeWithTrace, sink *dataflow.VisitorNode) {
-	s.Logger.Infof(" 💀 Tainted data flows to sink:")
-	s.Logger.Infof("  |- Data from %s (%s)", source.Node.Position(s), formatutil.Green(source.Node.String()))
-	s.Logger.Infof("  |- Reached sink at %s (%s)\n", sink.Node.Position(s), formatutil.Red(sink.Node.String()))
-
-	sinkPos := sink.Node.Position(s)
-	if callArg, isCallArgsink := sink.Node.(*dataflow.CallNodeArg); isCallArgsink {
-		sinkPos = callArg.ParentNode().Position(s)
+	s.Logger.Infof("  ┌──────────────────────────────")
+	s.Logger.Infof("  │ 💀 Tainted data flows to sink:")
+	s.Logger.Infof("  │ Data from source %s\n", formatutil.Green(source.Node.String()))
+	s.Logger.Infof("  │   ↳ Position: %s\n", source.Node.Position(s))
+	s.Logger.Infof("  │ Reached sink %s\n", formatutil.Red(sink.Node.String()))
+	s.Logger.Infof("  │   ↳ Position: %s\n", sink.Node.Position(s))
+	if s.Config.ReportPaths {
+		s.Logger.Infof("  ├──────────────────┄")
+	} else {
+		s.Logger.Infof("  └──────────────────┄")
 	}
+
 	if s.Config.ReportPaths {
+		s.Logger.Infof("%s", formatutil.Blue("  ├ TRACE:"))
+		sinkPos := sink.Node.Position(s)
+		if callArg, isCallArgsink := sink.Node.(*dataflow.CallNodeArg); isCallArgsink {
+			sinkPos = callArg.ParentNode().Position(s)
+		}
 		nodes := []*dataflow.VisitorNode{}
 		cur := sink
 		for cur != nil {
@@ -97,19 +106,22 @@ func logTaintFlow(s *dataflow.State, source dataflow.NodeWithTrace, sink *datafl
 			if nodes[i].Status.Kind != dataflow.DefaultTracing {
 				continue
 			}
-			s.Logger.Infof("%s - %s",
-				formatutil.Blue("  |- TRACE"),
+			s.Logger.Infof("%s %s",
+				formatutil.Blue("  ├───┬"),
 				dataflow.TermNodeSummary(nodes[i].Node))
 			// - Context [<calling context string>] Pos: <position in source code>
-			s.Logger.Infof("%s - Context [%s]\n",
-				"  |  ",
+			s.Logger.Infof("%s %s Context [%s]\n",
+				formatutil.Faint("  │  "),
+				formatutil.Blue("╞"),
 				dataflow.FuncNames(nodes[i].Trace, s.Logger.LogsDebug()))
-			s.Logger.Infof("%s - %s %s\n",
-				"  |  ",
+			s.Logger.Infof("%s %s %s %s\n",
+				formatutil.Faint("  │  "),
+				formatutil.Blue("└"),
 				formatutil.Yellow("At"),
 				nodes[i].Node.Position(s).String())
 		}
-		s.Logger.Infof("  ⎣ ENDS WITH SINK: %s\n", sinkPos.String())
+		s.Logger.Infof("  │ ENDS WITH SINK: %s\n", sinkPos.String())
+		s.Logger.Infof("  └──────────────────┄")
 		s.Logger.Infof(" ")
 	}
 }

cmd/argot/backtrace/backtrace.go

@@ -77,7 +77,7 @@ func Run(flags tools.CommonFlags) error {
 			ApplyRewrites: true,
 		}
 
-		c := config.NewState(cfg, targetName, target.Files, loadOptions)
+		c := config.NewState(cfg, targetName, target.Patterns, loadOptions)
 		var actual result.Result[config.State]
 		if target.UseProgramTransforms && len(target.ReflectValueCallInstances) >= 1 {
 			c.Logger.Infof("Reflect value call instances specified. Tool supports only 1 for now, will use the first.")
@@ -87,7 +87,7 @@ func Run(flags tools.CommonFlags) error {
 		} else {
 			actual = result.Ok(c)
 		}
-		c.Logger.Infof("Backtrace analysis of target \"%s\" = %v", targetName, target.Files)
+		c.Logger.Infof("Backtrace analysis of target \"%s\" = %v", targetName, target.Patterns)
 		c.Logger.PushContext(formatutil.Faint(targetName))
 		ptrState := result.Bind(result.Bind(actual, loadprogram.NewState), ptr.NewState) // build pointer analysis info
 		state, err := result.Bind(ptrState, dataflow.NewState).Value()