[D7] Poptin

* @branch is the wrong git branch name, you need to use 7.x-1.x
* Can you fix up all the coding standard errors from https://pareview.sh/pareview/https-git.drupal.org-project-poptin.git so that we have an easier time reviewing?
* watchdog("CURL Res","<pre>".print_r($err,true)."</pre>");": this looks vulnerable to XSS exploits. $err is coming from a cURL response, so we have to treat it as untrutest input. You need to use placeholders with watchdog.
* poptin_page_build(): the section in the render array should not be called "devel".
* poptin_signup() and poptin_login(): this is vulnerable to CSRF exploits. Data changing operations must be done through Drupal's form API or you have to validate a CSRF token yourself in code.
* poptin_page_build(): this looks vulnerable to XSS exploits. The user provided client ID from the database is printed into HTML without sanitization.

And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Right here https://git.drupalcode.org/project/poptin/blob/@branch/poptin.module#L161

$res = json_decode($response);

You are using json_decode, while this isn't a vulnerability in itself, you are using the response without any validation that the json object is what it expected to be.

poptin_render_html() is just a blob of html. This will be very bad DX if anyone wants to alter the output at all. Also, it contains lots of inline js: <a href="javascript:void(0)" class="pplogout">Deactivate Poptin</a> --which I would have to review your js to really comment on. Normally this shouldn't be handled by an anchor but instead should use a button element.

This module includes SweetAlert Javascript library which isn't GPLv2 and cannot be hosted on D.o

Also, shared accounts aren't allowed to commit on Drupal.org repositories. Commits can be done only from accounts used from a single user.

* poptin_page_build(): the section in the render array should not be called "devel".

what we call instead of "devel" ? @klausi
i need to add id and async="true" in script tag as like : <script id="pixel-script-poptin" src="https://cdn.popt.in/pixel.js?id=testetste" async="true"></script>

Instead of $page['page_bottom']['devel'], it should be $page['page_bottom']['poptin'] or something similar that starts with the module machine name.

Okay Its working

Hi,
Install coder , to check issues and solved the issue now there is no error showing on it . Click here to see screenshot of coder review's Report.

Also, the sweetalert.min.js file is licensed under the Academic Free License, which isn't compatible with the GPL-2+ license drupal.org repositories use.

So which one we Use instead of sweetalert.min.js ?

So which one we Use instead of sweetalert.min.js ?

If you have to use sweetalert, then find a way to include it as a dependency in composer/yarn/npm rather than put it's source code in the project repo.

Really, there is no need to add a modal library. Drupal comes with jquery and you can use that for modals if you need to.

Nobody has replied to #4.

There are still issues in the code, most of which has been already reported.

