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 → 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

We need to add a change record to cover this change. Also we should mention tests because quite a few have had to change as a result of this change.

Drafted https://www.drupal.org/node/3459840 → based on Alex's suggestions.

CR reads well. Nice!

The tests which are being changed to set up UID 0 do not need this change and should be reverted. Once that's done this can be set back to rtbc.

Fixed the single one thing that Alex asked and I dared to move this back to RTBC

I read the IS and the comments. Thanks for the up to date issue summary. There are no unanswered questions but there are changes to core/modules/views/tests/src/Kernel/Entity/RowEntityRenderersTest.php that alexpott said are not necessary. They are still in the MR. And then, looking at the MR I left a question about changes to another file.

I read the change record and it is easy to follow. It is good that includes what sites should do. Nice work. At first I thought the title could just say that the link is not displayed instead of 'restricts'. But maybe that is a good thing so people are curious and read the notice.

Setting to NW for the 2 comments in the MR.

Thanks @quietone for your high quality review again, I have fixed your findings, even more https://git.drupalcode.org/project/drupal/-/merge_requests/8317/diffs?co....

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

From what I can tell additional feedback has been addressed

FTR, opened 🐛 StringFormatter does not support entity uri_callback Active as a follow up, fixing that is not the scope of this issue.

Although this is a bug fix and security hardening I am not committing to 11.0.x or 10.3.x as it is a behaviour change and we do not want to break tests or surprise users that are currently relying on this functionality.

Committed and pushed 4c8c814c10 to 11.x and 1019986cb1 to 10.4.x. Thanks!

Also published the change record.

Automatically closed - issue fixed for 2 weeks with no activity.