Mark a function as escaped about twigbridge HOT 41 CLOSED

Seems like something to consider for 0.6.0.

Thoughts @barryvdh Good idea, and best way you think to implement it?

An option might be to have something like:

$functions = [

    'nav_link' => [
        'safe' => true
    ]

]

from twigbridge.

Right now, the HelperLoader accepts closures and strings, which are then converted to Twig_SimpleFunction objects. But it could easily be changed to simply also allow Twig_SimpleFunction:

if(is_a($twigFunction, 'Twig_SimpleFunction')){
    $functions[] = $twigFunction;
}else{
    // make Twig_SimpleFunction
}

Then you can just pass a Twig_SimpleFunction in the config, with the options array as third parameter.

'safe_function' => new Twig_SimpleFunction('safe_function', function () {
        return '<b>Safe!</b>';
    }, array(
    'is_safe' => array('html')
    ))

(Twig_Function_Function is deprecated since 1.12 and to be removed in 2.0)

from twigbridge.

I'm looking at this now. My intention is to support the following ways to load a function through the config file:

'functions' => [

    'secure_url',
    'link_to' => [
        'is_safe' => ['html'],
    ],
    new Twig_Function_Function($name, $options),
    new Twig_SimpleFunction($name, $callable, $options),

]

from twigbridge.

Any word on this? All the form helpers (form_open,form_close,form_text,form_label,...) are kind of nasty to work with right now.

from twigbridge.

If you want you can take a look at how I've done this now in the extensions in https://github.com/barryvdh/laravel-twigbridge
That also has a form extension to add the form helpers (html safe) by default.

from twigbridge.

Oh and you could set autoescape to false just for your form. http://twig.sensiolabs.org/doc/tags/autoescape.html

from twigbridge.

@barryvdh You're saying this will work out of the box with your version of TwigBridge? Giving this a whirl now... thanks!

A couple things off the bat:

• form_open seems to exist, but form_close doesn't. I assume this is handled by FormExtension.php, which supposedly imports all form_* functions (how that works as a function name, I don't know), so I'm not sure how I'm supposed to add it -- Edit: Yours requires you the call it like a function, form_close(), unlike Crowe's. I'm fine with this.
• You can't import templates without writing the extension unlike in Crowe's version (not a big deal)
• You seem to have is_safe enabled by default for all functions/filters. That's easy enough to change, but I'm not sure it's a very safe default.

from twigbridge.

I would really like to see this too. Being able to mark a function or alias as safe would save me from writing | raw all the time!

from twigbridge.

I would also like a way to indicate that the Laravel form helpers are fine to not escape, without writing raw constantly.

from twigbridge.

@glennjacobs @rickywiens See my fork. form_* functions will not be escaped. If you add Form to the facades list in the config,

'facades' => array(
    'Auth' => ['is_safe' => false],
    'Form' => ['is_safe'=> true],
    'URL' => ['is_safe'=>true],
)

You can write Form.open(...) as well and it won't be escaped.

Also see this thread.

Hasn't been thoroughly tested yet. Hoping @rcrowe or @barryvdh will reincorporate my changes.

from twigbridge.

Looks good, hoping it gets incorporated!

from twigbridge.

It function escaping/marking as safe already is incorporated in my twig bridge: https://github.com/barryvdh/laravel-twigbridge
Only the facades escaping not yet.

from twigbridge.

I'll get this into the 0.6 branch today. I'm vivalacrowe on skype if anyone wants to chat while im working on this.

from twigbridge.

I'm pretty busy today, but you can look at https://github.com/barryvdh/laravel-twigbridge/blob/master/src/Barryvdh/TwigBridge/Extension/HelperExtension.php
That works pretty good for me, you can use just the name, call a method on a class, use a callback, set an array with functions, combine the callback + array or just use a Twig_SimpleFunction:

'functions' => array(
    'simple_function',
    'class_function' => 'method@MyClass',
    'other_function' => array(
        'is_safe' => array('html')
    ),
    'call_me' => array(
        'is_safe' => array('html'),
        'callback' => function($value){ 
                return phone($value);
            }
    )
),

'filters' => array(
    'filter_this' => function($value){
            return doSomething($value);
        }
),

from twigbridge.

So this is working in 0.6 now, thanks @barryvdh & @mnbayazit for the work on this.

The config is a mess while I was testing some things, mainly as a note until I update docs / comments.

I would prefer not to mark the whole Facade as is_safe but the individual methods. Thoughts?

from twigbridge.

@rcrowe You don't have to mark the whole Facade as safe. Take a look at the method. Instead of

'facades' => array(
    'Form' => ['is_safe'=> true],
)

You can write

'facades' => array(
    'Form' => ['is_safe'=> 'methodPrefix.*'],
)

To flag specific methods as safe via a regex, or list them out explicitly:

'facades' => array(
    'Form' => ['is_safe'=> ['method1','method2',...],
)

That's the best I could come up with.

@barryvdh It doesn't break chaining per-se, but it only goes one level deep. Methods will only be marked as safe if they return a string. If they return an object which contains a method that returns a string and you call that sub-method, it won't be escaped. I couldn't figure out a nice way to get around that. The solution you offered up as is OK but it gives up the ability to be explicit about which functions are escaped.

from twigbridge.

@mnbayazit I've taken the code (minus the regex stuff for now) from your fork so facades support:

'Form' => array(
            'is_safe' => array(
                'open',
                'someOtherMethod',
            ),
        ),

So I was going to list methods. Just wondered peoples opinions on doing so. There might have been questions about keeping it updated etc.

from twigbridge.

@rcrowe Indeed. I was hoping the regex solution would help catch future functions, but it's not ideal -- it could pick up functions you don't want as well.

Another solution I thought of was to disallow calling the function at all unless it's explicitly listed. This forces the consumer to make a decision about whether or not it's safe. Actually, better would be to throw an error that tells them they need to add it to the config so that they're not confused when it's missing.

from twigbridge.

I think of would actually be better to create extensions for the functions
you need, instead of using the Facade, but that is a little bit more work.
I didn't look close enough probably, but didn't it return a TwigMarkup
object if it was possible to transform to string? So the next call in the
chain would hit the markup instead of the real object.. Or am I overlooking
something?
Op 20 mrt. 2014 20:16 schreef "Mark Bayazit" [email protected]:

@rcrowe https://github.com/rcrowe Indeed. I was hoping the regex
solution would help catch future functions, but it's not ideal -- it could
pick up functions you don't want as well.

Another solution I thought of was to disallow calling the function at all
unless it's explicitly listed. This forces the consumer to make a decision
about whether or not it's safe. Actually, better would be to throw an error
that tells them they need to add it to the config so that they're not
confused when it's missing.

Reply to this email directly or view it on GitHubhttps://github.com//issues/68#issuecomment-38209306
.

from twigbridge.

Yep, I think I agree. If you've gone to all that effort of setting an array of method / options, then you might as well keep an extension up to date. Also brings extra flexibility.

I'll keep the existing loader extensions for user options.

from twigbridge.

@barryvdh In the event that it's not a string but has a __toString method..yeah, you'd be hooped. I took that bit from your code, I don't know if it's good to try and convert it to a TwigMarkup or not in this case.

I don't like having to write my own facades for functionality that already exists and is expected to exist from the Laravel docs. They would just be calls into the existing facades anyway; serves almost no purpose IMO.

I haven't used Laravel extensively yet so I don't know how much of a problem maintaining this config would be. Blade doesn't have automatic escaping does it? You have to be explicit about everything? So there's nothing we can hook into...

@barryvdh You wrote the IDE-helper... could we maybe leverage that to create stubs for all the existing facades -- a new facade that makes a call into the existing facade -- and then we can go through each method by hand and mark them as safe or not?

How would we do this though? Functions are listed out in the config, but we don't have any way to annotate methods; PHP doesn't support decorators or attributes.

Instead of 'class_function' => 'method@MyClass' would it be possible to do 'MyClass.method' => 'method@MyClass'? Could we create some dumby class and add [magic] methods to it so that we get the nice syntax? Then we almost don't need the "facades" config, we can just list everything out in "functions". We still can't get around the multi-level objects though; I think you'd have to write something yourself for that.

from twigbridge.

I wrote extensions for the most common functions in my package, like form
and url helpers. A lot of Facades shouldn't be used in view, because they
shouldn't have much logic.

That syntax would add a Twig function class_function, whichs executes
method on MyClass.
Op 20 mrt. 2014 22:54 schreef "Mark Bayazit" [email protected]:

@barryvdh https://github.com/barryvdh In the event that it's not a
string but has a __toString method..yeah, you'd be hooped. I took that
bit from your code, I don't know if it's good to try and convert it to a
TwigMarkup or not in this case.

I don't like having to write my own facades for functionality that already
exists and is expected to exist from the Laravel docs. They would just be
calls into the existing facades anyway; serves almost no purpose IMO.

I haven't used Laravel extensively yet so I don't know how much of a
problem maintaining this config would be. Blade doesn't have automatic
escaping does it? You have to be explicit about everything? So there's
nothing we can hook into...

@barryvdh https://github.com/barryvdh You wrote the IDE-helper... could
we maybe leverage that to create stubs for all the existing facades -- a
new facade that makes a call into the existing facade -- and then we can go
through each method by hand and mark them as safe or not?

How would we do this though? Functions are listed out in the config, but
we don't have any way to annotate methods; PHP doesn't support decorators
or attributes. How does that 'class_function' => 'method@MyClass', syntax
work? Does that give access to MyClass.method or myclass_method?

Reply to this email directly or view it on GitHubhttps://github.com//issues/68#issuecomment-38225750
.

from twigbridge.

You could chain the Facade calls actually, by extending Twig_Markup and using that in the Facade caller, something like this:

<?php
class Twig_Markup_Chain extends Twig_Markup implements Countable
{
    protected $content;
    protected $charset;
    protected $source;

    public function __construct($source, $charset='UTF-8'){
        $this->source = $source;
        $this->charset = $charset;
        $this->content = null;
    }
    public function __toString(){
        return $this->getContent();
    }
    public function count(){
        return function_exists('mb_get_info') ? mb_strlen($this->getContent(), $this->charset) : strlen($this->getContent());
    }
    protected function getContent(){
        if(is_null($this->content)){
            $this->content = (string) $this->source;
        }
        return $this->content;
    }
    public function __call($method, $arguments){
        $this->source = call_user_func_array(array($this->source,$method), $arguments);
        $this->content = null;
        return $this;
    }
}

But I haven't really found how you can chain a safe Twig_SimpleFunction

from twigbridge.

I was in the process of moving Laravel facades, filters & functions to their own extensions when I came across an issue with the facade extension clashing with variables passed in through View::make(...). But this can also happen with Laravel functions & filters.

All a user has to do is pass in the following & the Config facade will break:

array(
    'Config' => 'foobar'
)

I wondered peoples thoughts on a couple of syntax ideas for the facades:

{{ bridge.config.get('app.timezone') }}
{{ laravel.config.get('app.timezone') }}

{{ laravel.config_get(...) }} 

FYI - Marking functions as safe has been completed.

from twigbridge.

You mean you want to move all the facades into a namespace? I'm not a fan of this idea. Let users choose the name they want to import the facade as, and it's their fault if they create a clash. View variables should trump global variables anyway.

from twigbridge.

The first example can already be done for most of the call, by using the global app variable. But you have to call the binding instead of the facade.
The second example, I wouldn't namespace that. I would just create an extension and create config_get as a function.

from twigbridge.

Yep, agree namespacing is crap.

As it stands all facades, functions & filters are in the config file. You can add, remove & edit as much as you want.

@barryvdh had talked about moving to their own extensions, so I was just bringing up the issue of not being able to edit the name say & clashes arising. I would have liked to separate out user & laravel extensions, thoughts @barryvdh ?

from twigbridge.

I'm not sure. I think when you provide extensions for the helper functions (asset/action/trans etc) and namespace the functions to their class/facade (form__, str__ config__, session__, url_* etc), it should be clear enough.
The default extension should provide the correct functionality for most of the users. Otherwise they could still remove that particular extension from their config and add own functions/extensions.
You could also look at the order of the extension, I'm not really sure if functions with the same name are ignored or replaced. So if the function loader overrules the extension, it is easy enough to add own functions.

from twigbridge.

Re-publish the TwigBridge config file. Our extensions now handle this. Just need to decide on which functions / filters should be escaped.

Thoughts?

from twigbridge.

You mean which of the "default" functions need to be escaped? Is there a list we can reference?

The obvious answer is that anything that generates HTML should be marked as safe, everything else should be escaped.

from twigbridge.

Right now, marked as safe are:

• Config
• Form
• HTML
• Session, only csrf_token
• Str
• Trans
• URL

Not safe:

• Auth
• Input
• Session, except csrf_token

What to do?

• HTML and Form need to be safe, because they generate html.
• Config and Trans can be safe, because they don't output strings directly related to the input, but only config/translation keys.
• Session isn't directly safe, because you can store user input in the session. csrf_token generates a token, so is safe.
• URL should only return valid url's? So can be safe, right?
• Str handles user input and doesn't necessarily create html, so it shouldn't be marked as safe, right?

So I think Str should be changed, but the rest is fine? Or should the functions that don't generate html, not be marked safe? (Like Trans, Config and csrf_token)

from twigbridge.

You can also look at this for examples: https://github.com/symfony/TwigBridge/blob/master/Extension/
The RoutingExtension uses a callback to isUrlGenerationSafe() to determine if it's safe, not really sure if that's needed in Laravel's case. Translation is not safe there.
And here: https://github.com/fabpot/Twig-extensions/blob/master/lib/Twig/Extensions/Extension/Text.php are the Text functions also not safe.

from twigbridge.

I don't think trans should be safe, you could be using placeholders like Hello :name and name would be coming from a user input.

Sent from my iPhone

On 24/06/2014, at 13:55, "Barry vd. Heuvel" [email protected] wrote:

Right now, marked as safe are:

Config
Form
HTML
Session, only csrf_token
Str
Trans
URL
Not safe:

Auth
Input
Session, except csrf_token
What to do?

HTML and Form need to be safe, because they generate html.
Config and Trans can be safe, because they don't output strings directly related to the input, but only config/translation keys.
Session isn't directly safe, because you can store user input in the session. csrf_token generates a token, so is safe.
URL should only return valid url's? So can be safe, right?
Str handles user input and doesn't necessarily create html, so it shouldn't be marked as safe, right?
So I think Str should be changed, but the rest is fine? Or should the functions that don't generate html, not be marked safe? (Like Trans, Config and csrf_token)

—
Reply to this email directly or view it on GitHub.

from twigbridge.

Good point.
So we should change Str and Translation to not safe.
URL has to be safe, because otherwise it encodes & to & So I guess it has to be safe, right?

from twigbridge.

And for filters, there also is pre_escape (http://twig.sensiolabs.org/doc/advanced.html#automatic-escaping), so trans/str could still be safe, but only when used as a filter, because we can encode the input itself.

from twigbridge.

@barryvdh What would be the disadvantage of making something like Config and URL not safe?

I think you're thinking about this backwards. Safety should be the default, therefore anything that has the slightest chance of spitting out HTML needs to be escaped automatically. If this behaviour isn't desired for some special case we can always override it with |raw.

URLs, for example, can contain single-quotes apparently which would cause your page to break if you tried writing <a href='{{ myurl }}'/> (I don't know why you would single-quote your attributes, but I've seen it done!) -- unless we're certain all the URL functions do escape &'"<>?

Escaping & to & doesn't break the URL if it's used in an attribute, but it will prevent problems if you try to display a bare URL that contains a valid entity. e.g., http://google.com?a=b<=d will render with a < if you just output on your page, but http://google.com?a=b<=d would have been safe both 'bare' and in an <a href.

from twigbridge.

I'm not really sure about the urls.
I'm also not really sure what the disadvantages are of marking it as not safe, other then not being able to put html in n your config without using raw. I thin the point of the is_safe option is that it can be trusted because it doesn't output user strings. But config only outputs strings the developer decides, so it could be trusted, right?

from twigbridge.

But config only outputs strings the developer decides, so it could be trusted, right?

In some sense, sure, but I don't want to have to think "did I put any quotes or ampersands or anything weird in any of my config settings?" every time I output something.

Not that I think there's ever a need to be echoing config settings to the browser, but you might want to do it once in awhile for debugging or I don't know what, but flagging it as safe when it could possibly not be seems more detrimental than helpful.

from twigbridge.

My first thought was that config_get should be marked as being safe.

However, after thinking it through, the rare cases where you need to output an unsafe config variable, it seems fair to have to use the raw filter.

I've removed is_safe from config_get.

from twigbridge.

Yes agreed, when we don't know for sure, better use a safe default :)

from twigbridge.

Just tagged 0.6.0.beta.1 with these changes made.

from twigbridge.