Contrib.social

H5PDefaultFormatter should add user cache context.

Open on Drupal.org →
Created on 4 March 2024, over 1 year ago

Problem/Motivation

We've found that sometimes the the H5PDefaultFormatter field formatter can show one user's data to others if the Save content state is turned on for H5P module. This happens because the H5PDefaultFormatter doesn't add user cache context, which it should. Since the cache issue may cause disclosure of user data I marked this issue as Critical. Please change a priority if I'm wrong.

Steps to reproduce:
1. Set up a new D10 website.
2. Install and turn on the H5P module.
3. Go to admin/config/system/h5p and check the Save content state option.
4. Visit admin/structure/block-content and click Manage fields CTA for the basic block type.
5. Click Create new field and add Interective Content - H5P field type, name it (for example, H5P), and save.
6. Go to the admin/structure/block and click Place Content for the content region. Then in the popup, click Add content block
7. Create a new block and upload H5P essay demo content to the H5P field. Save it, assign the block to the Content area, and make sure it only shows on the front page. Save your settings.
8. Create two users with the same role, e.g. "Authenticated user" only, at admin/people.
9. Log in with the first user(in my case, uid: 2) and visit the homepage. Enter "user 2" in the H5P essay's input field. Wait for it to save, then refresh the page to make sure it's saved.
10. Then log in as the second user (uid: 3 for me) and go to the homepage. You'll see that user 3 can view user 2's saved content.

I tested this using the block content type because I couldn't get the same result with Node content type. It might be due to how each entity handles caching. But since H5P can be added to any content type, this problem might happen elsewhere too.

Proposed resolution

Ideally, the H5PDefaultFormatter should use a different cache strategy depending on whether the Save content state option is enabled. However, because user data is always included in Drupal settings, we can simply add 'contexts' => ['user'] to the render array of H5PDefaultFormatter.

🐛 Bug report
Status

Active

Version

2.0

Component

Code

Created by

🇺🇦Ukraine id.aleks

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @id.aleks
  • Merge request !183425407: Added user context to the H5PDefaultFormatter render array. → (Open) created by id.aleks
  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇦Ukraine id.aleks

    I have opened the MR https://git.drupalcode.org/project/h5p/-/merge_requests/18. Also attaching the fix as a patch. Please review.

  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇬🇧United Kingdom catch

    This makes sense, but it should only add this cache context if 'save content state' is enabled I think - then when it's not enabled, there would still be a higher cache hit rate.

  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇦Ukraine id.aleks

    Hello @catch, thank you for checking this. I agree with your opinion. I have pushed new changes to the MR and also added a new patch to this comment. Please review.

  • Comment 4 days ago →
    🇺🇸United States illeace

    Patch #5 no longer applies to 2.0.x-dev so created a new patch file off the latest dev code. @id.aleks - please take a look and make sure I didn't miss anything.

    I have used similar steps to the ones you suggest for testing, and in my tests can confirm that the patch resolves the issue of showing a different user with the same role's cached progress. I also confirmed that the additional user context is only added when the Save content state option option is enabled.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024