Fix for Redirect URLs that do not contain a backslash (fixes 22308)#22315
Fix for Redirect URLs that do not contain a backslash (fixes 22308)#22315justin-nevitech wants to merge 1 commit intoumbraco:mainfrom
Conversation
…entOutOfRange exception
|
Hi there @justin-nevitech, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
|
This pull request has been mentioned on Umbraco community forum. There might be relevant details there: |
There was a problem hiding this comment.
Pull request overview
Fixes a crash in Umbraco’s URL resolution when legacy redirect routes are malformed (missing a / separator), preventing the Redirect URL Management page from failing during serialization.
Changes:
- Adds a
pos >= 0guard around route parsing inGetUrlFromRouteto avoid negative slicing/substring behavior. - Returns
nullfor malformed legacy routes that don’t contain a/, skipping invalid rows instead of throwing.
| // extract domainUri and path | ||
| // route is /<path> or <domainRootId>/<path> | ||
| var pos = route.IndexOf('/', StringComparison.Ordinal); | ||
| var path = pos == 0 ? route : route[pos..]; | ||
| DomainAndUri? domainUri = pos == 0 | ||
| ? null | ||
| : DomainUtilities.DomainForNode( | ||
| _domainCache, | ||
| _siteDomainMapper, | ||
| int.Parse(route[..pos], CultureInfo.InvariantCulture), | ||
| current, | ||
| culture); | ||
|
|
||
| var defaultCulture = _languageService.GetDefaultIsoCodeAsync().GetAwaiter().GetResult(); | ||
| if (domainUri is not null || | ||
| string.IsNullOrEmpty(culture) || | ||
| culture.Equals(defaultCulture, StringComparison.InvariantCultureIgnoreCase)) | ||
|
|
||
| if (pos >= 0) |
There was a problem hiding this comment.
The PR title mentions a missing backslash, but the code (and the PR description/issue) are handling a missing forward slash ('/'). Consider updating the title to match the actual delimiter to avoid confusion when searching/referencing this fix later.
| if (pos >= 0) | ||
| { | ||
| Uri url = AssembleUrl(domainUri, path, current, mode); | ||
| return UrlInfo.FromUri(url, Alias, culture); | ||
| var path = pos == 0 ? route : route[pos..]; | ||
| DomainAndUri? domainUri = pos == 0 | ||
| ? null | ||
| : DomainUtilities.DomainForNode( |
There was a problem hiding this comment.
This guard fixes GetUrlFromRoute for malformed routes without '/'. However, the same assumption (IndexOf('/') then slicing with route[pos..]) still exists in GetOtherUrls (see around NewDefaultUrlProvider.cs:131-137) and will still throw for the same malformed data. Consider applying the same pos>=0 check there too (or reusing GetUrlFromRoute) so malformed legacy routes can't crash other URL resolution paths.
|
We have a PR raised for this issue already @justin-nevitech - see #22309 I left some feedback on that one, but I think the same would apply to your suggested approach too. |
|
Hi @AndyButland Ok, I didn't realise there was already a PR as it wasn't linked to the issue. I will leave this then as it looks like @VargyasMoniLajos has responded to your suggestions. |
|
Yes, he's just updated so best now we progress that version. Thanks for the contribution though (normally a PR will link to an issue, so it's a bit more obvious if something is already underway). |
Prerequisites
I have added steps to test this contribution in the description below
If there's an existing issue for this PR then this fixes: #22308
Description
The Redirect URL Management page crashes if the umbracoRedirectUrl table contains a malformed row where the Url value does not contain a / character.
The root cause is in NewDefaultUrlProvider.GetUrlFromRoute, which assumes the route is always in the format {contentId}/{path}.
When IndexOf('/') returns -1 (no / present), the code proceeds to call Substring with a negative index, throwing an ArgumentOutOfRangeException. This propagates and crashes the entire Redirect URL Management page during serialization rather than gracefully handling the invalid row.
Fix
The existing route processing logic has been wrapped in a if (pos >= 0) guard, returning null when no / is found in the route:
This ensures that malformed routes without a / are safely handled without throwing an exception.
How to reproduce
The malformed URL is believed to be the result of a migration (the issue was originally reported on a v13 to v17 upgrade), however I was unable to reproduce the malformed data through normal site usage. To test this fix, the Url value of a row in the umbracoRedirectUrl table can be manually set to a value containing no / character (e.g. 1226#). The exact scenario that originally caused the malformed data to be written is unknown.
Steps to test