fix: match sender and origin on inspector RPC messages

[cyaiox]

Summary

MessageTransport from @dcl/mini-rpc uses its origin argument only as the postMessage targetOrigin. Inbound it delivers every message event that reaches the window, whatever the source:

window.addEventListener('message', event => { if (event.data) this.emit('message', event.data) })

The RPC pairs are kept apart by channel-id strings (IframeStorage, SceneRpcOutbound), which is a naming convention rather than a check. Outbound, it posts with '*'.

AuthenticatedMessageTransport subclasses the exported abstract Transport and pins both directions:

• inbound is delivered only when event.source is the iframe's own contentWindow and event.origin matches
• send targets that origin instead of '*'

Both initRpc and takeScreenshot now use it, replacing the last two MessageTransport constructions.

Why the origin comes from iframe.src

Not rebuilt from inspectorPort: EditorPage/component.tsx honours VITE_INSPECTOR_PORT, so the two differ in development. Reading it off the element also keeps working if the inspector is ever served from somewhere other than localhost.

Transport disposal

Both are now closed, which they weren't before:

• initRpc's transport is disposed alongside the clients it feeds
• takeScreenshot disposes the one it creates when no client is passed — restructured so the caller-supplied path returns early and the owned path uses try/finally

The iframe is recreated on every scene open, so each undisposed transport left another live message listener on window. Pre-existing with MessageTransport (which has no dispose at all); this PR is what makes it fixable, since the subclass adds one.

Test plan

transport.spec.ts, 5 cases: delivery from the expected peer and origin, drops from a different window, drops from a different origin, send targeting a concrete origin, and no delivery after dispose. Removing both peer checks fails exactly the two drop cases.

Worth noting one thing the tests taught me: they weren't isolated at first. Each transport attaches to window, and because received is a shared let, a transport left over from an earlier test kept delivering into the next test's array — invisible while the source check was in place, and it produced a spurious third failure the moment I reverted the guards to check them. There's now an afterEach that disposes, which is the same leak this PR fixes in takeScreenshot.

• npm run test:unit — main 74, preload 44, renderer 136, shared 33
• make typecheck (0 errors), npm run lint, npm run format
• open a scene: inspector loads, tabs select, entities render — i.e. the RPC handshake completes in both directions
• edit and save a scene (write_file over the storage channel)
• scene thumbnail still regenerates on save (takeScreenshot, the restructured path)
• open several scenes in a row, then check getEventListeners(window).message.length in devtools stays flat rather than growing per open
• with VITE_INSPECTOR_PORT set to a non-default port, the inspector still connects

🤖 Generated with Claude Code

https://claude.ai/code/session_01CW4SCgagWDAxR3PKJCMp5d

[decentraland-bot]

Review Summary

Verdict: ✅ Approve — clean, well-motivated security fix with proper tests and no blockers.

What this PR does

Replaces @dcl/mini-rpc's MessageTransport (which accepts every inbound message event regardless of source and posts to '*') with a new AuthenticatedMessageTransport that:

1. Validates inbound — delivers only when event.source is the expected iframe contentWindow and event.origin matches
2. Restricts outbound — postMessage targets the iframe's actual origin instead of '*'
3. Fixes a resource leak — transports are now dispose()d, preventing one message listener from accumulating on window per scene open

Findings

No P0 (blocker) or P1 (major) issues found.

[P2] !event.data falsy coercion (transport.ts:17) — The guard if (!event.data) return would silently drop messages with data: 0, data: "", or data: false. Since @dcl/mini-rpc messages are always { id, type, payload } objects, this is not a practical concern — and is consistent with the upstream MessageTransport's own if (event.data) pattern. A semantically tighter event.data == null would be more precise, but this is a stylistic nit, not a bug.

Security Assessment

• Origin validation: The dual event.source + event.origin check is the correct postMessage security pattern per MDN/OWASP guidance. ✅
• Outbound restriction: Targeting a specific targetOrigin instead of '*' prevents data from leaking if the iframe navigates unexpectedly. ✅
• Origin derivation: getIframeOrigin reads iframe.src via the URL constructor, which is safe — src is set by the host before initRpc runs. ✅
• No hardcoded secrets or credentials. ✅
• No injection vectors introduced. ✅

Architecture Notes

• Subclassing Transport is the right call — the base class is an abstract EventEmitter with send() and no dispose(), so composition would just re-implement the same contract.
• Asymmetry with the inspector iframe (which still uses MessageTransport) is correct by design — the host is the trust boundary; the iframe only talks to its parent.
• takeScreenshot refactor — The early-return for the caller-supplied client path, with try/finally for the owned-transport path, makes resource ownership explicit and the leak impossible.
• Test coverage — 5 focused cases covering accept, reject-by-source, reject-by-origin, outbound targeting, and post-dispose silence. Good.

ADR-6 Compliance

• PR title fix: match sender and origin on inspector RPC messages ✅
• Branch fix/rpc-transport-message-matching ✅

---

Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack

[github-actions]

Test this pull request on macos-latest

Download the correct version for your architecture:

mac-x64
mac-arm64

Click here if you don't know which version to download

For running this unsigned version of the app, you will need to run the xattr command on it:

1. Extract the app from the downloaded .dmg file (double-click it)
2. Place the extracted app anywhere you like in your file system
3. Open a terminal on the directory where the app is
4. Run xattr -c app-name, replacing "app-name" for the actual name of the app
5. Double-click the app ✅

[github-actions]

Test this pull request on windows-latest

Download the correct version for your architecture:

win-x64

[cyaiox]

Thanks — on the P2, I'd leave it as is, and the reason is a bit stronger than style.

The guard is redundant with mini-rpc's own check. RPC.handler runs every message through isMessage first:

this.handler = async (message) => { if (this.isMessage(message)) { switch (message.type) { ...

and isMessage begins:

return (value && value.id === this.id && typeof value.type === 'string'
        && messageTypes.includes(value.type) && ...

value && already rejects 0 / "" / false, and it additionally requires id to match this channel and type to be a known message type. So anything !event.data drops gets dropped one layer down anyway, and anything event.data == null would admit gets dropped there too — the two forms are observationally identical. Not worth a commit touching this file for zero behaviour change.

I also checked whether the guard might be filtering legitimate non-Message traffic, which would make a shape check meaningful: there's no non-RPC postMessage from the inspector to its parent, so it isn't.

The honest alternative would be deleting the line as dead code. I'd keep it — it matches upstream's pattern and avoids emitting obvious non-messages into the emitter, even though nothing downstream acts on them.

Happy to make either change if you'd rather; just say which.