smd_wrap: conditionally wrap stuff with tags and labels

Gocom · 2011-12-10 02:48:27

Bloke wrote:

and in Form wraptest: […] echo number_format('{smd_wrap_it}', 2);

That all good, except that number_format expects floats, not strings. It might be good to cast the value before passing it to number_format() to avoid potential future issues.

Btw Stef, do you have any security measurements in place? To make sure that {smd_wrap_it} tag results in a valid string, or can the tags be used to perform remote code execution attacks? In your example the value comes straight from the URL (or POST data) which is very bad if there is no validation.

As security can be a pain, I would suggest also providing real variables and functions (and TXP tags) that can be used to pass the data to other tags or PHP snippets. When a function or a variable is used in the code, there is no direct issue about remote code execution as there is with the search-and-replace tags (in which content is evaluated with rest of the code).

For example this is securer and doesn’t need any extra validation…

<txp:adi_gps />
<txp:output_form form="wraptest"><txp:variable name="num" /></txp:output_form>

<txp:php>
	echo number_format( (float) yield(), 2);
</txp:php>

…as it uses a function, which then returns the content, instead of the content being there when the code is evaluated. If you look at rah_function’s code you will see that I go extra mile (don’t ask me why I use eval() tho) and Instead of evaluating the code with the user provided content, I use variables. You should do something similar. I think.

Last edited by Gocom (2011-12-10 02:58:50)

Bloke · 2011-12-10 03:38:06

Gocom wrote:

In your example the value comes straight from the URL (or POST data) which is very bad if there is no validation.

It was only a test for jakob to see if number_format worked or not before I went ahead and added it, then found it didn’t work for him. I’d never expect anyone to do that in real life! *shudder*

use variables. You should do something similar. I think.

Thanks for the tip. I’ll look into it.

merz1 · 2011-12-10 09:48:06

[OT] As Alan asked for RSS info sources:

Textpattern RSS firehose: Try TXP Info Sources: Textpattern RSS feeds as dynamic OPML if you need one source for pretty much all TXP info available via RSS.

Last edited by merz1 (2011-12-10 09:49:19)

jakob · 2011-12-11 16:24:37

If it works to your satisfaction, let me know and I’ll add it as a transform

In the end I realised that I needed to trap more ‘inventively bad’ user input such as values with currency symbols, with spaces, with incorrectly used separators etc. for example:

198,000.00
240000
$ 123,457
420.000.00 (e.g. the user inputs with a German-formatted number with “.” as a thousands separator and the third-party software automatically appends the missing “.00” resulting in the same separator for decimals and thousands in the saved database value…)
345.678 €

For this I made an smd_macro entitled scrub_number with the following attributes and defaults:

value
decimals (default = 0)
format (default = 0)
dec_point (default = .)
thousands_sep (default = ,)

Code:

<txp:php>
// vars
$n = '{value}';
$format = '{format}';
$decimals = intval('{decimals}');
$dec_point = '{dec_point}';
$thousands_sep = '{thousands_sep}';
if ($n != '') {
  // remove all chars except numbers but leave commas and dots for the time being
  $n = preg_replace('/[^0-9.,]+/','',$n);
  // strip off any 2-digit decimal point data (comma or dot-separated)
  if ( in_array ( substr($n,-3,1), array (',','.') ) ) { 
    $n = substr($n,0,-3); 
  }
  // strip out any remaining non-digits
  $n = preg_replace('/[^0-9]+/','',$n); 
  if ($format == '1' ) {
   // output a formatted number
    echo number_format ($n, $decimals, $dec_point, $thousands_sep);
  } else {
   // output simple number with decimal places if required 
    echo sprintf('%01.'.$decimals.'f', $n);
  }
}
</txp:php>

I can then use this with <txp:scrub_number value='<txp:variable name="myvar" />' format="1" thousands_sep="." /> to get consistently formatted German-style numbers that are of the form ###.###. I can then use smd_wrap to add currency symbols, square metres or other info as required.

Notes/Questions:

