Content translation module - Information disclosure by insufficient access checking

A basic test of the base route.

Waking up testbot for #5

What about adding that only to node? It's only node that exposes the "problematic" permission atm...

What about adding that only to node? It's only node that exposes the "problematic" permission atm...

Doesn't it make sense to always have content translation require that you can view the entity before you can translate it? view AND translate as the requirements to access translations makes a ton of sense to me, and is exactly what this patch does.

So, can you clarify why you think this should only be done for nodes?

Well, it was a response to:

Not seeing an obvious way to add a generic "view" check for any entity type.

but obviously having it working for any entity type would be more desirable. I'll have a look to those failures later...

To cause this you need a silly misconfiguration of permissions. It's almost "works as designed". I don't think this is critical.

To cause this you need a silly misconfiguration of permissions. It's almost "works as designed". I don't think this is critical.

not at all. Sure, The steps to reproduce the issue produce an "extreme" permissions configuration case, but it is just to show the bug.
Users with 'Create|Delete|Edit translations' permission can access the translation overview page even if they can't access to the entity itself; and this include unpublished entity or entity that some content access module protects. And these cases are not related to a permission misconfiguration.

Another example that shows the bug: on a clean installation - standard profile:
1) Enable content_translation and language modules
2) Add a language in admin/config/regional/language
3) Enable translation for Article content type in admin/config/regional/content-language
4) Grant 'Create translations' permission to 'authenticated user' role
6) Create an Article node (node/1) and unpublish it
7) Login as a user with only 'authenticated user' role
8) Go to node/1/translations. You can see the translation overview and some entity info

@willzyx:

Thanks for the detailed description, those steps might be useful to provide additional test coverage, or at least improve the current one :)

Another example that shows the bug: on a clean installation - standard profile:
1) Enable content_translation and language modules
2) Add a language in admin/config/regional/language
3) Enable translation for Article content type in admin/config/regional/content-language
4) Grant 'Create translations' permission to 'authenticated user' role
6) Create an Article node (node/1) and unpublish it
7) Login as a user with only 'authenticated user' role
8) Go to node/1/translations. You can see the translation overview and some entity info

This is basically what the patch in #2558905-5: Content translation module - Information disclosure by insufficient access checking adds as test coverage.

Not seeing an obvious way to add a generic "view" check for any entity type. This might be failing since the test entity just has access TRUE.

I don't get that statement. Its just a bug in entity_test.routing.yml that we don't add the required access checking.
This is one of the reasons why #2350509: Implement auto-route generation for all core entities and convert all of the core entities. is kinda important. It allows us to standardize more.

Other test failures can be fixed by providing the needed permissions.

@dawehner in the test I see:

+++ b/core/modules/content_translation/src/Tests/ContentTranslationOperationsTest.php
@@ -75,6 +76,20 @@ function testOperationTranslateLink() {
+
+    // Ensure that an unintended misconfiguration of permissions does not open
+    // access to the translation form. @see https://www.drupal.org/node/2558905

the comment is wrong and probably the test should be updated. We are not talking of misconfiguration of permissions, the steps to reproduce in 13 (if you read it carefully :)) show that the bug is reproducible without a "particular" permissions configuration

the comment is wrong and probably the test should be updated. We are not talking of misconfiguration of permissions, the steps to reproduce in 13 (if you read it carefully :)) show that the bug is reproducible without a "particular" permissions configuration

Oh I see what you mean ...

Let's expand the test coverage for that.

We could do the following:

Seriously testbot ...

But seriously dawehner :)

Maybe we should re-use the code from update - that seems to be checking view access by checking the the route is accessible?

Well, this is about route access, so we can't really forward to the route access.

This looks ready to me, couple of minor issues that could be resolved on commit.
With regards to use of paths in tests, I think that we're ok

1. +++ b/core/modules/content_translation/src/Tests/ContentTestTranslationUITest.php
   @@ -35,11 +35,13 @@ protected function setUp() {
   +
   +
   

   exteme nit™: not needed (can be removed on commit)

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationOperationsTest.php
   @@ -75,6 +76,33 @@ function testOperationTranslateLink() {
   +    // access to the translation form. @see https://www.drupal.org/node/2558905
   

   Do we normally reference issues in tests? Only for security?

Do we normally reference issues in tests? Only for security?

Well, I think we should, it gives more information ...

Addressed 27 as well.

not sure if is in the scope of this issue but I think we should also take in account content_translation_translate_access() and consider to add an entity access check. It seems to suffer of the same bug, since no explicit entity access check is done in this function

Sure, why not

Looks good to me, thanks! RTBC +1

If we happen to reroll this:

+++ b/core/modules/content_translation/src/Tests/ContentTranslationOperationsTest.php
@@ -75,6 +76,63 @@ function testOperationTranslateLink() {
+   * @see acontent_translation_translate_access()

typo

Thank you plach

Yeah let's quick fix that.

:)

Nice tests and nice find @willzyx. Committed 112e9bf and pushed to 8.0.x. Thanks!