smd_wrap: conditionally wrap stuff with tags and labels

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.

Bloke · 2011-12-12 22:03:40

maniqui wrote:

transform="link" is processing the URLs it finds inside href attribute.

The reason is that transform="link" is a bit stupid. Its usage model was primarily “transfom this field, which I know will probably contain a URL, into an anchor”. The regex doesn’t look to see if anything beginning with ‘http’ has already been converted to an anchor, hence the problem.

Frankly I don’t know how to change the plugin’s regex to do that. I shamelessly copied it off the Interweb without a moment’s thought so it probably requires advanced regex-fu techniques like ‘is it a URL but NOT one that is inside href="" or src="", or err, are there any more HTML tag attributes that can contain URLs?’

If anybody with exemplary regex skillz could point me in the right direction I’d be extremely grateful. For reference here’s the beast I stole from codesnippets:

$pat = "@\b(https?://)?(([0-9a-zA-Z_!~*'().&=+$%-]+:)?[0-9a-zA-Z_!~*'().&=+$%-]+\@)?(([0-9]{1,3}\.){3}[0-9]{1,3}|([0-9a-zA-Z_!~*'()-]+\.)*([0-9a-zA-Z][0-9a-zA-Z-]{0,61})?[0-9a-zA-Z]\.[a-zA-Z]{2,6})(:[0-9]{1,4})?((/[0-9a-zA-Z_!~*'().;?:\@&=+$,%#-]+)*/?)@";

is it (or “would it be”) possible to make smd_wrap to work similarly to chh_if_data

Never paid much attention to how it worked so I didn’t know it did that. No doubt I can rip the concept from that plugin and make it optional here. Leave it with me and I’ll see what I can cook up as I rejig it for security as Gocom has raised.

In the meantime, your point about not being able to do certain things, well, yeah, not as neatly but there are two ways to perform your final example.

Method1: with suffix

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

Method 2: with add transform

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

Hope that gets you going for now while I bend it to do your bidding.

EDIT: ninja jakob was faster ;-)

Last edited by Bloke (2011-12-12 22:09:01)

maniqui · 2011-12-12 22:30:56

Hi guys, thanks for the prompt reply.

Bloke wrote:

The reason is that transform=“link” is a bit stupid. Its usage model was primarily “transfom this field, which I know will probably contain a URL, into an anchor”. (…)

Ah, ok.
As said, I was playing/testing/learning/taking-it-for-a-ride, and probably tested it on an edge situation (“first textilize and then linkify content”). I may have gone too far.

Re: chh_if_data vs smd_wrap & suffix/add/transform.

Thanks for the proposed solutions, jakob, Bloke.
Both solutions may fit the bill in simple situations (and maybe in complex ones too), but from a code readability POV, both made my eyes ~~bleed~~ just itch a little :)

Again, I wasn’t looking for a particular solution, but just testing the plugin and reporting findings :)

Bloke · 2011-12-12 22:42:38

maniqui wrote:

Both solutions may fit the bill in simple situations (and maybe in complex ones too), but from a code readability POV, both made my eyes ~~bleed~~ just itch a little :)

Then the antidote is v0.20 in which I’ve just added the content_mode attribute. It has options:

all to use the v0.10 functionality of “is there anything in the true branch whatsoever”
txp_tags which works exactly the way chh_if_data does, but uses ruud’s shiny regex from the Txp parser so it should handle same-tags-in-tags / tags-as-attributes, though it’s currently untested

I’ll tidy up the security hole and then release the beta later.

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

Textpattern CMS

Textpattern CMS support forum

#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

#46 2011-12-12 22:03:40

Re: smd_wrap: conditionally wrap stuff with tags and labels

#47 2011-12-12 22:30:56

Re: smd_wrap: conditionally wrap stuff with tags and labels

#48 2011-12-12 22:42:38

Re: smd_wrap: conditionally wrap stuff with tags and labels

Board footer