Information disclosure access bypass for revision log fields when the JSON:API module is enabled

larowlan → credited Berdir → .

larowlan → credited jannakha → .

WIP patch

Moved to an MR

Going to spend time on this this morning, thanks @larowlan!

Got testGetIndividual and testRevisions passing but testCollection is a new beast due to the filter access stuff now that we don't have the admin permission.

Also opened 📌 Tests extending jsonapi's ResourceTestBase are extremely hard to debug/maintain/deal with Active because these tests make working on these issues extremely time consuming.

We'll also need to fix getExpectedCollectionResponse (and whatever it calls for BlockContent) to remove the revision_log because during debugging I'm seeing it still come through....which I now realise is because we have view any basic block content history which grants access to the revision_log field based on the fieldAccess hook...this is impossible to deal with with this test setup.

Aha! we have setUpRevisionAuthorization...onward.

BlockContentTest is now blocked by 🐛 BlockContent JSON:API collection endpoint doesn't return unpublished block when filtered without administer block content permission Needs review to get testCollection working.

I think the MediaTest failure is due to the access cache bug in 🐛 MediaAccessControlHandler update/delete access caching is not correct Needs work

MediaTest::testRevisions is failing because the actual document is missing the revision_log field. It should be there because the user is granted the view all media revisions permission via setUpRevisionAuthorization....

The view all revisions operation does:

$access = $this->access($media_storage->load($entity->id()), 'view', $account, TRUE);

When debugging this it's getting a cached access result and the reason is "The 'view media' permission is required when the media item is published."

If I put $hasViewPerm = $account->hasPermission('view media'); right at the top of checkAccess, that's TRUE so something is off there with caching.

Screen showing #10

This is ready for a review, the final failure is fixed by 🐛 BlockContent JSON:API collection endpoint doesn't return unpublished block when filtered without administer block content permission Needs review which will need to be committed and then this branch will need a rebase.

Just RTBC'd 🐛 BlockContent JSON:API collection endpoint doesn't return unpublished block when filtered without administer block content permission Needs review so when that's merged this should be good.

The other issue is in! This just needs a rebase

Still need clarification on https://git.drupalcode.org/project/drupal/-/merge_requests/5058#note_220022 but otherwise should be good to go

Tested with that one line removed which caused failures. So assuming it is needed. @larowlan mind double confirming.

Left a review on the MR

Seems feedback has been addressed

Went through the code, overall looks very clean to me. Added two small nits, but will leave it at RTBC.

Also added a few comments, some of them should probably be addressed, others are more questions I suppose.

Thanks for the thorough review @Berdir, all feedback has been addressed.

The improvements look good to me.

I'm a little bit concerned about the performance impact of doing 3 entity access operations as part of that field access. I'm still not happy about the view all revisions access operation, that's still feels very strange. Obviously unrelated, but still not convinced that this is needed at all (are there any use cases where you can only access some revisions of an entity?) and if it is needed, I think it should be an access revision overview operation. Obviously I only just had that idea now not when reviewing it originally. Because in that case, we would not need to check that.

I think the horse has bolted on having view revision and view all revisions, although I agree with you that it's a little odd. I think the difference is mainly whether you can view an individual revision's page (i.e node/1/revision/1/view) vs the revision overview (i.e node/1/revisions).

> I think the difference is mainly whether you can view an individual revision's page (i.e node/1/revision/1/view) vs the revision overview (i.e node/1/revisions).

Yes, exactly, but then we shouldn't have to check both operations for the first case, meaning, we should make the "mostly" an "always/exlusively".

We could rename it and alias the current name with a deprecation. Obviously not here, but I opened 📌 Rename view all revisions entity access operation to view revision overview Active

While doing so, I noticed that route access is currently also not checking both for a single revision route, so we are in fact already treating them completely separate, and then IMHO we can remove the call here as well? See \Drupal\Core\Entity\Routing\RevisionHtmlRouteProvider::getRevisionRevertRoute.

For #25

The last paragraph in #25 was meant more as a comment on the operation name discussion, but yes, considering that route access it would be consistent to leave it out here as well and I think secure to I.

@Berdir can you confirm exactly what you're wanting removed? The check to view all revisions operation in the field access?

Fixed a typo and made the IS a tiny bit more structured

> @Berdir can you confirm exactly what you're wanting removed? The check to view all revisions operation in the field access?

Yes, I think we can skip that, because that's consistent with how access to the single-revision route works as well, it doesn't check all either.

@Berdir went to implement that, but it falls over immediately for BlockContent. We don't have a view revision operation. Editors who can see the list of revisions for block content should be able to see the revision logs. I think this is potentially enough to justify keeping it the way it is. Not every entity type has a view/view revision route, but may have a version history which displays the revision log.

Could we special case block content (via it's access control handler) and keep the others lean?

No strong opinion and don't want to block on that, but yes, just adding an extra case for that operation like MediaAccessControlHandler does might be an option?

Merging with upstream has broken the Media based tests because we now have a functioning media revision UI

✨ Add view tab + standard template for block content Needs work this is on my list (maybe this weekend) so hopefully block_content being that one off won't be an issue

But appears all threads have been addressed.

Posted some feedback on the MR. Thanks!

Actioned all feedback, fixed conflict, hopefully no more random test fails :)

Appears all threads have been resolved.

Committed and pushed 184f22eef0 to 11.x and 04c5013c3c to 10.2.x. Thanks!

As a security hardening this is eligible for backport to 10.1.x but it didn't apply cleanly due to 🐛 BlockContentAccessControlHandler requires access block library permission for update, delete and revisions operations Fixed . Looking at the conflict I am not sure it is worth attempting now, if someone wants to add this in 10.1.x then feel free to open a new MR.

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

This has some interesting side effects wrt. cacheability.

If you have json:api requests without the fields query param (i.e get all the fields for an entity) then you may now get UNCACHEABLE responses.

This is because we now check entity update access when checking field access for the revision log, which for quite a few entity types includes the user cache context (e.g microcontent, media)

Weirdly, we were running a version of this patch on 10.1 which had the same effect (returning a $result from checkFieldAccess that container the user cache context) but for whatever reason that context didn't get bubbled to the end response.

Now on 10.2, probably due to an unrelated bug fix, that cache context correctly bubbles up and makes the response uncacheable in dynamic page cache (DPC)

acbramley → closed merge request !5058