Skip to content

Commit

Permalink
API Normalise trailing slashes for all paths
Browse files Browse the repository at this point in the history
NOTE: There will be additional related PRs required for at least
silverstripe/cms and silverstripe/admin.

Co-authored-by: Guy Sartorelli <[email protected]>
  • Loading branch information
xini and GuySartorelli committed Jan 19, 2023
1 parent 6d45425 commit fbcf7dc
Show file tree
Hide file tree
Showing 19 changed files with 718 additions and 271 deletions.
54 changes: 54 additions & 0 deletions src/Control/Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
*/
protected HTTPResponse $response;

/**
* If true, a trailing slash is added to the end of URLs, e.g. from {@link Controller::join_links()}
*/
private static bool $add_trailing_slash = false;

/**
* Default URL handlers.
*
Expand Down Expand Up @@ -648,6 +653,7 @@ public static function join_links($arg = null)
parse_str($suffix ?? '', $localargs);
$queryargs = array_merge($queryargs, $localargs);
}
// Join paths together
if ((is_string($arg) && $arg) || is_numeric($arg)) {
$arg = (string) $arg;
if ($result && substr($result ?? '', -1) != '/' && $arg[0] != '/') {
Expand All @@ -658,6 +664,8 @@ public static function join_links($arg = null)
}
}

$result = static::normaliseTrailingSlash($result);

if ($queryargs) {
$result .= '?' . http_build_query($queryargs ?? []);
}
Expand All @@ -669,6 +677,52 @@ public static function join_links($arg = null)
return $result;
}

/**
* Normalises a URL according to the configuration for add_trailing_slash
*/
public static function normaliseTrailingSlash(string $url): string
{
$querystring = null;
$fragmentIdentifier = null;

// Find fragment identifier
if (strpos($url, '#') !== false) {
list($url, $fragmentIdentifier) = explode('#', $url, 2);
}
// Find querystrings
if (strpos($url, '?') !== false) {
list($url, $querystring) = explode('?', $url, 2);
}

// Normlise trailing slash
$shouldHaveTrailingSlash = self::config()->uninherited('add_trailing_slash');
if ($shouldHaveTrailingSlash
&& !str_ends_with($url, '/')
&& !preg_match('/^(.*)\.([^\/]*)$/', Director::makeRelative($url))
) {
// Add trailing slash if enabled and url does not end with a file extension
$url .= '/';
} elseif (!$shouldHaveTrailingSlash) {
// Remove trailing slash if it shouldn't be there
$url = rtrim($url, '/');
}

// Ensure relative root URLs are represented with a slash
if ($url === '') {
$url = '/';
}

// Add back fragment identifier and querystrings
if ($querystring) {
$url .= '?' . $querystring;
}
if ($fragmentIdentifier) {
$url .= "#$fragmentIdentifier";
}

return $url;
}

