On a site that is using Content Access (or any other contributed module or custom code that implements hook_node_grants), there will be leaked metadata when the destination path is a node. This is similar to #2923503: Destination Parameter fix but isn't mitigated by using getGeneratedUrl.
This core issue added the automated bubbling of cache context for node grants: #2557815: Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter().
We have worked around the issue by patching this module to simply paste the destination and $base_url together instead of using a Url object which can trigger this metadata leak. This module already checks for an external destination, so is there any downside to this method? We assume the return redirect will sort things out if there is an issue with the destination, and I don't know if the Url object would catch it anyway.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | callchain.png | 505.96 KB | roderik |
| #2 | screenshot-nimbus-capture-2020.12.03-14_16_19.png | 172.4 KB | joshua1234511 |
| samlauth-get-url-from-destination.patch | 4.99 KB | byrond |
Comments
Comment #2
joshua1234511Tested the patch. The issue is resolved of leaked metadata.
Login from SSO works as expected.
Comment #3
roderikMany thanks for this thorough bug report. And sorry for taking so long to respond - I admit to being reluctant/scared to dive into this.
Several things:
I agree that getUrlFromDestination() / the code being called from /saml/login & /saml/logout doesn't need to do Url magic - that can/should be done after the user returns, to /saml/acs & /saml/sls.
This means that /saml/login & /saml/logout are 'safe' again from the "leaked metadata" exception, with your code change: they contain no calls to 'exteral' Drupal code.
I see that /saml/acs is still 'safe' because it returns a Symfony RedirectResponse which is not affected by the "leaked metadata" exception. Unfortunately, /saml/sls is not safe anymore since 2 weeks ago when I committed #3183933: Allow External Redirects on Logout Responses.
So, I committed your code change with a TODO (that I hope to solve soon) and comments changed to fit my newest understanding/plan.
More details:
As you said, "leaked metadata" is happening during the
Url::fromUserInput($destination)->setAbsolute()->toString(TRUE)->getGeneratedUrl();call. But it is not done by that call directly; that isn't possible.I installed Content Access and was unable to reproduce the issue. So I think there are more/different modules at play. I was in a position to 'borrow' @joshua1234511's Drupal code, and discovered that there it's caused by Rules 8.x-3.0-alpha6, which is called through Core's content_moderation module during Url::fromUserInput():
It's a different Url::toString() call in Rules, which leaks the metadata that will cause the exception in the end. I found an existing issue #3161036: CurrentPathContext inadvertently bubbles up 'route' cache context and reviewed the patch.
The Rules patch will actually fix the bug. Unfortunately, unless/until the samlauth module works around any such bugs / catches and discards the metadata leaked by 'buggy' other modules, Single Logout still likely causes the same exception.
It's unfortunate that a call to an Url method can recursively execute other Url methods that cause the exception - which makes things harder to catch. I'll see about how I want to change that code. In the meantime / if you want to be sure that you don't get this exception anywhere else and/or want render caching to be optimized, and you have Rules 8.x-3.0-alpha6 installed, please apply #3161036-6: CurrentPathContext inadvertently bubbles up 'route' cache context 'variant 1'.
Comment #5
roderikComment #6
roderikAlright. Some followup: the SLS endpoint is impervious to the "leaked metadata" exception now, too. So 'buggy' other code (like the above Rules issue) doesn't need to be patched anymore to prevent exceptions on /saml/sls.
For this to happen, I had to start wrapping /saml/sls code inside a render context - something which I've been avoiding until now, for some reason. Now this is necessary... I went all in, and unified the code to do this on other endpoints too. And made a trait out of it, which other projects could copy.
https://www.drupal.org/commitlog/commit/79214/6b2f636daa85ad6f16a3bd42e1...