I expect with some regex-ninja one could simplify my code significantly. All recommendations welcome.
smd_macro: Is there a better practice for dealing with curly-brace denoted variables in php code that needs curly-braces in if-statements? php threw errors until I did it this way.
smd_macro: Is there a risk of variable clashes how I’ve done that and if so, what might be a better way of namespacing the variables?
Limitations: I’ve introduced an artificial limitation by stripping off 2-digit decimals. Also it doesn’t currently handle swiss-style number formats with minute/second apostrophes.
smd_wrap: Is it feasible to integrating this kind of more comprehensive number scrubbing and formatting into smd_wrap?

Gocom · 2011-12-11 17:13:23

jakob wrote:

smd_macro: Is there a better practice for dealing with curly-brace denoted variables in php code that needs curly-braces in if-statements? php threw errors until I did it this way.

There really should. It throws out errors as the curly tags are replaced before the code is evaluated. PHP doesn’t see tags, but just some code (the resulting value) it needs to process.

That also means that the snippet is extremely vulnerable to remote code execution attacks. Anyone that can pass a value to <txp:variable name="myvar" /> can execute any PHP code they want, whether it be an intentional attack or an accident. Even just using slashes in the value throws out errors.

smd_macro: Is there a risk of variable clashes how I’ve done that and if so, what might be a better way of namespacing the variables?

If you mean variables in the PHP code, no. All code wrapped with <txp:php> tags is in its own scope (context). <txp:php> is a function that evaluates the code contained inside the tags, and so the code is wrapped inside a function and only affects its own scope. Outside doesn’t care what is inside the tags, and inside doesn’t care what is outside.

If you refer to smd_macros curly tags, those are just search-and-replace tags and can not cause any conflicts.

Last edited by Gocom (2011-12-11 17:21:34)

Bloke · 2011-12-11 17:36:29

jakob wrote:

smd_macro: Is there a better practice for dealing with curly-brace denoted variables in php code that needs curly-braces in if-statements?

Not yet. 98% of the use cases I assumed would be to plug the variables into existing plugins or tags: that is, combining tags into a super-tag. Thus most of the time the destination plugin or tag would handle validation, error conditions and sanitization on your behalf.

If you’re wrinting PHP in a macro then Gocom is right: validation is your responsibility. I should probably mention that in the docs! In this case you could probably get away with:

$format = doSlash('{format}');

for a bit more safety. Not really thought it through.

smd_macro: Is there a risk of variable clashes

As Gocom says, if you’re not defining ot using global variables then you’re safe as everything executes in local scope.

smd_wrap: Is it feasible to integrating this kind of more comprehensive number scrubbing and formatting into smd_wrap?

I don’t think so at the moment because it doesn’t support arbitrary variable insertion/replacement into functions. You might be able to do it by chaining forms and using <txp:yield />, nesting smd_wrap to split + sanitize multi-variables injected into yield from the container, but by the time you’ve figured that out you might as well have used smd_macro, some rah_functions or just plain PHP!

Last edited by Bloke (2011-12-11 17:37:14)

jakob · 2011-12-11 18:24:25

Thanks for the constructive feedback. Well, the variable isn’t reachable from outside via an url variable, but you’re right, it would be better to trap any problematic database values. As I’m accessing a separate db-table here and the third-party software that saves that data probably (?) cleans it before saving, the risk is small.

Thanks for the info about the local scope. Similarly, following your hint, I discovered that the replacement values do work directly in the php code, so the earlier errors I got were purely of my own making :-)

So is this second version better:

<txp:php>
// sanitize vars
$n = doslash('{value}');
$decimals = intval('{decimals}');
$format = (doslash('{format}') == '1' ) ? true : false;
if ($n != '') {
  // remove all chars except numbers but leave commas and dots for the time being
  $n = preg_replace('/[^0-9.,]+/','',$n);
  // strip off any 2-digit decimal point data (comma or dot-separated)
  if ( in_array( substr($n,-3,1), array(',','.') ) ) { 
    $n = substr($n,0,-3); 
  }
  // strip out any remaining non-digits
  $n = preg_replace('/[^0-9]+/','',$n); 
  if ($format) {
   // output a formatted number
    echo number_format ($n, $decimals, htmlspecialchars('{dec_point}'), htmlspecialchars('{thousands_sep}') );
  } else {
   // output simple number with decimal places if required 
    echo sprintf('%01.'.$decimals.'f', $n);
  }
}
</txp:php>

