Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the extra path to be ignored when redirecting #2298

Merged
merged 2 commits into from
Dec 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this

# [Unreleased]
### Added
* *Nothing*
* [#2265](https://github.com/shlinkio/shlink/issues/2265) Add a new `REDIRECT_EXTRA_PATH_MODE` option that accepts three values:

* `default`: Short URLs only match if the path matches their short code or custom slug.
* `append`: Short URLs are matched as soon as the path starts with the short code or custom slug, and the extra path is appended to the long URL before redirecting.
* `ignore`: Short URLs are matched as soon as the path starts with the short code or custom slug, and the extra path is ignored.

This option effectively replaces the old `REDIRECT_APPEND_EXTRA_PATH` option, which is now deprecated and will be removed in Shlink 5.0.0

### Changed
* * [#2281](https://github.com/shlinkio/shlink/issues/2281) Update docker image to PHP 8.4
Expand Down
6 changes: 3 additions & 3 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"shlinkio/shlink-config": "^3.4",
"shlinkio/shlink-event-dispatcher": "^4.1",
"shlinkio/shlink-importer": "^5.3.2",
"shlinkio/shlink-installer": "^9.3",
"shlinkio/shlink-installer": "dev-develop#957db97 as 9.4",
"shlinkio/shlink-ip-geolocation": "^4.2",
"shlinkio/shlink-json": "^1.1",
"spiral/roadrunner": "^2024.1",
Expand Down Expand Up @@ -154,8 +154,8 @@
"@test:cli",
"phpcov merge build/coverage-cli --html build/coverage-cli/coverage-html && rm build/coverage-cli/*.cov"
],
"swagger:validate": "php-openapi validate docs/swagger/swagger.json",
"swagger:inline": "php-openapi inline docs/swagger/swagger.json docs/swagger/swagger-inlined.json",
"swagger:validate": "@php -d error_reporting=\"E_ALL & ~E_DEPRECATED & ~E_USER_DEPRECATED\" vendor/bin/php-openapi validate docs/swagger/swagger.json",
"swagger:inline": "@php -d error_reporting=\"E_ALL & ~E_DEPRECATED & ~E_USER_DEPRECATED\" vendor/bin/php-openapi inline docs/swagger/swagger.json docs/swagger/swagger-inlined.json",
"clean:dev": "rm -f data/database.sqlite && rm -f config/params/generated_config.php"
},
"scripts-descriptions": {
Expand Down
2 changes: 1 addition & 1 deletion config/autoload/installer.global.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
Option\UrlShortener\RedirectStatusCodeConfigOption::class,
Option\UrlShortener\RedirectCacheLifeTimeConfigOption::class,
Option\UrlShortener\AutoResolveTitlesConfigOption::class,
Option\UrlShortener\AppendExtraPathConfigOption::class,
Option\UrlShortener\ExtraPathModeConfigOption::class,
Option\UrlShortener\EnableMultiSegmentSlugsConfigOption::class,
Option\UrlShortener\EnableTrailingSlashConfigOption::class,
Option\UrlShortener\ShortUrlModeConfigOption::class,
Expand Down
8 changes: 6 additions & 2 deletions module/Core/src/Config/EnvVars.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,16 @@ enum EnvVars: string
case IS_HTTPS_ENABLED = 'IS_HTTPS_ENABLED';
case DEFAULT_DOMAIN = 'DEFAULT_DOMAIN';
case AUTO_RESOLVE_TITLES = 'AUTO_RESOLVE_TITLES';
case REDIRECT_APPEND_EXTRA_PATH = 'REDIRECT_APPEND_EXTRA_PATH';
case REDIRECT_EXTRA_PATH_MODE = 'REDIRECT_EXTRA_PATH_MODE';
case MULTI_SEGMENT_SLUGS_ENABLED = 'MULTI_SEGMENT_SLUGS_ENABLED';
case ROBOTS_ALLOW_ALL_SHORT_URLS = 'ROBOTS_ALLOW_ALL_SHORT_URLS';
case ROBOTS_USER_AGENTS = 'ROBOTS_USER_AGENTS';
case TIMEZONE = 'TIMEZONE';
case MEMORY_LIMIT = 'MEMORY_LIMIT';
case INITIAL_API_KEY = 'INITIAL_API_KEY';
case SKIP_INITIAL_GEOLITE_DOWNLOAD = 'SKIP_INITIAL_GEOLITE_DOWNLOAD';
/** @deprecated Use REDIRECT_EXTRA_PATH */
case REDIRECT_APPEND_EXTRA_PATH = 'REDIRECT_APPEND_EXTRA_PATH';

public function loadFromEnv(): mixed
{
Expand Down Expand Up @@ -125,11 +127,13 @@ private function defaultValue(): string|int|bool|null
self::DEFAULT_SHORT_CODES_LENGTH => DEFAULT_SHORT_CODES_LENGTH,
self::SHORT_URL_MODE => ShortUrlMode::STRICT->value,
self::IS_HTTPS_ENABLED, self::AUTO_RESOLVE_TITLES => true,
self::REDIRECT_APPEND_EXTRA_PATH,
self::MULTI_SEGMENT_SLUGS_ENABLED,
self::SHORT_URL_TRAILING_SLASH => false,
self::DEFAULT_DOMAIN, self::BASE_PATH => '',
self::CACHE_NAMESPACE => 'Shlink',
// Deprecated. In Shlink 5.0.0, add default value for REDIRECT_EXTRA_PATH_MODE
self::REDIRECT_APPEND_EXTRA_PATH => false,
// self::REDIRECT_EXTRA_PATH_MODE => ExtraPathMode::DEFAULT->value,

self::REDIS_PUB_SUB_ENABLED,
self::MATOMO_ENABLED,
Expand Down
13 changes: 13 additions & 0 deletions module/Core/src/Config/Options/ExtraPathMode.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Shlinkio\Shlink\Core\Config\Options;

enum ExtraPathMode: string
{
/** URLs with extra path will not match a short URL */
case DEFAULT = 'default';
/** The extra path will be appended to the long URL */
case APPEND = 'append';
/** The extra path will be ignored */
case IGNORE = 'ignore';
}
17 changes: 13 additions & 4 deletions module/Core/src/Config/Options/UrlShortenerOptions.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@
public string $schema = 'http',
public int $defaultShortCodesLength = DEFAULT_SHORT_CODES_LENGTH,
public bool $autoResolveTitles = false,
public bool $appendExtraPath = false,
public bool $multiSegmentSlugsEnabled = false,
public bool $trailingSlashEnabled = false,
public ShortUrlMode $mode = ShortUrlMode::STRICT,
public ExtraPathMode $extraPathMode = ExtraPathMode::DEFAULT,
) {
}

Expand All @@ -35,17 +35,26 @@
(int) EnvVars::DEFAULT_SHORT_CODES_LENGTH->loadFromEnv(),
MIN_SHORT_CODES_LENGTH,
);
$mode = EnvVars::SHORT_URL_MODE->loadFromEnv();

// Deprecated. Initialize extra path from REDIRECT_APPEND_EXTRA_PATH.
$appendExtraPath = EnvVars::REDIRECT_APPEND_EXTRA_PATH->loadFromEnv();
$extraPathMode = $appendExtraPath ? ExtraPathMode::APPEND : ExtraPathMode::DEFAULT;

Check warning on line 41 in module/Core/src/Config/Options/UrlShortenerOptions.php

View check run for this annotation

Codecov / codecov/patch

module/Core/src/Config/Options/UrlShortenerOptions.php#L40-L41

Added lines #L40 - L41 were not covered by tests

// If REDIRECT_EXTRA_PATH_MODE was explicitly provided, it has precedence
$extraPathModeFromEnv = EnvVars::REDIRECT_EXTRA_PATH_MODE->loadFromEnv();
if ($extraPathModeFromEnv !== null) {
$extraPathMode = ExtraPathMode::tryFrom($extraPathModeFromEnv) ?? ExtraPathMode::DEFAULT;

Check warning on line 46 in module/Core/src/Config/Options/UrlShortenerOptions.php

View check run for this annotation

Codecov / codecov/patch

module/Core/src/Config/Options/UrlShortenerOptions.php#L44-L46

Added lines #L44 - L46 were not covered by tests
}

