Problem/Motivation
Recent security releases have shown that the render system needs to be stricter about what it allow to be called by a callback. See:
- https://www.drupal.org/sa-core-2018-002
- https://www.drupal.org/sa-core-2018-004
- #2860607: Code execution via Twig templates (including inline)
Render list of callbacks to target:
- #access_callback
- #lazy_builder
- #post_render
- #pre_render
Because render callbacks use \Drupal\Core\Controller\ControllerResolverInterface::getControllerFromDefinition
this allows them to instantiate any service and call public methods on them. Therefore for non-ElementInterface objects we need an additional RenderCallbackInterface object to declare with objects contain a callback and which methods can be called on said object.
Procedural function support is removed.
Form callbacks will be handled in #2966711: Limit what can be called by a callback in form arrays
Proposed resolution
- In 8.7.x deprecate the ability to call any function using
call_user_func*()
and limit to object implementing TrustedCallbackInterface, ElementInterface or a closure. For non-ElementInterface objects also limit to specific methods to further narrow the surface area. - Try and provide a PHPCS fix that can auto-update code?
- In 8.8.x or 8.9.x remove the ability? Definitely for 9.0.0
Remaining tasks
Review.
User interface changes
None
API changes
- Add \Drupal\Core\Security\TrustedCallbackInterface.
- Add \Drupal\Core\Security\TrustedCallbackTrait to make doing a trusted callback simple
- Trigger a deprecation error if a render callback is a procedural function or an object that does not implement TrustedCallbackInterface or RenderElementInterface or a closure.
drupal_pre_render_links()
replaced with\Drupal\Core\Render\Element\Link::preRenderLinks()
color_block_view_pre_render()
replaced with\Drupal\color\ColorBlock::preRender()
filter_form_access_denied()
replaced with\Drupal\filter\Element\TextFormat::accessDeniedCallback()
history_attach_timestamp()
replaced with\Drupal\history\HistoryRenderCallback::lazyBuilder()
_toolbar_do_get_rendered_subtrees()
replaced with\Drupal\toolbar\Controller\ToolbarController::preRenderGetRenderedSubtrees()
toolbar_prerender_toolbar_administration_tray()
replaced with\Drupal\toolbar\Controller\ToolbarController::preRenderAdministrationTray()
views_pre_render_views_form_views_form()
replaced with\Drupal\views\Form\ViewsFormMainForm::preRenderViewsForm()
Data model changes
None.
Release notes snippet
The security fixes required for SA-CORE-2018-002 and SA-CORE-2018-004 along with other publicly disclosed security issues all indicate that the render system needs to be stricter about what may be called by a callback. If you have code that adds a render callback (#access_callback, #lazy_builder, #pre_render
or #post_render
), it might need to be updated to work in Drupal 9. Read more in the change record for limitations on what can be called by a callback in render arrays.
Comment | File | Size | Author |
---|---|---|---|
#86 | 83-86-interdiff.txt | 3.85 KB | mcdruid |
#86 | 2966327-2-86.patch | 115.97 KB | mcdruid |
#83 | 2966327-2-83.patch | 119.98 KB | alexpott |
#83 | 81-83-interdiff.txt | 1.3 KB | alexpott |
#81 | 2966327-2-81.patch | 119.85 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottComment #4
alexpottSomething like this. Just tacked render arrays so far. I think we should also have methods on the interface to get allowed methods so we can further lock down what can be called.
Comment #5
alexpottSome fixes.
Comment #8
alexpottThis isn't good. #pre_render's need to be serializable. So we shouldn't convert stuff to anonymous functions as this changes whether or not a renderable can be serialised.
Comment #10
alexpottThis patch takes the next step in protection by introducing a required method prefix of either
render
or the camel-ish-case version of the callback - eitherpreRender
,postRender
,accessCallback
orlazyBuilder
. The check is case-insensitive because PHP method names are.Comment #11
alexpottMore fixes.
Comment #14
alexpottA tweaked approach after talking to @timplunkett. No interdiff cause it would be massive.
Comment #16
alexpottMore fixes.
Comment #17
alexpottAdding some more callbacks for forms.
Comment #18
larowlanNice approach, looking good - going to be disruptive though - but security trumps BC right?
should we have trigger_error inside these too?
missing some docs here
nit: could use list() here
still needed given serialization comment?
The callback 'is' or the callback 'was'?
seems to be done?
Comment #19
alexpottThanks for the review @larowlan. Most of that is fixed in the attached patch. Used @todo's for the trigger_error(). Will update once a CR is written. I think given the scale we should work out renderer callbacks in this issue and do a followup issue for form callbacks.
I need to update the issue summary to detail why I think a solution that is strict as to what objects and what methods on those object can be used as callbacks in the way to go.
Patch attached also implements the checks on the render elements to ensure the callbacks are valid.
On the disruption I think it might be possible to scan contrib for use of the callbacks and email the maintainers to let them know of the change.
Comment #20
alexpottand yep Closure's are still allowed we use them in post_render's and a few other places.
Comment #22
alexpottDone some more thinking about RenderElement object. All the public functions are callbacks so I don't think checking buys us much. I think we need to add something to ElementInterface to point out that all public methods on these classes need to designed with security in mind since by their nature they are handling user input that should not be trusted.
Comment #23
alexpottMoving form callbacks to their own issue.
Comment #24
alexpottCreated a change record - https://www.drupal.org/node/2966725 and updated issue summary.
Comment #25
alexpottThinking about
CallbackTraits/PreRenderLinks
having a static method on trait is pretty pointless. Moved the preRenderLinks to the Link element so it's likedrupal_pre_render_link
which is already deprecated in favour ofLink::preRenderLink()
. So now we're deprecatingdrupal_pre_render_links()
in favourLink::preRenderLinks()
Comment #26
alexpottMore tidy-ups.
Comment #27
catchI prefer the new interface that specifies which methods can be used, just having the empty interface would still allow any method on that class which seemed still a bit leaky.
If we're going to do a hard API break in a minor release for this, should we consider a three stage break?
8.6 -> E_USER_DEPRECATED
8.7 -> E_USER_WARNING
8.8 -> exception
Comment #28
alexpott@catch I guess it just depends on how quickly we want to get to the more secure state? The progression in #27 makes sense but it does leave a long time for it to be open.
Comment #30
alexpottUpdating the deprecations with the CR.
Also re #27 - adding the method list was @tim.plunkett's idea.
Comment #34
alexpottFix test and add test coverage of Renderer::doCallback() deprecation messages.
Comment #35
alexpottComment #36
BerdirHm, I understand that we have to do something, but this is going to break dozens of contrib modules and countless custom modules/sites as well.
Agreed with @catch that we should not completely break this without having actually visible errors/warnings that site admins/users will see in their logs and have time to react to it. A lot of it might be hidden in rarely used features that not everyone will be testing when they update to a new minor version, so filling their logs with warnings gives them another 6 month to update their modules/code. If we don't want to wait for a full year then I can also see that we already switch to warnings in the next minor. Better than just going straight from deprecated (or would it be a non-suppressed deprecation message?) to not working.
Either way, it's pretty much guaranteed that this will result in lots of people complaining once we introduce that break.
Why make the interface/method render-specific? Maybe use something like SafeCallbackInterface or TrustedCallbackInterface or something.
Even if we only use it for render-related callbacks now, we might want to extend it/re-use it for something similar later?
Comment #37
alexpott@Berdir nice idea making this not render specific. Patch attached adds:
\Drupal\Core\Security\TrustedCallbackInterface
\Drupal\Core\Security\TrustedCallbackTrait (to make doing a trusted callback simple)
\Drupal\Core\Security\UntrustedCallbackException (to have a standard exception)
Also added a test of all of that.
Comment #38
alexpottComment #39
alexpottMissed a few important conversions.
Comment #40
alexpottOne thought about making it generic is that this opens the question: Are all trusted callbacks trusted to be called by any random implementation of trusted callbacks?
Comment #42
alexpottRe-rolled - that's see if any new render callbacks have been implemented in the last ten months. I've also bumped all the deprecations up to 8.7.0.
Comment #43
alexpottComment #44
alexpottUpdating the dynamic callback message to reflect that we missed 8.6.0 and target 8.7.0. Atm I've gone for removing support in Drupal 9.0.0 because we know that this is allowed. I think it is worth considering making the change earlier given the importance of improving security in this area.
Comment #45
alexpottDiscussed with @samuel.mortenson we agreed to postpone #2860607: Code execution via Twig templates (including inline) and bump this to a critical bug.
Comment #46
larowlanLooking good. I think we need to add deprecation tests for the 7 functional callbacks we deprecated (views, block, color, links, history, etc)
should we say 'must implement' instead of 'implements'?
Should we add a @see to the docblock pointing to the interface for easier cross-linking?
unrelated?
is enforced the right word here? unsupported perhaps?
is this used?
:)
should we call the parent here too?
should we use the array syntax here so we can use the ::class constant?
is that already done?
can we add an explicit case for objects with an __invoke method?
same here for __invoke?
Comment #47
alexpottThanks for the review.
1. Fixed
2. It is related - without this change it'll be the class of $this and not the class of the file - which is what we want.
3. Changed the wording
4. Yes - it is in tests and we need the option of throwing exceptions and not triggering silent deprecations
5. Thanks
6. Not sure BlockViewBuilder replaces build and buildMultiple... it does it all differently.
7. There's a dependency issue here - the string allows us to ignore it. And the is_callable() check works in our favour - this is how it works in HEAD
8. Indeed
9 & 10 I've added tests for
__invoke()
this is not that useful for render arrays because you need an object - InvokableObject::class is not callable - https://3v4l.org/3ZaZiComment #48
alexpottAs per #46 here are deprecation tests for all the things.
Comment #49
dww8.0.x-dev is a lie. ;)
Do we mean to un-deprecate this? I don't see any mention of it in the comments.
Everything in here seems unrelated. Is this a rebase #fail?
And here.
Eeek... and here.
Okay, aborting deep dive. I think there's major rebase badness in #48. And now that I see it, the patch size grew by over 100K, but the interdiff claims to be only 13k. ;) So, maybe only point 1 is actually valid... ;)
Comment #51
xjmComment #52
alexpottOops bad rebase alert. Here's a fixed up patch. @dww sorry wasting your time.
I guess we have to accept that this won't make it into 8.7.x but is a good candidate for early 8.8.x adopt because of the nature of the changes.
Comment #53
alexpottUpping all the deprecations to 8.8.0
Comment #54
larowlanThis looks good to me, +1 for an early 8.8 commit
We need a release notes snippet.
Comment #55
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext for Brisbane City Council commentedSupport for
TrustedCallbackInterface
would be a great feature to add to #2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems. Shameless self promotion on only a tangentially related issue :)Comment #56
dwwTrying again. Wow, this is a big + confusing patch. Mostly I found nits, some DX concerns.
Is it out of scope to fix these to be short array syntax as we move them?
Can $callback actually be of type "object"? I don't see that usage anywhere. I'm probably missing something.
Nit: "The callback's return value.
DX: This message isn't really true. ;) The render callback doesn't implement TrustedCallbackInterface. The callback must be a public method of a class that implements that interface. Not exactly sure the most concise way to say this. Perhaps something like "Render %s callbacks must be methods of a class that implements \DrupalCore\Security\TrustedCallbackInterface or be an anonymous function...." ?
Meanwhile, I think I'm really missing something, but I don't see how the "The callback was %s" part is working at all. We're passing "'%s'" as the 3rd arg to sprintf(). I don't see how that identifies the callback, neither here nor in doTrustedCallback().
I don't understand why we pass ElementInterface::class as the "extra trusted interface" here. Maybe a comment to help mere mortals understand what's happening?
Meanwhile, I don't understand why "doTrustedCallback()" is called "doTrustedCallback()" if it's handling untrusted callbacks. ;) The relationship between doCallback() and doTrustedCallback() doesn't make much sense to me.
Why do we support these options at all? If the point of this is security hardening, why do we ever invoke untrusted callbacks anymore?
DX: Would be helpful to say we mean the name(s) of the public method(s) provided by this class that we're trusting. I.e. we expect ['fooBar'] not ['WhateverIAm::fooBar()'] or worse ['SomeOtherClass::iTrustYou()'], etc.
Uhh, sort of. ;) More like "determines if a callback is trusted, if so, perform it, otherwise, maybe invoke it (!) and probably generate errors." I know that's not what we want to say here, but from my outsider perspective the DX of this is really confusing and weird.
Seems core actually only calls this once, from Renderer::doCallback(). Seems weird to pass this error message in like this.
This seems weird and dangerous. Why do we support this?
Why do we automatically trust \Closure? Perhaps we could comment this?
Oh, there it is! ;) Double sprintf() to the "rescue". :/ This is some obscure magic for the formatting of $message. If it has to work like this, can we please document it somewhere?
Side note: I don't see how this makes the callback inherently more trust-worthy. ;)
DX: Not clear why this is called "ColorBlock". Nor what "Render callback object" means. Help?
Do these need/want descriptions?
Eeek, good point! I didn't even pay attention to all the other implementations of trustedCallbacks to see if others should be doing this, too. :/
...
Kinda sad to completely copy/paste this huge block of code to wrap the second one in the legacy/deprecations stuff. :( Wonder if these can be refactored to better share code between the two test methods?
Nit: 80 chars.
Why is this @internal when nearly everything else added by this patch is not? (Oh, I see later, because it's replacing a _ prefixed function).
Nit: 80 chars.
Nit: 80 chars.
Why not the array_merge() approach from above? Consistency++, right?
Nit: missing period.
Wonder if all this would be more readable if:
a) This was multi-line.
b) The only thing we're saving with "array_merge($render_array" is "#type => 'container'" -- maybe the lesser evil is to just have a fresh render array each time? ;)
Ran out of steam and time. My eyes glazed over at this. ;) So close to the end of this monster patch, but I have to stop here. Sorry for the incomplete review!
Comment #57
alexpottThanks for the review @dww.
@param string|callable $callback
- objects which implement__invoke
are callable and this is tested wrt to \Drupal\Core\Security\TrustedCallbackTrait::doTrustedCallback() - it might not be leveraged by the render system but it probably would work currently because they are callable.Comment #58
alexpottSo yeah the \Drupal\Core\Render\Element\ElementInterface is added because basically objects that implement that exist to be render callbacks. Note that the extra security that this change offers of us and a massive reduction in scope in what can be a render callback. It is still up to the developer to ensure that that callback is safe to be used as a callback but at least with this change we are very sure about what can be used. As we've seen with https://www.drupal.org/sa-core-2018-002 these callbacks can be used in interesting ways when they can call anything.
If we don't have the extra interface functionality then all the methods like \Drupal\Core\Render\Element\RenderElement::preRenderGroup, \Drupal\views\Element\View::preRenderViewElement are going to have to be listed somewhere. Personally I don't think this is worth it.
Comment #59
pwolanin CreditAttribution: pwolanin at SciShield commentedFrom a quick look at this, it seems like it's only giving a deprecation notice and there is no way to turn on the blocking behavior in 8.8? I would want to add this via a setting or similar and think about turning it on by default for new installs?
Comment #60
dsnopekI think this patch looks great!
Here's some minor, nit-picky "review" that is totally ignorable:
Minor type-o:
Links::preRenderLinks() should be Link::preRenderLinks() - an -s on Links:: vs Link::
I think this is the code that would need to change if we wanted to implement something like @pwolanin suggests in #59 and make it possible to configure a site to throw an exception rather than call an untrusted callback. I think having this option is a great idea!
This appears to be wrapping in an anonymous function rather than converting this callback to a class and using TrustedCallbackInterface which seems like a less than ideal pattern. This is just a test so that's fine, but I guess I'd maybe worry that developers might see this pattern and use it in real code?
A question: why get_called_class() and not static::class? I'm not sure the latter is really better, but
get_called_class()
is a function call which seems like it'd be slower, but PHP might optimize these to be the same thing anyway.Same question here.
Comment #61
dwwRe: #59: Indeed. That's basically what #56.6 is about. See @alexpott's reply in #57: "Because it we don't we'll break the Drupal 8 world."
I agree that a setting or config knob to enable "break the Drupal 8 world so I can fix anything potentially insecure" mode would be a great approach, not just generate the deprecation warnings. That seems like some lovely opt-in additional hardening we could provide now, not just after D9. So yeah, +1 to all that. ;)
However, I'm not sure about enable-by-default for new installs -- it has more to do with what contribs are doing things that might be broken. It's not simply an existing vs. new install question. It's about how "bleeding edge" your contrib footprint is. So yes to an option, but no to enabling it by default (until more of contrib can catch up ... probably in the post D9 world).
Thanks,
-Derek
p.s. I missed #60. Yup to a lot of that. #60.3 is exactly #56.13. ;)
Comment #62
alexpottThanks for the reviews @pwolanin, @dsnopek and @dww.
Re #61.
1. Fixed
2. Discussed in the second part of this comment
3. Well to me an anonymous function requires that same review as a class implementing TrustedCallbackInterface but I concede this is not as discoverable so I've changed this to use a class as we probably should prefer that.
4. Fixed
5. Fixed.
I've also gone back on #46.2 - I've reverted the change I can't remember why it is needed - so this patch might fail because of it but at least we'll have a better answer than the one I gave in #47.
So I'm not a fan of another secret setting that adds to the complexity of the implementation. But I concede that a discussion around this ability is a good thing however I think this should occur in a follow-up because it is is not the meat-and-bones of the patch and discussing it and its implementation is simpler once we have the basic implementation in core. So I've opened #3048484: Allows sites to configure a harden security mode to throw exception in when non-trusted callbacks are made for further discussion of this.
Comment #63
dsnopek@alexpott Thanks for continuing to push this forward!
I've put a comment on #3048484: Allows sites to configure a harden security mode to throw exception in when non-trusted callbacks are made but just wanted to add here that the security value of this issue is greatly decreased without a way to make this throw an exception in Drupal 8. Without that, we're basically just accepting that there could be a path to escalate lesser vulnerabilities to RCE until Drupal 9.
Comment #64
joachim CreditAttribution: joachim as a volunteer commentedIt would be nice if this could make use of #2982950: [meta] Standardize the approach for capturing and invoking callables across various subsystems..
Comment #65
dsnopek@joachim I wasn't aware of that issue, but after a quick skim of it: Would using that simply be a matter of replacing the use of 'controller_resolver' here for 'callback_resolver'? If so, that's a relatively simple change, which I think could happen easily after this issue is committed - I don't see any reason to postpone this one on that one. Although, if that one does get committed first, yeah, we should certainly update this issue to use it. :-)
Comment #66
pwolanin CreditAttribution: pwolanin at SciShield commentedWithout the ability to turn on blocking we are not even partially solving this issue. I would go the other direction and put the protection / exception on by default and provide an opt-out as we did for the image cache query string protection.
We can then remove the opt-out in Drupal 9.
Comment #67
alexpott@pwolanin we are setting up the fix for Drupal 9 without doing this first making this sort of change is impossible. Let's get the plumbing in and then we can discussing bringing the additional optional protection in Drupal 8. Or even switching it the other way around and making it something you can turn off. But without the plumbing and giving contrib / custom a chance to upgrade all this is moot.
Comment #68
pwolanin CreditAttribution: pwolanin at SciShield commented@alexpott - ok, I understand this needs to be done in steps in some way, but I don't think it should go into core without at least the option to turn on the enforcing mode via a setting (roughly a 1 line change to this patch).
Looking at subclasses of Drupal\Core\Render\Element\ElementInterface, it includes some with methods like
\Drupal\file\Element\ManagedFile::valueCallback()
which can call other arbitrary callbacks in the array'#file_value_callbacks'
That method is actually part of \Drupal\Core\Render\Element\FormElementInterface. I haven't tried to see if I can exploit that example, but wondering if we can/should restrict further the methods allowed to be called on the
extra_trusted_interface
? Or add a blacklist?Comment #69
pwolanin CreditAttribution: pwolanin at SciShield commentedNot yet finding an exploit path there - I thought I could build an element and use array_filter to apply an arbitrary callback (e..g system()), but running into this check:
A #lazy_builder callback's context may only contain scalar values or NULL.
Still, any static method on any RenderElement in core or contrib (including a form element) could be targeted (obviously), and #file_value_callbacks still seems risky when we start considering value callbacks for form elements in the next issue.
Comment #70
dsnopekI still feel pretty strongly that we should have a way to turn on blocking for this right away. However, any kind of progress forward is better than no progress, so I'm going to mark this as RTBC, and perhaps we can get a follow-up in to have a "blocking mode" shortly after this one.
Comment #71
dww@alexpott addressed all my review concerns. I haven't had (and won't have) time to thoroughly review the last of the patch, but it was only in the final few pages of the test coverage that I ran out of steam. I also haven't reviewed the interdiffs since #62 but I'm sure it's all fine. +1 to RTBC so we can land this, un-postpone #3048484: Allows sites to configure a harden security mode to throw exception in when non-trusted callbacks are made and get on with the D8 hardening solution there (which will indeed be a very small patch once this is committed). $progress++ ;)
Thanks,
-Derek
Comment #73
alexpottvimeo was down - #3059022: If Vimeo is down our tests break
Comment #74
alexpottRerolled... git fell back to 3-way merge that was successful.
Comment #75
alexpottAdded a release note.
Comment #77
alexpottI <3 deprecated code testing.
Comment #78
catchCouple of smaller issues but one larger question (at the end)
Nit: Drupal\Core, not DrupalCore
Does this need an example or more documentation to indicate it's literally the method name and not the fully qualified classname + method name?
Seems slightly overkill to have to implement an interface with one method, that then points to the only other method on the class.
I'm wondering a bit if we couldn't have two interfaces using the $other_interface feature - one for classes like this, and one where it's a larger class and the methods actually need specifying individually.
Comment #79
alexpottThanks for the review @catch
Trusted callbacks are public methods on the implementing class
- I've made the fact that the return is a list of method names clearer in the docs. The problem with example code is that hard to know what to do here. Like do we need to have something like:\Drupal\Core\Render\Element\ElementInterface
. We could if you like provide a trait that implements the interface something like:A downside to this is that I think we need to make an assumption about the static-ness of the callback method. What do you think @catch? I tried implementing this locally but ended up not liking it because you still have to
implements TrustedCallbackInterface
and it's only really one line of "code".Also whilst proposing this I realised that there is an @todo on the existing TrustedCallbackTrait and this trait is badly named because you kind of expect it to implement the interface but it doesn't. This trait is about performing a trusted callback so it should be named accordingly.
Comment #80
catch#2: docs are better. I think your example code is probably the minimum amount of code we'd have to provide to have an example, which is a fair amount.
#3 while I can't think of a good name, why not just an empty interface you can implement if you're providing a single use class that's only purpose is to provide a render callback? Like RenderCallbackInterface or something like that?
Comment #81
alexpott@catch - Ah I see what you might be thinking... what about the attached solution?
Comment #82
dwwInterdiff in #81 looks good. One nit:
The comment no longer matches the code...
Otherwise, yeah, this seems cleaner and easier.
Comment #83
alexpott@dww excellent point.
Comment #84
dwwThe 3 points from @catch in #78 are all addressed:
#78.1 via #79
#78.3 via #83
#78.2 -- class docs are now this, which IMHO is clear enough:
Numerous thorough reviews on this. It's blocking #3048484: Allows sites to configure a harden security mode to throw exception in when non-trusted callbacks are made which would be a big security hardening win. Bot is happy with #83. Back to RTBC.
Thanks!
-Derek
p.s. This isn't a bug. This seems like a critical security hardening task, but not an actual bug or vulnerability.
Comment #85
catch#81 is what I was thinking and looks good.
Looks like some cruft crept in though:
We only need one of these.
Comment #86
mcdruidSimply removed that extra trait which catch spotted. Hopefully I've understood correctly and that is all that's needed.
Comment #87
alexpottWhoops. Thank @mcdruid. Yeah that got into the patch by mistake. @catch++
Comment #89
catchCommitted 5a42b47 and pushed to 8.8.x. Thanks!
Comment #90
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedFYI - the change record says:
I'm assuming that should be 8.8.x / 8.8.0 respectively?
It's also unclear from the change record whether it is a deprecation or a hard removal - from the issue summary I'm assuming a deprecation at this stage?
Edit: Didn't want to edit the CR in case is actually correct...
Comment #91
catchOhhh, no it's 8.8.x and a deprecation, will fix now!
Comment #93
rodrigoaguileraI was just trying to update my code for Drupal 8.8.0 but also keep it compatible for drupal 8.7.x.
Since none of the interfaces exist my only option is to convert the callbacks to anonymous functions. Am I right?
Comment #94
Berdir> I was just trying to update my code for Drupal 8.8.0 but also keep it compatible for drupal 8.7.x.
That's one of the reasons why several people here were against already adding a mode that enforces this.
Short answer is you can't do that. If you rely on a feature added in 8.8, no matter what, you are no longer compatible with Drupal 8.7, that's why it is too early to make this change in contrib modules, you need to wait at least until 8.8 is released (And update your .info.yml accordingly).
Comment #95
rodrigoaguileraWhy not just add the interfaces to the Drupal 8.7.x branch?
Drupal\Core\Security\TrustedCallbackInterface
and
Drupal\Core\Render\Element\RenderCallbackInterface
Comment #96
xjmI improved the release note a little. We'll also want to mention this again in the 9.0.0 release notes. Do we have a critical followup filed against 9.0.x yet to switch the default?
Comment #97
xjmComment #98
nagy.balint CreditAttribution: nagy.balint at Agence Inovae commentedIf I understand correctly, the only way to make a contrib module fully drupal 9 compatible is to drop 8.7 compatibility even though that version is supported til june 2020.
Cant there be something like #95 committed to 8.7 to solve that issue for contribs?
Comment #99
nagy.balint CreditAttribution: nagy.balint at Agence Inovae commentedThis workaround can work, but its not pretty that now basically most modules will have to add this dummy interface in their code.
https://git.drupalcode.org/project/address/commit/63a75fe
Comment #100
larowlanLooks like we missed a spot #3213572: #date_time_callbacks and #date_date_callbacks bypass the TrustedCallbackInterface protections
Comment #101
larowlanAnother spot we missed #3218381: #file_value_callbacks bypass the TrustedCallbackInterface protections