search

coderedcorp / wagtail-cache

A simple page cache for Wagtail based on the Django cache middleware.
Other
87 stars 29 forks source link

Optional installation section should not contain security-critical information #35

Closed dragon-dxw closed 4 months ago

dragon-dxw commented 2 years ago

If the steps in sections 1 of https://docs.coderedcorp.com/wagtail-cache/getting_started/install.html are followed, password-protected pages are given a Cache-Control header of max-age=300, leading to password-protected content being incorrectly and insecurely cached by CDNs. Only section 3, which is marked as optional, notes that these pages will have the incorrect header set and describe how to inherit the mixin to resolve this problem.

Steps to reproduce:

  1. pip install wagtail-cache,

  2. INSTALLED_APPS = [ 'wagtailcache', ... ] and

  3. MIDDLEWARE = [ 'wagtailcache.cache.UpdateCacheMiddleware', ... 'wagtailcache.cache.FetchFromCacheMiddleware', ]

At each point prior to the MIDDLEWARE, neither the page which requests your password nor the content protected by the password has a Cache-Control header set. After the MIDDLEWARE, the content has a Cache-Control header of max-age: 300.

vsalvino commented 2 years ago

Thanks for reporting the issue. Technically this falls under the "feature not a bug" category, as Wagtail itself DOES NOT set a cache-control header on those pages indicating it is private. Wagtail in fact does not set cache-control headers on any pages (except the admin, where the pages are already marked as private). Therefore it is up to the developer to decide how to handle the pages. Since no pages set cache-control, wagtail-cache assumes everything is public unless it is marked as private. Otherwise the package would be useless and not cache anything by default.

Section 3 in the docs is technically optional, but highly recommended to avoid the issue you mention about caching private pages. Perhaps we should change the section title to encourage folks to read it before deploying to production.

All that being said, I think there are really only 2 possible solutions here:

  1. Remove word "optional" from step 3 in docs.
  2. Make a change in Wagtail itself to automatically add a private cache-control header to pages with privacy set. One could essentially copy the code from our WagtailCacheMixin and move that into the Wagtail Page model itself, although I'm not sure if Wagtail would welcome such a change. I can bring it up with the core team.

If you have any further suggestions on how to improve this experience, do post them here as well.

dragon-dxw commented 2 years ago

They both sound like excellent approaches, thank you! I think it does make sense for Wagtail to take the lead on ensuring those pages aren't cached, although I have little idea of the ramifications of such a decision. I would be very happy if you'd bring it up with the core team.

dragon-dxw commented 2 years ago

Added a PR for removing "optional".

@vsalvino Did you persue the addition of Cache-Control headers in Wagtail core? Whilst I think you're in a very strong position to advocate for such a change, we've just had a review of our incident and came to the conclusion that not setting Cache-Control headers in Wagtail core is a bug, and would very much like to ensure future projects are less likely to stumble into this problem by raising this as an issue.

vsalvino commented 4 months ago

Closing this issue, as we have addressed it in the documentation, and there is a pull request in wagtail core to upstream some of this behavior.