[D6] UC Heureka

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxblazas1907106git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Review :

1) Please add README.txt, which describes your module information and configuration.
2) Your code repository is in master branch, create new custom branch eg: 6.x-1.x, move your code to it, and then delete the master branch.
3) Please add the URL of sandbox module, git clone repository.

Hi Blazas,

I can't see in the documentation anywhere that the your module depends on PHP's curl module.
Making that dependency explicit might prevent other users being confused on installation.

Also MENU_NORMAL_ITEM in hook_menu is redundant because it's default value.

Hi again,
I forgot to mention that hook_uninstall should be implemented to wipe out the variables this module generates.:q

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

Hi.

As already mentioned by PawelR, this module relies on the cURL extension to make requests. But neither Drupal, nor Ubercart require cURL, so it is not available on all installations. If anyone tries to enable this module without cURL extension they will get a Fatal error.

Take a look at Paypal module to learn how to properly check for cURL.

** uc_heureka.module, line 123

This line uses tab for indentation, but previous line uses spaces. Do not mix tabs and spaces.

** uc_heureka.install, line 9

Function comment mixes Drupal 6 and Drupal 7 styles. In Drupal 6 you would normally write Implementation of hook_uninstall(), in Drupal 7 it is standard to write Implements hook_uninstall().

uc_heureka.info, lines 6 and 9

The .info file has two core declarations.

You know, here is actually a patch that adds a hook_requirements() implementation with the cURL check. Just make sure to test that the patch actually works!!! Because I didn't test it ;)

I trust you will fix other minor issues yourself.

Removing review bonus tag, no reviews listed in the issue summary. Make sure to read #1410826: [META] Review bonus gain.

Removing review bonus tag, no reviews listed in the issue summary. Make sure to read #1410826: [META] Review bonus again.

Hi blazas

I don't know if you were aware, but there are still a few minor comments from PAReview on comment style

http://ventral.org/pareview/httpgitdrupalorgsandboxblazas1907106git

One other thing I noticed is that the git branch is named 6.x-1.0-beta3. The git branch should be named 6.x-1.x, and then you would (when it's a full project) create a release named 6.x-1.0-beta3 on that branch.

Looking through the code, it's hard to understand at points because I'm not familiar with the Heureka service. However I am familiar with Ubercart for Drupal 6. The uc_heureka_set_conversion_js function is adding Javascript to the page. What it does is add a script portion to the page being rendered, which contains a function to add more script, and that function is called the minute the page is displayed in the browser. Is there a reason why it's done that way, as opposed to adding all the JS that is required in one go?

Sorry I wasn't clear - by "the minute" I meant "the instant" or "the moment", not a 60s time span.

That makes sense if you have to include the JS as given.

Hi Blazas,

I've found one more for you.

In uc_heureka_set_conversion_js, you call variable_get('uc_heureka_api_conversion_key_field', '') and embed the result directly into the JS that goes into the page.

That variable is set in the admin.inc file via a form, where the site administrator can enter the value for the conversion key.

Nothing is done to validate that value. It's nearly as simple as embedding something like alert() into the conversion key that is entered to provide proof of concept that this allows arbitrary script injection. The severity of that would be mitigated by the fact that a permission is needed in order to set that variable, but it is a security flaw nonetheless.

You need to call a _validate() function on the form in your admin.inc file to check that the value is safe.

on the http://ventral.org/ is a problem because UC Heureka is just for D6 and comment functions for D6 is "Implementation". "Implements" is a standard for D7 and I guess script on http://ventral.org/ doesn't distinguish D6 and D7. So I don't what I have to do because it is correct.

The Drupal coding standards are the same for *all* branches. However, the coding standards have been revised over the years since many D6 modules were written, so you will find many D6 modules written to the old standards. Old code does not have to be re-written for the new standards, but any new code, like the module you're submitting, really should conform to the current standards.

Ventral *does* recognize different branches. Read the example usage at http://ventral.org/pareview to see how to specify a particular branch for code review. The default is the highest version branch, if a branch isn't specified.

Thank you for notifying the branch name. Now I create branch with name 6.x-1.0-rc1.

6.x-1.0-rc1 is also an invalid branch name. It should be 6.x-1.x as suggested in #15. Please read the branch naming conventions.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

trying again to remove tag.

branch has been renamed - 6.x-1x

You should also delete all the other branches.

thank you for your security report. So I added _validate() function and validating both of API key - for JS and Curl.

I looked in all the files in http://drupalcode.org/sandbox/blazas/1907106.git/tree/refs/heads/6.x-1.x and couldn't find that validation anywhere. I must be missing something: What file is it in, and what line number does it start on?

If you click on this link http://drupalcode.org/sandbox/blazas/1907106.git/tree/refs/heads/6.x-1.x, can you see an admin.inc file? For some reason I can't.

Removing review bonus tag, no reviews listed in the issue summary. Make sure to read #1410826: [META] Review bonus again.

removed automated reviews.

I removed the review comments from the issue summary that were only copies of automated ouputs. Again, make sure to read through the source code of other projects and provide direct feedback on that.

Review of the 6.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/uc_heureka.admin.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   44 | ERROR | Files must end in a single new line character
  --------------------------------------------------------------------------------
  
  Time: 0 seconds, Memory: 6.00Mb
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. "watchdog('heureka', 'Sending URL ' . $force_url, NULL, WATCHDOG_INFO);": do not concatenate variables into watchdog messages, use placeholders instead.
2. "watchdog('heureka', $result, NULL, WATCHDOG_WARNING);": This is vulnerable to XSS exploits. You never know what the return value of the service is and whether it contains malicious text - it is untrusted third party provided input anyway. You need to sanitize messages before sending them to watchdog. That can be done by using the appropriate placeholders for dynamic variables. Please read http://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. uc_heureka_set_user_data(): why do you need cURL and cannot use drupal_http_request()? Looks like a pretty simple request to me?
4. uc_heureka_set_conversion_js(): You are concatenating the product title directly into the script, which is user provided text and needs to be sanitized as well. I would recommend to use Drupal.settings for passing down variables from PHP to JS, then they get escaped in JSON-encoded settings array.. See http://drupal.org/node/304258
5. uc_heureka_settings_form_validate(): doc block is wrong, see http://drupal.org/node/1354#forms

You can add the review bonus tag again if you have done another 3 reviews of other projects.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

Edit first link