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

404 error page configured via Error404Collection doesn't return 404 in 11.2 #13907

Closed
abjerner opened this issue Mar 2, 2023 · 7 comments
Closed

Comments

@abjerner
Copy link
Contributor

abjerner commented Mar 2, 2023

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

11.2

Bug summary

Previously when setting up a 404 error page via the Error404Collection in appsettings.json, accessing a non-existing URL would trigger a 404 response with said error page.

With 11.2, the error page is still shown, but the server now incorrectly responds with a 200 status code.

When checking what has changed between 11.1 and 11.2, I don't see anything obvious that would cause this, but I've now been able to reproduce the issue across a few different sites.

release-11.1.0...release-11.2.0

As @nul800sebastiaan pointed out on Discord, this part seems to set the status code correctly. The file doesn't seem to have been changed from way before 11.0, so some other part of Umbraco may have changed instead:

https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Infrastructure/Routing/ContentFinderByConfigured404.cs#L134

Specifics

No response

Steps to reproduce

Umbraco 11.0

Install Umbraco 11.0:

# Ensure we have the latest Umbraco templates
dotnet new -i Umbraco.Templates::11.0.0

# Create solution/project
dotnet new sln --name "MySolution"
dotnet new umbraco --force -n "MyProject" --friendly-name "Administrator" --email "admin@example.com" --password "1234567890" --development-database-type SQLite
dotnet sln add "MyProject"

#Add starter kit
dotnet add "MyProject" package clean

dotnet run --project "MyProject"
#Running

The above snippet installs the clean starter kit, with features an error page. With this being a starter kit, the error page gets a reproduceable GUID key, so we can add an Error404Collection to Umbraco:Cms:Content in appsettings.json:

"Error404Collection": [
  {
    "Culture": "default",
    "ContentKey": "abd58ca0-c700-47f3-b7ab-6672583572ce"
  }
]

When rebuilding the site, and accessing a URL like /oh-noes, the server responds with the error page along with a 404 status code.

Umbraco 11.1

Upgrade Umbraco to 11.1:

Update-Package Umbraco.Cms -version 11.1

Rebuild the site, and access /oh-noes again. the server still responds with the error page and a 404 status code. When upgrading between 11.0 and 11.1, it doesn't appear necessary to run the Umbraco installer.

Umbraco 11.2

Now upgrade Umbraco to 11.2:

Update-Package Umbraco.Cms -version 11.2

This step requires running the installer to complete the upgrade, so do this first. Now when /oh-noes again, the server still responds with the configured error page, but now the status code is incorrectly 200.

Expected result / actual result

The error page should still return 404 as in previous releases of Umbraco 11, as the status code may be critical for packages and external tools to work correctly (eg. my own Skybrud Redirects, search engines and external tools that check for broken links).

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

Hi there @abjerner!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

  • We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@nikolajlauridsen
Copy link
Contributor

Hey @abjerner thanks for reporting, I can indeed reproduce that this is an issue.

The issue was introduced in #13767, turns out that is because the PublishedRequestFilterAttribute is what's setting the 404 status code.

I've made a fix for the issue her #13911 (the bug was on its way to v10 as well 🙈 )

@abjerner
Copy link
Contributor Author

abjerner commented Mar 3, 2023

Awesome. And thanks for a quick fix 👍

I had quick look through the changes between 11.1 and 11.2, and PublishedRequestFilterAttribute was also my best guess for the culprit although I wasn't sure how it was used.

@chrden
Copy link

chrden commented Mar 6, 2023

@nikolajlauridsen I have also found an issue with using Error404Collection with 'ContentXPath' on 11.0.0 and 11.2.0.

My XPath value is correct as I am not getting any errors when parsing the xpath value. When I enter an incorrect XPath value, I get the following, for example:
Could not parse xpath expression: "/root/homePage//error404Page"

When I attempt to access a page that does not exist with the following settings, I get the 'standard' Umbraco 404.

"Error404Collection": [
	{
		"Culture": "default",
		"ContentXPath": "/homePage//error404Page"
	}
]

However, when I use 'ContentKey' instead, as below, the 404 successfully returns my Error 404 node:

"Error404Collection": [
	{
		"Culture": "default",
		"ContentKey": "7ff6c39b-7c4c-46d4-b329-63879246a701"
	}
]

I upgraded from 11.0.0 to 11.2.0 but the issue persists on both versions.

Please let me know if you need more information, or if this should be created as a new issue.

Thanks

@nikolajlauridsen
Copy link
Contributor

This sounds like a different issue, could you please create a separate issue for it? Thanks.

@abjerner
Copy link
Contributor Author

@nikolajlauridsen maybe I'm missing something, but it seems this has only been fixed for U10. Will it be cherry picked to U11? Or should I create a PR for U11?

@bergmania
Copy link
Member

@abjerner. Currently we merge v10 to v11 and up on a weekly basis, so this is already in next v11. 2538f94

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants