refactor(root): html sanitization #7921
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
@novu/client
@novu/framework
@novu/headless
@novu/node
@novu/notification-center
@novu/providers
novu
@novu/shared
commit: |
| @@ -37,13 +37,15 @@ export class EmailOutputRendererUsecase { | |||
| const parsedMaily = await this.parseMailyContentByLiquid(transformedMaily, renderCommand.fullPayloadForRender); | |||
| const strippedMaily = this.removeTrailingEmptyLines(parsedMaily); | |||
| const renderedHtml = await mailyRender(strippedMaily); | |||
| // todo: the assumption was wrong, we can remove this as mailyRender already sanitizes the html | |||
There was a problem hiding this comment.
looks like renderedHtml is already sanitized. we could keep the sanitization as a safeguard.
There was a problem hiding this comment.
The comment in the code says something different. If mail/renderer already sanitizes let's remove it completely. There is no need for double sanitization.
There was a problem hiding this comment.
correct but then i though about it in more safe way to guard yourself in case the mainly logix changes.
| disableOutputSanitization: | ||
| (staticStep.controlVariables?.disableOutputSanitization as boolean | undefined) ?? false, | ||
| // TODO: add providers | ||
| disableOutputSanitization: true, |
There was a problem hiding this comment.
we are removing the responsibility of sanitization from the Framework to Novu
| @@ -9,6 +9,15 @@ export class InAppOutputRendererUsecase { | |||
| execute(renderCommand: RenderCommand): InAppRenderOutput { | |||
| const { skip, disableOutputSanitization, ...outputControls } = renderCommand.controlValues ?? {}; | |||
|
|
|||
| return outputControls as any; | |||
| if (disableOutputSanitization) { | |||
| return outputControls as any; | |||
There was a problem hiding this comment.
in case the user toggled the bypass option
apps/api/src/app/environments-v1/usecases/output-renderers/in-app-output-renderer.usecase.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
now we will sanitize only the possible "HTML" content while keeping the old flow desition contentType === 'editor'
| @@ -384,6 +380,14 @@ export class Client { | |||
| }; | |||
| } | |||
|
|
|||
| private shouldSanitize({ stepType, options }: { stepType: StepType; options: ChannelStepOption | undefined }) { | |||
There was a problem hiding this comment.
at the moment, we implemented more precise sanitization, applying it only to email and in-app. because for other channels, there's no reason to sanitize, which reduces computation and complexity for framework users.
There was a problem hiding this comment.
Nit-picky one: You can keep it simple for now and do (stepType: string, disableOutputSanitization: boolean); to avoid the extra types and such.
There was a problem hiding this comment.
in that case, we will have another compilation error.
i believe this one is the issue.
…-properly-for-email-subject-2
…or-email-subject-2' of github.com:novuhq/novu into NV-5376-special-characters-are-not-sanitized-properly-for-email-subject-2
…-properly-for-email-subject-2
djabarovgeorge
deleted the
NV-5376-special-characters-are-not-sanitized-properly-for-email-subject-2
branch
What changed? Why was the change needed?
to support the bypass view, i replicated the in-app view so the editor facade looks similar. now we have a header "email template editor" and a border wrapping the subject and body.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer