Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevents blocks sanitizing for undo plugin #1280

Closed
sos-productions opened this issue Aug 24, 2020 · 8 comments
Closed

Prevents blocks sanitizing for undo plugin #1280

sos-productions opened this issue Aug 24, 2020 · 8 comments

Comments

@sos-productions
Copy link

sos-productions commented Aug 24, 2020

I don't know how to desactivate sanitizing

I described the problem kommitters/editorjs-undo#16

Any idea welcome. Thanks

@gohabereg
Copy link
Member

Hi @sos-productions
It seems, you need an ability to customize tools sanitizer configuration.

At the moment for Paragraph Tool (or any other) you can override it's sanitize property and pass to the editor:

import Paragraph from '@editorjs/paragraph';

Object.defineProperty(Paragraph, 'sanitize',  {
  get() {
    return {
      text: { br: true, strong: true, ... }
    };
  }
});

new EditorJS({
  tools: {
    paragraph: Paragraph
  }
});

Or fork a tool and change static get sanitize() return value in sources.

The right way to overcome this issue in general — add ability to pass sanitize property to each Tool on editor initialization:

new EditorJS({
  tools: {
    paragraph: {
      sanitize: customSanitizeConfig
    }
  }
});

Disabling sanitize at all because only one plugin is unsafe.

@sos-productions
Copy link
Author

sos-productions commented Aug 28, 2020

Hi @gohabereg, I need to take a snapshot of the blocks for a history step whatever the content, at the beginiing it is done with undo.initialize call , later is done with .save() in registerChange() inside the Undo plugin.

so I need to disable sanitize temporarely to avoid crunching the tags to be able to go back on previous state on undo.undo() or undo.redo() smootthly.

Now my init of the Undo plugin is quite simple:

` /**
Initialize undo stack
**/
function initUndo(editor) {

    const undo = new Undo({editor});

    /*true is to avoid sanitizing Blocks, requires to patch editorjs*/
    editor.save(true).then((initialData) => {
        undo.initialize(initialData);
    }).catch((error) => {
        console.log('Saving failed: ', error)
    });

    editor.undo=undo;
}`

This problem occurs everytime a need to export throughout the editor.save() process whatever the tool. This is thus also the case for load ans save blocks to a .js local file by example.

block.save() is not affected by this sanitizing issue as there is no sanitize on the block content. So I thought a loop from first to editor.blocks.getBlocksCount() with block.save() would have to solved this.

Unfortunately ModificationsObserver interfers and perturbates creating extra step in undo stack so I finished by asking you, to add simply this this disableSanatize feature avoiding extra sanitize stuff I don't need at all as it is an internal process of the plugin Undo.

@gohabereg
Copy link
Member

Could you clarify please does state of your block on undo/redo history change differ from state you get when you save editors content?

If yes, it is not the right way to manage state. save method of a tool should return exact the same block state as it would be when you pass this state to render method.

If some content is stripped because of sanitize you just need to configure it properly.

If you want to disable sanitize at all you still able to do that thru configuration for each plugin.

My points are

  1. You already can manage sanitize configuration (even disable it at all).
  2. If output state of block doesn't reflect actual state (in DOM/memory), it means you manage it wrong

@sos-productions
Copy link
Author

sos-productions commented Aug 29, 2020

@gohabereg, what is the deal here, security?

If that is is really so, correct me if I am wrong saying it is too late to filter data on output with makeOutput.
as security starts filtering the source input instead to avoid malicious code injection such as XSS. As
I told in an other issue, you can not rely on html_janitor to ensure this because it has a flaw on it
#1275.

Whatever, this thing has to be verified and filtered by server itself because if configuration of sanitize is hacked
on client side then security is nuts.Fortunately, our friend https://github.com/editor-js/editorjs-php comes into play:

`
// Configuration is stored on server side
// data comes from editorsjs send by Ajax process transformed into array.
// to make things simple I patched Services_JSON (https://github.com/sos-productions/Services_JSON) to make json_decode($json,true) happy with the json from javascript world. I give the JSONFIX detail in Test-JSONFIX.php.

``
$editor = new EditorJS( $data, $configuration );

// Get sanitized blocks (according to the rules from configuration)

$blocks = $editor->getBlocks(); // TODO:to reproducethe drop feature of makeOutput() instead of a Exception crash.

$data='{"blocks":'.json_encode($blocks).'}';  

``
Thus as first security, I agree more with sanitize on block instead against XSS attacks with code injection such as in html-janitor, Block content should have aready been checked normally on the fly on each editor change on client side, ie when
a) on user change content block content. (manually and programaticlally with any tools calling insert(), clear().. )
b) on paste
c) on load with Render()
...
Maybe on other event, I have not the perfect knowledge off all the editorjs API for now.

@gohabereg
Copy link
Member

My main complaint is you can achieve your goal using current API.

We also sanitize HTML input when it comes from pasting, so filtering on output is just another layer. Agree, we can also filter content on render and insertion. For sure, it doesn't guarantee 100% defence from XSS or other input-based attacks — you need a server side input validation and output escaping for that. Nevertheless it doesn't mean we shouldn't filter input on client, does it? Having not too easy way to disable validation, we have foolproof which is good to have anyway I guess.

@sos-productions
Copy link
Author

sos-productions commented Aug 31, 2020

The patch applies again not on input but output here with save()...really the flag addition is a big compromise between complicated things, extra configuration for nothing , ie karma making things complicated. You don't want to add the patch considering it can be achieved in an other but complicated way refusing to admit it does not affect security at all. I will not fight with you, I just end saying your solution is not a good compromise on processing cost on client side, js is aleady slow if we could have avoided to overload it would have been nicer for the user.

@hata6502
Copy link
Contributor

hata6502 commented Oct 10, 2020

+1

This feature is useful for saving DRAFT data not only editorjs-undo.
Current save() methods may causes unaccesibility because Editor.js removes invalid blocks without noticing.

May be related to: https://www.w3.org/WAI/WCAG21/Understanding/on-input.html

@sosie-js
Copy link

sosie-js commented Oct 16, 2020

@hata6502
what your point out here is right, .save() with internal filtering is confusing, the method should have been renamed to filterAndSave(), so it is a in-out function with bord effects not only out as we can expect.

In the case we have to build the filter rules to reproduce no filtering from any data, we have to collect first all the tags from the data.blocks and accept them BEFORE editor instantiate as the data is bunkered into webpack modules closures making no more accessible for security.

We have to do each time we want to save() when data changes. ^^'. I provide this facility with tool-configurator plugin so I can tell you how a pain it is to load and handle promise to make it work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants