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

MR 3309 is HTTP 404, why is that?

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.

@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

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.

Would be nice. Will let you make that call as #3246579: Entity reference label formatter may render link to inaccessible entity 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?

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

Error, wrong file

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

@acbramley I hid all my unsuccessful attempts, sorry.

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

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

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 #3468031: StringFormatter does not support entity uri_callback 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.