Finally, I was mistaken with swiss/Italian number format as it/they uses the decimal point or comma. It is only the thousands separator that is different and that can be produced using the attribute thousands_sep="\'".

Gocom · 2011-12-11 19:26:59

Bloke wrote:

In this case you could probably get away with: $format = doSlash('{format}');

Wait? Uh, no sadly :/ Wrapping the tag with doSlash() will not do anything to prevent code execution. It will only help if the value is passed to query, and then only if the tag’s resulting value results to a valid string. The tags are parsed before the code is executed, and also doSlash() is ran after the injected code has already been executed.

validation is your responsibility

Would be cool if we could, but we just literally can not, because of the way the curly tags work. Validation will have to happen before the tags are passed to PHP scripts and TXP tag soup. Validation can not be done by us (Jakob or anyone else) in our macros. It will have to happen before the tags are replaced (parsed), in smd_macro’s and smd_wrap’s parsers to prevent injections.

The thing doesn’t have much to do with escaping or validation in our macros, but the execution order. What is parsed by first parser (smd_macro), will be picked by the second parser – resulting to injections if no escaping is done beforehand, or if the native parent language isn’t used.

These curly tags will never be completely trustworthy inside PHP, or for that matter in TXP tag structure either. At least can not take direct input from users, without validating (escaping) the input beforehand. Otherwise the tags can always be used to do injections, whether it’s passing <txp: /> tags, PHP, JavaScript or SQL.

If instead of curly tags, functions and TXP tags where used (i.e. smd_('value') and <txp:smd_tag r="value" />) there wouldn’t be any issue with code injections. This could also be done by changing smd_macro’s parser to return tags and functions instead of strings.

Last edited by Gocom (2011-12-11 19:46:00)

Bloke · 2011-12-11 20:14:42

Gocom wrote:

Wrapping the tag with doSlash() will not do anything to prevent code execution.

Gotcha. Told you I hadn’t thought it through :-p I’ll see if there’s anything I can do about it…

(i.e. smd_('value') and <txp:smd_tag r="value" />) there wouldn’t be any issue with code injections.

… and this is the kind of approach I’ve started taking in recent plugins. I’m offering the var_prefix attribute as standard so the values are returned like this:

$replacement = '{'. $var_prefix . $value . '}';

Also in things like smd_bio I’ve begun offering <txp:smd_bio_data item="some_value" /> for example which returns exactly the same thing as its equivalent {var_prefix.value} curly replacement. Do you think this is enough?

Unfortunately smd_macro is not one of the plugins that benefits from this yet, but I can consider how I might implement such a thing. I guess a fixed prefix would have to be used in this case, since the macro can’t guarantee to have its own var_prefix attribute. [ EDITED TO ADD: I fixed a few bugs and retrofitted smd_if with var_prefix a while ago and I’ve been testing it for a good few months. Think I’m happy with it. I haven’t done an smd_if_info tag yet and not sure I can, but I can try]

Last edited by Bloke (2011-12-11 20:17:45)

Gocom · 2011-12-11 21:34:20

Bloke wrote:

Do you think this is enough?

The <txp:tags /> sound very good. Prevents server-side injections. That’s very nice :) The prefixing helps to prevent collisions, but won’t fix injecting issues.

If it was up to me, I would remove curly tags completely in next (or two) major updates. Deprecating in next bigger release and removing later. But I’m big meanie. Removing features left and right all the time, even just for potential issues or time cuts.

The underlying security issues are scary to me, and end users might not understand the risks. It worries me that no one has said anything about any of this before. Which makes me really worried about what guys are using the tags for. If it’s anything that comes from an article (on multi-author site), requested URL, form, user profile, 3rd party feed, API etc. there is change that someone will at some point abuses the system and uses the power of the tags to inject and execute code. There is a door. Doors can be opened. The tags are impressively powerful, short to write, easy to use and open more options, but it also allows injecting.

The few situations where the tags can be safely used are when tag values are set by a site’s developer and can not be changed by anyone else, or when the curly tag parser is the only parser (no TXP tag or any scripting support).