/**
* @return array
*/
Expand Down
9 changes: 5 additions & 4 deletions src/Control/Director.php
Original file line number Diff line number Diff line change
Expand Up @@ -400,13 +400,13 @@ public static function absoluteURL(string $url, string $relativeParent = self::B
{
// Check if there is already a protocol given
if (preg_match('/^http(s?):\/\//', $url ?? '')) {
return $url;
return Controller::normaliseTrailingSlash($url);
}

// Absolute urls without protocol are added
// E.g. //google.com -> http://google.com
if (strpos($url ?? '', '//') === 0) {
return self::protocol() . substr($url ?? '', 2);
return Controller::normaliseTrailingSlash(self::protocol() . substr($url ?? '', 2));
}

// Determine method for mapping the parent to this relative url
Expand Down Expand Up @@ -686,7 +686,7 @@ public static function makeRelative($url)
$url = preg_replace('#([^:])//#', '\\1/', trim($url ?? ''));

// If using a real url, remove protocol / hostname / auth / port
if (preg_match('#^(?<protocol>https?:)?//(?<hostpart>[^/]*)(?<url>(/.*)?)$#i', $url ?? '', $matches)) {
if (preg_match('@^(?<protocol>https?:)?//(?<hostpart>[^/?#]*)(?<url>([/?#].*)?)$@i', $url ?? '', $matches)) {
$url = $matches['url'];
}

Expand Down Expand Up @@ -878,10 +878,11 @@ public static function fileExists($file)
*/
public static function absoluteBaseURL()
{
return self::absoluteURL(
$baseURL = self::absoluteURL(
self::baseURL(),
self::ROOT
);
return Controller::normaliseTrailingSlash($baseURL);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/Control/HTTP.php
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public static function setGetVar($varname, $varvalue, $currentURL = null, $separ
$isRelative = false;
// We need absolute URLs for parse_url()
if (Director::is_relative_url($uri)) {
$uri = Director::absoluteBaseURL() . $uri;
$uri = Controller::join_links(Director::absoluteBaseURL(), $uri);
$isRelative = true;
}

Expand Down Expand Up @@ -212,7 +212,7 @@ public static function setGetVar($varname, $varvalue, $currentURL = null, $separ
$newUri = $scheme . '://' . $user . $host . $port . $path . $params . $fragment;

if ($isRelative) {
return Director::baseURL() . Director::makeRelative($newUri);
return Controller::join_links(Director::baseURL(), Director::makeRelative($newUri));
}

return $newUri;
Expand Down
136 changes: 133 additions & 3 deletions src/Control/Middleware/CanonicalURLMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@

use SilverStripe\Control\Controller;
use SilverStripe\Control\Director;
use SilverStripe\Control\HTTP;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Core\Config\Configurable;
use SilverStripe\Core\CoreKernel;
use SilverStripe\Core\Environment;
use SilverStripe\Core\Injector\Injectable;
use SilverStripe\Core\Injector\Injector;

Expand All @@ -17,10 +18,31 @@
* - redirect basic auth requests to HTTPS
* - force WWW, redirect to the subdomain "www."
* - force SSL, redirect to https
* - force the correct path (with vs without trailing slash)
*/
class CanonicalURLMiddleware implements HTTPMiddleware
{
use Injectable;
use Configurable;

/**
* If set, the trailing slash configuration set in {@link Controller::add_trailing_slash} is enforced
* with a redirect.
*/
protected bool $enforceTrailingSlashConfig = true;

/**
* If enforceTrailingSlashConfig is enabled, this is the list of paths that are ignored
*/
protected array $enforceTrailingSlashConfigIgnorePaths = [
'admin/',
'dev/',
];

/**
* If enforceTrailingSlashConfig is enabled, this is the list of user agents that are ignored
*/
protected array $enforceTrailingSlashConfigIgnoreUserAgents = [];

/**
* Set if we should redirect to WWW
Expand Down Expand Up @@ -76,6 +98,39 @@ class CanonicalURLMiddleware implements HTTPMiddleware
*/
protected $forceSSLDomain = null;

public function setEnforceTrailingSlashConfig(bool $value): static
{
$this->enforceTrailingSlashConfig = $value;
return $this;
}

public function getEnforceTrailingSlashConfig(): bool
{
return $this->enforceTrailingSlashConfig;
}

public function setEnforceTrailingSlashConfigIgnorePaths(array $value): static
{
$this->enforceTrailingSlashConfigIgnorePaths = $value;
return $this;
}

public function getEnforceTrailingSlashConfigIgnorePaths(): array
{
return $this->enforceTrailingSlashConfigIgnorePaths;
}

public function setEnforceTrailingSlashConfigIgnoreUserAgents(array $value): static
{
$this->enforceTrailingSlashConfigIgnoreUserAgents = $value;
return $this;
}

public function getEnforceTrailingSlashConfigIgnoreUserAgents(): array
{
return $this->enforceTrailingSlashConfigIgnoreUserAgents;
}

/**
* @return array
*/
Expand Down Expand Up @@ -214,6 +269,7 @@ protected function getRedirect(HTTPRequest $request)
// Get properties of current request
$host = $request->getHost();
$scheme = $request->getScheme();
$url = strtok(Environment::getEnv('REQUEST_URI'), '?');

// Check https
if ($this->requiresSSL($request)) {
Expand All @@ -228,8 +284,16 @@ protected function getRedirect(HTTPRequest $request)
$host = "www.{$host}";
}

// Check trailing Slash
if ($this->requiresTrailingSlashRedirect($request, $url)) {
$url = Controller::normaliseTrailingSlash($url);
}

// No-op if no changes
if ($request->getScheme() === $scheme && $request->getHost() === $host) {
if ($request->getScheme() === $scheme
&& $request->getHost() === $host
&& strtok(Environment::getEnv('REQUEST_URI'), '?') === $url
) {
return null;
}

Expand Down Expand Up @@ -309,6 +373,72 @@ protected function requiresSSL(HTTPRequest $request)
return false;
}

/**
* Check if a redirect for trailing slash is necessary
*/
protected function requiresTrailingSlashRedirect(HTTPRequest $request, string $url)
{
// Get the URL without querystrings or fragment identifiers
if (strpos($url, '#') !== false) {
$url = explode('#', $url, 2)[0];
}
if (strpos($url, '?') !== false) {
$url = explode('?', $url, 2)[0];
}

// Check if force Trailing Slash is enabled
if ($this->getEnforceTrailingSlashConfig() !== true) {
return false;
}

$requestPath = $request->getURL();

// skip if requesting root
if ($requestPath === '/' || $requestPath === '') {
return false;
}

// Skip if is AJAX request
if (Director::is_ajax()) {
return false;
}

// Skip if request has a file extension
if (!empty($request->getExtension())) {
return false;
}

// Skip if any configured ignore paths match
$paths = (array) $this->getEnforceTrailingSlashConfigIgnorePaths();
if (!empty($paths)) {
foreach ($paths as $path) {
if (str_starts_with(trim($path, '/'), trim($requestPath, '/'))) {
return false;
}
}
}

// Skip if any configured ignore user agents match
$agents = (array) $this->getEnforceTrailingSlashConfigIgnoreUserAgents();
if (!empty($agents)) {
if (in_array(
$request->getHeader('User-Agent'),
$agents
)) {
return false;
}
}

// Already using trailing slash correctly
$addTrailingSlash = Controller::config()->uninherited('add_trailing_slash');
$hasTrailingSlash = str_ends_with($url, '/');
if (($addTrailingSlash && $hasTrailingSlash) || (!$addTrailingSlash && !$hasTrailingSlash)) {
return false;
}

return true;
}

/**
* @return int
*/
Expand Down Expand Up @@ -357,7 +487,7 @@ public function setEnabledEnvs($enabledEnvs)
protected function isEnabled()
{
// At least one redirect must be enabled
if (!$this->getForceWWW() && !$this->getForceSSL()) {
if (!$this->getForceWWW() && !$this->getForceSSL() && $this->getEnforceTrailingSlashConfig() !== true) {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Control/RequestHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ public function Link($action = null)
// Check configured url_segment
$url = $this->config()->get('url_segment');
if ($url) {
$link = Controller::join_links($url, $action, '/');
$link = Controller::join_links($url, $action);

// Give extensions the chance to modify by reference
$this->extend('updateLink', $link, $action);
Expand Down
5 changes: 3 additions & 2 deletions src/Dev/DebugView.php
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,9 @@ public function Breadcrumbs()
$pathLinks = [];
foreach ($parts as $part) {
if ($part != '') {
$pathPart .= "$part/";
$pathLinks[] = "<a href=\"$base$pathPart\">$part</a>";
$pathPart = Controller::join_links($pathPart, $part);
$href = Controller::join_links($base, $pathPart);
$pathLinks[] = "<a href=\"$href\">$part</a>";
}
}
return implode('&nbsp;&rarr;&nbsp;', $pathLinks);
Expand Down
2 changes: 1 addition & 1 deletion src/Dev/TaskRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public function index()

foreach ($tasks as $task) {
$list->push(ArrayData::create([
'TaskLink' => $baseUrl . 'dev/tasks/' . $task['segment'],
'TaskLink' => Controller::join_links($baseUrl, 'dev/tasks/', $task['segment']),
'Title' => $task['title'],
'Description' => $task['description'],
]));
Expand Down
2 changes: 1 addition & 1 deletion src/Forms/HTMLEditor/TinyMCEConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ protected function getConfig()
$settings = $this->getSettings();

// https://www.tiny.cloud/docs/tinymce/6/url-handling/#document_base_url
$settings['document_base_url'] = Director::absoluteBaseURL();
$settings['document_base_url'] = rtrim(Director::absoluteBaseURL(), '/') . '/';

// https://www.tiny.cloud/docs/tinymce/6/apis/tinymce.root/#properties
$baseResource = $this->getTinyMCEResource();
Expand Down
3 changes: 2 additions & 1 deletion src/View/SSViewer.php
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,8 @@ public function setTemplateFile($type, $file)
*/
public static function get_base_tag($contentGeneratedSoFar)
{
$base = Director::absoluteBaseURL();
// Base href should always have a trailing slash
$base = rtrim(Director::absoluteBaseURL(), '/') . '/';

// Is the document XHTML?
if (preg_match('/<!DOCTYPE[^>]+xhtml/i', $contentGeneratedSoFar ?? '')) {
Expand Down
Loading

0 comments on commit fbcf7dc

Please sign in to comment.