feat(cua-driver): add stable AX node identity tokens - #2210
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@f-trycua Could you please review this stable AX node identity PR? It follows the same safety boundary as #2166, where explicit page identity must be preserved and wrong-target execution must fail closed. This PR adds opaque Validation completed locally:
This is also the blocking dependency for Maka PR #984, which consolidates the downstream target-identity fixes. The downstream Maka adapter is intentionally fail-closed until this opaque-token protocol is available. Thanks! |
28bf19e to
ab2b704
Compare
|
Update after rebasing onto cua-driver-rs 0.8.1 (
The fork workflows are currently waiting for maintainer approval to run. Could a maintainer approve the workflows and review this PR? This is the remaining release dependency for Maka #984. |
|
Adversarial review found and fixed two protocol gaps in the previous revision. The latest head |
|
I reviewed the current head,
Local verification on this head passed:
The linked update's core test count is stale for the current head, and GitHub currently reports no checks for this branch. |
|
Addressed all three review blockers in
Validation on
The full stable Opaque-token/replay audit remains fail-closed: tokens are UUID v4 capabilities with server-side bindings, a new registry instance rejects pre-restart tokens as unknown, and a newer snapshot rejects the old same-window token as stale before any cache index can be reused. |
eeb24aa to
c40481b
Compare
|
Rebased the branch onto the latest upstream New base/head:
Conflict resolution summary:
Post-rebase validation:
The branch was pushed with an explicit lease against prior head |
c40481b to
0ed2b19
Compare
|
Closing. Everything in here that stands on its own has been sent separately, and what remains is not worth another rebase. Merged from the split:
Open from the split:
Point 1 of your review (empty snapshots panicking Deliberately not carried forward, with reasons in the PR body above: the UUID token format (conflicts with the documented 8–16 character greppable goal), What is genuinely left undone: the residual TOCTOU between Thanks for the reviews on #2608 and #2621 — splitting this up was the right call. |
Split status
set_valueAXValue read-back withverified/changedBoth are single-file, independent of each other, and independent of the token work below.
Re-scoped after review of upstream
mainTwo items I originally listed for extraction turned out not to be upstream defects at all, and I'm not proposing them as standalone fixes:
token_forreturningOptioninstead of panicking. Upstream'stoken_foris a pure formatting helper — it does not consult the registry and cannot panic. The.expect("token requested for an unregistered snapshot element")I was pointing at was introduced by this PR's own registry rework, not by upstream. It is an internal correctness issue of the mechanism proposed here, not a bug to fix inmain.Element-action TOCTOU. The race you identified in review —
— applies to the token-resolution path this PR adds. Upstream
ax/cache.rshas onlyupdate(pid, window_id, nodes); there is no generation tracking and noresolve_token, so there is nothing inmainto harden independently. This belongs with whatever token design lands, not before it.I'd rather say this explicitly than ship two PRs that claim to fix upstream bugs which aren't there.
Deliberately dropped from the split
Three parts of this PR should not land in their current form:
Opaque UUID tokens. Replacing the token body with a UUID v4 conflicts with the format goals already documented in
element_token.rs("8-16 char budget", "debug-grep-able without a side-table"), and at least one user in #2207 types tokens by hand on the command line. Replay resistance across daemon restarts is better served by a per-process nonce that keeps the readable shape, e.g.s{nonce}-{generation}:{index}.allow_pixel_fallback(via_token) = !via_token. This couples addressing to dispatch strategy, which are orthogonal. cua-driver's architecture is AX-first with a pixel ladder; a token should make "which element" more precise, not change "how it is delivered". The requirement it encoded ("never fall back to pixels") belongs in the downstream client, which can already enforce it by rejectingstructuredContent.path != "ax". If a driver-side switch is still wanted it should be an explicit opt-in parameter that leaves current behaviour unchanged by default, proposed separately.node_identity. The value stored waselement_ptr as u64— the same pointer the cache already holds. A fresh AX walk yields a newAXUIElementRef, so it cannot survive a re-snapshot and does not deliver what the PR title claimed. A real stable identity needsCFEqual/AXIdentifier(both already used inax/tree.rs), and that deserves its own change with its own justification.What remains for the token mechanism itself
The remaining pieces (opaque-capability tokens, generation binding, typed rejection codes) change a public contract, so per
rfcs/README.mdthey should go through an RFC issue rather than arriving as a PR. I'll open one rather than re-pushing this branch.Original description
Expand
Hardens the existing cua-driver
element_tokenprotocol on macOS without introducing a second token abstraction: keeps the cross-platformTokenRegistryas the single token source, widens the generation, attaches macOS AX node identity to the snapshot entry, binds token use to PID / window / generation / index / node identity, returns typed fail-closed error codes, retains the validated AX node forclickandset_value, and performsset_valuewrite/read-back on that same node reporting idempotent writes aschanged:false.