Proposal: Rework Response Conditionals

[aspidites]

a. from http://wiki.parpg.net/Proposal:Dialogue_Engine#Response_Conditionals
"Response conditionals are conditional statements that determine when and if a response is made available to the player for a particular dialogue node."

b. from http://parpg-trac.cvsdude.com/parpg/browser/trunk/game/dialogue/drunkard.yaml:

Code:

REPLY: "Gang up?  There's only one of me!"
CONDITION: "not pc.met('bart')"

c. from http://parpg-trac.cvsdude.com/parpg/browser/trunk/game/scripts/dialogueengine.py

Code:

condition_met = condition is None or \
                                eval(condition, cls.game_state)

By definition a. above, we see that conditionals will allow us to execute certain dialogue based on a condition, such as the example listed in b. above. The problem, is that the condition is evaluated by the code in c, which could potentially be dangerous, assuming that same section is fed malicious code.

My proposal is outlined and explained below.

d. dialogue_actions:

Code:

known_people = {}

def met(args):
    if args[-1].lower() == 'false':
        expected_result = False
    else:
        expected_result = True

    return expected_result == (args[1] in known_people[args[0]])

def meet(source, target):
    try:
        known_people[source].append(target)
    except KeyError:
        known_people[source] = [target]

e. action_parser.py

Code:

import dialogue_actions
from dialogue_actions import meet, met

def parse(condition):
    args = condition.split()
    function = args[1]
    args.pop(1)

    return getattr(dialogue_actions, function)(args)

e. modified dialogue_engine.py:

Code:

condition_met = condition is None or \
                                       parse(condition)

f. modified drunkard.yaml:

Code:

REPLY: "Gang up?  There's only one of me!"
CONDITION: "pc met bart false"

As can be seen by c. and d. above, the implementation removes the need for eval() completely, and the YAML file is more readable.

The above implementation is simply a trivial proof-of-concept and I don't plan to submit it as a patch, but I think I'm on the right track. I'd like to make the parser an object itself (ActionParser) instead of a module, and of course dialogue_actions needs to be renamed, since it clashes with dialogueactions.

[mvBarracuda]

Technomage mentioned that he's worried about malicious code in dialogues as well, but on the other hand you would have to maintain your own mini language in this case. Here's his reply from the IRC channel:

Ah, missed aspidites for the log: good point about eval. I was also concerned with security there, but I figured it would be more trouble than its worth to try and create our own mini-language. I still think Python is the way to go for conditionals - I believe there are ways to eval Python expressions in a limited environment, I will have to do some research first.

[aspidites]

A mini-language may in fact be more trouble than it is worth, but a mini-language is not the only solution to the problem, hence my proposed solution. Perhaps my original post was too much to digest, so I summarize:


right now, conditions look like this:
CONDITION: "not pc.met('bob')"

my original proposal would change that to:
CONDITION: "pc met bob false"

Under the hood, my version returns the same value as the original, but without the use of "eval()".

I'm tweaking the original concept so that the end result will be:
CONDITION: "pc not met bob"

Which is practically plain English. I'll let you know if I'm successful in doing that.

edit 1:
With my implementation, it is now possible to do any of the following:
CONDITION: "pc not met bob"
CONDITION: "not pc met bob"
CONDITION: "pc met bob false"

affirmatives are applied, though they can explicitly be used:
CONDITION: "pc met bob true"

Either is evaluated correctly. While this still needs to be looked over by current devs, I'll be patching the current code base to see how practical this system is in production code.

edit 2:
I thought I should note that the code is safe in that it doesn't run arbitrary python code, but rather checks for valid functions in (what I've now named) the actions module.

PS- Not related to conditions per se, but turns out my code can be used to turn regular sentences into valid python. For example, if Jack hasn't met Jill, the following are both valid:
meet("jack", "jill")
parse("jack meet jill")

[zenbitz]

