Sensitive information is disclosed via REST/jsonapi

Thank you, @Sam152, for generating this issue. It should be worth noting that this would otherwise be a security issue managed/addressed in private. However, since this module is still in RC, it can be handled in public. That said, this should be addressed before an official stable release goes out.

I don't feel that this is a security issue but a side-effect of exposing any config entity to Drupal's REST API. I agree most people would expect only the webform elements to be available. Keep in mind that webform elements could also include sensitive information like API keys, roles, usernames, default values, admin only elements, etc...

At the same time, I could see a mobile app needing the entire webform's configuration including the sensitive information to build a headless webform.

Here are the steps required replicate the issue...

• Enable HAL, REST UI, RESTful Web Services, Serialization (/admin/modules)
• Enable GET for Webform by checking off all options.(/admin/config/services/rest/resource/entity%3Awebform/edit)
• Open the Contact form (/form/contact?_format=hal_json)
• Confirm the entire forms configuration is visible.

Here is a screenshot of the exposed data...

I am little stumped on the best/long term solution. We might need to decide on a MVP solution with a long term plan. An MVP might be applying some basic permissions to the exported configuration. I think the long term plan has to address headless webform instances.

This is essentially the same problem as #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity", but worse, because webform config entities tend to contain more sensitive data than block config entities.

I see in https://cgit.drupalcode.org/webform/tree/src/WebformEntityAccessControlH... that webform already defined two custom operations: test and duplicate. IMHO this also needs a render operation, and that's the operation that should be checked when rendering a webform config entity into an HTML form.

@Wim Leers Thanks for pointing us in the right direction.

The viewing and rendering of a webform is controlled by whether a user can create a webform submission. We could add 'view own webform' (configuration) and 'view any webform' (configuration) permissions which would solely be for allowing applications to access a webform's configuration via the REST API.

BTW, The Webform REST module is providing individual endpoints for different aspects of a webform. Right now only elements are supported. I think the Webform REST module could be the long term approach.

The attached patch just returns AccessResult::forbidden(); for the webform 'view' operation and should confirm that the 'view' operation in the WebformEntityAccessControlHandler has actually never been used, which means we could make it applicable to the Webform REST API endpoint.

The attached patch removes all calls to $webform->access('view) and now the 'view' operation can be reserved for only accessing a webform's configuration via the REST API. I think there should be dedicated 'access own webform configuration' and 'access any webform configuration' permission which would be checked via the webform 'view' operation.

The EntityResource::get() method relies on the 'view' operation so right now I don't think there are are other immediate solutions.

The long-term approach might be to only use the 'view' operation for a rendering and for Drupal core to support a dedicated 'access_configuration' operation.

Even though the patch from #13 limits the 'view' operation to 'administer', 'access own webform configuration' or 'access any webform configuration' permission. I think other modules that call the 'view' operation for a webform could have a major problem. For example, I think entity references which call the 'view' operation would be broken via the approach in #13.

I think we have to create a dedicated operation for accessing a webform's configuration and then override the Webform's EntityResource::get() method to use this new operation.

What should the new operation be called?

• configuration
• access configuration
• view configuration

Are two word operation allowed/recommended?

Some questions/thoughts:

• Is using new operation the best approach? It might be the only approach
• Is using a new permission the best approach? The only alternative would be use the update operation to access webform configuration.

Some implementation notes:

• Need to use hook_rest_resource_alter() to override a Webform's EntityResource
• Can use contact_rest_resource_alter() as an example

Yes, it does seem that JSON API does also rely on the 'view' operation.
@see \Drupal\jsonapi\Controller\EntityResource::getIndividual

I am stumped because if we fix REST and JSON API by tweaking the 'view' operation to use the 'access webform configuration' permission, we will break entity references that point to webforms.

The patch switches from using the 'view' operation to the 'submission_create' operation for WebformEntityReferenceItem::checkAccess. I might have to look into this tweak because it could prevent the disabled/blocked webform message from being displayed to end-users.

I would suggest adding a custom operation, such as render.

• view => access the webform configuration which requires the 'access own/any webform configuration' permission.
• render => access the generated/rendered webform

The current patch uses the 'webform.submission_create' operation to grant users access to the rendered webform and the ability to create a new submissions. I would like to keep the 'webform.submission_create' operation and add support for the 'render' operation, which would do almost the same thing.

Does this approach seem reasonable?

Attached is the re-rolled patch with the 'render' operation being handled as a 'submission_create' operation without any change to the RELEASE-NOTES.md

I reviewed the patch using the steps from #3 and it does protect the webform's configuration and only allows authenticated users to access any or own webform configuration.

Attached is the final patch which I am going to be committing.

This causes a serious regression for us. We are using regular entity reference fields to reference to webforms and that checks the view operation (We are not using the webform reference field here because we don't need the additional UI options). As a result of this, webforms are no longer displayed.

As pointed out in another context, I still strongly believe that unconditionally exposing any entity type over REST is wrong for modules like json_api/rest :) Over and over again, this approach first causes security issues at first and then fixing them breaks other things :)

This is IMHO an API change, as was suggested in similar cases, one workaround would be to still allow the view operation for the html request format.

I'm reopening this for now, can also create a new issue if you prefer that.

@berdir your patch makes sense to me. If no one objects and and we fix the broken tests, I will commit the patch.

