The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
Related to #102, we seem to have a mix of hashing algorithms and seeds for placeholders in HEAD plus this patch:
FormBuilder: $placeholder = 'form_action_' . hash('crc32b', 'form_action' . $hash_salt); RouteProcessorCsrf: $placeholder = hash('sha1', $path); Renderer: $attributes = new Attribute(); $attributes['callback'] = $placeholder_render_array['#lazy_builder'][0]; $attributes['arguments'] = UrlHelper::buildQuery($placeholder_render_array['#lazy_builder'][1]); $attributes['token'] = hash('crc32b', serialize($placeholder_render_array)); $placeholder_markup = SafeMarkup::format('<drupal-render-placeholder@attributes></drupal-render-placeholder>', ['@attributes' => $attributes]); FilterProcessResult: $attributes = new Attribute(); $attributes['callback'] = $callback; $attributes['arguments'] = UrlHelper::buildQuery($args); $attributes['token'] = hash('sha1', serialize([$callback, $args])); $placeholder_markup = Html::normalize('<drupal-filter-placeholder' . $attributes . '></drupal-filter-placeholder>');I think a dedicated service for generating placeholders would be good. Perhaps with 2 methods, such as asHtml() (for the cases where the placeholder can be HTML markup, like the bottom two examples above) and asIdentifier() (for the cases where the placeholder needs to appear inside an attribute value and not be altered by functions such as Html::escape(), Html::cleanCssIdentifier(), UrlHelper::stripDangerousProtocols(), etc., like the top two examples). Then such a service could be well documented for why the chosen hashing algorithm and seed have the appropriate level of security.
Proposed resolution
TBD
Remaining tasks
TBD
User interface changes
None
API changes
Only additions
Data model changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2562341-7-do-not-test.patch | 10.53 KB | wim leers |
Comments
Comment #2
wim leers#2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags already has this. Let's postpone it on that.
Comment #3
wim leersTo clarify, we still need this issue, because it captures the actual need/purpose raised in #2504139-103: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at. Whereas #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags (which already introduces a placeholder generator service), only does it tangentially, and does not cater for the non-HTML placeholder use case. Once #2543334 lands, this issue can add the non-HTML placeholder use case to that service.
Comment #4
effulgentsia commented#2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags is in, so unpostponing. Tagging for rc target triage and raising to Major, because I think this would improve security, or at least security auditing. For example, the link CSRF token placeholder uses
hash('sha1')while the form CSRF token placeholder useshash('crc32b'). Is this arbitrary or are there security considerations for each one? Neither are commented with an explanation. I think a centralized method that standardizes on the hashing algorithm and documents security considerations would be helpful.Comment #5
effulgentsia commentedComment #6
wim leersWe've been converting everything to CRC32, we must've missed that one place, that probably happened due to parallel patches.
Comment #7
wim leersNote this is not at all security-related. That is why catch wanted to move away from using SHA1. The point is exactly that there is nothing security-related going on here. It's merely a placeholder that we want. There's no need for cryptographically secure hashes. There's just a need for hashes, period.
However, I agree that this is currently confusing, and because it is confusing, it could cause security problems indirectly: by causing developers to jump to the wrong conclusion.
At the same time, this reverts one bit in #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder that made things actually more unclear: that issue changed
$form['#token'] = $form_id;to$form['#token'] = $placeholder;, which was actually in hindsight a very strange and confusing thing to do. The clearer structure/code that this API addition brings made that very clear.API addition's signature:
I still need to update/add test coverage, but I first want a +1 or -1 for this direction.
Comment #8
wim leersForgot the patch. d.o form had timed out, forgot to repeat everything I did.
d.o--
Comment #9
xjmComment #21
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.
If this is still relevant could it this get an updated issue summary please.