Add additional html interface

Pushed for revision: https://reviews.mahara.org/#/c/1825/

Hi Ruslan,

Can you please provide some information on how to test the patch?

Thanks
Kristina

Hi Kristina. Testing is simple:

1. Go to Admin - Configure site - Additional Html
2. By switching the extra content location, add content to header, body start and body end.
3. Refresh the page and make sure the content is displayed.

To make a more sensible test:
1. Go to Admin - Configure site - Additional Html
2. Select "Within HEAD"
3. Obtain the tracking code from Google Analytics, having registered your Mahara site, insert the code in the Content field. Save the content.
4. Go to Google Analytics and make sure that the site is now traceable.

Hi Kristina, regarding your comment in the review. It works fine for me, the code appears on every Mahara page I tried. You have to click 'save changes', but that is obvious. Are you using a custom theme that is not default/raw based and overrides header/footer template?

Reviewed: https://reviews.mahara.org/1825
Committed: http://gitorious.org/mahara/mahara/commit/5e2f3e6d29602eb60eea7e3c19a314762d892a6b
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit 5e2f3e6d29602eb60eea7e3c19a314762d892a6b
Author: Ruslan Kabalin <email address hidden>
Date: Wed Oct 31 16:12:40 2012 +0000

Add Additional HTML page to Configure Site menu (bug #1073625)

This allow admin to embed something without modifying the template, e.g.
Google Analytics snippet.

Change-Id: I030b4d1c682708fdf988e9a4a0dea836b4f285b9
Signed-off-by: Ruslan Kabalin <email address hidden>

Reviewed: https://reviews.mahara.org/1824
Committed: http://gitorious.org/mahara/mahara/commit/dc3688e688c4058c9aeaaf433e648959f3b5a19d
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit dc3688e688c4058c9aeaaf433e648959f3b5a19d
Author: Ruslan Kabalin <email address hidden>
Date: Wed Oct 31 12:53:50 2012 +0000

Make editchangepage.json.php less "page" specific (bug #1073625)

It can be used for any site content, not site pages only.

Change-Id: Ia901ce1444ab26c50b6327089cf2040a95fbe148
Signed-off-by: Ruslan Kabalin <email address hidden>

I hate to reopen this ticket after it's been through so much code review and been accepted and it has such a great, polished UI. :(

However, I've noticed that it no longer actually works for the proposed purpose, of allowing the admin user to set up Google analytics. The reason for this, is because it passes the HTML through the clean_html function, and that strips out Javascript from it, and the modern Google analytics snippet is just Javascript: https://developers.google.com/analytics/devguides/collection/gajs/asyncTracking

I think this gets at a fundamental difficulty with this issue:

1. If we filter the HTML, then it prevents this feature from doing what it's really meant to do.

2. BUT if we don't filter the HTML, we provide a very easy path for someone who compromises an admin account, or finds a SQL injection that lets them do inserts or updates to arbitrary tables, to escalate that into pwning all visitors to the site. (i.e. they can put some JS on every page that redirects to wherever they want; or put some JS that forwards login information to an outside site)

So, I think we probably should scrap this feature, since with clean_html() it's not useful, and without clean_html() it's not safe.

I think an easier and safer way to achieve what we want would be to create some blank HTML files in, say, /local/additionalhtml/head.html, etc, and include those in every page. Then the site admin can just modify those in order to change what shows up on the page. This would be consistent with our security approach of assuming that filesystem access is more difficult for an attacker to achieve than login or database access.

It's also true that users could achieve this already, by editing the right template file. But I think the template directory structure is probably intimidating for new users, so it would make sense to add an easy API for them to do the very common task of adding a Google Analytics or other page tracking cookie.

Cheers,
Aaron

I guess the question about the patch at this point is, can it still do anything useful even with the additional HTML being run through clean_html()?

Cheers,
Aaron

Actually it turns out that you can already override templates via the /local directory. See https://bugs.launchpad.net/mahara/+bug/898437

Of course, that feature doesn't make it easy to add a snippet to the top of every page. You would have to:

1. Know about the almost completely undocumented feature ;)
2. Create the following files: local/theme/templates/header.tpl, local/theme/templates/header/head.tpl, and/or local/theme/templates/footer.tpl
3. And even then, you'd have problems if you're using a custom theme that overrides these.

So I still like the idea of adding some additional HTML snippet feature to the /local directory to simplify this process.

Some other folks here at Catalyst have also floated the idea of adding a feature that simply puts the Google Analytics snippet itself, into the header, if the user activates a certain config setting. I would not be opposed to that, but I fear that we at Catalyst would fall behind on updating it, just as we tend to do with the iframe filters. So, it would be better as a plugin system to allow for community contributors to create and maintain tracking snippet plugins.

Anyway, at this point I'm going to pursue the following:

1. I will revert the additional HTML UI feature because, as stated in my earlier note, it can't be effective and secure at the same time.
2. I will add an easier way to put the snippet into the page via some mechanism under the /local directory (or perhaps even via config.php? That's a little less obscure.)

It is worth noting that Moodle has a feature that is exactly like this, and they don't consider it a security risk. ( http://docs.moodle.org/25/en/Header_and_footer ). That's because Moodle's security standard is, in general, to assume that having admin user web access means you also have already compromised every other aspect of the system. Mahara, on the other hand, has been arriving at a security standard of assuming that the admin web account is easier for attackers to compromise than other aspects of the system, such as the database and the web server file system, because it can be compromised by browser and OS vulnerabilities. So, we try to avoid introducing features that would allow an escalation of privileges via the admin account. (This feature isn't exactly escalating privileges in the strictest sense... but in order for it to work, we'd have to allow the user to insert Javascript via the web UI, which is something we don't allow anywhere else in Mahara; and per our the-admin-is-insecure security policy, we can't allow it via the admin user either.)

Cheers,
Aaron

Removing the web UI. I've left the database and template stuff in place, so I can use that as the framework to make a non-web way to provide additional HTML.

https://reviews.mahara.org/#/c/2391/

Reviewed: https://reviews.mahara.org/2391
Committed: http://gitorious.org/mahara/mahara/commit/c61916a5d4f66dfcb1c6430e3625a143c4cf0140
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit c61916a5d4f66dfcb1c6430e3625a143c4cf0140
Author: Aaron Wells <email address hidden>
Date: Mon Aug 5 17:35:03 2013 +1200

Removing the Web UI for additional HTML items

Bug1073625: Removing this feature per Mahara's security standard of assuming the admin web account
is more easily compromised than the filesystem or database. In order for this feature to be useful,
it needs to be able to print Javascript, on every page in the site, in the header and footer.

Change-Id: Id2337c66d037dced514e0cc347370d97cac80093

I child bug has been spun off of this: https://bugs.launchpad.net/mahara/+bug/1213994

I'm re-incorporating the child bug back over into this bug. Here's the patch for it: https://reviews.mahara.org/#/c/2409/

Reviewed: https://reviews.mahara.org/2409
Committed: http://gitorious.org/mahara/mahara/commit/e266d79f1db0c57aceabc07e7d90ee599ebdbe0d
Submitter: Aaron Wells (<email address hidden>)
Branch: master

commit e266d79f1db0c57aceabc07e7d90ee599ebdbe0d
Author: Robert Lyon <email address hidden>
Date: Tue Aug 20 08:35:17 2013 +1200

Removing clean_html() from additionalhtml (Bug #1073625)

Now that the admin interface has been removed we can allow
the code in the db to be displayed 'as is' rather than
stripping out potentially bad code. (The feature is useless if
we strip out JavaScript, because most tracking cookies require
it)

Change-Id: Id5665afa75ee8fe229baea65332a09b32d827a54
Signed-off-by: Robert Lyon <email address hidden>

Okay, I decided to put the additionalhtml settings into the config.php: https://reviews.mahara.org/2416

You convinced me about having it outside web interface. In fact, I missed Aaron's earlier comments in August above.

Reviewed: https://reviews.mahara.org/2416
Committed: http://gitorious.org/mahara/mahara/commit/13fe50056f8ac3416db89df52ee0f36fa17ac3f3
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit 13fe50056f8ac3416db89df52ee0f36fa17ac3f3
Author: Aaron Wells <email address hidden>
Date: Tue Aug 20 16:17:31 2013 +1200

Changing additionalhtml to be based on config.php values

Bug #1073625

Change-Id: I79041c99681ad6304b81433da7d48dd459353589