Closed Bug 1513311 Opened 6 years ago Closed 6 years ago

Allow remote custom CSS to adjust section and/or card styles

Categories

(Firefox :: New Tab Page, enhancement, P1)

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 66
Iteration:
66.3 - Jan 7 - 20
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: github-merged)

User Story

1. from tab 1 about:config enable discovery stream `browser.newtabpage.activity-stream.discoverystream.config` with default layout (control)
2. open tab 2 new tab and see content
3. in tab 1, change endpoint to `…&layout_variant=dev-test-styles`
4. tab 2 should update with red text, green text, etc
5. in tab 1, change endpoint back to  `…&layout_variant=control`
6. tab 2 should update with no custom colors

Attachments

(4 files)

sample css result
(deleted), image/png
Details
Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4674
(deleted), text/x-github-pull-request
Details
dev-test-styles variant
(deleted), image/png
Details
QA Result : bug - 1513311.png
(deleted), image/png
Details
The idea is to allow exceptions or last-minute fixes to styling to avoid needing code changes and uplifts. For example: - tweaking the position of some title text by a few pixels - adjusting the size or color of the description - overriding the image positioning - showing/hiding of various card information (showing requires the content to have been rendered instead of omitted) Potentially this could be used for one-off campaign styling say.. some christmas theme wanting green and red border around the whole section?? So this would be sent down with the section definition instead of built into Firefox. My current thinking is that the section definition would have more JSON defining the section styling and card styling that then gets applied as inline css to the react component. This is as opposed to a plain css sheet injection that then would require appropriate selectors. Unclear if we want to filter style objects for accepted whitelisted css properties and/or values.
Just confirmed with product/design that generally this remote styling override is exceptional and/or experimental to try a different look/feel that is temporary for a special campaign or to rapidly experiment without needing to ride the trains. If something is known to be desired as a visual option, it should eventually ride the train instead of relying on overrides. Some more concrete examples: - adjusting border, padding on cards - adding transitions/animations - showing content ::before/::after - specially styling nth child The original approach I had in mind to avoid "injected CSS file" would be to narrowly target specific elements, e.g., card.title, and apply some inline style. I.e., support css declarations on some properties/values but not support css selectors. However, this doesn't allow for the expressiveness to do nth-child or animation keyframes or special :hover styling, etc., and potentially these restrictions result in the functionality not being used that much. Assuming each section in the layout either gets a class name assigned in a known pattern or explicit set from the remote json layout definition (e.g., {title: "headlines", className: "section1", view: "hero"}), then an actual css file/string would then just do something like: .section1 .card:first-child .card-details:hover { transform: rotate…
Alternative implementations for "injected CSS file" approach that we discussed but doesn't satisfy some of the desired product features: Allow each remote json section definition to specify some styleOverrides: { sectionTitle: [declarations], cardTitle: […], cardContext: […], cardDetails: […], … Then in the existing Card.jsx for say the title: https://github.com/mozilla/activity-stream/blob/8f0bbbffef8b115da304cfd641bd04dd95baba85/content-src/components/Card/Card.jsx#L236 <h4 className="card-title" dir="auto">{link.title}</h4> make it conditionally add inline styles <h4 className="card-title" style={overrides.cardTitle} … Or to wrap it in a higher order component: <OverrideableStyle type="cardTitle"><h4 … Or something that post-processes the render output of various components to conditionally override styles.
Assignee: nobody → edilee
Severity: normal → enhancement
Iteration: --- → 66.1 - Dec 10-23
Priority: -- → P1
Attached image sample css result (deleted) —
freddyb, we're looking to augment the existing about:newtab with some remote css capability. Instead of pure remote css directly served by pocket, I was wondering if it makes sense to scope things with a bit more structure instead of only relying on the content security policy, and hopefully there's some common practices or simple things we can do. For example, the server can provide some JSON that specifies css selectors and css declarations that the client processes before generating text used as CSS: 1) prefix selectors to scope styles to a block/parent "styles": { ".outer,.inner": "color: red;" } becomes #component-1-1 .outer, #component-1-1 .inner { color: red; } 2) reject undesired declarations, e.g., contains "}" or url()s that aren't http/resource/data "styles": { ".foo": "} * { …", ".bar": "background-image: url(badscheme:…)" } These would just be ignored. I've attached a screenshot of the result with (1) but not (2) for this JSON: "components0": [ {"type": "TopicsHeader"}, {"type": "Orange Stories", "styles": {"": "background-color: orange;"} }, {"type": "Blue Stories", "styles": {"": "background-color: blue;"} } ], "components1": [ {"type": "TopSites"}, {"type": "Stories", "styles": {",div": "outline: 1px solid green; background-color: rgba(128, 0, 0, 0.7); padding: 1em;} * {color: red !important;"} } ] And the matching generated text: #component-0-1 {background-color: orange;} #component-0-2 {background-color: blue;} #component-1-1 ,#component-1-1 div {outline: 1px solid green; background-color: rgba(128, 0, 0, 0.7); padding: 1em;} * {color: red !important;}
Flags: needinfo?(fbraun)
Iteration: 66.1 - Dec 10-23 → 66.2 - Dec 24 - Jan 6
Hi Ed, I've leave the need-info for Freddy but jumping in with a few thoughts/questions: 1. What is the server-side (pocket I guess, but specifically what server, who owns it, what is the process for uploading content etc etc). I assume from the above that we are not planning on relying on the Pocket server controls at all, and restrict the capability here on the client side as much as possible. 2. Could we avoid some risk by using CSSOM to make the style changes you want to make instead of generating CSS? Further feedback inline. PS Im crap at CSS so take with grain of salt :) (In reply to Ed Lee :Mardak from comment #4) >... > > For example, the server can provide some JSON that specifies css selectors > and css declarations that the client processes before generating text used > as CSS: > > 1) prefix selectors to scope styles to a block/parent > > "styles": { > ".outer,.inner": "color: red;" > } > > becomes > > #component-1-1 .outer, > #component-1-1 .inner { > color: red; > } Are their selectors that would allow "escape" from the target element (e.g. ~ )? styles": { ".outer ~ p": "color: red;" } That would select some other element I think? > 2) reject undesired declarations, e.g., contains "}" or url()s that aren't > http/resource/data > > "styles": { > ".foo": "} * { …", > ".bar": "background-image: url(badscheme:…)" > } How are you going to ignore these? If the plan was blacklist, that's a recipe for problems. If you are worried about {, then you need to be worried about newline, and anything else that can result in malformed CSS which might change the meaning of the rules. CSSOM might help you here Im guessing. (I'm suggesting CSSOM, as with for HTML injection, the standard approach to create "safe" html from an untrusted string is to use a DocumentFragment, and set its innerHTML, and then prune unwanted nodes with a whitelist. I wonder if a similar approach exist for CSS) The bottom line here is that at a minimum I'd need to see the code (and I couldn't find it) but better would be to have a strategy first in place explaining how this approach you are taking is ensured to be safe. It should be possible but I've not seen such an approach for semi-trusted CSS before. Maybe freddy has a better ideas.
(In reply to Paul Theriault [:pauljt] from comment #5) > 1. What is the server-side I had been thinking the styling overrides would be part of the Pocket server layout response from bug 1515354 test: https://getpocket.com/v3/newtab/layout?version=1&layout_variant=dev-test-1&consumer_key=40249-e88c401e1b1f2242d9e441c4 spec: https://getpocket.com/v3/newtab/layout.yml Perhaps that's a separate security review item overall, but specifically for the styling aspects, I would guess if it's okay that Pocket servers control the content/stories and some layout already, then adding styling isn't really much additional concern (??) > 2. Could we avoid some risk by using CSSOM to make the style changes you > want to make instead of generating CSS? That should be doable without limiting the desired expressiveness. It probably will introduce some complexity to coordinate with React lifecycle events. > Are their selectors that would allow "escape" from the target element (e.g. ~ )? The sibling selector with naive prefixing of desired #component-id would allow modifying the styles of other components (rather than only the descendants), but the intent of styling overrides is to change the styles of the components to begin with. ;) Although this sibling example shows we should be careful about the tree structure of the elements. It seems that selector patterns generally increase the restrictions -- even with :not, it is selecting a more specific target. There are grid || column related combinator that don't have a direct parent / child relationship, but at least with component prefixing, the left-hand-side would not be a column… > (I'm suggesting CSSOM, as with for HTML injection, the standard approach to > create "safe" html from an untrusted string is to use a DocumentFragment, > and set its innerHTML, and then prune unwanted nodes with a whitelist. I > wonder if a similar approach exist for CSS) From my quick testing, it does seem like CSSOM will help in at least for the two examples: > ".foo": "} * { …", stylesheet.insertRule(ruleString) is expecting only one rule at a time rejecting the "} * {" with "SyntaxError: An invalid or illegal string was specified" > ".bar": "background-image: url(badscheme:…)" Inserting a rule then reading out the .cssText does normalize whitespaces and no/single/double quotes, so we can then whitelist allowed protocols for url functions without worrying about other parsing differences. E.g., "#foo {background-image: url(bar ) } " becomes "#foo { background-image: url(\"bar\"); }" and strange stuff like "#foo {background-image: url (bar); color: red}" just removes offending: "#foo { color: red; }" Definitely seems like CSSOM will help, and hopefully there's existing code / patterns already in use!
Iteration: 66.2 - Dec 24 - Jan 6 → 66.3 - Jan 7 - 20

heycam and I discussed last week some approaches to sanitize server provided "CSS", and generally leveraging CSSOM allows safer checking of both declarations and selectors rather than reimplementing parsing css with raw css and more reusability than shadow DOM.

Declarations can be checked with something like document.createElement("dummy").style = declarations where only valid declarations are accepted and selectors, etc. are rejected. Then individual normalized property values can be read out for additional checks, e.g., verify url("…") patterns, ignore certain properties or values.

Selectors can be checked by adding a rule with no declarations, e.g., sheet.insertRule(selectors + "{}") where insertRule allows only one rule at a time preventing sneaking in declarations with {}/newlines/etc.

To additionally scope server provided selectors, we can prefix a container selector in front of each selector split on "," if the server wants to style multiple descendants with the same declarations. Notably, a simple selectors.split(",") results in invalid selectors if a selector included commas as a string, e.g., div[attr="a,b"], however given our existing usage of attributes (currently class, dir, title), it seems acceptable to limit the expressiveness of selectors using commas in strings.

Compared to raw css, specifically setting style declarations and selectors limits the CSS to style rules (as opposed to import, media, keyframes, etc.) And prefixing a selector limits the concatenated selector to siblings and descendants where notably siblings could also be styled with the functionality here anyway.

Clearing the need-info, as we've just discussed the details in our meeting.
Ed will work on a CSS sanitizer to ensure all URLs are pointing to our CDN (or are strictly local, pending a decision from Product team).

I'll provide some test cases early on and perform a security test on the sanitizer, once implemented.

Flags: needinfo?(fbraun)

nchapman says allowing pocket cdn would be nice, but starting with only chrome/resource if it helps keep things simple.

mathijs says images could be hosted at https://img-getpocket.cdn.mozilla.net

I suppose a simple implementation could check the normalized url() declaration for those 3 url prefixes. This happens to disallow relative urls which should be fine as it could just be written as resource://activity-stream/…

freddyb, here's a try build of https://github.com/mozilla/activity-stream/pull/4674

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a45865f11249653859224fea7fffa9c688943b6a

The PR has a sample pref with a data uri layout JSON that includes styles and an example screenshot.

If you have some selectors and declarations to try out, I can take a screenshot of the result and console.

Flags: needinfo?(fbraun)
Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/e34ed22aef02831be42ad4e8892481dd8c55e17f Bug 1513311 - Allow remote custom CSS to adjust section and/or card styles (#4674)
Attached image dev-test-styles variant (deleted) —

For QA testing, changing the json pref config browser.newtabpage.activity-stream.discoverystream.config to be enabled and use layout_variant=dev-test-styles instead of layout_variant=dev-test-1 should result in something like attached.

Keywords: github-merged
Blocks: 1521093

https://hg.mozilla.org/mozilla-central/rev/381457de43c0

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Clearing needinfo in favor of spin-off bug 1521479

Flags: needinfo?(fbraun)
User Story: (updated)
Attached image QA Result : bug - 1513311.png (deleted) —

QA Results:

Tested on :

FF Nightly version : 66.0a1 (2019-01-22)

OS : Mac and Windows 10

Works as expected. New Layout Output in the attachment QA Result : bug - 1513311.png

Looks good to me.

Closing as verified.

Status: RESOLVED → VERIFIED
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: