Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add megaif, a more flexible version of if #98

Closed
wants to merge 1 commit into from

Conversation

Sergejack
Copy link

Allow to use any kind of object and object related properties/methods inside a conditional statement.

First branch ever, did my best to follow the guidelines right...

@jimmyhchan
Copy link
Contributor

Thanks for the pull request.

Imo this is very similar to the existing deprecated if helper. I see the
subtle difference in use cases but I would still prefer using the eq ne
etc. Helpers instead. The number based interpolation is nice but because
it's Dust the cond parameter in the old helper also does string
interpolation but we didn't have caching.

this is interesting but the existing if helper is deprecated because it's
too flexible. The issue is that it runs as an eval which is slow insecure
and difficult to use in a controlled manner. This does very similar things
and also uses eval

@prashn64
Copy link
Contributor

prashn64 commented Nov 6, 2014

I think another approach without the eval would be to make "and" and "or" logical helpers that work with the eq/ne/lt/gt helpers. That would be immensely helpful.

@rragan
Copy link
Contributor

rragan commented Nov 6, 2014

Some time back I worked up an "or" helper. It can't stand alone as it depends on internals used by eq/lt etc. At the time, there seemed little sentiment for more logic functions but now with the direction to deprecate "if" maybe I should resurrect it. The "and" can be done by nesting EQ and the others but "or" needs help. Opinions?

@Sergejack
Copy link
Author

I don't really get why you fear eval is insecure. As far as I know, either the web master is the only one to write content and there no need to even try to prevent him to do all the javascript he want. Either some untrustworthy people can write content parsed by dust such as <img ... onload="killAllKittens()"/> or anything (and dust couldn't possibly go the wrong "let's prevent everything!" way).

I really can't figure wherever dust (and its helpers) could stand when there's already a security breach to begin with...

IMHO, cursing "eval" is witch hunting.

@Sergejack
Copy link
Author

Anyway, if eval is your "no go", why not parsing a custom syntax? (and caching it's parsed result)
One that only allows \s|(&{,2))|(|{,2))|...|({\d+})

It would be way more easy than using nested helpers, I bet.

{@test cond="{0} != {1} && {0} > {2}" p0=1 p1=2 p2=0}ok{:else}ko{/test} => ok
{@test cond="alert({0})" p0=1}ok{:else}ko{/test} => _log('Invalid syntax for cond')

@rragan
Copy link
Contributor

rragan commented Nov 6, 2014

See my if helper that does not use eval. It does cache parse results. https://github.com/rragan/dust-motes/tree/master/src/helpers/control/if

@sethkinast
Copy link
Contributor

eval isn't insecure simply by virtue of being eval-- it's because you as
the developer aren't necessarily the only one populating variables that
make it into an {@if} expression. Maybe you are using some variables that
are populated by the result of a GET or POST request. Especially in a Node
context, there's too much ability to get code executed with the full server
permissions for that request.

eval also blows away any ability for the Javascript interpreter to do any
sort of optimizations on not only the code inside the eval, but the entire
enclosing scope.

We don't want to prevent you from using eval, but it's also not something
we really want to support as part of the core helpers suite.

@jimmyhchan
Copy link
Contributor

Closing this as this moves us away from the direction we want to go into which is far more specific, discreet logic vs being even more general. Please continue the discussion in #95.

We have #100 for the or helper #103 for first and last helper and helpers for eq, lt, etc. and an eval-less if implementation https://github.com/rragan/dust-motes/tree/master/src/helpers/control/if

@jimmyhchan jimmyhchan closed this Nov 12, 2014
@sethkinast
Copy link
Contributor

I wanted to say thanks for taking the time to submit a carefully-crafted PR. You included tests and justification-- it's a good PR!

If you plan on putting a good bit of work into a PR you might want to reach out via an Issue first to test your hypothesis before spending too much time if the change is something you want to get into the repo.

Thanks again for taking the time to contribute and I hope to see more helpers coming from you.

@rragan
Copy link
Contributor

rragan commented Nov 12, 2014

I took a chance. I had the code laying around already and had to harden it for cases where whitespace was preserved. This has been discussed in the past but foundered on the logic/no-logic debate -- that is when I first wrote it. If I could pull it off without change to the internal filter method, I would just publish it over on dust-motes but internal changes are needed to pull it off.

I have no cogent thoughts on other ways to do this short of big language surgery though or falling back on eval-less @if I did.

@sethkinast
Copy link
Contributor

Oh, I meant to SergeJack about his mega-if.

I think the {@or} helper needs to go in, we need that functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants