Harden ID lookup against form control shadowing - #164
Closed
myabc wants to merge 9 commits into
Closed
Conversation
myabc
force-pushed
the
fix/realm-safe-active-element
branch
from
September 5, 2026 15:06
67c29fb to
d3f8623
Compare
`saveAndRestoreFocus` resolves the document that owns the focus by reading `ctx.target.ownerDocument`. `<form>` is [LegacyOverrideBuiltIns], so a named control such as `<input name="ownerDocument">` installs an own property on the form that shadows `Node.prototype.ownerDocument`. When the morph target is such a form, `doc` is an `HTMLInputElement`, `doc.activeElement` is `undefined`, and the function early-returns, so focus and selection restoration silently stop happening. Add an `ownerDocumentOf` helper that reads through the prototype getter, caching the descriptor once at module scope. The getter is also realm-safe: it brand-checks the internal slot rather than the realm.
myabc
force-pushed
the
fix/realm-safe-active-element
branch
from
September 5, 2026 15:09
d3f8623 to
72a4471
Compare
myabc
changed the title
myabc
marked this pull request as ready for review
September 5, 2026 16:14
There was a problem hiding this comment.
🟡 Changes recommended
There is still a shadowing vulnerability in the ID-move path (querySelector/getAttribute usage) that can break morphing for forms with named controls shadowing those members.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens Idiomorph’s ID-based matching and focus restoration logic against HTMLFormElement “named property” shadowing (LegacyOverrideBuiltIns), and extends tests to cover cross-realm and shadowing edge cases.
Changes:
- Introduces prototype-resolved helpers (
ownerDocumentOf,idAttributeOf) and uses them in key ID-matching paths to avoid shadowed DOM properties/methods. - Updates ID discovery to call
querySelectorAllfromElement.prototypewhen appropriate, mitigating form-control method shadowing. - Adds/extends tests for shadowed
ownerDocument,querySelectorAll,getAttribute, and cross-realm active-value/focus behaviors.
File summaries
| File | Description |
|---|---|
| test/restore-focus.js | Adds a regression test ensuring focus/selection restoration works when a form control shadows ownerDocument. |
| test/realm.js | Extends realm tests to assert value-attribute preservation, textarea active-value ignoring, and algorithmic focus preservation. |
| test/core.js | Adds a “load without DOM constructors” test and new regressions for form method shadowing (querySelectorAll, getAttribute). |
| src/idiomorph.js | Adds prototype-resolved helpers and updates ID lookup / ID-element enumeration to be resilient to form-control shadowing. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+567
to
570
| (idAttributeOf(ctx.target) === id && ctx.target) || | ||
| ctx.target.querySelector(selector) || | ||
| ctx.pantry.querySelector(selector) | ||
| ); |
74fc41b routed saveAndRestoreFocus through the target's document, but four other reads still resolve against the host document: - morphNode's `ignoreActive` check - ignoreAttribute's `ignoreActiveValue` branch - ignoreValueOfActiveElement, which also compares to `document.body` - createActiveElementAndParents Focusing an element inside an iframe sets the host document's activeElement to the `<iframe>` element itself, so all four comparisons are false and `ignoreActive`, `ignoreActiveValue` and the algorithmic focus preservation are silently inert when morphing across realms. Resolve each through ownerDocumentOf() instead, which is both shadow-safe and realm-safe.
`<form>` is [LegacyOverrideBuiltIns], so a control named
`querySelectorAll` installs an own property that shadows the method.
findIdElements guards with `?.`, which only catches null/undefined: a
shadowed name is a truthy non-callable, so it is called and throws
`TypeError: rootElt.querySelectorAll is not a function` whenever the
morph root is such a form.
Call the method off `Element.prototype` instead, gated on is.element()
so the existing tolerance for text, comment, document and fragment roots
is kept: `?.` conflated "not an Element" with "Element whose method is
shadowed", and the two need different handling. Calling off the
prototype is realm-safe for the same reason the `ownerDocument` getter
is: it brand-checks the internal slot, not the realm.
The three `getAttribute?.("id")` reads become a named idAttributeOf()
helper for the same reason.
myabc
force-pushed
the
fix/realm-safe-active-element
branch
from
September 5, 2026 16:48
72a4471 to
0ec49fa
Compare
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This and #163 are pre-emptive fixes to possible issues that I found when reviewing @lizarusi's #156
Note
Depends on #163; its changes are included until that PR merges.
Read ID attributes and query ID-bearing elements through native methods to avoid form controls shadowing
getAttributeorquerySelectorAll. Resolve those methods on first use to preserve imports without a DOM. Restore the comment explaining the realm-safe type checks.Tests cover form method shadowing and extend the realm suite with value-attribute, textarea, and algorithmic focus assertions. Upstream now supplies the target-document focus implementation. Broader form attribute-method shadowing remains outside this change.