return new self(
defaultDomain: EnvVars::DEFAULT_DOMAIN->loadFromEnv(),
schema: ((bool) EnvVars::IS_HTTPS_ENABLED->loadFromEnv()) ? 'https' : 'http',
defaultShortCodesLength: $shortCodesLength,
autoResolveTitles: (bool) EnvVars::AUTO_RESOLVE_TITLES->loadFromEnv(),
appendExtraPath: (bool) EnvVars::REDIRECT_APPEND_EXTRA_PATH->loadFromEnv(),
multiSegmentSlugsEnabled: (bool) EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->loadFromEnv(),
trailingSlashEnabled: (bool) EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv(),
mode: ShortUrlMode::tryFrom($mode) ?? ShortUrlMode::STRICT,
mode: ShortUrlMode::tryFrom(EnvVars::SHORT_URL_MODE->loadFromEnv()) ?? ShortUrlMode::STRICT,
extraPathMode: $extraPathMode,

Check warning on line 57 in module/Core/src/Config/Options/UrlShortenerOptions.php

View check run for this annotation

Codecov / codecov/patch

module/Core/src/Config/Options/UrlShortenerOptions.php#L56-L57

Added lines #L56 - L57 were not covered by tests
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Psr\Http\Message\UriInterface;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;
use Shlinkio\Shlink\Core\Config\Options\ExtraPathMode;
use Shlinkio\Shlink\Core\Config\Options\UrlShortenerOptions;
use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType;
use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException;
Expand Down Expand Up @@ -51,7 +52,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface

private function shouldApplyLogic(NotFoundType|null $notFoundType): bool
{
if ($notFoundType === null || ! $this->urlShortenerOptions->appendExtraPath) {
if ($notFoundType === null || $this->urlShortenerOptions->extraPathMode === ExtraPathMode::DEFAULT) {
return false;
}

Expand All @@ -75,7 +76,11 @@ private function tryToResolveRedirect(

try {
$shortUrl = $this->resolver->resolveEnabledShortUrl($identifier);
$longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request, $extraPath);
$longUrl = $this->redirectionBuilder->buildShortUrlRedirect(
$shortUrl,
$request,
$this->urlShortenerOptions->extraPathMode === ExtraPathMode::APPEND ? $extraPath : null,
);
$this->requestTracker->trackIfApplicable(
$shortUrl,
$request->withAttribute(REDIRECT_URL_REQUEST_ATTRIBUTE, $longUrl),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@
use Mezzio\Router\RouteResult;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\Attributes\TestWith;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Server\RequestHandlerInterface;
use Shlinkio\Shlink\Core\Action\RedirectAction;
use Shlinkio\Shlink\Core\Config\Options\ExtraPathMode;
use Shlinkio\Shlink\Core\Config\Options\UrlShortenerOptions;
use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType;
use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException;
Expand Down Expand Up @@ -57,8 +59,8 @@ public function handlerIsCalledWhenConfigPreventsRedirectWithExtraPath(
ServerRequestInterface $request,
): void {
$options = new UrlShortenerOptions(
appendExtraPath: $appendExtraPath,
multiSegmentSlugsEnabled: $multiSegmentEnabled,
extraPathMode: $appendExtraPath ? ExtraPathMode::APPEND : ExtraPathMode::DEFAULT,
);
$this->resolver->expects($this->never())->method('resolveEnabledShortUrl');
$this->requestTracker->expects($this->never())->method('trackIfApplicable');
Expand Down Expand Up @@ -102,12 +104,17 @@ public static function provideNonRedirectingRequests(): iterable
];
}

#[Test, DataProvider('provideResolves')]
#[Test]
#[TestWith(['multiSegmentEnabled' => false, 'expectedResolveCalls' => 1])]
#[TestWith(['multiSegmentEnabled' => true, 'expectedResolveCalls' => 3])]
public function handlerIsCalledWhenNoShortUrlIsFoundAfterExpectedAmountOfIterations(
bool $multiSegmentEnabled,
int $expectedResolveCalls,
): void {
$options = new UrlShortenerOptions(appendExtraPath: true, multiSegmentSlugsEnabled: $multiSegmentEnabled);
$options = new UrlShortenerOptions(
multiSegmentSlugsEnabled: $multiSegmentEnabled,
extraPathMode: ExtraPathMode::APPEND,
);

$type = $this->createMock(NotFoundType::class);
$type->method('isRegularNotFound')->willReturn(true);
Expand All @@ -127,11 +134,15 @@ public function handlerIsCalledWhenNoShortUrlIsFoundAfterExpectedAmountOfIterati

#[Test, DataProvider('provideResolves')]
public function visitIsTrackedAndRedirectIsReturnedWhenShortUrlIsFoundAfterExpectedAmountOfIterations(
ExtraPathMode $extraPathMode,
bool $multiSegmentEnabled,
int $expectedResolveCalls,
string|null $expectedExtraPath,
): void {
$options = new UrlShortenerOptions(appendExtraPath: true, multiSegmentSlugsEnabled: $multiSegmentEnabled);
$options = new UrlShortenerOptions(
multiSegmentSlugsEnabled: $multiSegmentEnabled,
extraPathMode: $extraPathMode,
);

$type = $this->createMock(NotFoundType::class);
$type->method('isRegularNotFound')->willReturn(true);
Expand Down Expand Up @@ -171,8 +182,10 @@ function () use ($shortUrl, &$currentIteration, $expectedResolveCalls): ShortUrl

public static function provideResolves(): iterable
{
yield [false, 1, '/bar/baz'];
yield [true, 3, null];
yield [ExtraPathMode::APPEND, false, 1, '/bar/baz'];
yield [ExtraPathMode::APPEND, true, 3, null];
yield [ExtraPathMode::IGNORE, false, 1, null];
yield [ExtraPathMode::IGNORE, true, 3, null];
}

private function middleware(UrlShortenerOptions|null $options = null): ExtraPathRedirectMiddleware
Expand All @@ -182,7 +195,7 @@ private function middleware(UrlShortenerOptions|null $options = null): ExtraPath
$this->requestTracker,
$this->redirectionBuilder,
$this->redirectResponseHelper,
$options ?? new UrlShortenerOptions(appendExtraPath: true),
$options ?? new UrlShortenerOptions(extraPathMode: ExtraPathMode::APPEND),
);
}
}