Workflow [PR], commit [e906759]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR substantially improves /schema's UI and closes the earlier cross-origin and async-staleness regressions, but the remembered-credentials flow is still not bound tightly enough to the actual backend URL. A login saved for one same-origin endpoint can be auto-replayed to a different same-origin endpoint on a later plain revisit, so I would still request changes.

Findings

❌ Blockers

• [programs/server/schema.html:2238] The remembered-login contract is still origin-scoped while /schema's backend contract is URL-scoped. storeCredentials accepts any same-origin #url, but a plain revisit resets #url to location.origin and autoLoadOnOpen reuses whatever PasswordCredential the browser returns without verifying that it belongs to the current backend. Concrete trace: load https://example.com/ch-proxy, save credentials, reopen plain /schema; the saved login can now be auto-posted to https://example.com/?... instead of /ch-proxy.
  Suggested fix: either persist credentials only for the exact default target, or record/check the saved target URL and abort auto-load unless it exactly matches the current #url.

Tests

• ⚠️ Add a focused DOM-stub regression for the remembered-login flow: save a credential for a same-origin non-root URL, reopen plain /schema, and assert that autoLoadOnOpen does not reuse that credential for location.origin.

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions: bind remembered credentials to the exact backend URL before auto-reusing them, and add a regression for the same-origin non-root revisit case.

Addressed the security finding in 4c4380e.

The auto-load on page load is now gated on the target being same-origin (isSameOriginTarget). Opening /schema?url=https://attacker.example no longer issues any request on load — a cross-origin target requires an explicit Load click, so password-manager-filled credentials for the trusted page origin are never auto-forwarded to an attacker-chosen server. The no-?url and explicit same-origin ?url cases still auto-load as before.

Added focused harness coverage for this path (a cross-origin ?url makes zero requests on page load, an explicit Load does issue the request, and the same-origin default still auto-loads).

Addressed the AI review (Request changes) credential-probe race in fedcf27774c.

checkCredentials now snapshots the url / user / password the probe is testing and requires them — and the probe sequence number — to still be current before calling setCredentialStatus. chQuery reads those fields synchronously when it builds the request, so the snapshot is exactly what the probe queried. A SELECT version() result that arrives after the user edits a field, or after loadAll runs with different credentials, can therefore no longer repaint the now-stale ok/fail status onto the current fields. Verified with a DOM-stub harness reproducing the race (a stale ok/fail probe after a field edit no longer paints, while a matching probe and the newest of two concurrent probes still paint as before).

Also merged master (0c8d84e54fd).

The two red checks are pre-existing flakes unrelated to this HTML-only change:

• 04402_map_functions_lowcardinality — a master-wide map-key-ordering flake under randomized map_buckets settings (fails on master and across unrelated PRs); a fix is already in progress in Fix flaky test 04402_map_functions_lowcardinality #108677.
• Hung check failed, possible deadlock found (arm_msan stress) — tracked by Hung check failed, possible deadlock found #107941.

Addressed the AI review (Request changes) follow-up on the credential-status invariant in 61460e39635.

The remaining two stale-status paths are now closed:

1. URL change no longer leaves a stale green. The input listener that calls resetCredentialsStatus was extended from ['user', 'password'] to ['url', 'user', 'password'], so editing the target URL after a successful probe clears the credential status — it is no longer green for an unvalidated target.

2. loadAll is guarded. The snapshot logic from checkCredentials is factored into a shared credentialSnapshot. loadAll captures the snapshot before its awaited chQuery and only calls setCredentialStatus/storeCredentials (success) or setCredentialStatus(false) (failure) while the current url/user/password still match the values that were loaded. chQuery reads those fields synchronously when building the request, so the snapshot is exactly what was queried.

This makes every credential-status update and credential store conditional on the current fields matching the values that were actually probed or loaded — the verdict's minimum required action.

Verified with a DOM-stub harness over the extracted functions: a URL edit clears a green probe; a loadAll whose target changed mid-load neither paints nor stores; a matching loadAll paints and stores; the stale-probe and newer-probe-supersedes invariants still hold.

Also merged master (5e59dec4851, clean — master does not touch schema.html).

The one red check, 00409_shard_limit_by, is an unrelated pre-existing timeout flake: it failed 3× directly on master (PR=0) on 2026-06-17 with the same Timeout context, and this is an HTML-only change that cannot affect a LIMIT BY SQL test. The fresh push re-runs CI.

Addressed the AI review (Block) cross-origin credential-leak finding on the URL paste path in abf020a8e47.

The paste listener on the url/user/password fields scheduled checkCredentials unconditionally, and checkCredentials → chQuery reads $('url').value as the host and forwards user/password to it. So a trusted /schema page with password-manager-filled credentials would POST SELECT version() with those credentials to a freshly pasted https://attacker.example before any explicit Load, bypassing the isSameOriginTarget gate added for page-load auto-load.

The paste probe is now gated on isSameOriginTarget($('url').value), mirroring the page-load invariant: a cross-origin target is contacted only after an explicit Load/Enter, while same-origin targets keep live credential validation on paste. Verified with a DOM-stub harness over the extracted functions: pasting a cross-origin URL (or pasting into user/password while the URL is cross-origin) issues zero requests, while same-origin and relative/empty (default /schema) targets still issue exactly one probe.

The one red check, Hung check failed, possible deadlock found (arm_tsan stress), is the known master-wide flake tracked by #107941 — the hung stacks are Poco TCP-server threads under the AST fuzzer, and this is an HTML-only change that cannot affect a server-side deadlock.

Addressed the AI review (Block) in-flight cross-origin credential-leak finding on the loadAll batch in 250f826666e.

The earlier commits gated the page-load auto-load (4c4380e3bc8) and the paste probe (abf020a8e47) on isSameOriginTarget, but an already-running same-origin load batch was still steerable mid-flight: chQuery re-read the live url/user/password on every call, so a URL pasted into the field between two of the batch's awaited queries (columnsQ, dictsQ, refreshesQ, or the loadViewsLoad query) would POST the password-manager-filled credentials to the freshly pasted, possibly cross-origin, server without an explicit Load.

loadAll now captures the connection once via connectionSnapshot and passes that snapshot to every chQuery in the batch through the new chQuery(query, conn) parameter, so the whole load is bound to the target the user committed to — a mid-load field change can no longer redirect later queries or their credentials to a different origin. loadViewsLoad reuses the batch's captured connection (and snapshots its own when invoked interactively from the lookback selector); chQuery reads the live fields only for the single-shot credential probe. The isCurrent predicate still gates the credential-status paint/store, so a stale result cannot repaint the form.

This is the verdict's minimum required action: every query in one loadAll batch is now bound to the same captured connection values.

Verified with a DOM-stub harness over the page's own loadAll/chQuery: a same-origin load whose url is changed to a cross-origin target after the first query issues zero requests to the new origin and paints no stale credential status, whereas the pre-fix code leaked the three remaining batch queries to the attacker origin. (As with the other programs/server/*.html web UIs, there is no committed CI runner for these DOM-stub checks, so the harness is run locally.)

Addressed the AI review (Request changes) follow-up on the async load race in eb1f10f4d3b.

The earlier connectionSnapshot commit (250f826666e) bound a batch's requests and credential status to one captured connection, but loadAll still applied its results unconditionally: a same-origin auto-load could start, the user could edit url / credentials and press Load, and if the newer load rendered first the older batch's awaited queries could still return and overwrite state.tables, the status text, the query_views_log heat-map, and the rendered graph with data from the superseded connection.

The fix takes the verdict's monotonic-load-id option. A new loadGeneration is bumped by each loadAll; the load captures the generation it started in and applies its result (state, status, heat-map, render) only while it is still the latest one — a superseded batch returns before touching state or rendering. loadViewsLoad reuses the caller's generation from loadAll, and reads the current one (without bumping it) for a standalone lookback reload, so its heat-map application is gated the same way while a standalone reload never cancels an in-flight loadAll.

