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.

Sign up for GitHub

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

Jump to bottom

Zope should not set Content-Type on empty response for exception #1089

Closed
mauritsvanrees opened this issue Jan 9, 2023 · 2 comments
Closed

Zope should not set Content-Type on empty response for exception #1089

mauritsvanrees opened this issue Jan 9, 2023 · 2 comments
Labels
bug

Comments

@mauritsvanrees
Copy link
Member

See community.plone.org where @fredvd describes the problem in depth. The problem is caused by #1082 which tried to fix a regression in the Zope security fix. That is Zope 4. I need to check Zope 5 still, but I expect the same problem. Let me describe the problem here as well.

We start with Plone 5.2.10, Zope 4.8.3:

  • Install Plone, put Varnish caching server in front. Activate plone.app.caching, tweak some settings to cache resources less long for testing. I skip details and variations because they are not relevant to Zope.
  • Request a css resource from Plone (or rather a page the uses this resource). This gets stored in Varnish and in the browser. Content-Type is text/css.
  • After a while do a hard refresh of the page, or use a new browser to request the resource again.
  • Varnish requests the resource again from Plone with an IfNotModifiedSince header.
  • Plone sees there are no changes. plone(.app).caching handles this by raising an exception: Intercepted, with a status 304.
  • Varnish gets a 304 NotModified with an empty body.
  • Important: this response has no Content-Type header.
  • All is well.

Now the same, but with Plone 5.2.10.2, which uses Zope 4.8.6:

  • Varnish gets a 304 NotModified with an empty body.
  • This response does have a Content-Type header: text/html. The header is added by this code (Zope 4.x).
  • Result: Varnish updates the Content-Type in its cache, and delivers the CSS with Content-Type text/html. I don't know if this could be considered a bug in Varnish.
  • Depending on browser defaults or extra headers injected by nginx/Apache, the browser refuses to load this CSS because it has the wrong Content-Type. Result is an ugly page.

Note that if you remove Varnish from this stack, the same happens to the headers, but it has no bad effect: the browser directly gets the 304 from Plone, and does not care about the changed Content-Type.

The solution as I see it: if Zope is handling an exception view and the body is empty, then do not insert a Content-Type header. This works when I try it in Plone. Is anyone wary of new regressions this would introduce?
See PR #1088 for Zope 4. I will look at Zope 5 as well.
This was referenced Jan 9, 2023
Merged
Merged
@mauritsvanrees
Copy link
Member Author

Zope 5.7.3 (Plone 6.0.0.2) has the same problem. PR #1090.

@dataflake dataflake added the bug label Jan 9, 2023
@dataflake
Copy link
Member

Fixed by PRs #1088 and #1090

mauritsvanrees added a commit to plone/plone.app.caching that referenced this issue Jan 9, 2023
@mauritsvanrees
Revert changes to tests to work with the Zope security fix.
75c80a6 
We must have an empty byte, not text, otherwise it is an indication that we get a possibly wrong Content-Type in a 304 status.
See zopefoundation/Zope#1089.

This reverts commit e6941fd.
mauritsvanrees added a commit to plone/plone.app.caching that referenced this issue Jan 9, 2023
@mauritsvanrees
Revert changes to tests to work with the Zope security fix.
da84b9d 
We must have an empty byte, not text, otherwise it is an indication that we get a possibly wrong Content-Type in a 304 status.
See zopefoundation/Zope#1089.

This reverts commit e6941fd.
@mauritsvanrees mauritsvanrees mentioned this issue Jan 9, 2023
Merged
mauritsvanrees added a commit to plone/plone.app.caching that referenced this issue Jan 9, 2023
@mauritsvanrees
Revert changes to tests to work with the Zope security fix.
9accdb1 
We must have an empty byte, not text, otherwise it is an indication that we get a possibly wrong Content-Type in a 304 status.
See zopefoundation/Zope#1089.

This reverts commit 52885bf.
@mauritsvanrees mauritsvanrees mentioned this issue Jan 9, 2023
Merged
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jan 9, 2023
@mauritsvanrees
[fc] Repository: plone.app.caching
9b2f0d7 
Branch: refs/heads/master
Date: 2023-01-09T21:30:27+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.caching@da84b9d

Revert changes to tests to work with the Zope security fix.

We must have an empty byte, not text, otherwise it is an indication that we get a possibly wrong Content-Type in a 304 status.
See zopefoundation/Zope#1089.

This reverts commit e6941fdfe6c8a3c98af98f4986e6932739adfca6.

Files changed:
A news/1089.bugfix
M plone/app/caching/tests/test_profile_with_caching_proxy.py
M plone/app/caching/tests/test_profile_without_caching_proxy.py
Repository: plone.app.caching

Branch: refs/heads/master
Date: 2023-01-09T22:45:00+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.caching@c3ee243

Merge pull request #109 from plone/maurits-fix-tests-again

Revert changes to tests to work with the Zope security fix. [master]

Files changed:
A news/1089.bugfix
M plone/app/caching/tests/test_profile_with_caching_proxy.py
M plone/app/caching/tests/test_profile_without_caching_proxy.py
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jan 10, 2023
@mauritsvanrees
[fc] Repository: plone.app.caching
1506971 
Branch: refs/heads/2.x
Date: 2023-01-09T21:58:03+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.caching@9accdb1

Revert changes to tests to work with the Zope security fix.

We must have an empty byte, not text, otherwise it is an indication that we get a possibly wrong Content-Type in a 304 status.
See zopefoundation/Zope#1089.

This reverts commit 52885bf0f81ccb655196a65226e424ae74b9b253.

Files changed:
A news/1089.bugfix
M plone/app/caching/tests/test_profile_with_caching_proxy.py
M plone/app/caching/tests/test_profile_without_caching_proxy.py
Repository: plone.app.caching

Branch: refs/heads/2.x
Date: 2023-01-09T22:48:22+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.caching@e357e9b

Fixed header tests on Python 2.

Files changed:
M plone/app/caching/tests/test_profile_with_caching_proxy.py
M plone/app/caching/tests/test_profile_without_caching_proxy.py
Repository: plone.app.caching

Branch: refs/heads/2.x
Date: 2023-01-10T14:31:04+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.caching@b40e63e

Merge pull request #110 from plone/maurits-fix-tests-again-2x

Revert changes to tests to work with the Zope security fix.

Files changed:
A news/1089.bugfix
M plone/app/caching/tests/test_profile_with_caching_proxy.py
M plone/app/caching/tests/test_profile_without_caching_proxy.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mauritsvanrees @dataflake