Remove inline Javascript, part I #9513

pabzm · 2024-06-21T15:51:43Z

As part of #9508 here's a change that removes add_script() and transports js_commands in JSON, which then gets interpreted as callbacks to call with given arguments, instead of evaling strings.

This does not tackle the many inline event handlers, in order to keep it manageable.

pabzm · 2024-06-21T16:02:17Z

@alecpl I don't know yet why the tests fail, will have a look at that on monday. But if you have the time maybe you can already have a look if you (roughly) agree?

pabzm · 2024-07-08T08:11:12Z

@alecpl Could you have a look at this, please? I'll go forward and work on extracting inline event handlers, but I'd like to know as early as possible about vetos from your side.

alecpl

Some preliminary comments. Overall not bad.

program/js/rcmail-init.js

program/js/googiespell_init.js

alecpl · 2024-07-08T17:44:42Z

skins/elastic/watermark.html

-    }
-  } catch (e) { }
-</script>
+<script src="./dark-mode.js"></script>


This probably would be better to move to ui.js.

But then we lose the conditional in layout.html: https://github.com/pabzm/roundcubemail/blob/965e50ea084d799bc0b337e542ba32054dec582e/skins/elastic/templates/includes/layout.html#L31
Do you want to drop that?

Also using us.js as dependency in watermark.html doesn't look good, I think.

Even if you want to reduce the amount of JS-files I'd argue that this code snippet is really well suited for its own file.

program/include/rcmail_output_html.php

program/actions/mail/compose.php

program/actions/settings/index.php

program/include/rcmail_action.php

alecpl · 2024-07-08T18:05:54Z

program/include/rcmail_output_html.php

-        if (!empty($this->script_files['foot'])) {
-            $page_footer .= array_reduce((array) $this->script_files['foot'], $merge_script_files);
-        }
+        $page_footer .= html::div(['id' => 'js-data', 'style' => 'display: none', 'hidden' => true], $this->get_js_commands());


We need to encode the div content here. E.g. to not contain < and >, etc. Then I think we should be able to do .text() on the client side.

I would rather go with the current code, because it's safe enough (JSON-encoded), and encoding and then decoding everything adds a (maybe small but still) performance weight. Also, it works, why change it?

While doing the requested changes the data handling broke again, so I decided to use a data-attribute instead. That works for all cases.

program/include/rcmail_output_html.php

pabzm · 2024-07-09T17:34:35Z

I rebased onto the latest master to get rid of the unrelated phpstan errors, and then force-pushed. Sorry if that muddles up your review process.

program/actions/mail/compose.php

program/js/editor.js

alecpl · 2024-07-09T17:56:01Z

program/include/rcmail_output_html.php

-     * @param string $script   JS code snippet
-     * @param string $position Target position [head|head_top|foot|docready]
-     */
-    public function add_script($script, $position = 'head')


Instead of removing the API we should consider using the nonce for these. It would make the plugin developers life easier. However, there will be problem with execution order, I suppose, so I'm not sure.

I would prefer to not offer that option, because it's cleaner without it, and I guess most plugin authors wouldn't bother to clean up their code if it still works, which factually will degrade the CSP-related security of most Roundcube installations.
As you see in this branch, converting code to use external script files isn't really hard. It's just some chores to modernise ones code.

Also the potential problems with code order you mentioned might well result in a steady stream of issues, which we would need to take care of – that's a work load I would very much like to avoid, especially since Roundcube only loses security advancements from it.

alecpl · 2024-07-14T09:44:55Z

Another case to figure out is rcmail_html_page class. It's used to display attachment contents, i.e. without the rcmail client context, but contains onclick handler. I think it might be a case for nonce based solution. On the other hand it's an iframe some maybe it could be handled by the parent in onload.

pabzm · 2024-07-16T12:33:39Z

This isn't meant to completely remove all inline-JS at once, that would be too big a change, I figured. It's only the first part, as the title says.

I already made some progress on further changes to take care of event-handlers (which led to #9544), but I'd like this to be merged before I open another PR to avoid overload.

alecpl · 2024-07-16T12:50:56Z

Well, this is one big BC break. I'd rather see it completed before we make the decision to merge. There will be no return.

This allows to call it on specific elements only, e.g. after they've been inserted late to the DOM.

There's no apparent reason for them to be static, and no explanation, but as instance methods they are directly callable from the de-inlined event-handlers and we save some helper methods, which is good.

This make it easier for the calling code.

Have to repeat attaching event handlers after a clone().

This allows to strip 'unsafe-eval' from the CSP.

innerHTML requires 'unsafe-eval' in the CSP, while innerText doesn't.

If the last argument to a data-on* attribute is an object (associative array in PHP), it is used as options, which allow to specify if preventDefault() should be called on the event. This is relevant for some parts of the code and got lost in previous changes.

pabzm · 2024-09-05T12:41:59Z

(Rebased to latest of "master")

pabzm force-pushed the extract-inline-js branch from 2b6bcd8 to 7fd087f Compare June 21, 2024 15:53

pabzm changed the title ~~Extract inline JS code, to allow stricter CSPs~~ Remove inline Javascript, part I Jun 21, 2024

pabzm force-pushed the extract-inline-js branch from 7fd087f to 61f1775 Compare June 21, 2024 15:59

pabzm force-pushed the extract-inline-js branch 6 times, most recently from c8efaeb to abe9212 Compare June 24, 2024 10:19

pabzm self-assigned this Jun 24, 2024

pabzm force-pushed the extract-inline-js branch 6 times, most recently from 1bce625 to 965e50e Compare June 25, 2024 15:37

pabzm marked this pull request as ready for review June 25, 2024 15:47

pabzm requested a review from alecpl June 25, 2024 15:47

alecpl requested changes Jul 8, 2024

View reviewed changes

pabzm force-pushed the extract-inline-js branch 2 times, most recently from a2b0156 to e8036cf Compare July 9, 2024 17:27

pabzm requested a review from alecpl July 9, 2024 17:34

alecpl reviewed Jul 9, 2024

View reviewed changes

pabzm force-pushed the extract-inline-js branch from 03c7037 to e5f629b Compare July 10, 2024 10:02

pabzm added 27 commits September 5, 2024 14:41

fixup! Replace inline scripts by JSON-data with instructions

ac2bb21

fixup! De-inline event handlers from plugins+actions

9098a60

fixup! De-inline event handlers from plugins+actions

846bf9b

fixup! De-inline event handlers from plugins+actions

4062096

Make a browser test a little more resilient

7e8010b

Let browser wait longer for downloads

04937df

Allow to run event-listener-attacher for given root element

e64be8f

This allows to call it on specific elements only, e.g. after they've been inserted late to the DOM.

Fix subscription toggle of newly created folders

d93b381

De-inline program/include

a76698d

Move rcube_webmail static methods to instance

8b71716

There's no apparent reason for them to be static, and no explanation, but as instance methods they are directly callable from the de-inlined event-handlers and we save some helper methods, which is good.

fixup: de-inline program/actions

f7c63e0

fixup de-inline actions

31233ec

De-inline skins/elastic

e15b8e1

fixup de-inline program/include

c180952

WIP: Send strict CSP (strict for JS)

00babee

De-eval HTTP response handling

73958c5

WIP: TODO comment

d08d6cb

Fix log statement

4909f13

json_encode in html class

e7bc61a

This make it easier for the calling code.

Remove useless call

f9abd32

De-inline app.js, part 1

bf1faee

Fix image upload

e08c14a

Have to repeat attaching event handlers after a clone().

Hand around Nodes instead of using innerHTML

d6daeed

This allows to strip 'unsafe-eval' from the CSP.

Use jQuery event setters and helpers to get rid of innerHTML

de84737

innerText is enough here, don't need innerHTML

6e2e1a1

innerHTML requires 'unsafe-eval' in the CSP, while innerText doesn't.

WIP: Use jQuery to attach event-handlers based on attributes

fedc7d2

pabzm force-pushed the extract-inline-js branch from 72cfd49 to fedc7d2 Compare September 5, 2024 12:41

pabzm mentioned this pull request Sep 16, 2024

INBOX empty but properties shows right size and numer of emails #9634

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove inline Javascript, part I #9513

Remove inline Javascript, part I #9513

pabzm commented Jun 21, 2024 •

edited

Loading

pabzm commented Jun 21, 2024

pabzm commented Jul 8, 2024

alecpl left a comment

alecpl Jul 8, 2024

pabzm Jul 9, 2024

alecpl Jul 8, 2024

pabzm Jul 9, 2024

pabzm Jul 9, 2024

pabzm commented Jul 9, 2024

alecpl Jul 9, 2024

pabzm Jul 10, 2024

pabzm Jul 10, 2024

alecpl commented Jul 14, 2024

pabzm commented Jul 16, 2024

alecpl commented Jul 16, 2024

pabzm commented Sep 5, 2024

Remove inline Javascript, part I #9513

Are you sure you want to change the base?

Remove inline Javascript, part I #9513

Conversation

pabzm commented Jun 21, 2024 • edited Loading

pabzm commented Jun 21, 2024

pabzm commented Jul 8, 2024

alecpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabzm commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecpl commented Jul 14, 2024

pabzm commented Jul 16, 2024

alecpl commented Jul 16, 2024

pabzm commented Sep 5, 2024

pabzm commented Jun 21, 2024 •

edited

Loading