Pageless Node titles still link to the node page even when access is denied

[jenlampton]

Description of the bug

When a pageless node renders via it's node.tpl.php file, the heading is still wrapped in a link, even when the person viewing the node does not have access to view the page the link links to.

Steps To Reproduce

To reproduce the behavior:

1. Create a pageless node
2. Create a block view of pageless nodes (full view mode)
3. Place the view block on the home page
4. Log out
5. Visit the home page

Actual behavior

Node titles are linked to node pages, even though anons cannot access these pages.

Expected behavior

Node titles should not be linked to node pages when the viewer cannot access these pages.

---

This issue may be a blocker for https://github.com/backdrop/backdrop-issues/issues/4903

[jenlampton]

I was thinking that we might be able to remove the links around node titles in preprocess_node, but looking at the node template, the anchor tag is hard coded:

<h2><a href="<?php print $node_url; ?>" rel="bookmark"><?php print $title; ?></a></h2>

The only way to print a node title without this link would be to change the template file itself.

We currently have some PHP checks around the title for if we are on the node page, and if the title exists:

  <?php if (!$page && !empty($title)): ?>
    <h2><a href="<?php print $node_url; ?>" rel="bookmark"><?php print $title; ?></a></h2>
  <?php endif; ?>

We would need to add an additional check, and then print a title without a link as an alternative. I debated adding a new variable like "Hidden", but then decided it would be simpler to check the variable we already have $node_url, since we can modify that in preprocess.

This would also allow more future flexibility, as there could someday be other reasons not to print a link tag around the node title.

  <?php if (!$page && !empty($title)): ?>
    <?php if (!empty($node_url)): ?>
      <h2><a href="<?php print $node_url; ?>" rel="bookmark"><?php print $title; ?></a></h2>
    <?php else: ?>
      <h2><?php print $title; ?></h2>
    <?php endif; ?>
  <?php endif; ?>

Adding additional checks into node.tpl.php will solve the problem for new sites, but anyone who has overridden the template file in a custom (or contrib) theme, wouldn't benefit from this bugfix. I'm thinking this is preferable to running the risk of affecting existing sites.

[stpaultim]

I created a patch based upon this PR and applied it to the local version of 1.x with my Pull Request for https://github.com/backdrop/backdrop-issues/issues/4903. With this patch applied

1. As an admin the titles for the 3 Promoblocks were still links to the content. But, when I logged out and viewed the page as an unauthenticated user, the Titles were no longer links.

I'm going to mark this "Works for Me."

Also giving this the "Milestone Candidate" label. This is a blocker for https://github.com/backdrop/backdrop-issues/issues/4903

[klonos]

This makes sense, and the solution seems reasonable to me 👍🏼 ...code is minimal and looks good, but there's a stray space and I'm wondering if the URL should be set to blank instead of FALSE. Otherwise RTBC.

[docwilmot]

Just tested this and although it works, I feel we need a bit more follow up for linking hidden nodes:

• the read more link on these hidden nodes still appears, and link goes to 404
• with Views lists, node titles still show as hyperlinks when using fields
• even though this PR works with Basis, I thought it didnt for a while, because my default theme overrides node.tpl.php. I suppose this is beyond core's control, but we should consider mentioning this in the release notes/blog post etc.

[jenlampton]

the read more link on these hidden nodes still appears, and link goes to 404

Pushed up a recent change that fixes this.

with Views lists, node titles still show as hyperlinks when using fields

I looked into this, but we don't currently have the rendered node to pass into node_type_get_type() here in order to find out if the node has a hidden path. I'm hesitant to load a node where we didn't load one before due to performance implications, so I'd rather we find a way to pass along the node type as an additional_field much like we do with nid. Since that seems complicated, however, I propose we postpone this until later.

Since there is a checkbox in the view for architects to un-link the title field on views, I feel this is less urgent a fix than the node title and read more link.

even though this PR works with Basis, I thought it didnt for a while, because my default theme overrides node.tpl.php. I suppose this is beyond core's control, but we should consider mentioning this in the release notes/blog post etc.

Good idea.

[olafgrabienski]

with Views lists, node titles still show as hyperlinks when using fields

Since there is a checkbox in the view for architects to un-link the title field on views, I feel this is less urgent a fix than the node title and read more link.

