diff --git a/internal/driver/chrome/driver.go b/internal/driver/chrome/driver.go index a819351..97498ee 100644 --- a/internal/driver/chrome/driver.go +++ b/internal/driver/chrome/driver.go @@ -787,16 +787,39 @@ func (d *Driver) EvaluateExtractors(ctx context.Context) (map[int]json.RawMessag return nil, fmt.Errorf("decode extractor map: %w", err) } result := make(map[int]json.RawMessage, len(stringMap)) - for key, value := range stringMap { + for key, entry := range stringMap { index, err := strconv.Atoi(key) if err != nil { return nil, fmt.Errorf("non-integer extractor key %q", key) } - result[index] = value + reading, err := extractorReading(entry) + if err != nil { + return nil, fmt.Errorf("extractor %d: %w", index, err) + } + result[index] = reading } return result, nil } +// extractorReading unwraps one entry of the page's extractor table. The page +// wraps every reading in a {"value": ...} envelope (evaluateExtractors in +// pkg/spec/src/web-runtime.ts) because JSON has no undefined: an absent `value` +// is the getter returning undefined, and returning it as an empty payload is +// what makes the goja host record undefined too. Reading it as JSON null would +// claim the getter returned null, so `x.current === undefined` would answer one +// thing on native and another on web. +func extractorReading(entry json.RawMessage) (json.RawMessage, error) { + var envelope struct { + Value json.RawMessage `json:"value"` + } + if err := json.Unmarshal(entry, &envelope); err != nil { + return nil, fmt.Errorf( + "reading %s is not a {\"value\"} envelope; the page and the host are "+ + "running different bundles: %w", entry, err) + } + return envelope.Value, nil +} + // SetLastAction installs the previous step's action as state.lastAction inside // the page runtime. The page cannot derive it: only the runner knows which // action was actually applied. Without this call every web state.lastAction is diff --git a/internal/driver/chrome/driver_test.go b/internal/driver/chrome/driver_test.go index 234223f..36ef3fb 100644 --- a/internal/driver/chrome/driver_test.go +++ b/internal/driver/chrome/driver_test.go @@ -9,6 +9,7 @@ import ( "net" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -772,7 +773,7 @@ func TestEvaluateExtractors_WaitsOutARouteTransition(t *testing.T) { const root = document.getElementById("app").attachShadow({mode: "open"}); root.innerHTML = '
ledger
'; window.__sanderlingExtractors__ = function () { - return {0: Array.from(root.querySelectorAll('[id$="Screen"]')).map(e => e.id).join(",")}; + return {0: {value: Array.from(root.querySelectorAll('[id$="Screen"]')).map(e => e.id).join(",")}}; }; window.startTransition = function () { const incoming = document.createElement("div"); @@ -885,3 +886,82 @@ func TestEvaluateExtractors_ReportsAMissingTable(t *testing.T) { "extractor table; a page that cannot be read must not read as empty", values) } } + +// TestEvaluateExtractors_KeepsUndefinedApartFromNull covers the wire the page's +// readings cross. JSON has no undefined, so the web runtime wraps each reading +// in a {value} envelope: written straight into the table, an extractor that +// returned undefined lost its whole index to JSON.stringify and the host kept +// goja's dump-derived reading for it while the rest held the page's. An absent +// value has to arrive as an empty payload, which is what makes the verifier +// record undefined (the value the native host records for the same getter); +// arriving as JSON null would claim the getter returned null. +func TestEvaluateExtractors_KeepsUndefinedApartFromNull(t *testing.T) { + const page = `` + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html") + _, _ = w.Write([]byte(page)) + })) + defer server.Close() + + d := New() + defer d.Terminate(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := d.Launch(ctx, server.URL, false, nil); err != nil { + t.Fatalf("Launch: %v", err) + } + + values, err := d.EvaluateExtractors(ctx) + if err != nil { + t.Fatalf("EvaluateExtractors: %v", err) + } + if len(values) != 3 { + t.Fatalf("the page reported 3 readings, %d survived the wire: %v", len(values), values) + } + if got := values[0]; len(got) != 0 { + t.Errorf("the undefined reading arrived as %s, want an empty payload; "+ + "the verifier records anything else as a value the getter never returned", got) + } + if got := string(values[1]); got != "null" { + t.Errorf("the null reading arrived as %s, want null", got) + } + if got := string(values[2]); got != `{"balance":7}` { + t.Errorf("the object reading arrived as %s, want {\"balance\":7}", got) + } +} + +// TestEvaluateExtractors_RejectsAnUnenvelopedReading is the loud failure a page +// running an older @sanderling/spec produces. Its readings are bare values, and +// a bare value is indistinguishable from a reading whose getter returned that +// value, so accepting them silently puts the two engines on different bundles. +func TestEvaluateExtractors_RejectsAnUnenvelopedReading(t *testing.T) { + const page = `` + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html") + _, _ = w.Write([]byte(page)) + })) + defer server.Close() + + d := New() + defer d.Terminate(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := d.Launch(ctx, server.URL, false, nil); err != nil { + t.Fatalf("Launch: %v", err) + } + + values, err := d.EvaluateExtractors(ctx) + if err == nil { + t.Fatalf("EvaluateExtractors accepted %v from a page whose readings are not "+ + "enveloped; the page and the host are running different bundles", values) + } + if !strings.Contains(err.Error(), "different bundles") { + t.Errorf("EvaluateExtractors failed with %q, want it to name the bundle mismatch", err) + } +} diff --git a/pkg/spec/src/web-runtime.ts b/pkg/spec/src/web-runtime.ts index eac9ff3..244bc34 100644 --- a/pkg/spec/src/web-runtime.ts +++ b/pkg/spec/src/web-runtime.ts @@ -501,9 +501,18 @@ function defineLockedGlobal(name: string, value: unknown): void { }); } -function evaluateExtractors(): Record { +// Each reading is wrapped in a {value} envelope because JSON has no undefined. +// Written straight into the map, an extractor whose getter returned undefined +// (folio's on(route, tag) off its own screen, which is most extractors on most +// steps) had its whole INDEX dropped by JSON.stringify, and the host kept goja's +// dump-derived reading for it while the rest held the page's. Inside the +// envelope the same drop means "this getter returned undefined", which is what +// the goja host records for the same getter; a JSON null would instead claim it +// returned null, and `x.current === undefined` would answer differently on the +// two hosts. +function evaluateExtractors(): Record { const state = buildState(); - const result: Record = {}; + const result: Record = {}; for (let i = 0; i < extractors.length; i++) { const entry = extractors[i]; if (!entry) continue; @@ -520,7 +529,7 @@ function evaluateExtractors(): Record { extracting = false; } entry.currentValue = value; - result[i] = sanitize(value); + result[i] = { value: sanitize(value) }; } return result; } diff --git a/pkg/spec/test/web-runtime.test.ts b/pkg/spec/test/web-runtime.test.ts index 0e80c5b..a5ac7e6 100644 --- a/pkg/spec/test/web-runtime.test.ts +++ b/pkg/spec/test/web-runtime.test.ts @@ -206,6 +206,13 @@ function withState(run: () => void) { } } +// Every reading leaves the runtime inside a {value} envelope, so an extractor +// whose getter returned undefined keeps its index instead of being dropped by +// JSON.stringify. +function readingOf(values: Record, index: number): unknown { + return values[index]!.value; +} + test("named() sets the extractor's display name", () => { const handle = __testing__.runtime.extract(() => "home").named("route"); const entry = __testing__.extractors.find((e) => e.handle === handle); @@ -251,6 +258,33 @@ test("an uncaught cross-extractor read aborts evaluateExtractors", () => { ); }); +// JSON.stringify drops an undefined-valued key, so a reading written straight +// into the table took the extractor's whole INDEX with it when the getter +// returned undefined - folio's on(route, tag) off its own screen, which is most +// of its extractors on most steps. The host then kept goja's dump-derived value +// for those and the page's for the rest, and a property comparing previous to +// current across that split convicts an app that did nothing wrong. +test("an extractor that returned undefined keeps its index through JSON", () => { + __testing__.extractors.length = 0; + __testing__.runtime.extract(() => undefined); + __testing__.runtime.extract(() => null); + __testing__.runtime.extract(() => 5); + let table: Record = {}; + withState(() => { + table = __testing__.evaluateExtractors(); + }); + + const overTheWire = JSON.parse(JSON.stringify(table)) as Record; + assert.deepEqual(Object.keys(overTheWire), ["0", "1", "2"]); + // undefined and null have to stay distinguishable across the wire: the goja + // host records undefined for a getter that returned undefined, so reporting + // null instead would make `x.current === undefined` answer one thing on + // native and another on web. + assert.equal("value" in overTheWire["0"]!, false); + assert.equal(overTheWire["1"]!.value, null); + assert.equal(overTheWire["2"]!.value, 5); +}); + // state.lastAction is the one piece of state the page cannot observe for // itself: only the runner knows which action it actually applied. While the web // runtime hardcoded null there, a spec property gated on the last action (e.g. @@ -262,12 +296,12 @@ function lastActionSeenByASpec(pushed: unknown): unknown { .__sanderlingSetLastAction__ as (value: unknown) => void; __testing__.extractors.length = 0; __testing__.runtime.extract((state) => (state as { lastAction: unknown }).lastAction); - let out: Record = {}; + let out: Record = {}; withState(() => { setLastAction(pushed); out = __testing__.evaluateExtractors(); }); - return out[0]; + return readingOf(out, 0); } test("state.lastAction carries the action the host pushed", () => { @@ -289,11 +323,11 @@ test("state.lastAction is null when the host pushed nothing", () => { function sanitizeViaExtract(value: unknown): unknown { __testing__.extractors.length = 0; __testing__.runtime.extract(() => value); - let out: Record = {}; + let out: Record = {}; withState(() => { out = __testing__.evaluateExtractors(); }); - return out[0]; + return readingOf(out, 0); } test("sanitize breaks a self-referential cycle instead of overflowing", () => { @@ -471,7 +505,7 @@ test("ax.findAll resolves a selector path segment by segment", () => { const values = __testing__.evaluateExtractors(); // Scoped to the head match: the cards come from the HomeScreen node, not // from a document-wide sweep for AccountCard. - assert.deepEqual(values[0], ["first", "second"]); + assert.deepEqual(readingOf(values, 0), ["first", "second"]); } finally { g.document = originalDocument; g.window = originalWindow; @@ -506,11 +540,11 @@ test("ax.find and ax.findAll label the element with its selector", () => { return ax.findAll({ testTag: "TxnSubmit" }); }); const values = __testing__.evaluateExtractors(); - const found = values[0] as Record; + const found = readingOf(values, 0) as Record; assert.equal(found.__sanderlingSelector, "testTag:TxnSubmit"); // findAll passes each element through map(); passing the callback by // reference would hand the array INDEX to the runtime as the selector. - const all = values[1] as Record[]; + const all = readingOf(values, 1) as Record[]; assert.equal(all[0]!.__sanderlingSelector, "testTag:TxnSubmit"); } finally { g.document = originalDocument;