SRI + XHTML #5742
SRI + XHTML #5742
Conversation
|
|
||
| list($file) = explode('?', $path, 2); | ||
| if (!empty($file) && is_readable($file)) { | ||
| $content = @file_get_contents($this->assets_dir . $file); |
There was a problem hiding this comment.
Here you use assets_dir, but one line above you don't.
| if (!empty($file) && is_readable($file)) { | ||
| $content = @file_get_contents($this->assets_dir . $file); | ||
| if ($content !== false) { | ||
| return 'sha384-'.base64_encode(hash('sha384', $content, true)); |
There was a problem hiding this comment.
Would be good to check if 'hash' function exists, e.g. when you set $this->assets_sri.
| protected function include_assets_sri($output) | ||
| { | ||
| return preg_replace_callback( | ||
| '!(src|href|background)=(["\']?)([a-z0-9/_.?=-]+)\2([\s>])?!i', |
There was a problem hiding this comment.
W3C specification talks only about link and script tags. Do we really want to handle all others, e.g. img?
There was a problem hiding this comment.
I went this route because it was easier to implement for everything and because future browsers may support SRI in other tags. I concur that for performance reasons this might not be the best approach.
There was a problem hiding this comment.
A slightly off-topic comment: the current way of implementing these features using templates (i.e., generate a document, then use regexes) seems error-prone and inefficient. I think a DOM-based (or similar) document generation approach could be more reliable and flexible (even though it's hard to say what the effect on performance may be.) Have you considered changing this?
|
Would be also add a note to defaults.inc.php about the new option and performance implications. |
Added assets_dir to is_readable check
|
Can't seem to be able to reply to your last comment, but I added a commit that documents the option in defaults.inc.php. |
| if (!empty($file) && is_readable($this->assets_dir . $file)) { | ||
| $content = @file_get_contents($this->assets_dir . $file); | ||
| if ($content !== false) { | ||
| return 'sha384-'.base64_encode(hash('sha384', $content, true)); |
There was a problem hiding this comment.
I propose to use hash_file() instead of hash() and file_get_contents().
| @@ -84,6 +85,7 @@ public function __construct($task = null, $framed = false) | |||
| $this->set_env('skin', $skin); | |||
|
|
|||
| $this->set_assets_path($this->config->get('assets_path'), $this->config->get('assets_dir')); | |||
| $this->assets_sri = $this->config->get('assets_sri') && in_array('sha384', hash_algos(), true); | |||
There was a problem hiding this comment.
I don't think we need to check hash_algos(), but I would check if function_exists('hash_file').
|
Thank you for submitting a Pull Request (PR) to the Roundcube GitHub project. You are receiving this message because your PR has conflicts which need to be resolved. We are trying to catch up on our backlog of old PRs and get them merged in (where appropriate). Therefor, we request the following: Step 1. Rebase from the latest Roundcube branch master into your PR. Step 2. Re-test to make sure your new code still works as expected Step 3. Comment back here once it has been tested and will merge cleanly. Once this has been done we will treat it like a new Pull Request and consider it for acceptance. Apologies for the inconvenience. Thank you for contributing to Roundcube! |
|
@corrideat: Have you progressed on your PR? |
@Neustradamus This is a pretty old PR that I didn't update. At the moment, I'm no longer running RC but I'll see whether I can update it or close it |
I've written a patch that does the following:
src="a'; now, it enforces that the same quotation style be used)