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

Ignore query string on original URL #43

Closed
ronaldbarendse opened this issue Mar 14, 2019 · 5 comments · Fixed by #129
Closed

Ignore query string on original URL #43

ronaldbarendse opened this issue Mar 14, 2019 · 5 comments · Fixed by #129
Assignees
Labels
umbraco/v8 Issues and tasks related to Umbraco 8.

Comments

@ronaldbarendse
Copy link

When the original URL contains a query string, e.g. /bad-url?cookie=yes and a redirect exists for /bad-url, it doesn't match. Only when the option 'Forward query string' is enabled, the redirect kicks in (although it keeps the unnecessary query string)...

I would expect the redirect would work anyway, because anyone could just add a query string to the bad/old URL (some tools do this, e.g. link shortners, social media, Google ads).

@ronaldbarendse
Copy link
Author

Should be easily fixed by removing && x.ForwardQueryString from:

sql = new Sql().Select("*").From(RedirectItemRow.TableName).Where<RedirectItemRow>(x => x.RootNodeId == rootNodeId && x.Url == url && x.ForwardQueryString);

@abjerner
Copy link
Member

@ronaldbarendse thanks for reporting 👍

As the package has worked by checking against both the path and query for a while now, I think it will be a breaking change to skip the check on the query string.

I also fear that changing this could have more consequences. For instance it will be harder to check if a redirect already exists as it can then be either a full match or a partial match.

Nevertheless, I see your reasons for creating this issue. I will try to raise this issue with my colleagues to find the best approach.

@ronaldbarendse
Copy link
Author

No breaking change is needed, as it first checks for a redirect with the query string and if it doesn't find one, tries to get the redirect only by URL.

The ForwardQueryString option should only impact the handling of matched redirects, not be a predicate when looking up redirects, right?

@ronaldbarendse
Copy link
Author

@abjerner Any chance of looking at the submitted PR? With the latest commit, this should work as expected: https://github.com/skybrud/Skybrud.Umbraco.Redirects/pull/44/files?w=1

So if a redirect without query string exists, but the incoming URL does have a query string, it first checks for the exact match (URL and query string), but if it can't find one, checks for a redirect without a query string.

@MMasey
Copy link

MMasey commented Oct 6, 2021

Out of curiosity, what's the current status of this issue? I have a V8 site that is experiencing this same issue. For a quick patch I could fork the repo and pull this @ronaldbarendse's fix above and build it locally, but just wanted to see if it might be actioned in this repo.

I know you're probably super busy @abjerner so no worries.

@abjerner abjerner added the umbraco/v8 Issues and tasks related to Umbraco 8. label Feb 17, 2022
abjerner pushed a commit that referenced this issue Feb 17, 2022
…129)

* Added missing ForwardQueryString check to GetRedirectByPathAndQuery

GetRedirectByPathAndQuery(guid, string, string) does the same thing as GetRedirectByUrl(guid, string, string) so consolidated the logic variations, added the fallback ForwardQueryString check, and marked GetRedirectByUrl(guid, string, string)  as obsolete.

Fixes #128 and along with other recent code should fix #43 and #104

* Removed obsolete method from alpha release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
umbraco/v8 Issues and tasks related to Umbraco 8.
Projects
None yet
3 participants