1. The @branch branch has not been removed
2. The code doesn't follow the Drupal coding standards
3. In poptin_page_build(), the render array section should not be called "devel"
4. poptin_signup() and poptin_login() should not access any data through $_POST
5. watchdog() is not correctly used, especially for the part concatenating dynamic values into a single string
6. Redirection in Drupal is done with a Drupal function, not header()
7. The table used from the module cannot be described as The base table for nodes. since it's not what the table contains
8. A module doesn't need to drop the tables it uses in hook_uninstall(), since that is already done from Drupal core
9. poptin_render_html() needs to be rewritten; a form is created using the Drupal form API
10. Module functions need to have a name starting with the module machine name and an underscore; is_poptin_user() is not a correct function name
11. It's not clear what poptin_redirect_link() is doing: It loads the content of the database table, checks if two database fields aren't empty, and (for each database line for which those two fields aren't empty strings) sets $redirect_link to 'https://app.popt.in/overview'
12. It's not clear what the two template files are for, since the module is not altering the template files used from any page

klausi explained which security issues are in the code. I will not repeat here what he said, but those issues need to be fixed.

I have removed @branch and set 7.x-1.x as default, and do lots of changes are u mentioned, Please review it .
Thanks

@ravi2432 May you point out which of the commits listed on https://www.drupal.org/node/3045139/commits are done from you?

- The @branch branch has not been removed -> done and set 7.x-1.x as default branch
- The code doesn't follow the Drupal coding standards -> check this link https://pareview.sh/pareview/https-git.drupal.org-project-poptin.git-7.x...
- In poptin_page_build(), the render array section should not be called "devel" -> i changed it and set used "poptin"
- poptin_signup() and poptin_login() should not access any data through $_POST -> we used this $query_params_get = drupal_get_query_parameters($_GET);
- watchdog() is not correctly used, especially for the part concatenating dynamic values into a single string -> watch dog removed
- Redirection in Drupal is done with a Drupal function, not header() -> used drupal_goto function
- A module doesn't need to drop the tables it uses in hook_uninstall(), since that is already done from Drupal core -> removed hook_uninstall() function
- Module functions need to have a name starting with the module machine name and an underscore; is_poptin_user() is not a correct function name -> function is removed
- It's not clear what poptin_redirect_link() is doing: It loads the content of the database table, checks if two database fields aren't empty, and (for each database line for which those two fields aren't empty strings) sets $redirect_link to 'https://app.popt.in/overview' -> we check the value , if user is login then we redirect to our website
- It's not clear what the two template files are for, since the module is not altering the template files used from any page -> we used one template file another one is removed

git command is not working , so we did work on git from browser directly.

Thanks

Please check and let me know issues

The question I asked has not been answered. Let's start from the begin.

All the commits done on the project, including the changes done from https://git.drupalcode.org/project/poptin are listed on https://www.drupal.org/node/3045139/commits. What I asked to ravi2432 is which of those commits have been done from him. With that, I am not asking a description of the changes ravi2432 did, but which of those commits have been done from ravi2432. For example, did ravi2432 did the commit 2b5efea, or the commit 1b9809b?

In other words, which of the commits listed on https://www.drupal.org/node/3045139/commits have been done from ravi2432?

I did not commit anything, all commit done by Poptin user.
I just assist him.

Comment #20 says:

I have removed @branch and set 7.x-1.x as default

Technically, removing a branch is doing a commit.

So all is good now?

OK, now we are getting closer to what I was asking.

How many developers are writing the code of this module? What are their drupal.org usernames?
How many people commit the code on the drupal.org repository? What are their drupal.org usernames?

In that case, it should be ravi2432 to apply.

`poptin` will comment soon

See comment #33.

ravi2432 is developing the module.

If ravin2432 is developing the module, it's him to write the code. How can the whole development be done by another user?

Shared accounts aren't allowed to commit on Drupal.org repositories.

If Potpin the account is going to be committing on the repo then Poptin the account needs to be one person and not many.

Seems like ravi2432 is the account that should actually be applying here.

Since it's ravi24f32 the developer, it's ravi2432 who should apply for this application, since:

• These applications are per user, not per project
• These applications are for the user who writes the code, not the team for which that user works

As per comments written here, and the commits listed on https://www.drupal.org/node/3045139/commits, I believe the poptin account is used from more than one person. The fact the account and the project share their name (and the image) with the domain name used by a company makes me believe more in this.
Anyway, since the user who applied for this application is not the user writing the code, this is quite secondary.

I think we can continue here and approve ravi2432. They can then be added to the project and opt into security support for the project.

Another round of review:
* poptin_menu(): Why are you using the "administer users" permission? Your module is not really related to adminsitering user accounts, so you should create your own permission.
* poptin_page_build(): doc block is wrong, should be "Implements hook_page_build().", see https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
* poptin_page_build(): Your are directly printing the client ID to HTML without sanitizing it. This is currently not a security issue because an attacker would need the "administer users" permission to insert a malicious client ID. You should still sanitize it to not allow XSS attack vectors via the client ID.
* poptin_render_html(): it is a bit ugly to have a large blob of HTML here, you should use teh template system and put this into a template file.
* "'mssg' => "User Registered successfully",": all user facing strings must run through t() for translation.
* you should not have any die() calls in your code, use drupal_json_output() or similar functions to send JSON correctly.

Otherwise I did not see any security issues anymore, so this should be good to go.

Thank you for your contribution! I am going to update ravi2432's account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

* poptin_menu(): Why are you using the "administer users" permission? Your module is not really related to adminsitering user accounts, so you should create your own permission. -> So I created My own permissions.
*poptin_page_build(): doc block is wrong, should be "Implements hook_page_build(). -> done
* poptin_page_build(): Your are directly printing the client ID to HTML without sanitizing it. This is currently not a security issue because an attacker would need the "administer users" permission to insert a malicious client ID. You should still sanitize it to not allow XSS attack vectors via the client ID. -> i don;t know How to did, i used this $client_id = filter_var($poptin_db['client_id'], FILTER_SANITIZE_STRING);. is anything i have to use please let me know.
* "'mssg' => "User Registered successfully",": all user facing strings must run through t() for translation. -> done.
* you should not have any die() calls in your code, use drupal_json_output() or similar functions to send JSON correctly.-> done. i used drupal_json_output() and for die i used drupal_exit();.

please check and if all ok then you can remove the tag
This project is not covered by Drupal’s security advisory policy.

Thanks

See the list of sanitization functions. (Don't use the ones with a name starting with an underscore.)
In the specific, check_plain() is the function to use, as the client ID is not HTML markup.

We gave you the role to opt into security coverage in your present and future projects, which include also https://www.drupal.org/project/poptin. It's your task to edit your projects to opt into security coverage.

@Kiamlaluno
if i have to release a new version of my module in Drupal 7
than we have to tag like 7.x-1.0.1 ?
or there is any other way?

@ravin2432 The tag should be 7.x-1.1. See also Release naming conventions, which says the release and the tag names to use.

HI Kiamlaluno,

I have uploaded the Drupal 8 code on git, can you please review it, "https://git.drupal.org/project/poptin.git 8.x-1.x".

So If there is any issue please let me know, and i will resolve it , and release for drupal 8.

Thanks

@ravi: we are done with reviewing for the purpose of approving you here, so this issue is closed. Please ask your colleagues or other community members to do any follow-up reviews.

Thanks!