When it comes to preventing curly tags’ injection potential, escaping could cause more problems than it fixes. Usually you will expect to get what you ask, but when there is escaping going on, you don’t and you have no idea why it is happening. And replacing the curly tags with TXP tags in the (curly tag’s) parser level (which would prevent injecting as long as TXP’s parser is secure) sound pretty backwards to me and isn’t completely issue free as you can tell where the tags are used.

Last edited by Gocom (2011-12-11 21:38:46)

Bloke · 2011-12-12 13:14:13

Gocom wrote:

I would remove curly tags completely in next (or two) major updates

I can see the attraction to this, and you make some compelling arguments. In some cases I may do that, but in others — such as smd_macro — the additional overhead of having users jump through the hoops of writing <txp:smd_macro_info item="some_variable" /> instead of {some_variable} that they have defined as a tag attribute is a barrier to usability. After all, it’s not the user’s fault that my code is insecure, so punishing them by making them write longer output and more complicated, nested tag-in-tag soup is perhaps a tad harsh!

What I might experiment with is what you suggest in the end of your post: writing a <txp:smd_macro_info /> tag and then every time I encounter {some_variable} in the form/container that matches this plugin’s replacement list, replace it with <txp:smd_macro_info item="some_variable" /> instead of the direct value.

Two potential problems with this:

all {replacement} tags that were used as attributes would need to be surrounded by single quotes so the path is not backwards-compatible, but I can make this clear in a major version bump
some plugins — most notably smd_query — have the notion of preparse for handy <txp:yield /> trickery. Forcing the replacement tag to become a real tag might mean that the act of parsing the container executes the tag — which doesn’t yet have the plugin’s context set — and thus returns nothing. Not sure as I haven’t tried it yet. In smd_query’s case I can detect if preparse is not being used and only swap the {replacement} under that circumstance, or swap them out afterwards and parse again (which is slower of course). Not sure which way to go. If people are using preparse, the implication is they know what they’re doing anyway, so can accept the risks of their actions, as long as I make them clear in the docs :-) But my conscience nags me about being more secure in this instance because it deals with the database directly.

While it is ‘backwards’ to inject a tag in place of a {replacement} I wonder if the greater security outweighs the awkwardness of the code and the potential inconvenience to users of using far wordier syntax in some cases.

When writing the real ‘replacement’ tag, I’ve made sure it offers benefits over its {replacement} counterpart. Most notably in smd_bio v0.40 and smd_query v0.60 (as yet unavailable) which are my experimental play pens, I’ve permitted wraptag, class and break in the replacement tag. This allows you to do things like:

<txp:smd_blah_info item="smd_thing1, smd_thing2, smd_thing3" break="td" />

Which is way neater than both:

<td>{smd_thing1}</td>
<td>{smd_thing2}</td>
<td>{smd_thing3}</td>

and:

<td><txp:smd_blah_info item="smd_thing1" /></td>
<td><txp:smd_blah_info item="smd_thing2" /></td>
<td><txp:smd_blah_info item="smd_thing3" /></td>

Plus, it means if some items are empty they’re simply omitted from the output and not wrapped, so you don’t get empty <td></td> cells or require conditionals around each item. Winner!

Some fields — for example Txp-generated IDs and replacements that the plugin creates like row counts — are just as valid and safe with a {replacement} as a real tag. OK, perhaps the txp_file ‘id’ column might get compromised somehow, but MySQL should guard against that on auto-increment fields. Ummm, right? For that reason I will probably not deprecate the curly syntax so the user has a choice if they understand the risks or the plugin offers a zero-risk option like its own generated counter. But I might discourage curly syntax use for user-entered data like filenames, descriptions, captions, etc.

What I don’t quite understand is your closing statement:

replacing the curly tags with TXP tags in the (curly tag’s) parser level… sound pretty backwards to me and isn’t completely issue free as you can tell where the tags are used.

Can you please explain how knowing where the tags are used creates an issue? Sorry, me = thick.

When it comes to preventing curly tags’ injection potential, escaping could cause more problems than it fixes.

Agreed. I guess if I didn’t go with a curly-tag-to-real-tag replacement scheme, using strip_tags() by default, with an option to not use it if you know what you’re doing, is no good either then?!

Last edited by Bloke (2011-12-12 13:23:48)

Gocom · 2011-12-12 15:25:50

Bloke wrote:

Can you please explain how knowing where the tags are used creates an issue? Sorry, me = thick.

Just same as what you said about tags in tag attributes. When a tag is placed inside a PHP block, it won’t be parsed, and you can neither know in which context the tag is in within the PHP code. Tags can also be an issue when wrapped in a container tag that doesn’t invoke parser().

Agreed. I guess if I didn’t go with a curly-tag-to-real-tag replacement scheme, using strip_tags() by default, with an option to not use it if you know what you’re doing, is no good either then?!

It (strip_tags()) could prevent TXP tag injection, sure. Could work fine as long as users remember to escape their brackets and do not miss any “brockets”. As you probably know, the function works just by doing a simple search-and-replace. It doesn’t check that a tag really is a tag and that it has an ending. It removes anything from a bracket to a bracket. Strip_tags doesn’t <3 Jaffa cakes like I do.

Last edited by Gocom (2011-12-12 15:30:31)

maniqui · 2011-12-12 19:14:16

I’m playing with smd_wrap, learning to use it.
Here is a quick report, not sure if a bug, or a bad use (abuse) of its transform powers.

I tried to chain textile and link, to get the best of both worlds. Like telling smd_wrap to do this: “hey, smd_wrap, apply Textile, and then, also try to linkify any plain text URL that lazy author didn’t…”

If you wrap this snippet of plain text with <txp:smd_wrap transform="textile,link></txp:smd_wrap>…

This is a snippet of plain text to try @<txp:smd_wrap transform="textile,link></txp:smd_wrap>@ (I'll use that code to wrap this very same snippet of plain text). 

Here is a to-be-textilezed "link":http://textpattern.com. This one will get mangled.

Here is a to-be-textilezed self-link: "$":http://textpattern.com. This one too.

Here is a to-be-linkified link: http://textpattern.com
Here is a to-be-linkified link on its own line:

http://textpattern.com

And that's all.

You will get some mangled HTML in the output:

<p>This is a snippet of plain text to try <code>&lt;txp:smd_wrap transform=&quot;textile,link&gt;&lt;/txp:smd_wrap&gt;</code>. </p> 

<p>Here is a to-be-textilezed <a href="<a href="http://textpattern.com">http://textpattern.com</a>">link</a>. This one will get mangled.</p> 

<p>Here is a to-be-textilezed self-link: <a href="<a href="http://textpattern.com">http://textpattern.com</a>"><a href="http://textpattern.com">http://textpattern.com</a></a>. This one too.</p> 

<p>Here is a to-be-linkified link: <a href="http://textpattern.com">http://textpattern.com</a><br /> 
Here is a to-be-linkified link on its own line:</p> 

<p><a href="http://textpattern.com">http://textpattern.com</a></p> 

<p>And that’s all.</p>

It seems that the link transformation is also applied to already existing anchors.

Another quick test I’ve tried, this time just using <txp:smd_wrap transform="link"></txp:smd_wrap>

http://www.textpattern.com 

www.textpattern.com 

<a href="http://www.textpattern.com">http://textpattern.com</a>

The mangled output:

<a href="http://www.textpattern.com">http://www.textpattern.com</a> 

<a href="www.textpattern.com">www.textpattern.com</a> 

<a href="<a href="http://www.textpattern.com">http://www.textpattern.com</a>"><a href="http://textpattern.com">http://textpattern.com</a></a>

As you can see, transform="link" is processing the URLs it finds inside href attribute.
As I said, this may be seen as an abuse of smd_wrap powers, and its transform chain, or a bug :)

maniqui · 2011-12-12 20:32:14

Another quick report, for those trying to replace chh_if_data with smd_wrap..
There is a subtle difference between these plugins.

With chh_if_data:

<txp:chh_if_data>
  <section class="extra_content">
    <txp:custom_field name="extra_content" escape="" />
  </section>
<txp:else />
   You will get here if the custom field "extra_content" is empty.
</txp:chh_if_data>

