Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.with drupal 7.35 the destination url parameter is sanitized in bootstrap.inc
i'm using an external library that passes a destination array as url parameter.
this creates a warning, because url_is_external($path) expects $path to be a string and is using some string functions like strpos on it.
Furthermore the $_GET['destination'] gets unset.
function _drupal_bootstrap_variables() {
....
// Sanitize the destination parameter (which is often used for redirects) to
// prevent open redirect attacks leading to other domains. Sanitize both
// $_GET['destination'] and $_REQUEST['destination'] to protect code that
// relies on either, but do not sanitize $_POST to avoid interfering with
// unrelated form submissions. The sanitization happens here because
// url_is_external() requires the variable system to be available.
if (isset($_GET['destination']) || isset($_REQUEST['destination'])) {
require_once DRUPAL_ROOT . '/includes/common.inc';
// If the destination is an external URL, remove it.
if (isset($_GET['destination']) && url_is_external($_GET['destination'])) {
unset($_GET['destination']);
unset($_REQUEST['destination']);
}
// If there's still something in $_REQUEST['destination'] that didn't come
// from $_GET, check it too.
if (isset($_REQUEST['destination']) && (!isset($_GET['destination']) || $_REQUEST['destination'] != $_GET['destination']) && url_is_external($_REQUEST['destination'])) {
unset($_REQUEST['destination']);
}
}
}
a possible fix would be a check if $_GET['destination'] or $_REQUEST['destination'] are arrays.
...
if ((isset($_GET['destination']) && !is_array($_GET['destination'])) || (isset($_REQUEST['destination']) && !is_array($_REQUEST['destination']))) {
...
}
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | drupal-destination-url-parameter-bug-2456331-1.patch | 850 bytes | mukki182 |
Comments
Comment #1
mukki182 CreditAttribution: mukki182 commentedadded a patch
Comment #2
mukki182 CreditAttribution: mukki182 commentedComment #3
mukki182 CreditAttribution: mukki182 commentedComment #4
mukki182 CreditAttribution: mukki182 commentedComment #5
Elijah LynnJust ran into this as well with a $form['destination'] and the POST has an array in $_REQUEST['destination'] now and gives the same error. The workaround would also be to refactor the form I guess, would much rather core take care of this though. I will consider this patch.
Thanks for posting this issue.
Comment #6
Elijah LynnThe commit message for this code in Drupal is simply "Drupal 7.35'. There is no issue associated with this change.
http://cgit.drupalcode.org/drupal/commit/?id=b44056d2f8e8c71d35c85ec5c2f...
Comment #7
Elijah LynnI should add that $_REQUEST is populated with values from $_POST.
Comment #8
Elijah LynnThis appears to be an okay solution and is working. I get the feeling we don't need this for our $_POST case but until I figure that out I feel good enough to run this in production.
Comment #9
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI am very back-and-forth on this issue.
a) It is a regression as the security fix has broken existing code
b) It is a security fix and destination is really internal to Drupal
To check for is_array() is fine, but the consequence would be to still unset it as IIRC an array even is valid as destination.
So thanks for the bug report, but please do not try to deal with the destination parameter, it is bad if someone can lead you to vulnerablesite.com automatically or do a phishing attempt.
Here is a work-around:
- In $settings.php check for $_GET['destination'] and if its an array in your preferred format put it into another variable instead.
settings.php is before _drupal_bootstrap_variables() so that should work.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe same code exists in Drupal 8 also: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!EventSubscriber!R...
So Drupal for a long time has assumed in various places that $_GET['destination'] is a string -- Drupal 7.35 didn't introduce that, just made the assumption happen on all page requests so it's more visible now. (But it may actually have introduced the assumption that $_REQUEST['destination'] is a string.)
What Fabianx said makes sense to me - is_array() could be used to avoid the warnings, but the most reasonable thing to do if it is an array still sounds like unsetting it.
Comment #21
larowlanThis is no longer an issue in Drupal 9, as we support an array for the destination parameter.
You can see this by going to manage fields on the article content type and clicking add field, then adding a new field
The url has
?destinations[0][route_name]=entity.field_config.node_field_edit_form&destinations[0][route_parameters][field_config]=node.article.kjljhg&destinations[0][route_parameters][node_type]=article&destinations[1][route_name]=entity.node.field_ui_fields&destinations[1][route_parameters][field_config]=node.article.kjljhg&destinations[1][route_parameters][node_type]=article&destinations[2]=/admin/structure/types/manage/article/fields/add-fieldI.e an array of destinations so that you go between the 'manage field storage', 'manage field' and then back to 'manage fields'.
Comment #22
luenemannThis is still a problem in Drupal 9 and 10.
Add
?destination[]to URL.