This module tracks each user event like User Login, User Logout and User Node view events and also store these event data in your database. This data is then exposed to views so you can report on it if required.
Project link
https://www.drupal.org/project/rtub
Git instructions
git clone --branch 8.x-1.x https://git.drupalcode.org/project/rtub.git
PAReview checklist
https://pareview.sh/pareview/https-git.drupal.org-project-rtub.git
Comments
Comment #2
apadernoThank you for applying! rtub-dev.8.x isn't a correct branch name, which eventually would be 8.x-1.x. Wrong branches need to be removed.
Comment #3
maheshkp92Comment #4
maheshkp92I have deleted the incorrect branch called "rtub-dev.8.x" from these project repository.
Thanks for your support!
Comment #5
VernitPAEeview say readme missing. Please add it
According to Drupal standards, modules should include a useful README.file.
https://www.drupal.org/docs/develop/documenting-your-project/module-docu...
Comment #6
VernitComment #7
VernitPlease create a dev release to enable automated testing.
Comment #8
VernitComment #9
vuilThank you for the contribution!
Please, fix the Pareview mentioned issue (at first):
README.md
orREADME.txt
is missing, see the guidelines for in-project documentation.Then the project will go through the manual security review process.
Please, be patience through the whole review process.
Comment #10
maheshkp92I have fixed all PAReview issues for this project. I have also created dev release "8.x-1.x" for this project.
Thank you all for your support!
- Mahesh Patil
Comment #11
vuilThank you for the contribution!
1. Please, update the deprecated constant
REQUEST_TIME
into your code:The
REQUEST_TIME
global constant has also been deprecated, scheduled to be removed before 9.0.0See more: https://www.drupal.org/node/2785211
Replace the
REQUEST_TIME
with the\Drupal::time()->getRequestTime()
2. Replace the deprecated
UrlGeneratorTrait
:trait
\Drupal\Core\Routing\UrlGeneratorTrait
is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use\Drupal\Core\Url
instead.https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Routing%2...
3.
EntityManagerInterface
is deprecated in the latest Drupal 8 version and should be replaced withEntityTypeManagerInterface
.https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
Comment #12
maheshkp92Thanks for providing valuable feedback.
As per your recommendation, I have removed the following deprecated function from this Module and replace it with the appropriate functions.
Thanks,
Mahesh Patil
Comment #13
vuilI have not found any security related issues into the code.
Comment #14
klausiThanks for you contribution!
Please fix vulnerability 1 and 2, then we can continue here.
Comment #15
parvind87 CreditAttribution: parvind87 at Icreon commentedThanks for your contribution.
Review of the 8.x-1.x branch (commit dfcf442):
No automated test cases were found, did you consider writing PHPUnit tests? This is not a requirement but encouraged for professional software development.
Comment #16
maheshkp92@klausi,
I have fixed following 2 vulnerability in below commit -https://git.drupalcode.org/project/rtub/commit/e62f7c7075f22ca2150e623fd...
1. PostNodeDataController: this is vulnerable to access bypass attacks. An attacker can send an arbitrary user ID, so they can write arbitrary events for other users. You need to take the user ID from the current logged in user and not from the sent POST data.
2. PostNodeDataController: a second vulnerability is the event type. An attacker can send arbitrary event types and messages, so they could send fake logout events for example. You could generate an HMAC for the JS settings you are creating with Crypt::hmacBase64(). The data would be a string concatenated of all the setting values. The key would be Settings::getHashSalt(). If you exclude the user ID then it would even work for cached node pages since the HMAC would be the same for all users.
Thanks,
Mahesh Patil
Comment #17
vuilComment #18
apadernoComment #19
maheshkp92Please provide Security Approval for this module.
Thanks,
Comment #20
batkorError in this line , add a space
https://git.drupalcode.org/project/rtub/blob/8.x-1.x/src/Controller/Post...
Best practical used function.
#Example
And use
Please view
csrfToken
and system routesystem.csrftoken
for get user token.UP:
Why use js for check node view?
Comment #21
maheshkp92Hi @batkor,
Thank you for your evaluation.
I have fixed most of the issues below commit.
https://git.drupalcode.org/project/rtub/commit/942ad5967f2352783d1bbae2a...
Why use js for check node view?
I have tried to trigger node view events using various hooks like entity_view, various hook_alter. But, the node view event is not captured by the above-mentioned hooks on a cached page, so that why I am using js to capture node view event on the cached/non-cached page in D8.
Comment #22
batkor@maheshkp92 Good question.
Comment #23
vuilI update the summary only.
Comment #24
apadernoComment #25
maheshkp92Please help me closing the issue to release the stable version of this module.
Thanks,
Mahesh Patil
Comment #26
jayprakash01 CreditAttribution: jayprakash01 at Material commentedThanks for the contribution @maheshkp92 !
I have not find any security related issues in the code.
Thanks
Comment #27
batkorComment #28
apadernoHashes are not checked using the
==
operator. There is a PHP function to compare two hashes, which is used also from Drupal.Since the code isn't executed for the anonymous users, it probably makes more sense to use the csrf_token service.
There isn't no need to use
print_r()
to pass a string towarning()
. Something went wrong is also a message that doesn't allow to understand what happened; it should be replaced from the message associated with the exception.Every function should be prefixed from the module machine name. A comment on this application already suggested to remove it and use the string directly.
Comment #29
maheshkp92Hi, kiamlaluno
I have fixed below issues:
All the above issues fixed in below commit:
https://git.drupalcode.org/project/rtub/commit/3325915b0a1af6ad962ac242e...
Thanks!!
Comment #30
AkashKumar07 CreditAttribution: AkashKumar07 at OpenSense Labs commentedComment #31
apadernoThank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:
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.
Comment #33
apaderno