"Create new revision" is overridable regardless of permissions

This might be a duplicate to 📌 Make Add content/media default publishing option descriptions consistent RTBC .

The last submitted patch, 3: 3384520-3.patch, failed testing. View results →

omkar.podey → made their first commit to this issue’s fork.

This JSON:API test coverage was written without my involvement, see #2943899: Moderation state field cannot be updated via REST, because special handling in ModerationStateFieldItemList → .

The failing assertion:

      // Information disclosure prevented: when a malicious user correctly
      // guesses the current invalid value of a field, ensure a 200 is not sent
      // because this would disclose to the attacker what the current value is.
      // @see rest_test_entity_field_access()
      $response = $this->request('PATCH', $url, $request_options);
      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nrest_test_validation: REST test validation failed\n", $response);

That is now a 403 response instead of a 422.

3 observations:

1. Did you already step through the code starting in NodeAccessControlHandler, where access to modifying vid is being denied, to follow the trace to how this gets mapped to a 422 response? If you did, I can’t find it in this issue.
2. That test coverage was added in #2821077: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors → . Did you read that issue, to try to understand why your change is causing the 403, and whether that is indeed also a sec vulnerability?
3. Possibly something is causing a new revision to be created automatically, which then results in vid being validated, and that causes the 403?

Removing tag, reviewed and tested

Added a few comments.

Also, that the media type settings basically has the same info as node type, with the administer media permission, but media isn't fixed yet here.

One last thing, to make this issue even more fun. I'm not even 100% sure that fixing access to do what the permission says is even the right thing to do at this point. It's likely been like this for a very long time, and sites probably rely on it, because while they might to allow users to decide if a new revision should be created or not, but not things like changing the author or sticky/promoted flags and whatever else these administer permissions allow.

which I now realize is exactly what 📌 Make Add content/media default publishing option descriptions consistent RTBC is about, it's updating the description to match how it actually works, which is IMHO fine. There's still things to fix and improve here, like the invalid field access operations, but we wouldn't actually change NodeAccessControlHandler here.

Added some new comments.

To be clear, as I said in #12, I'm not convinced that this is the right thing to do. I think the referenced issue that just fixes the docs is the better direction. If someone wants to alter access they can do so, but this is going to be a behavior change for many sites that currently rely on this being visible.

I would suggest that we extract the few unrelatd bits that make sense, like the JS fix, and then instead get the other issue in and close this as won't fix?

To keep this from stalling, unless anyone has a counter argument for #14 going to move to NW for #14