hypercaster

Sorry for the long delay. Some notes on the module:

• The $Id$ lines are not necessary, no matter what the Coder module tells you, since drupal.org no longer uses CVS.
• You don't need to include your own copy of the GPL, since when modules are released on drupal.org, LICENSE.txt is added automatically.
• The convention is generally to make permissions strings all lowercase to prevent errors when using incorrect capitalization, so "Administrate HyperCaster" and such should be lowercased.
• All of your functions should be prefixed with the module name, so functions like editconfig_xml() should be hypercaster_editconfig_xml(). This prevents modules from accidentally using the same function name and causing errors.
• In hypercaster_defaultconfig_xml(), you probably should not use $base_url, as it includes the "http://" portion of the URL -- and so file_get_contents() will make an HTTP request to retrieve the file, rather than simply reading it off the disk. Removing $base_url should work much better.
• In your README.txt, lines should wrap at 80 characters.

That's all I've found in a quick look through, and I haven't actually installed the module. But seeing as you've been waiting over a month, I figured any useful feedback would help you out.

I am also slightly worried that hypercaster_build_embed_code() and hypercaster_embed_display() create raw HTML without sanitizing any of their inputs. Is it possible for a user to enter parameters with HTML code in them and have them output by hypercaster? If so, you need to use check_plain() on each variable output in the raw HTML to ensure they are properly sanitized.

Hi Alex

Thanks for your notes and feedback! Notes have now been fixed and committed. If you want to install and test the module you need to put the Hypercaster-player folder in the Hypercaster module directory. A version of the Hypercaster-player that works with the Drupal module can be downloaded as a zip from http://people.dsv.su.se/~brom/hypercaster-player-download/

If you don't want to install the module you can find a working example at http://play.dsv.su.se/node/1344.

If I understand you correctly I think the only actual place where a user can insert html is in the embed code and then let hypercaster_embed_js() run it. I have wrapped the arguments sent to hypercaster_embed_js() with check_plain() now so I think it should be safe now.

A couple of comments about hypercaster_embed_js():

function hypercaster_embed_js() {
  $id   = check_plain(arg(1));
  $width  = check_plain(str_replace('width=', '', arg(2)));
  $height = check_plain(str_replace('height=', '', arg(3)));
  $node = node_load($id);

* For node_load(), I think check_plain() is unnecessary since the input will be sanitized by db_query() later on.
* For $width and $height it would be better to just cast them to integers IMO: $width = (int) str_replace('width=', '', arg(2));.

You can sanitize the input in hypercaster_embed_js(), but it's still hard to tell for sure that there are no other ways that the variables inserted in the HTML in hypercaster_build_embed_code() and hypercaster_embed_display() can't be spoofed somehow. The easiest way to deal with that is to sanitize the variables at the same time as they're being inserted:

$object_params[] = '<param name="movie" value="' . check_plain($player_path . $vars) . '"></param>';

That way you don't have to trace the function calls back to the actual user input to make sure that it's sanitized – there is no way of bypassing it.

Hi Kleve,

I think this is good enough to approve now. I do have some minor feedback I hope you can address before making this a full project:

• Place the downloaded hypercaster folder within you hypercaster module folder without renaming it. Putting files in your module folder can complicate updates. I'd recommend using the Libraries API instead. Regardless of where you put the files, you should check if the files are actually there before you try to use them. hook_requirements() is a good place to do this.
• You should remove the mention of the GPL from individual files. The standards Drupal.org LICENSE.txt file will be added to downloadable releases automatically.

I don't think you really need the GPL license stuff at the top. A LICENSE.txt file is added to your project by Drupal.org.

I'd say add missing code comments (ex: Implements hook_nodeapi()., Implements hook_perm().) and maybe some extra comments on the admin form but those are pretty minor.

Those are all pretty minor so I think you're ok to go.

Thanks for your contribution, Kleve! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.