Skip to content

Follow-ups from #592: loopback token budget key, focus-trap stacking order, focus after Add PC #595

Closed

Description

@devswha

Follow-ups from the review of #592 (lane rev-astra, review #592 (review)). None blocked the merge; recorded so they are not lost. Not reproduced with tests by the lead.

  1. Shared token budget keyed per loopback source address. After fix: pre-release review fixes from the second Codex pass #592 the shared budget is keyed on the real peer. A local process can connect from any address in 127.0.0.0/8, so rotating the source address still gets a fresh shared budget per address; sharedFailures is also an unbounded map. Consider one shared key for every loopback peer and a cap (or periodic sweep) on the map. Only reachable from processes already running on the PC.
  2. Focus-trap order follows mount order, not stacking order. openTraps in src/lib/useFocusTrap.ts takes the last mounted trap as the top one. A dialog mounted earlier but shown on top (z-index or a portal opened later) would lose Tab to one beneath it.
  3. No initial focus after Add PC closes over a trap. With a custom trap beneath a native showModal() dialog (fix: pre-release review fixes from the second Codex pass #592 lets the trap stand down meanwhile), closing Add PC does not put the focus back into the trap's surface when nothing restored it.

Activity

  1. devswha commented on Oct 9, 2026

    @devswha
    OwnerAuthor

    New finding on item 1, from trying it in #626 (closed): keying the shared budget on one "loopback" key is the wrong fix.

    • A process on this PC can leave X-Forwarded-For out. authClient then returns the bare loopback address, which has no shared key at all (server/auth.ts L132-L136, L160-L163), so rotating through 127.0.0.0/8 already gets a fresh per-address budget each time. That is true with or without the per-peer shared key, so the shared key is not what a local guesser goes around.
    • One key for all loopback peers lets 50 wrong tokens through one proxy hold every not-yet-admitted device behind every other proxy on the PC, tailscale serve included.

    So the options for item 1 are:

    1. Leave the rate limit as is and bound only the sharedFailures map (a cap or a sweep, as authAttempts already has). This stops the memory growth and changes nobody's access.
    2. A budget for all of this PC's own connections (bare loopback, any 127.x), separate from the forwarded share. This slows a local guesser, but a local process that can reach the port can usually read the token file anyway.

    My recommendation is 1. Items 2 and 3 (focus-trap order, focus after Add PC) are unchanged on v0.4.2 (src/lib/useFocusTrap.ts L49, L94-L96); not reproduced in a browser in this round.

  2. devswha commented on Oct 17, 2026

    @devswha
    OwnerAuthor

    Closing. Item 1 was fixed by #1059 (d0ca874): the shared failure bookkeeping is now capped. Items 2 and 3 (focus-trap stacking order, focus after Add PC) were checked in #1059 and did not reproduce in the shipped app. Please reopen with a reproduction if either shows up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions