Opened 3 years ago

Closed 3 months ago

#1929 closed New feature (answered)

Allow to customize some CSS of optOut widget

Reported by: halfdan Owned by:
Priority: normal Milestone: 2.x - The Great Piwik 2.x Backlog
Component: Core Keywords:
Cc: Sensitive: no

Description (last modified by vipsoft)

Features to add the OptOut widget (now in CoreAdminHome):

  • option to disable the widget
  • using &css=.. as a parameter allows the user to style the content of the iframe
    • should guard against XSS; blacklist url(), expression(), and @import
  • using &control=.. the user can choose between 3 different control mechanisms (button, checkbox, textlink)
  • parameter for more terse text (i.e., as originally implemented)

Attachments (3)

OptOut.patch (3.8 KB) - added by halfdan 3 years ago.
ticket_1929.patch (10.7 KB) - added by thommyhh 2 years ago.
ticket_1929_02.patch (10.7 KB) - added by thommyhh 2 years ago.

Download all attachments as: .zip

Change History (39)

Changed 3 years ago by halfdan

comment:1 Changed 3 years ago by vipsoft (robocoder)

  • Owner set to vipsoft
  • Status changed from new to assigned

Thanks, I'll commit as soon as I resolve the broken webtest...

comment:2 Changed 3 years ago by vipsoft (robocoder)

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [3555]) fixes #1929 - thanks halfdan; more refactoring

comment:3 Changed 3 years ago by vipsoft (robocoder)

  • Description modified (diff)
  • Milestone changed from Piwik 1.1 to 1.2 Piwik 1.2
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Summary changed from Extend OptOut plugin to Extend OptOut widget

These enhancements were undone by r3556. I'm re-opening to continue the discussion from #1919.

comment:4 Changed 3 years ago by vipsoft (robocoder)

(In [3816]) refs #1919, refs #1929, refs #1982 - delete tracking cookie when opting out (i.e., ignore cookie is set)

comment:5 Changed 3 years ago by vipsoft (robocoder)

Time permitting, provide some workaround for the multi-domain issue raised in #1376.

comment:6 Changed 3 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from reopened to closed

let's create a new ticket when we have optOut improvements

comment:7 Changed 3 years ago by vipsoft (robocoder)

  • Milestone changed from 1.2 Piwik 1.2 to 1.x - Piwik 1.x
  • Resolution fixed deleted
  • Status changed from closed to reopened

The original OptOut ticket is already closed. This ticket is for the improvements reverted in r3556.

comment:8 Changed 3 years ago by matt (mattab)

  • Summary changed from Extend OptOut widget to Allow to customize some CSS of optOut widget

I slightly lower scope of this ticket, to allow customizing a few CSS such as

  • font size
  • font family
  • font color
  • background color

and check each substring that they have only valid characters (eg. numbers/letters/commas)

comment:9 Changed 3 years ago by kkretsch

Wht is the current status or solution zu this?
None of our websites use a serif-like font, I would only need a minimal solution to set a system wide style for all websites. And I don't want to mess around in the code and rework after each update.

Changed 2 years ago by thommyhh

comment:10 Changed 2 years ago by thommyhh

I had another idea to resolve the css problem of the optout iframe. I decided to add a new field to the site record called "css_path". It is shown in the SitesManager. The url of the iframe gets a new parameter "idsite" containing the site's id. If there is a css path set for that site a <link> is inserted in the optout, containing the path to the "main_url" appended with the "css_path".

I fairly new to piwik development so I may have missed something, e.g. the field must still be included in the database updates, I haven't found this, as well as the information about appending the "idsite" Parameter to the <iframe> url.

I created a patch based on the current trunk. May anyone take a look at it?

Changed 2 years ago by thommyhh

comment:11 Changed 2 years ago by thommyhh

I found a little mistake that led to a template error because of a missing variable if the "idsite" parameter was not set or not css path was set for this site.

Therefore I attached "ticket_1929_0.patch".

comment:12 Changed 2 years ago by matt (mattab)

thommyhh thanks for the patch, but probably a easier way to implement this feature would be to add GET parameters to the iframe, for example

  • fontSize integer
  • fontColor hex
  • backgroundColor hex
  • etc. for the styles we want to allow overriding to allow better iframe look and feel when integrated in websites.

If you like to produce patch for this implementation we would gladly commit it indeed, thanks!

comment:13 Changed 2 years ago by thommyhh

I noticed the idea of changing some css parameters, but I have set also properties like padding and/or margin. So I thought it is a better idea to have the possibility to add a complete css file to include instead of changing some hard coded properties. Also, with the way you mentioned above you can only set the properties for a single element, e.g. the <body> tag, instead of styleing the <body> or the <p>.

What do you mean with the last point? What to override?

comment:14 Changed 2 years ago by vipsoft (robocoder)

I think the most flexible path is a table. (Piwik CMS, anyone?)

Piwik_templates

  • template_id - auto inc int
  • path - text (controller name + action name)

Piwik_template_content

  • template_id - int
  • lang - char(2)
  • css - text
  • html body - text

(Defer disabling the widget to ACL implementation.)

comment:15 Changed 2 years ago by thommyhh

Do you mean a table holding css definitions for each page?

comment:16 Changed 2 years ago by vipsoft (robocoder)

A table holding css and body text that could be localized.

If css is non-empty, a <style> section is appended to the <head>.

If body is empty, then the default template is used.

If lang is empty, this is the default if the user's locale doesn't exist.

comment:17 Changed 2 years ago by thommyhh

The problem here is, that you want the css under control of the editors/admins of the website not the piwik user/admin. It may be the same, but doesn't need to. That's why we want to <link> a css file from the webserver.

In our case the piwik admins and website admins are the same. But it can easily be two different companies not giving each other permission to access their system.

comment:18 Changed 2 years ago by thommyhh

Another problem you seem not to address is the different css for each site configured in piwik. Thats the problem we have here. We are starting to use one piwik installation for several websites and every website has of course its own styles, that we want to use in the opt out iframe as well.

comment:19 Changed 2 years ago by vipsoft (robocoder)

This is just a brain dump while I'm on the train:

  • expose lightweight API to get / set cookie state
  • provide a javascript client to set / delete first party cookie (#1376)
  • refactor widget to use API; serves as a reference/example implementation

Customizing css and/or text becomes external to Piwik. (In the absence of theming plugin.)

comment:20 Changed 2 years ago by matt (mattab)

We need to keep this easy and simple: css provided as GET parameters is enough. What are the parameters we wish to customize?

Then we only allow characters a-Z0-9 for example, and a few others like #. etc.

Because security wise, we cannot allow to insert Xss in this iframe, and is why we must restrict css that can be overridden. Please submit a simple patch if you can :)

comment:21 Changed 2 years ago by thommyhh

Overriding some css properties for the whole document is definitely not enough. You can not style the document like this. You can only style one element. I would then suggest to use to originally intended version with adding the css file via get parameter (&css=foo).

I think there is no xss vulnerability because if you are at the point that you can change the "&css" Parameter in the iframe you must have already got into the website itself and the owner has a much bigger problem than the manipulated Parameter. And as the file is only <link>ed, I don't see the problem here.

comment:22 Changed 2 years ago by matt (mattab)

Overriding some css properties for the whole document is definitely not enough. You can not style the document like this. You can only style one element. I would then suggest to use to originally intended version with adding the css file via get parameter (&css=foo).

What styles do you need exactly? Are there really too many? most css styles can fit into a few properties.

I think there is no xss vulnerability because if you are at the point that you can change the "&css" Parameter in the iframe you must have already got into the website itself and the owner has a much bigger problem than the manipulated Parameter.

What we are concerned here is not website vuln, but Piwik vuln, where a XSS in Piwik itself is of course considered a big issue for us. See http://ha.ckers.org/xss.html for more info and why it is not acceptable to load a user submitted css.

comment:23 Changed 2 years ago by vipsoft (robocoder)

  • Owner vipsoft deleted
  • Status changed from reopened to new

comment:24 Changed 2 years ago by thommyhh

First to the xss stuff. I tried in in IE6, it works but I still don't get the point why this is critical to Piwik. There is nothing stored at this point so the xss vulnerability is only non-persistent and I don't see the attack vector here.

The problem about the css properties is, that I want to use several css properties like padding, margin, font-style, color, may be line-height, text-decoration... There are to much css properties to make this hardcoded what properties can be used. And additionally you can e.g. then only style the <body> tag and not use the normal css rules.

Another important point is, that the control should be at the website editors/admins not the Piwik admins.

comment:25 Changed 2 years ago by matt (mattab)

The xss could allow to steal a piwik user cookie, session, etc.

If we have to use GET parameters, what CSS properties would be required?

comment:26 Changed 2 years ago by kkretsch

At least the font-family and color are a must have.

comment:27 Changed 2 years ago by thommyhh

Well, as css inclusion is not an option because of xss and css properties set by get params isn't either because of missing flexibility I think I will go back to my first idea to set a css path in the sitesmanager.

As I said before adding style properties to the <body> tag does not make sense. There must be the possibility to add complete css definitions like

* {
  margin: 0;
  padding: 0;
}

body {
  color: ...;
  font-family:....;
  font-size: 12px;
}

p {
  margin-top: 10px;
}

....

Do you see a way to do this?

comment:28 Changed 22 months ago by oputz

Have tried the ticket_1929_02.patch with Piwik 1.8.2.
Result in Settings > Websites > "CSS Path":

Notice: Undefined index: css_path in .../piwik/tmp/templates_c/%%EE^EE8^EE8EF7B4%%SitesManager.tpl.php on line 215

Backtrace -->
#0 Piwik_ErrorHandler(...) called at [.../piwik/tmp/templates_c/%%EE^EE8^EE8EF7B4%%SitesManager.tpl.php:215]#1 include(...) called at [.../piwik/libs/Smarty/Smarty.class.php:1263]#2 Smarty->fetch(...) called at [.../piwik/core/View.php:133]#3 Piwik_View->render(...) called at [.../piwik/plugins/SitesManager/Controller.php:66]#4 Piwik_SitesManager_Controller->index(...) called at [:]#5 call_user_func_array(...) called at [.../piwik/core/FrontController.php:138]#6 Piwik_FrontController->dispatch(...) called at [.../piwik/index.php:53]

Is there a solution?

comment:29 Changed 16 months ago by matt (mattab)

  • Milestone changed from 1.x - Piwik 1.x to 1.9.3 - Piwik 1.9.3

comment:30 Changed 14 months ago by lundj

Is this planned for the next release or what is the current blockade to get this rtbc?

comment:31 Changed 14 months ago by matt (mattab)

I guess a nice and powerful solution would be to ask users to put a custom CSS file in eg. piwik/custom-optout.css and, if this file exists, will be output in the opt out HTML.

It's not planned but we could add this Im sure!

comment:32 Changed 13 months ago by matt (mattab)

  • Milestone changed from 1.12 - The Great 1.x Backlog to 2.x - Piwik 2.x

comment:33 Changed 13 months ago by matt (mattab)

There is a nice solution posted in: http://forum.piwik.org/read.php?6,102702 to allow customize the opt out iframe CSS.

comment:34 Changed 12 months ago by greg (gka)

Here's a very simple fix for custom out-out styling:

https://github.com/gka/piwik/compare/piwik:2.x-twig...2.x-optout-custom-style

It works as proposed by Matt before, but it also allows for different styles. The user can put custom CSS files in the /themes/ folder. The file names must start with optout_, e.g. optout_foo.css.

When embedding the iFrame the user must add a GET parameter style, e.g. &style=foo. Piwik then checks if the file exists and includes it.

The characters :, ., and / are replaced to prevent XSS attacks.

Simple solution, I think we should add it to Core.

comment:35 Changed 12 months ago by oputz

You mean folder plugins/CoreAdminHome/templates/ ?

comment:36 Changed 3 months ago by matt (mattab)

  • Resolution set to answered
  • Status changed from new to closed

Kuddos to Jens Averkamp who released a plugin that lets you customize the CSS!!
Check it out at http://plugins.piwik.org/CustomOptOut

Please let him know any feedback on https://github.com/Zeichen32/PiwikCustomOptOut/issues

Cheers

Note: See TracTickets for help on using tickets.