Verified with a DOM-stub harness over the page's own loadAll / loadViewsLoad / chQuery (the test the verdict asks for): two overlapping loads where the first connection goes stale before it applies — the first load updates neither state nor the graph after the newer load wins; a load superseded during its heat-map query does not overwrite the newer heat-map; and a single un-raced load still applies and renders normally. The same harness against the pre-fix code does clobber state.tables back to the superseded connection and wipes the newer heat-map, so the check is meaningful. (As with the other programs/server/*.html web UIs there is no committed CI runner for these DOM-stub checks, so the harness is run locally.)

The three red checks are pre-existing flakes unrelated to this single-file HTML change (programs/server/schema.html, +245/-118 — it cannot affect a C++ unit test or server-side mutations):

• FunctionsStress.stress / AllTests (Unit tests, msan function_prop_fuzzer) — a master-wide function-property-fuzzer flake (a reinterpret(CAST(... AS DateTime64(2)), 'DateTime64(3)') const-folding mismatch). On 2026-06-30 it failed across ~50 PRs and master itself (pull_request_number = 0); tracked by FunctionsStress #107242.
• Upgrade check (amd_release) — Error message in clickhouse-server.log: a throwIf-based test mutation logs FUNCTION_THROW_IF_VALUE_IS_NON_ZERO from MutatePlainMergeTreeTask; a master-wide upgrade-check flake that hit 32 PRs on 2026-06-30.

The fresh push re-runs CI.

@groeneai, provide a fix for https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108724&sha=48e7880e5fbbdf289911c01ba3f856af22d85f64&name_0=PR&name_1=Stress%20test%20%28arm_asan_ubsan%29

Addressed the AI review (Request changes) follow-up on the async heat-map race (programs/server/schema.html:1055) in a5bc6caac14.

loadGeneration (eb1f10f4d3b) guards a whole loadAll batch, but the system.query_views_log heat-map is also reloaded standalone on a lookback change, and that path reused the current loadGeneration without a token of its own — so two heat-map reloads could share a generation and both pass isLatestLoad.

The fix adds a monotonic viewsLoadGeneration bumped by every loadViewsLoad (both the loadAll-driven and the standalone one). A reload is the latest only while both its loadGeneration and its viewsLoadGeneration are still current, so a stale batch neither resets nor applies state.loadByMv / loadByEdge / loadMax. Because state.nodes is only ever replaced by a loadAll that then runs its own loadViewsLoad (bumping the token), "the graph changed under me" is exactly "the token moved", so the token catches both flagged races: a standalone reload superseded by a same-generation loadAll graph swap, and two quick lookback changes where the older query returns last. The map reset moves behind the isLatestLoad guard so a superseded reload leaves the current heat-map intact instead of blanking it (the unavailable/disabled paths still clear it, gated the same way).