I agree. Also, there will be views built just for people with permission to view hidden paths. So, in any case let's keep the option to link the title field again.

[olafgrabienski]

views built just for people with permission to view hidden paths (...) let's keep the option to link the title field again

Oh, I just saw the follow-up #5214 where the expected behavior differentiates between people with access to "view hidden paths" and those without this access. This makes my suggestion unnecessary.

[docwilmot]

Wondering if we could add a redirect to $_SERVER['HTTP_REFERER'] for users without access? So if you click node title it just bounces you back to where you came from. Maybe with a watchdog, so the admin knows she needs to sort out the node template.

[klonos]

So if you click node title it just bounces you back to where you came from.

I don't think that that's good UX. Besides, the fix here will make it so that these title's won't be links (for people w/o access) to begin with. So I don't see the point really.

[docwilmot]

the fix here will make it so that these title's won't be links

The problem being that we cant actually fix this. See https://github.com/backdrop/backdrop-issues/issues/5181#issuecomment-908563837. Any theme with a node.tpl.php will still need to specifically remove links from node titles, which core has no control over, and Views will still link titles and users without permissions will still get a 404. So my suggestion is a fallback for these circumstances where we just cant avoid it.

[stpaultim]

In reading the issue, I'm not sure if this should still be "works for me" or if it we still want to address the concern about @docwilmot? @klonos - what you do think about the comment previous to this one?

[docwilmot]

Well, it seems somehow this is already the default behavior? In the sandbox, I duplicate the promoted content view and added a node with hidden path and browse as anonymous. On the front page, clicking the title redirects to the front page, and on a view page it redirects to that view path as well! I imagine somehow node access is intervening somewhere?

In either case my point is unnecessary it seems, so WFM it is.

P.s. Views titles still link. I wonder if we should add logic to the Views field handler?

[klonos]

Views titles still link. I wonder if we should add logic to the Views field handler?

I think so, yes.

[herbdool]

A reminder that there are a few suggested changes in the PR as well.

[jenlampton]

A reminder that there are a few suggested changes in the PR as well.

I've merged the suggestions, and rebased off the latest 1.x.

I also changed the check for the teaser display mode for to "not the full" display mode, since other display modes also have links, and would usually link back to the full display.

Still needs tests.

[jenlampton]

Okay, tests added. Unfortunately, they are failing on the readmore link because node_build_content() isn't actually called when building home page teasers? I've got to figure out where the more links are being added...

[jenlampton]

Found it in node.entity.inc.

It looks like the only thing that uses node_build_content() anymore is search module, so the changes included there are probably not necessary (since it's never called with view mode "teaser"). I'd love to deprecate that function for 2.x and only have one copy of that code. I'll create a follow-up issue for that.

The good news: tests are passing 🎉 Ready for more reviews :)

[yorkshire-pudding]

Thank you @jenlampton . I've tested in three scenarios:

1. Add a block to layout with existing content of a pageless node (my use case that brought me here)
2. Add a view block of pageless nodes set to full content
3. Change the view block to display teaser with a read more link

All three work perfectly in terms of hiding the links for those without permission. I checked the source and there also isn't a hidden bookmark link that will cause problems for Google search index.

Many thanks - WFM / LGTM

[klonos]

I had a look at the code, and it looks good. Only left minor nit-picking suggestions re things like inline comments and an superfluous blank line etc.

[herbdool]

@yorkshire-pudding would you still mark it as RTBC now after minor fixes to PR?

[yorkshire-pudding]

@herbdool - I just tested quickly again and all results the same. No change to RTBC status. Thanks

[quicksketch]

Thanks @herbdool and @yorkshire-pudding for pushing this one to the finish! I merged https://github.com/backdrop/backdrop/pull/3712 into 1.x and 1.27.x. Thanks @jenlampton, @klonos, @docwilmot, @stpaultim, and @olafgrabienski for the work on this since 2021.

I was the one that marked this as "needs change record", but seeing where this ended up, I'm not sure that's true. There's no API change here, but there is a difference in the default templates. Sites that have overridden the node.tpl.php file could have already fixed this problem, or are not affected by it (if the node type is not hidden). So I'm not sure there's any before/after changes here.

For now I'm removing the label and fully closing this issue. Please reopen if anyone feels differently.