Problem/Motivation

At the moment it seems LinkFormatter doesn't prevent inaccessible links (e.g: links to unpublished nodes) from being displayed.

Steps to reproduce

- Create a link field in e.g: article nodes
- In "Manage display" use LinkFormatter or LinkSeparateFormatter
- Create an unpublished article node (U)
- Create a published article node (P) and link node U
- As anonymous, we see the link to node U which leads to a 403

Proposed resolution

Some sites may want to render those inaccessible links so I propose to add a setting (checkbox) in the formatter to run access checks.
If it is checked, and the user doesn't have access, the link won't be displayed.

CommentFileSizeAuthor
#32 3231247-11.3.x-32.diff17.77 KBherved
#32 3231247-32.diff18.33 KBherved
#18 linkformatter_access_check-3231247-18.patch16.31 KBherved
#15 linkformatter_access_check-3231247-15.patch16.14 KBherved
#14 interdiff-4-14.txt6.14 KBherved
#14 linkformatter_access_check-3231247-14.patch16.12 KBherved
#4 interdiff_2-4.txt5.78 KBherved
#4 linkformatter_access_check-3231247-4.patch13.54 KBherved
#2 linkformatter_access_check-3231247-2.patch6.49 KBherved

Issue fork drupal-3231247

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

herved created an issue. See original summary.

herved’s picture

herved commented
Status: Active » Needs review
StatusFileSize
new linkformatter_access_check-3231247-2.patch6.49 KB

First draft proposal on the approach.
Tests still todo.

Status: Needs review » Needs work

The last submitted patch, 2: linkformatter_access_check-3231247-2.patch, failed testing. View results

herved’s picture

herved commented
Status: Needs work » Needs review
StatusFileSize
new linkformatter_access_check-3231247-4.patch13.54 KB
new interdiff_2-4.txt5.78 KB

Tests should pass with this, let's see.
New tests validating the added access_check still todo.

Is this approach decent? It is somewhat similar to what EntityReferenceFormatterBase does.
I thought about adding this in the field storage/instance setting but \Drupal\link\Plugin\Field\FieldType\LinkItem doesn't have such access check.
Only \Drupal\Core\Field\FieldItemList has an access check but it applies to the whole item list which wouldn't be viable AFAICS.
Another way I think would be to use #access_callback, similar to what \Drupal\Core\Url::toRenderArray does, but then the link field (wrapper) would be rendered, even if all items are inaccessible. The only emptiness check happens in \Drupal\Core\Field\FormatterBase::view.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

This should probably be a private issue, unpublishing

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
herved’s picture

herved commented