Verified with a DOM-stub harness over the page's own extracted loadViewsLoad (the test the verdict asks for): a standalone reload superseded by a same-generation graph swap no longer leaks the old-graph key (only the new node's heat-map remains), and two quick lookback changes keep the newest result while the older query is dropped. The same harness against the pre-fix code does leak the stale key and corrupt the newer heat-map, so the guard is meaningful. As with the other programs/server/*.html web UIs there is no committed CI runner for these DOM-stub checks, so the harness is run locally.

The one red on the previous head, Stress test (arm_asan_ubsan) / AddressSanitizer (STID: 3243-1df9), is an unrelated pre-existing flake: it is an allocation-size-too-big inside MsgPackSchemaReader::readObject (src/Processors/Formats/Impl/MsgPackRowInputFormat.cpp) under a fuzzed schema-inference input — a member of the recurring stress-test large-allocation class (8 such stress failures across 7 distinct STIDs over 60 days), and this single-file HTML change cannot affect a C++ format reader. The fresh push re-runs CI.

@alexey-milovidov the Stress test (arm_asan_ubsan) red (AddressSanitizer (STID: 3243-1df9)) is a real, deterministic trunk bug, not a flake — and a fix is already open: #109019.

Root cause: MsgPackSchemaReader::readObject() (src/Processors/Formats/Impl/MsgPackRowInputFormat.cpp) unpacks with the default msgpack::unpack_limit() (cap 0xffffffff). msgpack-c's create_object_visitor eagerly malloc(count * sizeof) from a fuzzed array32/map32/str32/bin32/ext32 header before reading the payload, bypassing the CH memory tracker. An over-declared header requests a single ~128 GiB allocation — exactly the 0x2000000008 in the report.

Deterministic repro (any build):

printf '\xdd\xff\xff\xff\xff' | clickhouse local --input-format MsgPack --file - -q "desc file(stdin)"

\xdd = array32, \xff\xff\xff\xff = declared length 0xffffffff -> 0xffffffff * 8 + 8 = 0x2000000008.

#109019 bounds unpack_limit by the bytes currently buffered, so an over-declaration throws msgpack::size_overflow (routed through the existing grow-and-retry loop, rejected as UNEXPECTED_END_OF_FILE) before any eager alloc. Covers all 5 eager-alloc paths (array/map/str/bin/ext); validated both directions on an ASan build; regression test 04366_msgpack_schema_inference_huge_container.

You're right that this PR (HTML-only) cannot cause it — the fix belongs in #109019, not here. CIDB: STID 3243-1df9 = 1 hit / 1 PR (this one) / 0 master in 60d.

The new Upgrade check (amd_release) red is unrelated to this HTML-only change — it is known master-side log noise, not a regression from this PR.

The flagged test is Error message in clickhouse-server.log, and the <Error> lines are remote-database-engine connection errors from a single stateless test:

<Error> mysqlxx::Pool: Failed to connect to MySQL (fake_db@192.0.2.1:3306 as user user): mysqlxx::ConnectionFailed
<Error> Application: Connection to mysql failed 1 times
<Error> DatabaseMySQL: Code: 279. ... ALL_CONNECTION_TRIES_FAILED

192.0.2.1 is an RFC 5737 test address; a DatabaseMySQL engine pointed at it logs <Error> connection retries, and the Upgrade check asserts the server log contains no <Error> lines after the upgrade restart. This is pervasive across master and unrelated PRs (the same test failed on 12 PRs today and 38 yesterday) and cannot be affected by a change to programs/server/schema.html.

It is already being fixed by an open PR that allowlists exactly these lines in tests/docker_scripts/upgrade_runner.sh: #108560 . Once that merges, a master re-merge here will clear the Mergeable Check.

The CH Inc sync red (private #62490, run 28500606703) is two unrelated Cloud-only integration-test flakes, not a regression from this HTML-only change (programs/server/schema.html):

• test_distributed_cache/test.py::test_idle_client_ttl — AssertionError: client2 cache was purged despite continuous activity (assert 0 > 0), a distributed-cache TTL timing flake. It passed on the in-run retry ([gw0] PASSED); the shard was still marked FAIL because this test is not in the flaky allowlist.
• test_refreshable_mv_no_multi_read/test.py::test_refreshable_mv_attach_without_multi_read — assert 'Disabled' in 'Scheduled'; the harness itself marked it flaky, matching issue #62697.

Neither a distributed-cache client TTL test nor a refreshable-MV attach test can be affected by a change to programs/server/schema.html. I re-ran the two failed shards (gh run rerun 28500606703 --failed) to clear the sync gate.

Re-merged `master` (7a74bdb83af) to clear the last red check.

The only failing check was Upgrade check (amd_release) / Error message in clickhouse-server.log, i.e. the pervasive DatabaseMySQL-connection log noise (mysqlxx::Pool: Failed to connect to MySQL (fake_db@192.0.2.1:3306 …)), unrelated to this HTML-only change. Its allowlist fix, #108560, merged into master at 03:19Z today and adds exactly these lines to tests/docker_scripts/upgrade_runner.sh:

| grep -av -e "mysqlxx::Pool.*Failed to connect to MySQL" \
| grep -av -e "Application: Connection to mysql failed" \
| grep -av -e "DatabaseMySQL.*Connections to mysql failed" \

which cover all three <Error> lines from the failure. master does not touch programs/server/schema.html, so the merge was conflict-free and the net diff is unchanged (only schema.html). This should turn the Upgrade check and the Mergeable Check green on the re-run.

Addressed the AI review (Request changes) credential-status freshness finding (programs/server/schema.html:776) in f1bcccfde75.

The remaining stale-status path is now closed. The credential status (the green/red on the user/password fields and the Connected to ... line) was keyed only by field equality: checkCredentials used a probe-only checkCredentialsNum, and loadAll gated its paint/store on conn.isCurrent() alone. Two operations against the same url/user/password both satisfy field equality, so an older callback could repaint the status after a newer one had already won — the finding's exact reproduction (start two loads for the same connection, let the newer succeed, then let the older fail; the page keeps Loaded 3 tables. while the stale callback flips the credential fields to fail).

The fix takes the verdict's shared-token option: checkCredentialsNum is replaced by a single monotonic credentialStatusGeneration bumped at the start of every operation that can repaint the credential status — the checkCredentials probe and each loadAll batch. Each captures the token and applies its paint/store only while it is still the latest one and the fields still hold the probed values (conn.isCurrent()), so the latest-started probe-or-load is the sole authority over the status. A superseded operation — even one on an identical connection — no longer repaints a stale ok/fail.

Verified with a DOM-stub harness over the page's own loadAll / checkCredentials / chQuery (the test the verdict asks for): the flagged load-vs-load race and the paste-probe-vs-newer-load race now both suppress the stale paint (the newer result stays), while the pre-existing invariants still hold — a single un-raced load still paints and reports, a field edit before a probe returns still suppresses the paint, and a newer probe still supersedes an older one. The same harness against the pre-fix code does flip the fields to fail on both races, so the check is meaningful. (As with the other programs/server/*.html web UIs there is no committed CI runner for these DOM-stub checks, so the harness is run locally.)

The two red checks are the known AST-fuzzer flake, unrelated to this single-file HTML change (programs/server/schema.html, which cannot affect a server-side query pipeline):

• Stress test (arm_tsan) / Block structure mismatch in A stream: different number of columns: (STID 0993-308a) and Stress test (arm_ubsan) (STID 0993-386d) — both are DB::JoinStep::updatePipeline assertBlocksHaveEqualStructure logical errors on fuzzed __table3.number joins, members of the recurring 0993-* "Block structure mismatch in A stream" AST-fuzzer class (tracked by Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-28b7) #107953, Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-27f0) #108142, Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-2cc2) #108210, Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-300c) #108332). No schema.html frames appear in either stack.

The fresh push re-runs CI.

Addressed the AI review (Request changes) blocker on the standalone heat-map reload (programs/server/schema.html:1074) in 2aa81014dfd.

loadGeneration / viewsLoadGeneration / credentialStatusGeneration bind a load's requests and status to one captured connection, but the system.query_views_log heat-map is also reloaded standalone when the lookback selector changes, and that path took conn = conn || connectionSnapshot() — with conn omitted it read the live form fields. So after a successful load a user could edit url to a cross-origin target and then merely change the lookback, without any explicit Load, and chQuery(q, conn) would POST user / password to that target. Even for a benign different server it overlays that server's query_views_log onto the graph still belonging to the previous load — the verdict's exact reproduction.

The fix takes the verdict's "bind to the last successfully loaded connection" option. A new loadedConnection records the connection that produced the current state.nodes; loadAll sets it the moment it commits its batch as the latest load (right after the isLatestLoad guard). A standalone loadViewsLoad now binds to loadedConnection instead of the live fields, so a lookback change re-queries only the server that produced the displayed graph. Contacting a newly-edited (possibly cross-origin) target still requires an explicit Load, which runs loadAll and re-commits both state.nodes and loadedConnection together. If no load has committed a connection yet it fails closed and contacts nothing (there is no graph to key a heat-map against anyway). The loadAll-driven path is unchanged — it still passes its own captured conn.

Verified with a DOM-stub harness over the page's own extracted loadViewsLoad / connectionSnapshot (the test the verdict asks for): after a same-origin load, editing url to an attacker origin and changing the lookback no longer contacts that origin — the reload queries the committed connection and keys the heat-map to the displayed graph; the loadAll-driven call (explicit conn) is unchanged; and a standalone reload with no committed connection contacts nothing. The same harness against the pre-fix code does POST to the attacker origin, so the check is meaningful. As with the other programs/server/*.html web UIs there is no committed CI runner for these DOM-stub checks, so the harness is run locally.

The fresh push re-runs CI.

Addressed the AI review (Request changes) credential-scoping blocker (programs/server/schema.html:718) in 159cc187245.

storeCredentials persisted the entered credentials via navigator.credentials.store for any target, including a cross-origin #url. PasswordCredential is scoped to the page origin, not to the backend named in #url (its name is a display label, not an isolation key), so an explicit cross-origin Load stored those credentials under /schema's own origin. On a later plain visit (no ?url=), #url defaults back to location.origin and the page auto-loads it, so the browser could autofill the remembered cross-origin credentials into the fields and that same-origin auto-load would POST them to this page's own server instead of the cross-origin one they belong to — the verdict's exact reproduction.

The fix takes the verdict's first option: storeCredentials now stores only when isSameOriginTarget($('url').value) — the same invariant that already gates the page-load auto-load and the paste probe. play.html can store unconditionally because it never auto-contacts a server on open; /schema does, so it must not persist a credential that its own auto-load could later misdirect.

Verified with a DOM-stub harness over the page's own storeCredentials / isSameOriginTarget (the regression the verdict asks for): a same-origin login is stored, a cross-origin login (both https://other and protocol-relative //other) is not, an unparseable #url fails closed, and the empty-password / no-PasswordCredential cases store nothing. The same harness against the pre-fix code does store the cross-origin credentials, so the check is meaningful. As with the other programs/server/*.html web UIs there is no committed CI runner for these DOM-stub checks, so the harness is run locally.

The fresh push re-runs CI.

Addressed the AI review's sole remaining blocker (the ⚠️ Request changes on the remembered-credentials flow, programs/server/schema.html:726) in 0dc76ad2005.

The play.html-style saved login now actually works on revisit:

• The connection inputs are wrapped in a <form> with name= attributes (as in play.html), so the browser can offer to save and later autofill them, and the form submit (Enter) loads the schema.
• Before the same-origin auto-load, when the fields are empty, the page retrieves the saved credential via navigator.credentials.get({ password: true, mediation: 'silent' }) and fills the fields, then loads — reusing the existing isSameOriginTarget gate so a cross-origin ?url= still neither retrieves a credential nor auto-loads, and mediation: 'silent' keeps the open non-interactive.

Verified with a DOM-stub harness over the page's own autoLoadOnOpen / isSameOriginTarget (27 checks), covering the stored-credential, no-credential, URL-supplied-credential, cross-origin, file:, no-PasswordCredential, and throwing-get() cases. The remaining CI reds are the known unrelated flakes (Server died / AddressSanitizer (STID: 0883-4595) #107487 and test_rabbitmq_mv_combo timeout), which an HTML-only change cannot cause.

Addressed the AI review's sole remaining blocker (the ⚠️ Request changes on the credential auto-load, programs/server/schema.html:2226) in e906759af92.

autoLoadOnOpen checked isSameOriginTarget and the empty user/password fields only before awaiting navigator.credentials.get. That retrieval is async, so while it was pending the user could paste a cross-origin URL into #url (or start typing their own credentials); the callback then unconditionally filled the saved same-origin login and called loadAll, which reads the live fields via connectionSnapshot. A same-origin login remembered by the Credential Management API could therefore be POSTed as user/password to a user-chosen (possibly attacker) target on page open — the same leak isSameOriginTarget guards against on this path.

The fix snapshots the URL up front and, after the retrieval, re-establishes both invariants (URL unchanged, fields still empty). If either changed, the whole auto-load is aborted — the user is now driving and will load explicitly via the form (Load / Enter). The re-check sits after the try/catch, so it also guards the path where navigator.credentials.get throws.