StringFormatter always displays links to entity even if the user in context does not have access

larowlan → credited xjm → .

larowlan → credited mxr576 → .

larowlan → credited hchonov → .

MR 3309 is HTTP 404, why is that?

It was hoisted into the security queue. @larowlan reckon we could get it reinstated?

Our patches from the original sec issue could be re-uploaded here, but most probably an MR would be easier for everyone. Let that be 3039 or a new one.

The patch was originally based of 3039.

I have the code for the MR locally. I'll push the branch as a new MR if we cant get 3039 back.

@drumm had to delete it from gitlab, we will need to open a new one, if you've got the branch locally please go ahead dpi

No worries.

MR !3531 replaces !3309

Let me know if this was a wrong way to test.

I added a user field to a content a type.
Made sure anonymous users cannot view information about user
Open private browser and I see the link to the user. This is with the patch.

@smustgrave that might be \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceLabelFormatter, which also suffers the same problem in 🐛 Entity reference label formatter may render link to inaccessible entity Closed: duplicate .

The solution/tests are similar. Maybe the issues should be merged?

Would be nice. Will let you make that call as 🐛 Entity reference label formatter may render link to inaccessible entity Closed: duplicate still needs tests so wouldn't want to hold this fix up for that.

But if we keep separate how to best test this one?

In most cases StringFormatter link to the entity its attached to, so you'd need to deny access to view the canonical page. Could do this with a hook_entity_access, or regular permissions.

1. Add a string field to an entity (Text (Plain))
2. Manage display
3. Configure field to display
4. Formatter should be string (labeled: "Plain text", class is StringFormatter )
5. Configure formatter
6. Click Link to the XYZ checkbox.
7. Save.

The fork is no longer private and should be usable as normal again.

kksandr → made their first commit to this issue’s fork.

kksandr → closed merge request !4856

kksandr → reopened merge request !4856

kksandr → closed merge request !4856

kksandr → closed merge request !4857

Patch for Drupal 10.1.x

@kksandr you've opened multiple MRs and uploaded multiple patches, can you please close/hide the irrelevant ones and explain your work?

@acbramley Sorry, my mistake. I updated MR to be compatible with the 11.x branch. After that, I tried to apply MR as a patch on my site (Drupal 10.1), but it did not apply, so I separately rerolled the patch for 10.1.x

@kksandr which MR is the correct one?

There was an existing one https://git.drupalcode.org/project/drupal/-/merge_requests/3531

You've created 3 more:
https://git.drupalcode.org/project/drupal/-/merge_requests/4856
https://git.drupalcode.org/project/drupal/-/merge_requests/4857
https://git.drupalcode.org/project/drupal/-/merge_requests/4858

This makes it very confusing to review, please close the incorrect ones.

kksandr → changed the visibility of the branch 3336994-stringformatter-access to hidden.

kksandr → changed the visibility of the branch 3336994-stringformatter-access-r5 to hidden.

@acbramley I hid all my unsuccessful attempts, sorry.

kksandr → closed merge request !4858

Created a new MR because the old one was targetted 10.1.x and I did not have perm to change that.

smustgrave → changed the visibility of the branch 3336994-stringformatter-access-r2 to hidden.

smustgrave → changed the visibility of the branch 3336994-stringformatter-access-r2 to active.

Bummer can't hide 3531 without hiding 8317 so marked in the issue summary

Ran test-only feature https://git.drupalcode.org/issue/drupal-3336994/-/jobs/1841768 which choses all the coverage

Reviewing code and changes make sense.

Only concern I suppose I would have is seeing all the test updates if this would break contrib tests potentially. But doing a deprecation not sure make sense since the link renders a 403.

LGTM

xjm → closed merge request !3531

I closed the superfluous merge request. Unfortunately, the canonical one mentioned in the IS has merge conflicts. So this needs those conflicts resolved. (To receive credit for resolving merge conflicts, the conflict and how it was resolved need to be documented on the issue.) Thanks!

 git rebase origin/11.x
Auto-merging core/modules/block_content/tests/src/Functional/BlockContentListViewsTest.php
Auto-merging core/modules/views/tests/src/Kernel/Handler/FieldFieldTest.php
CONFLICT (content): Merge conflict in core/modules/views/tests/src/Kernel/Handler/FieldFieldTest.php
error: could not apply d0714b0f46... Fix failing test
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".

Resolved conflicts caused by added void return type hints.

Rebase seems good