-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
…ute and relative paths
@@ -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, |
There was a problem hiding this comment.
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 :/
packages/framework/package.json
Outdated
@@ -154,6 +154,7 @@ | |||
"dependencies": { | |||
"@novu/shared": "workspace:*", | |||
"ajv": "^8.12.0", | |||
"ajv-errors": "^3.0.0", |
There was a problem hiding this comment.
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 /.', |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
""
if: { | ||
properties: { | ||
url: { | ||
pattern: '^/', // Check if url starts with a slash (relative path) | ||
}, | ||
}, | ||
}, | ||
then: { | ||
properties: { | ||
target: { | ||
default: '_self', | ||
}, | ||
}, | ||
}, | ||
else: { | ||
properties: { | ||
target: { | ||
default: '_blank', | ||
}, | ||
}, |
There was a problem hiding this comment.
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
There was a problem hiding this 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?
@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]*)$'; |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json: [ | ||
{ message: "must have required property 'numberType'", path: '' }, | ||
{ message: "must have required property 'booleanType'", path: '' }, | ||
], |
There was a problem hiding this comment.
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({ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
What changed? Why was the change needed?
Framework:
redirect
schema for thein-app
stepScreenshots
Workflow definition:
Notifications result:
Studio validation errors: