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

fix(framework,dal): fix the default redirect behaviour, support absolute and relative paths #6443

Merged
merged 12 commits into from
Sep 9, 2024

Conversation

LetItRock
Copy link
Contributor

What changed? Why was the change needed?

Framework:

  • enhanced error messages for the JsonSchema validation
  • support absolute and relative paths in the redirect schema for the in-app step
  • default redirect target values: absolute --> _blank, relative --> _self

Screenshots

Workflow definition:
Screenshot 2024-09-04 at 21 40 06

Notifications result:
Screenshot 2024-09-04 at 21 39 56

Studio validation errors:
Screenshot 2024-09-04 at 21 49 45
Screenshot 2024-09-04 at 21 49 29

Copy link

linear bot commented Sep 4, 2024

@@ -54,6 +54,7 @@ const messageSchema = new Schema<MessageDBModel>(
content: Schema.Types.String,
resultContent: Schema.Types.String,
url: Schema.Types.String,
target: Schema.Types.String,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing the schema type, without this piece the target field is not saved in Mongo :/

@@ -154,6 +154,7 @@
"dependencies": {
"@novu/shared": "workspace:*",
"ajv": "^8.12.0",
"ajv-errors": "^3.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added ajv-errors for the custom error messages on JsonSchema, check example below

url: {
type: 'string',
pattern: ABSOLUTE_AND_RELATIVE_URL_REGEX,
errorMessage: 'The URL must be an absolute URL (excluding mailto) or a relative URL starting with /.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom error message

target: { type: 'string', enum: ['_self', '_blank', '_parent', '_top', '_unfencedTop'], default: '_blank' },
url: {
type: 'string',
pattern: ABSOLUTE_AND_RELATIVE_URL_REGEX,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we support absolute or relative URLs, regexp verified on:

Matches:
http://example.com
https://example.com/path
http://example.com?asdf=asdf&url=asdf
https://example.com?asdf=asdf&url=asdf
https://example.com/asd/?asdf=asdf&url=asdf
https://example.com/#/asd/?asdf=asdf&url=asdf
/path/to/resource
/path/to/resource?asdf=asdf&asdf=asdf
/another/path
/folder/file.html

Doesn't matches:
ftp://example.com/resource
mailto:someone@example.com
./relative/path
../up/one/level
#fragment
""

Comment on lines 20 to 39
if: {
properties: {
url: {
pattern: '^/', // Check if url starts with a slash (relative path)
},
},
},
then: {
properties: {
target: {
default: '_self',
},
},
},
else: {
properties: {
target: {
default: '_blank',
},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditional sub-schema, used to set the default target value depending whether it's absolute or relative URL

Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much does the new package add to @novu/framework?

@LetItRock
Copy link
Contributor Author

LetItRock commented Sep 4, 2024

How much does the new package add to @novu/framework?

@SokratisVidros ~6.3kb, 2.2gzip, https://bundlephobia.com/package/ajv-errors@3.0.0

import { Schema } from '../../../types/schema.types';
import { ExtendedJsonSchema, Schema } from '../../../types/schema.types';

const ABSOLUTE_AND_RELATIVE_URL_REGEX = '^(?!mailto:)(?:(https?):\\/\\/[^\\s/$.?#].[^\\s]*)|^(\\/[^\\s]*)$';
Copy link
Contributor

@rifont rifont Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're introducing support for the mailto URL scheme here, should we also include the tel scheme too (ref)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rifont we are excluding actually

@@ -21,6 +21,7 @@ const redirectSchema = {
if: {
properties: {
url: {
type: 'string',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to supress this strict validation warning:
image

json: [
{ message: "must have required property 'numberType'", path: '' },
{ message: "must have required property 'booleanType'", path: '' },
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated error messages list to include all reported errors

zod: null, // Zod has no support for `allOf`
},
},
},
{
title: 'should successfully validate polymorphic `anyOf` properties',
schemas: {
zod: z.object({
Copy link
Contributor

@rifont rifont Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following test updates change the test subject from a polymorphic array of items to a polymorphic object, better matching the typical usage of anyOf and working around a short-circuit issue in ajv: ajv-validator/ajv#662

Copy link
Contributor

@rifont rifont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@LetItRock LetItRock merged commit 4522dd3 into next Sep 9, 2024
28 checks passed
@LetItRock LetItRock deleted the com-197-fix-default-redirect-behaviour branch September 9, 2024 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants