Support MMM scripts sending MMM commands to MMM through `chat:` with proper impersonation

phylll commented 2 years ago

Fun stuff.

This works, in that the dynamically generated script is being executed and the only output is oops!:

!mmm script
!mmm     chat: !mmm script
!mmm     chat: !mmm   set customizable foo = "bar"
!mmm     chat: !mmm   chat: oops!
!mmm     chat: !mmm end script
!mmm end script

This, in contrast, crashes the sandbox:

!mmm script
!mmm     chat: !mmm customize export to chat
!mmm     chat: !mmm script
!mmm     chat: !mmm   set customizable foo = "bar"
!mmm     chat: !mmm   chat: oops!
!mmm     chat: !mmm end script
!mmm end script

The generated code, by itself, is correct and runs fine:

!mmm customize export to chat
!mmm script
!mmm   set customizable foo = "bar"
!mmm   chat: oops!
!mmm end script

phylll commented 2 years ago

This came up in trying to automate generation of missing weapon-specific config scripts in https://github.com/phylll/mychs-macro-magic/issues/25#issuecomment-991580952

michael-buschbeck commented 2 years ago

Haha. Love the spirit. :D

I tried to reproduce the crash (in our test game) in order to see the crash message, but... it worked for me, and produced the following output:

(From Mych's Macro Magic): !mmm customize !mmm set foo = "bar" !mmm end customize

Logged in as a GM. Not sure if that makes a difference (though if it does that'd be a useful data point).

phylll commented 2 years ago

Hm, I was also a GM. Tried it again (literally copy&pasting the code block above), and the result was a crashed sandbox and this:

TypeError: Cannot read property 'get' of undefined
TypeError: Cannot read property 'get' of undefined
    at MychScriptContext.whisperback (apiscript.js:1071:56)
    at MychScriptContext.error (apiscript.js:1090:14)
    at MychScript.continueExecute (apiscript.js:3233:26)
    at MychScript.startExecute (apiscript.js:3210:14)
    at apiscript.js:534:24
    at eval (eval at <anonymous> (/home/node/d20-api-server/api.js:168:1), <anonymous>:65:16)
    at Object.publish (eval at <anonymous> (/home/node/d20-api-server/api.js:168:1), <anonymous>:70:8)
    at /home/node/d20-api-server/api.js:1736:12
    at /home/node/d20-api-server/node_modules/firebase/lib/firebase-node.js:93:560
    at hc (/home/node/d20-api-server/node_modules/firebase/lib/firebase-node.js:39:147)

michael-buschbeck commented 2 years ago

Must be this line:

Not sure why this.playerid would be invalid (or undefined) here. Perhaps a connection to the MMM "impersonation for API calls from MMM scripts" feature (see #78)? And why did it work when I tried it?

phylll commented 2 years ago

My phyllo (GM) generally cannot retrieve a name. I thought that was universal among GMs, to the point that I used if isdenied(sender.name) as my best way so far to determine if a script is being run by the GM ... but we might be talking about different names.

michael-buschbeck commented 2 years ago

The sandbox-crashing error you've discovered happens before the script even gets to the point of asking for displayname – it happens when it's asking for the get() method itself, which of course doesn't exist in JavaScript's undefined value. Apparently getObj("player", this.playerid) returns undefined – that would happen if this.playerid isn't an existing (or valid) player ID – like, for example, "API".

The #78 fix for API script calls from within MMM scripts requires the called API script to be registered after MMM itself so that MMM gets a chance at modifying msg.playerid (normally "API" for any chat message send through the API) in the shared msg struct that's sequentially passed to all API script handlers before the called API script gets it. However, MMM isn't registered "before MMM".

...but of course MMM should be plenty able to pretend anything it wants to itself – so I'm guessing that's just an oversight of mine.

By the way: sender.name returns sender.token_name falling back to sender.character_name – so if you select a sender in the chat drop-down that has no like-named token on the board or character in the journal, you'll get unknown. In normal groups that use their real names instead of their character names as their "player name" you'd get that result whenever the player doesn't select one of their characters' names.

michael-buschbeck commented 2 years ago

...but of course MMM should be plenty able to pretend anything it wants to itself – so I'm guessing that's just an oversight of mine.

Indeed it is. The sequencing of actions in MMM's chat message handler goes like this:

Ingest message metadata (e.g. player ID, roll results, selected tokens) from the msg structure into the context object that will be associated with the next MMM command.
If this message isn't an API message, stop. Otherwise…
If there's anyone to impersonate, modify msg.playerid accordingly.
If this API message starts with !mmm, parse and process it, using the context object created in the first step.

…and so on. The important bit is that I put the "impersonation" part after the "ingest metadata" part – actually after the "isn't an API message" bail-out.

There's no compelling reason for that to be so, though. At worst, impersonation won't affect a non-API chat message at all – at best it will make MMM's impersonation feature more compatible with other API scripts that maybe do something with regular chat messages as well (e.g. chatsetattr's embed-commands-between-table-rows feature).

michael-buschbeck commented 2 years ago

Repro command (caution: crashes the API sandbox):

!mmm chat: !mmm do whisperback("He's dead, Jim")

michael-buschbeck / mychs-macro-magic

Support MMM scripts sending MMM commands to MMM through `chat:` with proper impersonation #148