Hello @larowlan, what does that mean?
Do you consider this as a security issue? (I don't think it is but... idk)
Thanks

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

herved’s picture

herved commented

Note that I wrote this patch initially with the idea to make it non-disruptive.
If the goal is to apply access checks by default then we may need to reverse the logic.

Though I wonder, is there a reason why \Drupal\Core\Url::toRenderArray has the access check, but \Drupal\Core\Url::toString doesn't?
Couldn't we include this access check in toString by default? Perhaps via an "access" key set to TRUE by default in \Drupal\Core\Url::$options which we can disable if needed?).

herved’s picture

herved commented
StatusFileSize
new linkformatter_access_check-3231247-14.patch16.12 KB
new interdiff-4-14.txt6.14 KB

Reroll for 10.1.x and adapted the unit tests.

Functional tests are still needed but it would be great to get some feedback on the whole issue/approach before moving forward with that.
As-is, the access check happens in a similar way to EntityReferenceFormatterBase but I wonder if this access check should even happen at this level, and in this way (or perhaps using a #pre_render or #access_callback)...
This could extend way beyond link formatters.

herved’s picture

herved commented
Status: Needs review » Needs work
StatusFileSize
new linkformatter_access_check-3231247-15.patch16.14 KB

Fixing PHPStan issues.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

republishing after discussion that this can be handled in public.

herved opened merge request !9050

herved’s picture

herved commented
StatusFileSize
new linkformatter_access_check-3231247-18.patch16.31 KB

MR created (PS: Nightwatch tests seem unstable, I had to re-run).

Please check #4 and #13, I feel like this issue extends beyond link formatters so leaving to needs work...
Here is a static patch from MR for composer.

herved’s picture

herved commented
herved’s picture

herved commented
Status: Needs work » Needs review

Rebased and moving to review, to get feedback on the approach, thanks.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Left some comments on MR.

herved’s picture

herved commented

Thanks, though I wasn't looking for a code review yet (tests and upgrade path still todo) but rather opinions on the overall approach / scope as it may extend way beyond link formatters.
See #13.

joachim’s picture

joachim commented

I am marking the following as duplicates:

- #2938807: Only display internal links to content the user has access to view -- older, but has no code
- #3368223: Link field > Access to internal links is not checked on display. -- more recent, has a MR
- #2968609: Link fields do not check access for entity refs -- older, has a MR, but the MR has not been updated in longer

Code from those issues with MRs should be compared with what is here, in case there are ideas that should be integrated into the MR here.

Participants in all those issues should have credit added here.

joachim’s picture

joachim commented
Issue tags: +Needs tests

#3368223: Link field > Access to internal links is not checked on display.'s MR is pretty basic.

#2968609: Link fields do not check access for entity refs's MR has a lot of stuff that isn't here.

1. More work on the URL, e.g.:

      if ($url->isRouted() && preg_match('/^entity\.(\w+)\.canonical$/', $url->getRouteName(), $matches)) {
        // Check access to the canonical entity route.
        $link_entity_type = $matches[1];
        if (!empty($url->getRouteParameters()[$link_entity_type]))

Should we be doing that here?

2. More stuff on caching that I've not looked at in depth

3. A functional test (though it could really be a kernel test)

joachim’s picture

joachim commented
Category: Feature request » Bug report

Also, outputting broken internal links really looks like a bug to me -- one of the duplicate issues filed it as such too.

joachim’s picture

joachim commented

I've tested the current MR with field data in all forms:

- a path alias to a node: /mynode
- a system path to a node: /node/1
- using the lookup to store the entity

In all cases, the item is skipped when the user doesn't have access, so that's working well!

I don't know about the caching side of things though -- see my comment above.

dcam made their first commit to this issue’s fork.

dcam’s picture

dcam commented
Category: Bug report » Feature request

In my opinion this isn't a bug. Wanting to link to a page that can give a 403 response, for instance prompting users to authenticate to view content behind a pay wall, is an equally valid use case. That's borne out by the fact that this is made optional with the formatter setting. It isn't only for the sake of backward compatibility. It's because the current behavior is a thing people would actually want to do.

herved’s picture

herved commented

I agree with #28, displaying inaccessible URLs is a totally valid use case.
So indeed this is not a bug, and a formatter settings does make sense.
When I created this issue, I was confused why \Drupal\Core\Url::toRenderArray had the access check, but \Drupal\Core\Url::toString didn't. But I see now that #3343153: Remove deprecated code from \Drupal\Core\Url removed ::toRenderArray.

I also agree with all review comments so if anyone wants to address those, please feel free. Or I will find some time later this week.
Cheers.

herved’s picture

herved commented

Answering to #24:
1. If I'm not mistaken, this looks like a crude attempt to check entity access.
AccessManager::checkNamedRoute I used here is a much cleaner alternative.
2 & 3. The access cacheability is collected with the current MR here in the formatter, but we indeed still need a test here to validate the access is respected and the cacheability as well if possible.
I'll work on that next.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

herved’s picture

herved commented
Status: Needs work » Needs review
StatusFileSize
new 3231247-32.diff18.33 KB
new 3231247-11.3.x-32.diff17.77 KB

Applied suggestions, attaching MR snapshot and also patch that applies to 11.3.