diff --git a/cmd/internal-tools/defect-identity/identity.go b/cmd/internal-tools/defect-identity/identity.go index cd138f7..a6ba82c 100644 --- a/cmd/internal-tools/defect-identity/identity.go +++ b/cmd/internal-tools/defect-identity/identity.go @@ -6,6 +6,7 @@ import ( "github.com/priyanshujain/sanderling/internal/trace" "github.com/priyanshujain/sanderling/internal/tracecorpus" + "github.com/priyanshujain/sanderling/internal/verifier" ) // Instance is one defect as the draft identifies it across runs: the property @@ -23,6 +24,11 @@ type Instance struct { // run count only if one run reported the same property twice, and the // latch says it cannot. Reports int `json:"reports"` + // RedactedOrigin marks a row whose origin action reached the trace with its + // typed value redacted, so `full` keyed it by selector instead. Two runs + // that typed different values into that field are one row here, which makes + // the count of such rows a floor rather than a total. + RedactedOrigin bool `json:"redacted_origin,omitempty"` } // Unattributed is a violation that carries no origin, so the identity rule @@ -76,6 +82,18 @@ func (c Corpus) Singletons() int { return count } +// DegradedIdentities counts instances the action key could not be computed for +// in full, which are the rows a reader has to treat as a lower bound. +func (c Corpus) DegradedIdentities() int { + count := 0 + for _, instance := range c.Instances { + if instance.RedactedOrigin { + count++ + } + } + return count +} + func identify(runs []tracecorpus.Run, mode actionKeyMode) (Corpus, error) { corpus := Corpus{Runs: len(runs)} byKey := map[identityKey]*Instance{} @@ -111,17 +129,19 @@ func identify(runs []tracecorpus.Run, mode actionKeyMode) (Corpus, error) { if detected.Screen == "" { corpus.UnnamedScreen++ } + action, redactedOrigin := actionKey(origin, mode) key := identityKey{ property: property, - action: actionKey(origin, mode), + action: action, screen: detected.Screen, } instance, seen := byKey[key] if !seen { instance = &Instance{ - Property: property, - OriginAction: key.action, - WitnessScreen: key.screen, + Property: property, + OriginAction: key.action, + WitnessScreen: key.screen, + RedactedOrigin: redactedOrigin, } byKey[key] = instance order = append(order, key) @@ -162,15 +182,21 @@ func index(steps []trace.Step) map[int]trace.Step { return byIndex } -// actionKey renders the action the origin step chose. The action recorded on a -// line is the one applied after observing it, which is the alignment that -// makes an origin index name an action at all. -func actionKey(origin trace.Step, mode actionKeyMode) string { +// actionKey renders the action the origin step chose, and reports whether the +// key had to be degraded to the selector. The action recorded on a line is the +// one applied after observing it, which is the alignment that makes an origin +// index name an action at all. +// +// A typed value the record redacted is the same string for every value typed +// into that field, so keying on it would merge distinct actions while reading +// as a whole-action key. The key drops it and says it did, because an identity +// that cannot be computed has to show as an undercount rather than as a count. +func actionKey(origin trace.Step, mode actionKeyMode) (string, bool) { if origin.NextAction == nil { - return "none" + return "none", false } if origin.ActionSkipped != "" { - return "none (" + origin.ActionSkipped + ")" + return "none (" + origin.ActionSkipped + ")", false } action := *origin.NextAction key := action.Kind @@ -183,8 +209,11 @@ func actionKey(origin trace.Step, mode actionKeyMode) string { key += fmt.Sprintf(" (%d,%d)", action.X, action.Y) } if mode != byFullAction { - return key + return key, false + } + if action.Text == verifier.RedactedInputText { + return key + " text=redacted", true } return fmt.Sprintf("%s text=%q at=(%d,%d)->(%d,%d)", - key, action.Text, action.X, action.Y, action.ToX, action.ToY) + key, action.Text, action.X, action.Y, action.ToX, action.ToY), false } diff --git a/cmd/internal-tools/defect-identity/identity_test.go b/cmd/internal-tools/defect-identity/identity_test.go index a6804ec..c050d8d 100644 --- a/cmd/internal-tools/defect-identity/identity_test.go +++ b/cmd/internal-tools/defect-identity/identity_test.go @@ -1,10 +1,12 @@ package main import ( + "strings" "testing" "github.com/priyanshujain/sanderling/internal/trace" "github.com/priyanshujain/sanderling/internal/tracecorpus" + "github.com/priyanshujain/sanderling/internal/verifier" ) // TestOneDefectSeenTwiceIsOneInstance: two runs report the same property from @@ -109,6 +111,74 @@ func TestTheStrictActionKeySplitsWhatTheSelectorKeyMerges(t *testing.T) { } } +// TestARedactedTypedValueDegradesTheFullKeyVisibly: two runs typed different +// values into one field, both reached the trace redacted, and the whole action +// can no longer tell them apart. The pair is one row, and the report has to say +// so rather than let it read as one defect found twice. +func TestARedactedTypedValueDegradesTheFullKeyVisibly(t *testing.T) { + first := run(t, 3, violating(1, "/login", + typing("id:password", recordedText(t, "hunter2")), "staysSignedIn", 1, 1)) + second := run(t, 5, violating(1, "/login", + typing("id:password", recordedText(t, "correct horse")), "staysSignedIn", 1, 1)) + + corpus := identified(t, byFullAction, first, second) + if len(corpus.Instances) != 1 { + t.Fatalf("instances = %d, want the redacted pair to be one row: %+v", + len(corpus.Instances), corpus.Instances) + } + report := rendered(corpus) + if !strings.Contains(report, "1 identity") || !strings.Contains(report, "redacted") { + t.Fatalf("report does not say one identity rests on a redacted value:\n%s", report) + } +} + +func TestARedactedOriginKeepsTheSelectorApart(t *testing.T) { + first := run(t, 3, violating(1, "/login", + typing("id:password", recordedText(t, "hunter2")), "staysSignedIn", 1, 1)) + second := run(t, 5, violating(1, "/login", + typing("id:pin", recordedText(t, "hunter2")), "staysSignedIn", 1, 1)) + + corpus := identified(t, byFullAction, first, second) + if len(corpus.Instances) != 2 { + t.Fatalf("instances = %d, want two fields to stay two rows: %+v", + len(corpus.Instances), corpus.Instances) + } + if report := rendered(corpus); !strings.Contains(report, "2 identity") { + t.Fatalf("report does not count both degraded identities:\n%s", report) + } +} + +func TestARedactedOriginDegradesNothingUnderTheSelectorKey(t *testing.T) { + only := run(t, 3, violating(1, "/login", + typing("id:password", recordedText(t, "hunter2")), "staysSignedIn", 1, 1)) + + if report := rendered(identified(t, bySelector, only)); strings.Contains(report, "redacted") { + t.Fatalf("selector key reads no text, so nothing degrades:\n%s", report) + } +} + +// recordedText renders a typed value the way the runner records it, so what the +// key sees is redaction as it really happens and not a placeholder the test +// wrote itself. +func recordedText(t *testing.T, typed string) string { + t.Helper() + recorded := verifier.RecordedActionText(verifier.Action{ + Kind: verifier.ActionKindInputText, + On: "id:password", + Text: typed, + }, nil) + if recorded == typed { + t.Fatalf("typed value %q reached the record unredacted", typed) + } + return recorded +} + +func rendered(corpus Corpus) string { + var report strings.Builder + render(&report, corpus) + return report.String() +} + func identified(t *testing.T, mode actionKeyMode, runs ...tracecorpus.Run) Corpus { t.Helper() corpus, err := identify(runs, mode) diff --git a/cmd/internal-tools/defect-identity/main.go b/cmd/internal-tools/defect-identity/main.go index a341f35..f99d450 100644 --- a/cmd/internal-tools/defect-identity/main.go +++ b/cmd/internal-tools/defect-identity/main.go @@ -100,6 +100,13 @@ func render(out io.Writer, corpus Corpus) { fmt.Fprintf(out, "\n%d distinct defect(s) over %d run(s); %d seen in exactly one run\n", len(corpus.Instances), corpus.Runs, corpus.Singletons()) + if degraded := corpus.DegradedIdentities(); degraded > 0 { + fmt.Fprintf(out, + "%d identity(ies) rest on the origin selector alone, because the value typed "+ + "there is redacted in the record; two runs that typed different values into "+ + "that field read as one, so the count above is a floor for those\n", + degraded) + } if corpus.UnnamedScreen > 0 { fmt.Fprintf(out, "%d violation(s) witnessed on a screen the app does not name, "+