Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
9 Feb 2012 at 17:14 UTC
Updated:
4 Jan 2014 at 01:39 UTC
Jump to comment: Most recent
Comments
Comment #1
phoenix commentedHi malcolmp
This could be a nice module for chess lovers.
- Could you add the git command to get your sandbox. This will make it easier for reviewers to quickly get your code:
git clone http://git.drupal.org/sandbox/malcolmp/1426274.git ltpgn- You still have code in the master branch. Please empty the master branch. The only thing that you can put on the master branch is a README.txt referring people to the feature branches. Check it here.
- Use the t() function on all of your strings. This is necessary for multilingual sites. This way they'll be able to translate them.E.g.: I see this in your hook_menu:
- I have the impression that you're not completely following the drupal coding standards. I'll check it with an automated script:
Here is the link if you want to run the script again to check your corrections: http://ventral.org/pareview/httpgitdrupalorgsandboxmalcolmp1426274git
There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-1.x branch:
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. Get a review bonus and we will come back to your application sooner.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #1.0
malcolmp commentedAdded git command
Comment #2
malcolmp commentedThanks for the quick response. I didn't have coder module set to pick up all the issues, and I had not seen drupalcs at all. I think I have fixed all those issues now.
Comment #3
kongoji commentedHi malcolmp,
Nice module, I like chess!
Here below some stuff I noticed during a manual review of your module:
1) Consider creating a .install file to delete variable
ltpgn_settingson module uninstall2) It's a very good thing that you use
sites/all/librariesfor your module, but wouldn't be more simple to use Libraries API for that?In some websites there is the needing to have specific libraries for each domain, so
sites/all/librariessometimes could becomesites/<domain_name>/libraries. Libraries API would manage this for you.3) Can you please document why you have validation checks for
'Frame height must an integer greater than 50','Frame width must be greater than 50'and'Maximum file size must be greater than 10'? Are them chess application limits?Greetz
Comment #4
kongoji commentedForgot to move the project to needs work status.
Comment #5
malcolmp commentedHi kongoji,
Thanks for taking the time to review my module.
1) I added a .install to delete variable ltpgn_setttings.
2) I thought the libraries API might be overkill, but it turned out to be easy, so I added that in. Also added it as a dependency and to installation instructions in README.txt
3) I thought there should be some validation on these fields. There is no special reason for these values, but if you had an IFrame as small as 50x50 you wouldn't be able to see the chess game, and if you had a file size of less than 10 then there wouldn't be many moves in the game! I guess its just finding some sensible minimum values. I added some documentation for this. Also added some descriptions on the form to explain what they are for.
Also reran the code review
thanks.
Comment #6
prashantgoel commentedplease visit http://ventral.org/pareview/httpgitdrupalorgsandboxmalcolmp1426274git for the list of errors being generated.
Comment #7
patrickd commenteddon't block deeper reviews because of minor issues found by automated reviews
Comment #8
crobinson commented1. README.txt has the wrong path for libraries (all/sites/libraries -> sites/all/libraries).
2. Please number or otherwise mark your instructions. It is hard to see these are separate steps.
3. The Drupal standard is not to include author names in code (@file comments in .install, etc.).
4. Whitespace formatting is inconsistent. Two lines in ltpgn.module (210-211), then one line (218, which is correct), then none (nothing between 229 and 230). Please review. When you do this, also correct the automated code review in http://drupal.org/node/1434388#comment-5843356.
5. You have implemented libraries_api, but not cleaned up lpgn_admin():
In general, avoid depending on DOCUMENT_ROOT (use base_path()), and there's no need to call ltpgn_settings() twice.
6. Maybe it is because I am new to PGN but I did not understand the max_size setting. Please add a #description that explains what the limit will do if the file is out of range. Will it be blocked? Ignored? Deleted?
7. 'chess pgn game' is a confusing permission since you also have a 'chess pgn admin'. Consider an alternative like 'chess pgn edit'.
8. This is just a suggestion but please consider making the file_directory_path() . '/pgn' storage location configurable in your admin settings. Hard-coding this is bad because there might be a file or directory called 'pgn' on the site already, and you do not detect this.
Note to other reviewers: I am still learning Drupal's security standards. This module executes a third-party library in an IFRAME on the local site. Because it is same-domain the IFRAME can access the parent container. Does this need additional review/vetting?
Comment #9
ankitchauhan commentedmanual review:
Major things you should fix:
http://drupal.org/node/318#naming
function _ltpgn_add_update($node) {}regards
Comment #10
malcolmp commentedHi everyone, thanks for the reviews.
crobinson
I have fixed points 1,2,3,4.
For point 5) I have removed the redundant call to ltpgn_settings(). base_path() only seems to give me the url of the viewer but I need DOCUMENT_ROOT in order to check the existence of a file. I wanted to put in a check that the viewer was correctly installed. I don't see any other way of doing this with the libraries api for D6. Have you any suggestions?
6)I added a #description
7)I changed chess pgn game to chess pgn user
8)I made the pgn storage location configurable.
Regarding the question about security, this is a good point. I guess there is not much I can do if I don't trust the author of the ltpgnviewer library, although it does occur to me that I can make it more secure by preprocessing the pgn file as it is loaded (to remove any html etc.) - so I will have a look at this.
ankitchauhan
I renamed _ltpgn_add_update() to ltpgn_add_update().
Please can you give me some more information regarding the permissions problem as I was unable to reproduce this error. Is default/files writable to the web server? Did default/files/pgn not get created or was it unwritable? did you get any error messages? I tried both as an admin and authenticated user on a fresh drupal 6 install and I could not get any problem. I added an extra check to the return code from file_check_directory() to make sure the pgn directory got created ok.
Thanks.
Comment #11
ankitchauhan commentedHi
my default/files writable to the web server and default/files/pgn get created but it wasn't unwritable
Comment #12
malcolmp commentedThat is very strange. default/files/pgn is created by file_check_directory() in the same way as other modules do. The chmod in there would have to be failing. If you use another module that creates a file, for example change the color scheme on the Garland theme, so that default/files/color is created does that work?
I made some changes to the error checking, so maybe you will get an error message out now. Please can you try again. Thanks.
Also I have added code to remove html tags from the .pgn file for security reasons, added an examples directory with a sample pgn file in, and mentioned this in README.txt.
Comment #13
malcolmp commentedSetting this back to needs review, as I have made required changes, and I cannot reproduce the permissions issue.
Comment #14
crobinson commentedI can't reproduce it either, and permissions are a frequent problem in Drupal. Maybe it was something server-related? In any event, the code changes look good to me. However, I'm adding the :security tag because I think I was supposed to do that last time around. I'm not sure anybody actually does anything with the flag, and stripping HTML is a very good idea, but at one point I had a message from Patrick that the security tag should be left even if an issue was fixed, for "statistical purposes."
Comment #15
misc commentedSuggestions:
Other than that:
Thanks for your contribution, malcolmp! 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 mind: Commit messages - providing history and credit and Release naming conventions.
Thanks to the dedicated reviewer(s) as well.
Comment #16.0
(not verified) commentedAdded link to example site