That is, if the custom field is empty, the condition will evaluate to false (and thus, return the contents of the false branch), even if there is some content inside the true branch (in this case, plain HTML: <section class="extra_content">...</section>).

A similar example but with different output, with smd_wrap:

<txp:smd_wrap>
  <section class="extra_content">
    <txp:custom_field name="extra_content" escape="" />
  </section>
<txp:else />
   Even if custom field "extra_content" is empty, we won't get here.
</txp:smd_wrap>

In this case, even when the custom field is empty, smd_wrap returns the true branch, because it “sees” some content (the plain wrapping HTML).

The working smd_wrap example (the one equivalent to the chh_if_data example above):

<txp:smd_wrap wraptag="section" class="extra_content">
    <txp:custom_field name="extra_content" escape="" />
<txp:else />
   Now, if custom field "extra_content" is empty, we get here.
</txp:smd_wrap>

But there are some situations where you can’t get the same results with smd_wrap (and wraptag/class attributes).
For example, with chh_if_data:

<txp:chh_if_data>
    <txp:custom_field name="extra_content" escape="" />
    <p>And that was the extra content, for you!</p>
<txp:else />
   You will get here if the custom field "extra_content" is empty.
</txp:chh_if_data>

As said, if custom field is empty, you get the output from the false branch.
Now, with smd_wrap:

<txp:smd_wrap>
    <txp:custom_field name="extra_content" escape="" />
    <p>And that was the extra content, for you!</p>
<txp:else />
   Even if custom field "extra_content" is empty, we won't get here.
</txp:smd_wrap>

You will always get the contents of the true branch, even when custom field is empty.
And I can’t think yet of a working example without doing some more extra variable/if_variable (or chh_if_data…) effort.

Stef, unless I’m missing something obvious, is it (or “would it be”) possible to make smd_wrap to work similarly to chh_if_data? Maybe via some extra attribute?

Thanks :)

jakob · 2011-12-12 21:49:56

Hola Tocayo! Would this work:

<txp:smd_wrap wraptag="section" class="extra_content" transform="add|after|&lt;p&gt;And that was the extra content, for you!&lt;/p&gt;">
    <txp:custom_field name="extra_content" escape="" />
<txp:else />
   Now, if custom field "extra_content" is empty, we get here.
</txp:smd_wrap>

I’ve not tried it but it’s worth a try. It should definitely work without tags if the <p> notation doesn’t work. It’s a bit of a kerfuffle though I agree.

Textpattern CMS

Textpattern CMS support forum

#31 2011-12-10 02:48:27

Re: smd_wrap: conditionally wrap stuff with tags and labels

#32 2011-12-10 03:38:06

Re: smd_wrap: conditionally wrap stuff with tags and labels

#33 2011-12-10 09:48:06

Re: smd_wrap: conditionally wrap stuff with tags and labels

#34 2011-12-11 16:24:37

Re: smd_wrap: conditionally wrap stuff with tags and labels

#35 2011-12-11 17:13:23

Re: smd_wrap: conditionally wrap stuff with tags and labels

#36 2011-12-11 17:36:29

Re: smd_wrap: conditionally wrap stuff with tags and labels

#37 2011-12-11 18:24:25

Re: smd_wrap: conditionally wrap stuff with tags and labels

#38 2011-12-11 19:26:59

Re: smd_wrap: conditionally wrap stuff with tags and labels

#39 2011-12-11 20:14:42

Re: smd_wrap: conditionally wrap stuff with tags and labels

#40 2011-12-11 21:34:20

Re: smd_wrap: conditionally wrap stuff with tags and labels

#41 2011-12-12 13:14:13

Re: smd_wrap: conditionally wrap stuff with tags and labels

#42 2011-12-12 15:25:50

Re: smd_wrap: conditionally wrap stuff with tags and labels

#43 2011-12-12 19:14:16

Re: smd_wrap: conditionally wrap stuff with tags and labels

#44 2011-12-12 20:32:14

Re: smd_wrap: conditionally wrap stuff with tags and labels

#45 2011-12-12 21:49:56

Re: smd_wrap: conditionally wrap stuff with tags and labels

Board footer