Are we accepting dialog submissions from blind sources automatically?   I mean... you are expecting someone to put a malicious script  (system('rm -rf /*')  into a dialog file and then commit it to SVN?    I mean, I guess some disgruntled dev could do this to us personally... but we would know who it was (commit logs). 

More importantly, I think technomage is working on the dialog engine refactor - so you guys should compare notes or work on different parts of the code.

[aspidites]

The security issue was just one concern. I also think the punctuation in the YAML files obsfucates things a bit, and my code addresses that.

[Technomage]

eval() will allow malicious data to compromise your entire system, kill your cat, eat your dog and make love to your wife.

Couldn't have said it better myself  Even if there is little to no chance of malicious code being injected via a dialogue, a dialogue writer could still make a mistake that causes some side effects which are difficult to debug.

That said, there are ways to make eval *relatively* safe as described here: http://stackoverflow.com/questions/661084/security-of-pythons-eval-on-untrusted-strings. Basically you set up a restricted execution environment that allows only a certain subset of Python expressions and modules to be accessed, limiting the exploitative potential of eval.

While your suggestion is good aspidites, I think that there are some benefits to using Python syntax for conditional statements: its standardized, and we don't have to worry about writing and maintaining our own parsing code. Your parsing code is pretty simple, but ultimately we will want to expand it and that's when things start getting messy really fast. Your patch has the benefit that it defines a limited grammar, but eval has the ability to do that already as stated above.

I think the bigger issue is how conditional statements are defined and evaluated. When I rewrote the DialogueEngine I basically ported the old techdemo1 method of evaluating conditional expressions unchanged, so that's why it might seem a bit sticky. Right now the objects that are "allowed" in a conditional expression is defined at runtime by a "game_state" dictionary which is passed into eval. Currently, this game_state dictionary contains only two objects: 'pc' which is a reference to the player character instance, and 'quest' which is a reference to the quest engine instance. Its a strange way of doing things, and I'd rather see conditional logic implemented in its own module for the sake of modularity.

Another issue is embedding the Python expressions in YAML, as aspidites mentioned. The (') and (") quote characters have meaning in both YAML and Python, so it gets a little confusing when quoting Python strings. I put double quotes (") around all of the Python expressions since its customary in Python to use the single quote (') when possible and YAML doesn't evaluate special characters for scalars encased in double quotes. The YAMLDialogueParser actually figures this out automatically when it dumps a dialogue to a file, so eventually when we have a GUI tool up and running the issue will probably be moot.

[aspidites]

Just a final comment: I'm not sure the advent of a GUI will make this a moot point. A lot of GUI editors provide a "show source" option, so readability remains a concern (of mine).

That said, I concede, agreeing to disagree.

[Technomage]

Not trying to club you over the head aspidites. Just want to get the dialogue rolling, so to speak 

Anyway you bring up a good point: Python doesn't embed in YAML particularly well, and syntax really does matter even if we have a nifty GUI to hide the kludge. However as long as the statement is encased in double quotes within the YAML file I don't see this as a huge issue - it may look a bit funky, but anyone familiar with Python's syntax will get it right away and even those who aren't familiar will catch on pretty quick. I think these relatively minor faults are worth the expressive power of Python. At least I can't think of a situation where this would result in anything worse than "slightly ugly", but I could be wrong.

I actually stopping being lazy and took the time to look at eval more closely, and as it turns out its not a simple thing to make it securely evaluate an external string. Apparently the ast module has more secure functions for loading dynamic Python code, I'll have to look into it. I don't want to get too involved in security here, though: if someone really wants to inject malicious code into PARPG the dialogue files are probably not the easiest way to do it.

[mvBarracuda]

I don't want to get too involved in security here, though: if someone really wants to inject malicious code into PARPG the dialogue files are probably not the easiest way to do it.

Good point, you can basically inject malicious code in the entire Python code of PARPG. But as we have control over what gets into the version control repository (by deciding who gets write access to the repo) I don't think that it's really realistic security issue.