CVS edit link for atheneus

On my most recent project involving order processing I needed to be able to pass a parameter from my node module to a regular Drupal page that would then allow the client to place the value of the parameter in arbitary positions in their page. Although, I could achieve this by using a PHP filter to print request parameters from the global $_GET request array, there was a conflicting requirement that editors could not have access to PHP. The obvious solution was to use Token Filter and to use a Token provider module that would filter, tokenize and expose request parameters. I was unable to find such a module. Therefore, I have written a module "Token request parameters" (token_request_params) that provides these tokens, that can then be used within a Token filter.

The module is 100% complete and functional. I have run it through Coder to ensure Drupal coding standards compliance and all reported issues have been fixed. I would like to contribute this module back to the community. I have a D6 version right now and will start work on updating and testing on D7 soon.

I had considered whether this functionality should be provided as a patch to Token Filter considering the small size of that module and the dependency on that module. However, I think it should probably remain discrete functionality, since there may be other use cases requiring these tokens outside of the concerns of Token Filter. I believe there is a valid separation of concerns. As far as I know there is no similar project available. If there is, I am more than willing to submit a patch. I asked around on #drupal-support IRC if anyone knew of a similar project.

Thanks!

CommentFileSizeAuthor
#14 token_request_params.6.x.0.1.2beta.zip5.06 KBatheneus
#9 token_request_params.6.x-0_1_1_beta.zip3.98 KBatheneus
#2 token_request_params.zip2.51 KBatheneus

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This sounds scary from a security point of view, but I'll wait for the archive.

atheneus’s picture

atheneus commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new token_request_params.zip2.51 KB
atheneus’s picture

atheneus commented

Yes - any feedback on security issues are certainly welcome. I am providing raw as well as cleaned (check_plain()) variables. I think there may be a need to restrict the size and number of parameters that can be tokenized. Ultimately, this is addressing a security concern of using $_GET in PHP filtered blocks.

My main security concern is with introducing a vector for a DoS attack, particularly with POST parameters.

atheneus’s picture

atheneus commented

FYI: I am currently working on this module to add the ability for site owners to restrict what parameters will be available for tokenizing. They will be able to specify the full match of the parameter name or use a wildcard - so they would be able to tokenize only parameters with a particular prefix or suffix. For example: 'myparams_*' will tokenize only those parameters that begin with 'myparams_'.

I am also adding a limit on the number of parameters that can be tokenized, with the limit configurable by the site owner. For example the default might be the first 10 parameters, but the site owner would be able to raise or lower this limit.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
function _token_request_params_build_tokens($type = NULL) {
  $tokens = array();

  foreach ($_GET as $key => $value) {
    // Set our safe filtered tokens
    $tokens[$type . '-param-' . $key] = check_plain($value);
    // Provide the raw tokens as is generally expected
    $tokens[$type . '-param-' . $key . '-raw'] = $value;
  } // end foreach

  return $tokens;
}

$key needs to be check_plain()'ed in any case here.

I'm very nervous about the -raw tokens. It's not like other tokens couldn't contain totally unsafe values too, but these tokens here make a couple of attacks very very easy. Too easy, IMHO.

atheneus’s picture

atheneus commented

Thanks for the review and feedback sun. I caught the omission of the check on the key a couple of days ago, while working on adding the extra settings for restricting what parameters will be tokenized.

I share the concern about the -raw. As I continue to think about it, I can't see any legitimate use cases that would require access to the unfiltered values.

I will fix and reupload the latest version: unless, you think the whole concept completely stinks. I'm fine with that. Though, suggestions as to an alternative approach to satisfy the same use case would be highly valued.

Thanks.

atheneus’s picture

atheneus commented
Component: Miscellaneous » Code
Status: Needs work » Needs review
StatusFileSize
new token_request_params.6.x-0_1_1_beta.zip3.98 KB

Attached is an updated version of this module. I removes the '-raw' tokens, adds a check_plain() to the parameter key and also forces users to specify the parameters that will be tokenized in site configuration as well as limiting the availability of parameter tokens to specific user-defined pages.

I have also made the help page text more comprehensive and useful.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This needs an additional security review from someone else than me.

@atheneus: Sorry - if this code wouldn't have potential impacts on site security, then this application process wouldn't take so long.

atheneus’s picture

atheneus commented

@sun: no worries. It's always better to err on the side of caution. Is there any way to refer this issue to the security team to get a security review?

atheneus’s picture

atheneus commented
Component: Code » Miscellaneous
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: +Module review

I am adding the review tags.

atheneus’s picture

atheneus commented
StatusFileSize
new token_request_params.6.x.0.1.2beta.zip5.06 KB

Uploading the latest version:

Fixed bug in POST / GET settings.
Added configuration option to restrict the size of values that can be tokenized.

atheneus’s picture

atheneus commented
Status: Needs review » Closed (fixed)

Since there's little interest in this module, I'm going to go ahead and close this, to reduce the noise.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Closed (fixed) » Needs review

@atheneus: It's not true there is little interest in this module; as reported from sun, it needs a more detailed review from somebody who can make a security review of the code.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Why is the module requiring also token_filter.module?

atheneus’s picture

atheneus commented

Oh ok - wasn't getting upset or anything. I just didn't want to be cluttering up the queue.

As for the token_filter dependency, that is an artifact of the original deployment to the specific use-case where this was derived. Since the client was going to install & configure, I wanted to make sure they had everything installed that needed to be installed for all the components to work. It was in there as a hint more than anything.

This module actually has no direct dependency on token_filter, only on Token. It could easily be used by any other module that uses tokens.

I'll remove it from the .info file.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I'm not sure why the key needs to be check_plained. They key is referenced, but never inserted. The only place that keys are listed is in hook_token_list which this module doesn't provide (and if it did, the values would come from the token_request_params_filter variable which of course should be filtered in that case). It doesn't hurt to check_plain those values, though.

There is some existing work in #550164: Query string tokens but I think this is fine as a separate project because the need for it is so specialized.

three nitpicks:

  1. we don't do "@author or @copyright or @license" blocks in our repository, but that can be fixed later).
  2. you pass the $request variable by reference, but I'm not sure that it's necessary to do so and passing by reference in PHP 5.3 will cause errors, so you could remove that
  3. We don't generally do inline comments nor "END" comments like // END foreach

IMO, the design of the module where admins have to set which tokens are allowed and the proper use of check_plain make this a reasonable module. Another security point would be to add to the README.txt for the module that admins should put the token filter BEFORE the HTML filter.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

And to be clear: the nitpicks should not hold back approval of the module.

I also suggest to @athenus that you leave this module in a dev state for a while (like a few months) before releasing a 1.0 version.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

As far as I can see, I don't see any security problems with the module, and I think it can be useful.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Fixed

Basing on what reported from greggles, and what I have seen in the code, I am marking this report as fixed.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

*fixed* ;)

atheneus’s picture

atheneus commented

That's great! I appreciate you guys taking the time to provide a review and great feedback. All the points mentioned are well taken and will be included before the first release.

Again I really appreciate your efforts!

Status: Fixed » Closed (fixed)
Issue tags: -Module review

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Module review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Component: Miscellaneous » new project application
Assigned: Unassigned » avpaderno
Issue summary: View changes