Duplication of lists of tags and attributes in code

[trichoplax]

Describe the bug Not a bug, more a bug waiting to happen. The lists of allowed HTML tags and allowed tag attributes are present in two different places in the codebase, each with a comment warning that the other list must be updated at the same time. Failing to do so will break either the warnings during editing, or the rendering of the saved post (depending on which list has something missing).

app/assets/javascripts/post.js lines 1 to 6:

// IF YOU CHANGE THESE VALUES YOU MUST ALSO CHANGE app/helpers/posts_helper.rb
const ALLOWED_TAGS = ['a', 'p', 'span', 'b', 'i', 'em', 'strong', 'hr', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6',
  'blockquote', 'img', 'strike', 'del', 'code', 'pre', 'br', 'ul', 'ol', 'li', 'sup', 'sub', 'section', 'details',
  'summary', 'ins', 'table', 'thead', 'tbody', 'tr', 'th', 'td', 's'];
const ALLOWED_ATTR = ['id', 'class', 'href', 'title', 'src', 'height', 'width', 'alt', 'rowspan', 'colspan', 'lang',
  'start', 'dir'];

app/helpers/posts_helper.rb lines 73 to 76:

      # IF YOU CHANGE THESE VALUES YOU MUST ALSO CHANGE app/assets/javascripts/posts.js
      self.tags = %w[a p span b i em strong hr h1 h2 h3 h4 h5 h6 blockquote img strike del code pre br ul ol li sup sub
                     section details summary ins table thead tbody tr th td s]
      self.attributes = %w[id class href title src height width alt rowspan colspan lang start dir]

Expected behavior The list should be present in only one place, so it can never become unsynchronised. Ideally this would be in a config file rather than hardcoded, so each instance can choose independently which tags and attributes to allow, without having to make code changes.

Another potential approach would be to add these lists to the site settings so an admin can amend the lists through the user interface.

[cellio]

I agree this is a problem waiting to happen. I don't know how we get JavaScript code and Ruby code to share a single source of truth, but it would be great to figure something out here.

[Oaphi]

We can use an assets preprocessor to achieve this, methinks. Certainly a "nice to have" thing, although I don't think it should be high on our priority list as those of us involved with development at this point are aware of the requirement. That, and it seems like Rails is switching assets preprocessor from Sprockets to Propshaft in 8.0 - I have no idea how it works, unfortunately.

[cellio]

@Oaphi should we defer this until Rails 8.0, then?

[Oaphi]

@Oaphi should we defer this until Rails 8.0, then?

Unsure, it might be quite a while before we upgrade - if anyone wants to tackle this issue in the meantime, no objections from me! Just noting that we'll might need to redo the change at some point.

[trichoplax]

I added this as something important to do at some point - I definitely don't see it as urgent. I like the idea of eventually being able to allow new tags for Codidact without worrying that this forces other instances to either do the same or maintain a fork.

I agree that for the Codidact team the two lists are unlikely to go out of sync any time soon. When we can, I'd like us to remove the burden of keeping them in sync for other instances. Thinking about allowing the <math> tag made me think to raise this - allowing that tag would be a straightforward change for us to make, but currently it would also mean allowing it for every other instance (including any we don't know about).