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:
create new commenttype

When you edit the already existing 'Default comments' comment type under admin/structure/comment/manage/comment path, it already has the setting enabled:
edit commenttype

If the setting is TRUE, the output will be like these:
when nofollow is true on permalink
when nofollow is true on comment's subject

If it's FALSE then:
when nofollow is false on permalink
when nofollow is false on comment's subject

API changes

No API changes are required.

Data model changes

No data model changes are required.

Original report by Giacomo-Google

CommentFileSizeAuthor
#46 enable_rel_nofollow-2927750-46.patch24.98 KBtatarbj
#43 enable_rel_nofollow-2927750-43.patch25 KBtatarbj
#41 enable_rel_nofollow-2927750-41.patch24.94 KBtatarbj
#36 enable_rel_nofollow-2927750-36.patch38.09 KBtatarbj
#35 enable_rel_nofollow-2927750-35.patch24.66 KBtatarbj
#33 enable_rel_nofollow-2927750-33.patch24.49 KBtatarbj
#31 enable_rel_nofollow-2927750-31.patch17.11 KBtatarbj
#27 enable_rel_nofollow-2927750-27.patch16.22 KBtatarbj
#26 enable_rel_nofollow-2927750-26.patch12.56 KBtatarbj
#24 enable_rel_nofollow-2927750-24.patch11.94 KBtatarbj
#21 enable_rel_nofollow-2927750-21.patch11.41 KBtatarbj
#18 enable_rel_nofollow-2927750-18.patch5.48 KBtatarbj
#17 enable_rel_nofollow-2927750-17.patch23.98 KBtatarbj
#15 enable_rel_nofollow-2927750-15.patch4.51 KBtatarbj
#13 enable_rel_nofollow-2927750-13.patch4.51 KBtatarbj
#13 nofollow_false_2.png152.35 KBtatarbj
#13 nofollow_false_1.png159.93 KBtatarbj
#13 nofollow_true_2.png150.27 KBtatarbj
#13 nofollow_true_1.png151.21 KBtatarbj
#13 create_new_commenttype.png78.51 KBtatarbj
#13 edit_commenttype.png81.31 KBtatarbj
#7 enable_rel_nofollow-2927750-7.patch3.72 KBtatarbj
#5 enable_rel_nofollow-2927750-5.patch3.66 KBtatarbj
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Giacomo-Google created an issue. See original summary.

mlhess’s picture

Version: 9.x-dev » 8.5.x-dev
Component: link.module » comment.module
mlhess’s picture

The 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.

tatarbj’s picture

Assigned: Unassigned » tatarbj

I'm starting to investigate on it

tatarbj’s picture

Status: Active » Needs review
FileSize
3.66 KB

Hi 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.

Status: Needs review » Needs work

The last submitted patch, 5: enable_rel_nofollow-2927750-5.patch, failed testing. View results

tatarbj’s picture

Oh a small mistake was given, here is the right patch (hopefully).

tatarbj’s picture

Status: Needs work » Needs review
mlhess’s picture

I 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.

pwolanin’s picture

tatarbj’s picture

Thanks 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.

tatarbj’s picture

Status: Needs review » Needs work
tatarbj’s picture

Status: Needs work » Needs review
FileSize
81.31 KB
78.51 KB
151.21 KB
150.27 KB
159.93 KB
152.35 KB
4.51 KB

Hi 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.

Status: Needs review » Needs work

The last submitted patch, 13: enable_rel_nofollow-2927750-13.patch, failed testing. View results

tatarbj’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

Sorry, here is the right patch!

Status: Needs review » Needs work

The last submitted patch, 15: enable_rel_nofollow-2927750-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tatarbj’s picture

Status: Needs work » Needs review
FileSize
23.98 KB

Because of the tests or creation comment types failed, I update the patch with making them passed.

tatarbj’s picture

messing up with other changes that are not meant for this issue... this patch is the right one, please use this for further discussion!

The last submitted patch, 17: enable_rel_nofollow-2927750-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 18: enable_rel_nofollow-2927750-18.patch, failed testing. View results

tatarbj’s picture

Status: Needs work » Needs review
FileSize
11.41 KB

to 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.

The last submitted patch, 18: enable_rel_nofollow-2927750-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 21: enable_rel_nofollow-2927750-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tatarbj’s picture

one test was missing to be updated.

tatarbj’s picture

Status: Needs work » Needs review
tatarbj’s picture

and another missing test should be passed now.

tatarbj’s picture

and there are tests outside of comment module that should be also modified for making this patch pass.

The last submitted patch, 24: enable_rel_nofollow-2927750-24.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 26: enable_rel_nofollow-2927750-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 27: enable_rel_nofollow-2927750-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tatarbj’s picture

Status: Needs work » Needs review
FileSize
17.11 KB

added more places where nofollow was still not set, hopefully nothing left behind.

tatarbj’s picture

Issue summary: View changes

I've updated the description as now the implementation of the issue passes the tests.

tatarbj’s picture

Here is the solution with test coverage (only previously existing tests got updated and a new test class implemented)

Status: Needs review » Needs work

The last submitted patch, 33: enable_rel_nofollow-2927750-33.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tatarbj’s picture

Status: Needs work » Needs review
FileSize
24.66 KB

as path is not previously loaded, tests failed in the new class. i've fixed it now.

tatarbj’s picture

... and user is not logged in in the test so path will be still not available. now i fix this too.

The last submitted patch, 35: enable_rel_nofollow-2927750-35.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 36: enable_rel_nofollow-2927750-36.patch, failed testing. View results

Ivan Berezhnov’s picture

Issue tags: +CSKyiv18

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tatarbj’s picture

Status: Needs work » Needs review
FileSize
24.94 KB

Hi 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.

Status: Needs review » Needs work

The last submitted patch, 41: enable_rel_nofollow-2927750-41.patch, failed testing. View results

tatarbj’s picture

Status: Needs work » Needs review
FileSize
25 KB

Test node as an entity didn't exist, so i fixed it for this patch - let's see.

Status: Needs review » Needs work

The last submitted patch, 43: enable_rel_nofollow-2927750-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

greggles’s picture

Thanks 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.

tatarbj’s picture

Hey @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.

tatarbj’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: enable_rel_nofollow-2927750-46.patch, failed testing. View results