The IP Address Entity module introduces an "IP Address Entity" on your site, supporting the following IP Address Types (ie. bundles):

  • Simple IP Address
  • Simple IP Address Range (defined via the 'start' and 'end' addresses of the range)
  • CIDR IP Subnet (defined via a 'network' address and numeric CIDR mask)
  • Network with Netmask (defined via a 'network' address and IP Netmask)

The module provides an admin interface at admin/structure/ipaddrs for the management of these IP Address types (bundles), as well as the definition of new 'custom' types.

It also supports full add/edit/delete/view functionality for individual instances of these IP Address types, and the use of fields to extend IP Address types.

IP Address Ranges can be created as standalone instances, or can also have individual Simple IP addresses created for each address within the range.

Sandbox Link: http://drupal.org/sandbox/jthorson/1171434

Posting this project as an alternative for review (relative to the behemoth of a module included in my first application).

Comments

sreynen’s picture

sreynen commented
Status: Needs review » Reviewed & tested by the community

This is still a pretty large project, so I was surprised when I found no issues looking through it.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

+1

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Reviewed & tested by the community » Needs review

I see a bit of unescaped output:

   5 function ipaddr_overview_types() {
   6   // Build an administration table
   7   $header = array(t('IP Address Type'), t('Machine name'), t('Description'), array('data' => t('Operations'), 'colspan' => '4'));
   8   foreach(ipaddr_types() as $type => $info) {
   9     $type_url_str = str_replace('_', '-', $type);
  10     $row = array($info->name, $info->type, $info->description); <--- UNESCAPED

I highly recommend reviewing the doc pages about Writing secure code. I would also recommend that you or any reviewers test the module with invalid or XSS input.

sreynen’s picture

sreynen commented
Status: Needs review » Needs work

Needs work, right?

jthorson’s picture

jthorson commented
Status: Needs work » Needs review

Updated output sanitization, removed some un-needed functions, and fixed an issue with cidr ranges with 255.255.255.255 broadcast addresses.

Sanitization occurs in the ipaddr_types() and ipaddr_type_name() function calls, so if it looks like it's missing at the output location, one of these was probably called one or two steps up the process chain.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Oh, you're right - that just kinda defies the usual standard of sanitize only on output - I wasn't looking at ipaddr_types() at all.

jthorson’s picture

jthorson commented

No, you were correct ... it was missing.

But when I went to add it, I realized that I have a few spots where I pass the output of ipaddr_types directly into theme_table ... so rather than adding code to walk through the array and sanitize at every one of those points, I added it at the database call. (Having nothing but a CLI editor at midnight last night, I guess I was feeling a bit lazy.)

My comment was in #5 was just to point this out for any followup review, because I realize it isn't quite as obvious, and might look like I hadn't actually addressed the issue.

jthorson’s picture

jthorson commented

As this was submitted as an alternative application to http://drupal.org/node/1144198 (posted May 1st), any objections if I up the Priority as per the newly implemented priority rules?

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

What module did you think this module had a duplication conflict with?

sreynen’s picture

sreynen commented
Status: Needs review » Reviewed & tested by the community

davidhernandez, I don't see any mention of duplication, so I'm confused about your question.

It looks like the issue in #3 has been addressed and all output is now sanitized. Everything else looks good to me, so marking RTBC.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Sorry, I should have been more clear. jthorson posted that on the "second opinion" thread. http://groups.drupal.org/node/157669 . Now that I think about it, was that just an example? I guess I'm think.

jthorson’s picture

jthorson commented

Yeah ... I just used my own link as an example ... sorry for the confusion!

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Reviewed & tested by the community » Fixed

You don't need the module or any include that doesn't contain a class in your files[]

I'd rather see a file header on every file (@file) and a better function header on every function.

Your README.txt is in DOS format I believe (with CR's). Might mess some things up.

Thanks to all for contributing and reviewing. git vetted user role granted.

Please review other applications so maybe we can catch up someday.

jthorson’s picture

jthorson commented

rfay,

Thanks for the feedback ... I didn't realize I hadn't gone back and cleaned up the documentation yet! I'll be sure to do that before promoting the project.

Thanks again!

Status: Fixed » Closed (fixed)

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