Those who have created templates to be used with CK editor know that including them in the editor is a painful process because it requires adding HTML in a javascript configuration file inside the CK editor module.

This module defines its own node type and each node of this type will be available as a template in the CK editor. Optionally, the Codemirror javascript library can be dropped into the codemirror directory of the module to get coloured HTML syntax and automatic indentation for creating new templates.

Comments

wkjagt’s picture

wkjagt commented

Link to the project page: http://drupal.org/sandbox/wkjagt/1078178

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Needs work
Issue tags: +PAreview: security

Sorry this has taken so long to get a review. You might want to contact the maintainers of the ckeditor module to see if they have feedback on this. They may be more interested in having it as a patch to their module than a separate project, but either way it's best to involve them in the discussion since they are in the best place to provide a review/advice on the module.

Suggestions

I think your project would be helped with a README.txt file.

http://drupalcode.org/sandbox/wkjagt/1078178.git/blob/refs/heads/master:... looks like it uses tabs instead of 2 spaces for indentation.

The need for your hook_nodeapi isn't clear to me. It would be great to add more comments explaining what's happening.

ck_templates.css does not follow the CSS standard formatting.

Required changes

In ck_templates.tpl.php print $node->body; is an XSS injection vulnerability. You should be using check_markup.

_ck_templates_fetch is vulnerable to access bypass because it does not check that the user has the rights to edit these nodes. It would be ideal to use node_access or db_rewrite_sql to verify that the user can see the nodes you are fetching.

Instead of _ck_templates_escape_for_javascript can you use check_plain or filter_xss($string, array()); ? I'm not confident that your code to avoid javascript is robust enough to handle all types of xss.

As I read the code a bit more it seems like the configuration necessary to use this module is:
1. People who can create this node type must have access to an input format that allows them to inject XSS (based on this help tip).
2. Drupal's regular node access might not make sense because the module defines it's own permissions for "view own templates" and "view all templates" which could be considered to be more important than the Drupal node access system.

If you document those two facts in the README.txt then I think the module could be ready without addressing all of the items I raised above (the issue with _ck_templates_escape_for_javascript should still be addressed). That said, it seems like node storage (or at least storing in node bodies) might not be the best way to store this information since you are having to skip passed some of the things that are usually features of the node system.

misc’s picture

misc commented

The applicant has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

misc’s picture

misc commented
Status: Needs work » Closed (won't fix)

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256#abandonedtwoweekscontact