Problem/Motivation
The original issue was reported by Giacomo Gnecchi Ruscone who is an employee of Google. Here is his message:
"We'd like to know if the Drupal working group would consider no-following the comment sections by default to curb the abuse from spammers we're seeing on sites running on Drupal (even the latest versions).
A couple of examples of the type of stuff we're still seeing around:
https://goo.gl/rd2RBp
https://goo.gl/ZdtACn
https://goo.gl/bnnZz2
Thank you for the consideration, happy to provide additional details if needed and to take part in the discussion."
The problem is when a comment is posted it doesn't have the requested nofollow rel added its links in order to let search engines know the site wants to get these links to be not followed.
Proposed resolution
As @mlhess agreed before, the proposed way to resolve the issue is the following: Adding a new setting to comment type entity types, that is by default set to TRUE. It adds the nofollow rel to the links, if it's FALSE then it doesn't add it. As the interface of making this change on the website is already controlled by 'administer comment types' permission, there is no need for add new access control for this change.
Remaining tasks
Update description of the issue to follow the issue summary template.Implement the new setting for comment type entity type.- Manual reviews are needed.
- Test coverage for the new setting must be implemented.
- Documentations should be updated or even created for comment types.
User interface changes
When you create a new comment type under admin/structure/comment/types/add path:
When you edit the already existing 'Default comments' comment type under admin/structure/comment/manage/comment path, it already has the setting enabled:
If the setting is TRUE, the output will be like these:
If it's FALSE then:
API changes
No API changes are required.
Data model changes
No data model changes are required.
Original report by Giacomo-Google
Comment | File | Size | Author |
---|---|---|---|
#46 | enable_rel_nofollow-2927750-46.patch | 24.98 KB | tatarbj |
#31 | enable_rel_nofollow-2927750-31.patch | 17.11 KB | tatarbj |
#13 | nofollow_false_2.png | 152.35 KB | tatarbj |
#13 | nofollow_false_1.png | 159.93 KB | tatarbj |
#13 | nofollow_true_2.png | 150.27 KB | tatarbj |
Comments
Comment #2
mlhess CreditAttribution: mlhess as a volunteer commentedComment #3
mlhess CreditAttribution: mlhess as a volunteer commentedThe security team works with Giacomo on google related issues. I can confirm that he is a google employee.
Also I think that doing this makes sense. It would make Drupal less of an attack target if comment links were not followed by indexers.
Comment #4
tatarbjI'm starting to investigate on it
Comment #5
tatarbjHi folks,
I've created a patch that adds the nofollow rel to comments' titles and its permalink too. During the implementation i've noticed there is a bug that i also fix here in order to add properly the attributes to the permalinks (without this change the issue cannot be solved).
Maybe a more global solution could be to implement a setting to comment module that makes this rel parameter optional (by default added to the places where we need it) - let me know what do you think about it!
I've also updated the tests in order to check the nofollow rel exists.
Bests,
Balazs.
Comment #7
tatarbjOh a small mistake was given, here is the right patch (hopefully).
Comment #8
tatarbjComment #9
mlhess CreditAttribution: mlhess as a volunteer commentedI am not sure how this should be built, but I would think it could either me
1) a setting by content type by and comment that is enabled by default
2) Something with a permission, you have to have a perm to get the nofollow removed. (So by default you don't have the perm unless your an admin)
3) Moved to the filter html input filter, but that would have other far reaching complications.
Comment #10
pwolanin CreditAttribution: pwolanin at SciShield commentedI thought core already did this - here's some related old issues:
Comment #11
tatarbjThanks for the feedbacks @mlhess!
After thinking a bit about the proposed ways, I'm fully agree with the first one, but the others are a bit too far from what we would like to achieve here and without too much hassle the first seems to be able to be implemented - I've started to look for the hows and will get back when I have a version that could be checked in the next days!
@Pwolanin thanks for the references but i wouldn't attach them to here and the newest one is still just about D6, so it seems we have nothing as plan from the past for this to be addressed.
Bests,
Balazs.
Comment #12
tatarbjComment #13
tatarbjHi folks again!
I've finished a first version of the proposed solution which is implemented as the following:
- A new setting has been added to comment types called 'Nofollow' that is by default TRUE.
- When a comment type is attached to an entity type, then the output of a created comment entity will fetch the information by its comment type regarding this setting and will add to its's title and permalinks' rel attributes the nofollow value, if the setting is FALSE, then it will not.
Here are some screenshots in order to understand it better:
When you create a new comment type under admin/structure/comment/types/add path:
When you edit the already existing 'Default comments' comment type under admin/structure/comment/manage/comment path, it already has the setting enabled:
If the setting is TRUE, the output will be like these:
If it's FALSE then:
Currently tests are not done yet, but i'm working on them!
Any feedbacks are highly appreciated!
Bests,
Balazs.
Comment #15
tatarbjSorry, here is the right patch!
Comment #17
tatarbjBecause of the tests or creation comment types failed, I update the patch with making them passed.
Comment #18
tatarbjmessing up with other changes that are not meant for this issue... this patch is the right one, please use this for further discussion!
Comment #21
tatarbjto make the comparison-ish tests pass, i upload a new patch that adds the new setting to all the tests that i've found in core with its default setting - it seems that's a reason why the tests fail.
Comment #24
tatarbjone test was missing to be updated.
Comment #25
tatarbjComment #26
tatarbjand another missing test should be passed now.
Comment #27
tatarbjand there are tests outside of comment module that should be also modified for making this patch pass.
Comment #31
tatarbjadded more places where nofollow was still not set, hopefully nothing left behind.
Comment #32
tatarbjI've updated the description as now the implementation of the issue passes the tests.
Comment #33
tatarbjHere is the solution with test coverage (only previously existing tests got updated and a new test class implemented)
Comment #35
tatarbjas path is not previously loaded, tests failed in the new class. i've fixed it now.
Comment #36
tatarbj... and user is not logged in in the test so path will be still not available. now i fix this too.
Comment #39
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #41
tatarbjHi folks again,
I'm back from my holidays and checked what was wrong with the last patches, fixed it for now and also rolled it out on 8.6.x - let's see what CI says --- the current patch has the implementation and the new tests too!
Bests,
Balazs.
Comment #43
tatarbjTest node as an entity didn't exist, so i fixed it for this patch - let's see.
Comment #45
gregglesThanks for your work on this, tatarbj. Are you able to continue working on this and looking into the test failure? I think once that's done it's ready for a review.
Comment #46
tatarbjHey @greggles,
thanks for your words, I've left this issue a bit behind, now I'm giving a new chance to fix this last issue with the new test case.
Bests,
Balazs.
Comment #47
tatarbjComment #49
tatarbjI'm bringing this issue to the security improvements sprint of DrupalCamp Ruhr this weekend and work on it with sprinters - marking it with the tag of #dcruhr18_SecImproveSprint.
Test coverage should be finished and other tests may be required to bring it to rtbc which is the main target here.
Comment #50
tstoecklerComment #51
pwolanin CreditAttribution: pwolanin as a volunteer commentedThe feature request here is not clear to me. What is the problem to be solved? You don't want the links internal to Drupal to be followed? Or you want the nofollow attribute to be added to links in the content of the comment?
Comment #52
pwolanin CreditAttribution: pwolanin as a volunteer commentedMaybe the problem is that the default text format for comments doesn't have the filter_html_nofollow options set to TRUE. It seems like the request here is to create a default text format for comments with that turned on?
Comment #53
gregglesYes, I think either create one like that or modify the existing one to be like that.