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

Sanitize mixin for plugins? #131

Closed
KawaiiZapic opened this issue Oct 27, 2021 · 11 comments
Closed

Sanitize mixin for plugins? #131

KawaiiZapic opened this issue Oct 27, 2021 · 11 comments
Labels

Comments

@KawaiiZapic
Copy link

KawaiiZapic commented Oct 27, 2021

Allow plugins to modify sanitize rules.
Maybe like this:

const remark = jojo.remake;
const process = sch => {
  sch.tagName?.push("iframe");
  return sch;
};

export default function ExamplePlugin {
  return {
    remark: u => u.use(remark),
    sanitize: sch => process(sch)
  }
}

This will help plugins decoupling from the editor.

@pd4d10
Copy link
Owner

pd4d10 commented Nov 20, 2021

From a technical perspective, I guess it can be implemented. But it seems would disrupt user expectations in some cases.

For example, there are 2 plugins A and B. A allows iframe, while B doesn't allow it. If the apply order is A->B, then the iframe related contents will also be removed.

@KawaiiZapic
Copy link
Author

From a technical perspective, I guess it can be implemented. But it seems would disrupt user expectations in some cases.

For example, there are 2 plugins A and B. A allows iframe, while B doesn't allow it. If the apply order is A->B, then the iframe related contents will also be removed.

I think the example more look like a developer issue 🤔.
I don't think there is any case need to disallow a tag or something else, because a plugin is some code to add some new features but not to be a security guard.

About the security question, I think plugin can get a CSRF token from parser instance and apply to element, and then add a filter plugin at the bottom of process chain, to sanitize all insecure content in UGC.

@pd4d10
Copy link
Owner

pd4d10 commented Nov 20, 2021

and then add a filter plugin at the bottom of process chain

Yeah, If I don't understand it wrong, the current sanitize process is exactly the same as you described

Take the iframe case as an example, it can be added via rehype plugins. The sanitize step only takes effect once after remark process.

@github-actions
Copy link
Contributor

This issue is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale label Mar 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2022

This issue was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this as completed Apr 3, 2022
@aprendendofelipe
Copy link
Contributor

What do you think about the plugin passing a schema instead of passing the function that handles the schema?

const remark = jojo.remake;
const sch = {
  tagNames: ["iframe"]
};

export default function ExamplePlugin {
  return {
    remark: u => u.use(remark),
    schema: sch
  }
}

So the main library is responsible for properly merging the schemas and sanitizing globally.

A special merge function will be needed to merge the arrays, but I think I have a good idea of how to do that.

That way plugins are prevented from deleting permissions needed by other plugins, they can only add.

Any other custom schemes are still possible by passing the sanitize function directly to the Editor or Viewer. Thus allowing new additions or deletions in the scheme.

@pd4d10 pd4d10 reopened this Jan 10, 2023
@github-actions github-actions bot removed the stale label Jan 11, 2023
@pd4d10
Copy link
Owner

pd4d10 commented Jan 23, 2023

That way plugins are prevented from deleting permissions needed by other plugins, they can only add.

This seems to be reasonable. But it would also lead to breaking changes, and need to be released as a major version.

I'll draft a v2 plan soon, including this feature.

@pd4d10 pd4d10 mentioned this issue Feb 9, 2023
9 tasks
@github-actions
Copy link
Contributor

This issue is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label Mar 25, 2023
@pd4d10 pd4d10 removed the stale label Mar 30, 2023
@github-actions
Copy link
Contributor

This issue is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label May 30, 2023
@pd4d10 pd4d10 removed the stale label May 31, 2023
@github-actions
Copy link
Contributor

This issue is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label Jul 31, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

This issue was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this as completed Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants