getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

[3.6.0-beta.2] Stuck in an unsaved changes loop #3736

Closed MaluNoPeleke closed 3 years ago

MaluNoPeleke commented 3 years ago

Describe the bug
After updating to Kirby 3.6.0-beta.2 I am stuck in a loop of unsaved changes for articles, no matter how often I press on discard, save or close.

To Reproduce
Steps to reproduce the behavior:

  1. Open any (?) page in the panel
  2. Get an unsaved changes notification
  3. Click on Discard or Save
  4. Go to 2.

Expected behavior
If there is no change Kirby should not show unsaved changes.

Screenshots
screencast-dev peleke de-2021 09 30-20_23_18

Kirby Version
Kirby 3.6.0-beta.2

Console output
None

Desktop (please complete the following information):

Additional context

afbora commented 3 years ago

I found the source of the problem. When we copy and paste, it transfers the links to the panel as they are. But when you focus on the input, the Kirby automatically adds the rel attribute to the links: rel="noopener noreferrer nofollow".

Related line: https://github.com/getkirby/kirby/blob/develop/panel/src/components/Forms/Writer/Marks/Link.js#L96-L99

I'm not sure, but maybe we can solve it by defining the paste rule.

bastianallgeier commented 3 years ago

I wonder if we should remove the rel attribute from the Mark in JS. We already have the referrer policy in the head so we don't need it for security reasons.

afbora commented 3 years ago

I'm not very familiar with these topics, but after a bit of google I read in a few places that noopener and noreferrer provide extra security 🤷‍♂️

afbora commented 3 years ago

https://github.com/getkirby/kirby/issues/3008 I think this is the same issue.

afbora commented 3 years ago

Maybe @sebastiangreger can give us some insight on this?

sebastiangreger commented 3 years ago

As for noreferrer, this should indeed be covered by the policy in the head – it applies to any elements in the page, unless overridden on purpose, as we discussed in https://github.com/getkirby/kirby/pull/3677#issuecomment-917944392 . Some very old browsers do not know noopener, and noreferrer would instead also cover its security-relevant function (see next paragraph) as well; off the new 3.6 compatibility list, this would apply to Android Browser <94 and Chrome for Android <94 (both compatible 92+ according to your docs), all others you listed do understand noopener and hence not require this fallback.

The noopener value in the rel attribute is security-critical when opening links with target="_blank" as it opens an attack vector: https://web.dev/external-anchors-use-rel-noopener/ Most recent browser versions imply that by default (see https://caniuse.com/mdn-html_elements_a_implicit_noopener), but since – even with the new minimum requirements of K3.6 – the Panel works also with versions predating that change, this is probably – and unfortunately, considering this issue – an important one to keep. Regrettably, there is no global head meta tag equivalent to the referrer policy.

Unrelated to the JS issue at hand, nofollow is, in my opinion, not necessary in the panel application, as its only function is to tell search engines not to pass the "link juice" to pages linked to. Since the code appearing in panel fields will not be crawled by search engines, I'd consider this one obsolete.

afbora commented 3 years ago

As far as I understand, we only need to add noopener anchor to links.

@bastianallgeier I think it will be resolved if we default it to rel="noopener" when pasting. Is it possible?

bastianallgeier commented 3 years ago