Active
Project:
Drupal core
Version:
main
Component:
routing system
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
2 Jul 2015 at 15:12 UTC
Updated:
1 Jul 2025 at 23:02 UTC
Jump to comment: Most recent
Problem/Motivation
#2512452: Confirm form cancel button can lead to external domain added an exception when an external URI is passed to UrlHelper::fromInternalUri().
This inadvertently fixed the security issue with #2507831: Harden redirect responses to make external URIs opt in (was SA-CORE-2015-002 foward-port).
However it means that instead of the security issue, end-users can still trigger exceptions on a working site. This is not good and we should catch the exception in FieldUI::getNextDestination() the same as we do in ConfirmFormHelper.
Proposed resolution
Apply the same pattern used in ConfirmFormHelper to FieldUi::getNextDestination()
Additionally, in all places where we use UrlHelper::fromInternalUri() we should either:
1. Be calling it on validated user input (a path alias, a path for a menu link)
OR
2. Catch the exception and silently produce an empty or default URI instead, or if not silently add a trigger_error() to log what is likely to be an attempted open redirect attack.
Remaining tasks
User interface changes
API changes
Not as such, but #2512452: Confirm form cancel button can lead to external domain arguably changed the API by throwing an exception in more cases and core needs updating for that as well as possibly documentation on UrlHelper::fromInternalUri().
Comments
Comment #1
cilefen commentedComment #2
unstatu commentedI'm not very sure we need to change the code here because the
FieldUI::getNextDestinationmethod is already forcing the path to be correct:Comment #3
unstatu commentedComment #4
unstatu commentedComment #5
xjmComment #7
dawehnerJust some random idea. Instead of throwing the exception we could also encode the error into the resulting URL object.
This would then look something like this:
This allows callers to still check for errors, but make the API save against potential missing catching.
Comment #8
effulgentsia commentedDiscussed with @xjm, @catch, @alexpott, and @Cottser on a triage call, and we agreed this is a Major bug, per https://www.drupal.org/core/issue-priority#major-bug:
Comment #18
larowlanComment #20
tim.plunkettNo patch
Comment #22
sayco commentedFirst of all, I think the ticket needs to be updated with the information that the fromInternalUri is now part of the Url class, not the UrlHelper.
The other thing about this (before we apply any changes in the code) is that the implementation of the function is wrongly designed. It breaks the SRP, as it’s not only responsible for creating the internal URI, but also for validation against the external URI. I think this is the wrong approach.
I know it requires a lot of changes, but the fromInternalUri should be responsible for building the internal URI from whatever is given, or it should return a null value or raise an exception. Whatever is using the function should be responsible for handling the exception or null value, checking what the given value is, and taking appropriate actions depending on the outcome. It shouldn’t be the responsibility of the fromInternalUri method to know about other dependencies and all the use cases.
We also should avoid adding any silent caches in the code. At the very least, logging with debug level should be added. Otherwise, it’s hard to guess that anything unexpected happened in the code.
Comment #26
xjmAdding credits for the triage from #8.