Lets fix the broken test then, also added test coverage for this, used the already injected request stack and added the request format cache context (fancy, didn't even know that one existed!)

Hm, to be honest, I'm not sure I understand what you're doing exactly with render/submission_create operations. render doesn't seem to be used at all.

Possibly it's actually the view operation that should be changed to submission_create if the request format is html so that whatever you're doing there is also respected for regular entity reference fields?

Also, the test that is currently failing fails due to a validation error: "Invalid Drupal.org organization name.". Is that really running against the d.o API? that might not be the best idea :)

Here's a version of the patch that does that.

For what's its worth, I still disagree with Wim in the referenced core issue in that the view operation is something different on html request format vs others *for any entity type* and that blocks really aren't that special in that regard.

@Berdir The Contribute module's integration test does run against Drupal.org. I know it is not a great idea but it did succeed in catching a major issue with Drupal.org REST API that is breaking this functionality. I just disabled the broken test and will re-queue the tests

I added the 'render' operation from #4 because it does make sense. The 'render' operation is handled via a 'submission_create' operation.

I am also not sure what is the right solution.

> One minor nit, the "request_format" cache context should be added to all return values for "view", not only for HTML format ones, since access checks should not be cached universally if another request format is used first.

I believe I'm doing that because if we don't go into this condition, then it will go into the $operation == 'view' condition below and then I'm also adding the request format there.

And for users who have permissions to actually view the webform configuration, the request format also doesn't matter, they always have access.

Due to a large amount of early returns and re-creating access objects instead of adding to them, it's not easy to do that in a different way.

I think the patch that got committed in #23 introduced regression beyond what berdir reported. Especially the following code or at least the access result message doesn't make any sense to me:

    // Return forbidden access result with a reason for 'view' operation
    // so that blocked REST API requests can better understand
    // why access was denied.
    if ($access_result->isNeutral() && $operation === 'view') {
      $access_result = AccessResult::neutral("The 'administer webform' or 'access own or any webform configuration' permission is required.");
      $access_result->cachePerPermissions()->cachePerUser();
    }

It could be understood as one should grant 'administer webform', 'access own webform configuration' or 'access or any webform configuration' to anonymous users. Because when the execution reaches this line, the request is coming from an anonymous user and you don't want to grant these permissions to this role.

Even worse, even if you grant 'access own webform configuration' or 'access or any webform configuration' to anonymous users, this still doesn't work and again, nobody wants to grant 'administer webform' to anonymous users.

Using JSON API and trying to get webform definition in a decouple application using a URL similar to

http://example.com:8080/jsonapi/webform/webform?filter[id][value]=my_custom_webform

The only permissions that allow accessing the webform definition is `Administer webforms` which `Allows administration of global YAML configuration and options.`.

Permission `access any webform configuration` doesn't work for the anonymous user.

Maybe we should include a separate permission that allows reading that configuration but without administration permissions.

So I feel that Berdir's comment from #38

Due to a large amount of early returns and re-creating access objects instead of adding to them, it's not easy to do that in a different way.

Hints at a much larger issue which is the Webform module's access controls have become a little overwhelming. This is making it very difficult for us to resolve this issue.

I created and will work on #3005581: [meta] Improve test coverage, refactor, and stabilize webform access controls

Remaining issues

• View operation needs to be correctly supported. Entity Reference field uses the view operation to determine access.
• Viewing rendered webform (aka HTML) should use expected permissions and access rules
• View webform configuration (aka NOT HTML) should have dedicated own and any permissions and access rules with users, roles, and permissions should be supported

Tasks

• Add 'Access Webform configuration' (configuration) to access rules
• Write update hook.
• Isolate all view operation access checks in WebformEntityAccessControlHandler and add 'request_format' cache context.
• Update view access response message.
• Update tests
• Write change record documenting webform configuration permission and access rule.

Changes to be reverted…

• QueryStringWebformSourceEntity should use 'view' operation
• Route _entity_access should used webform.view
  • entity.webform.assets.javascript
  • entity.webform.assets.css
  • entity.webform.confirmation

Attached is a patch that combines #33 and allows individual webform configuration access to be managed using an access rule.

The main change that needs to be reviewed is in \Drupal\webform\WebformEntityAccessControlHandler::checkAccess. I am specifically trying to keep the access checking and access result context inside one understandable code block.

I also created a draft change record.

If anyone has time available, please review this patch.

I am hoping to commit this patch before the next week's release.

Hi jrockowitz!

I can validate that your latest proposed patch has fixed the Webform not being visible for anonymous users after core update to 8.6.2 (even after updb) and it fixed this issue in 2 different sites of mine. Good job!

I propose moving to RTBC after a couple more confirmations.

Cheers!

I think we have fixed the REST API issue but this ticket #3010727: Error processing Webform Tokens, sending leaked metadata is indicating that JSON API may still be using the HTML request format.

Attached patch includes JSON API test coverage.

re-rolling #55 with a couple of items that were already committed

@aaronbauman Thanks for the re-roll. I forgot to merge 8.x-5.x into my feature branch.

This issue needs to be resolved before I tag a stable release.

The patch includes test coverage and there is change record, so I decided to commit it.

I am going to tag another RC and hopefully, this patch solves the existing regressions and security issues.