feat: theming backend #8083
feat: theming backend #8083
Conversation
c099095 to
c7e97fa
Compare
| return nil, fmt.Errorf("cannot use both legacy color properties (primary, secondary) and the new CSS property simultaneously") | ||
| } | ||
|
|
||
| if hasLegacyColors { |
There was a problem hiding this comment.
If the old color codes are now legacy, would it make sense to rewrite them to CSS and save them in the css property of the theme resource instead? I.e. so we can remove the colors fields from the theme resource and only have the css field.
There was a problem hiding this comment.
@mindspank does it make anything easier in UI if we deprecate old format all together?
There was a problem hiding this comment.
Not majorly, I would rather keep the old for now and we can refactor that out later
runtime/parser/parse_theme.go
Outdated
| // Allowed URL schemes | ||
| var allowedSchemes = regexp.MustCompile(`^(https?|data:image|#)`) |
There was a problem hiding this comment.
Does our CSP policies need to be updated to support arbitrary references? (And can we do it safely?)
There was a problem hiding this comment.
Good question. Will need to test it on non-localhost to make sure.
runtime/parser/parse_theme.go
Outdated
| // Allowed URL schemes | ||
| var allowedSchemes = regexp.MustCompile(`^(https?|data:image|#)`) | ||
|
|
||
| func sanitizeCSS(c string) (string, error) { |
There was a problem hiding this comment.
Just a question – how safe do you feel this is? Quick effort or thorough? And how serious are the attack opportunities?
There was a problem hiding this comment.
This was quick effort. We definitely need a deeper look at this before we release this feature.
There was a problem hiding this comment.
We can also sanitize on the frontend as opposed to the backend, library support should be a bit wider there
There was a problem hiding this comment.
@AdityaHegde I see you removed the sanitization – in that case, does it still need to traverse the AST or could it just propagate directly to the proto after it parses successfully?
There was a problem hiding this comment.
We are removing comments to make things easy to parse. Also bracket balancing is not an error, we do it after traversing.
0f43b2a to
31256e5
Compare
| message ThemeCSS { | ||
| string primary = 1; | ||
| string secondary = 2; | ||
| map<string, string> properties = 3; | ||
| } |
There was a problem hiding this comment.
nit: what do you think about updating the naming as below:
- With
ThemeCSS, it's not actually clear that all these strings are colors. Consider naming itThemeColorsinstead - Call the map
variablesinstead ofproperties(since they're CSS color variables, right?)
| message ThemeCSS { | |
| string primary = 1; | |
| string secondary = 2; | |
| map<string, string> properties = 3; | |
| } | |
| message ThemeColors { | |
| string primary = 1; | |
| string secondary = 2; | |
| map<string, string> variables = 3; | |
| } |
There was a problem hiding this comment.
Ya, I was not a 100% sure what to name these.
| string primary_color_raw = 3; | ||
| string secondary_color_raw = 4; |
There was a problem hiding this comment.
If it's easy, it would be nice to remove the legacy fields from the proto and just map them to the light property (guessing it'd make the UI handling simpler also?)
There was a problem hiding this comment.
Good point. Lets do that once UI has been updated? Otherwise main will be in bad state.
Closes APP-429
Implements the backend parser for css based themes
Checklist: