search

neon-jungle / wagtail-schema.org

Schema.org JSON-LD tags for Wagtail sites
BSD 2-Clause "Simplified" License
70 stars 17 forks source link

CSRF errors cause ld_for_site template_tag to fail, causes 500 error #16

Open dkirkham opened 1 year ago

dkirkham commented 1 year ago

Under some circumstances - such as CSRF errors - django core will attempt to render the 403_csrf.html template, and the context will not include the request key. If the template includes wagtailschemaorg_tags and in particular ld_for_site the template tag will itself cause a KeyError exception. This creates a 500 response and spams the error logs:

...
File "/home/staging/3.8/lib/python3.8/site-packages/django/template/base.py", line 958, in render_annotated
   return self.render(context)
 File "/home/staging/3.8/lib/python3.8/site-packages/django/template/library.py", line 239, in render
   output = self.func(*resolved_args, **resolved_kwargs)
 File "/home/staging/3.8/lib/python3.8/site-packages/wagtailschemaorg/templatetags/wagtailschemaorg_tags.py", line 17, in ld_for_site
   site = Site.find_for_request(context["request"])
 File "/home/staging/3.8/lib/python3.8/site-packages/django/template/context.py", line 83, in __getitem__
   raise KeyError(key)

From what I've read, ideally, template tags facing this sort of problem should fail silently.

As the ld_for_site template tag is often in a base.html file included into most page templates and in this case, the 403_csrf.html template, it is a bit messy to work around this issue in the template system. My workaround, until this package is fixed, is to reimplement ld_for_site in my own code, returning an empty string if the request key is not present.

seb-b commented 1 year ago

We could check for request in the template tag, wouldn't hurt

It's tempting to put the tags from this package in a base template but there are templates where that doesn't make sense (404, 500, csrf error etc). Schema tags are usually used for SEO and these pages aren't typically indexed

I'd recommend having a intermediary template that your pages inherit from that includes the schema tags that inherits your base template so the structure looks something like

base.html -> page_base.html (ld tags here) -> page_1.html, page_2.html etc
         \
           -> 404.html, 403_csrf.html etc
dkirkham commented 1 year ago

Yes, makes sense.

I found another templatetag with the same problem, and I had a deadline, so I've hardcoded the 403_csrf.html for now. I'll apply your solution when I get a chance.