Missing URL validation that can lead to internal error

[Arciiix]

Hello, it's me again 😄 Earlier on, when I started working on this project, I noticed one bug.

The problem

When you configure the webhook integration and forget to include the protocol before the URL, the app accepts it. But later, when it's time to make a request to that specific endpoint, an internal error occurs. Example "localhost:8080/webhook" instead of "http://localhost:8080/webhook"

To reproduce

1. Configure the webhook integration
2. Configure the URL as one of those below, in this way:
   • localhost/test
   • localhost:8080/test
   • 192.168.0.101/test
   • 192.168.0.101:8080/test etc. - each of those URLs lacks the protocol (most likely http:// or https://)

When you send the notification, the webhook channel fails (you can see the red indicator), but there isn't any error description. When you look into the logs, you can see an exception: System.NotSupportedException: The "localhost" scheme is not supported. or (with IP version) System.InvalidOperationException: An invalid request URI was provided. Either the request URI must be an absolute URI or BaseAddress must be set.

To fix

What I would consider adding is some validation that would warn the user at the stage of setting the URL.

Do you think it's a good idea? If so, I can work on this - you can assign this issue to me.

Have a nice day!

[SebastianStehle]

Good catch.

You could just assign a pattern to the URLProperty in the HttpIntegration.

See:

• https://github.com/notifo-io/notifo/blob/main/backend/src/Notifo.Domain.Integrations.Abstractions/IntegrationProperty.cs#L40

But I think it would be better to add a special "Format" enum, that also accepts None, Email and Url. Because then we can improve the UI to show the proper HTML input.

[Arciiix]

Sure, I'll do it soon. Thank you!

[Arciiix]

Hey, I made some basic functionality of what we've talked about. I don't want to create the pull request yet since it needs some testing. Can you please take a look at my fork and say if I'm doing it the way you wanted? Also, I was thinking: maybe we should remove the Password PropertyType and also move it to my newly created PropertyFormat - since password is text in the end, and format maybe sounds to be more suitable since it changes the input type. I was thinking that maybe we can say that PropertyType regards to what the value is internally, and PropertyFormat - to what user sees it is. What do you think?

[SebastianStehle]

This is exactly what I had in might. Good job. There are small things, but I can comment it on the PR.

I would not change the password field. Format means the text format. There could be multiple editors. Password means something that should be kept in secret. It could also have implications for the implementation, e.g. how they are saved in the DB.

[Arciiix]

Hey, Maybe not related to this issue directly, but I've noticed that the input for phone number is a bit inconsistent. Sometimes the phone number is treated as text, sometimes as number.

For example, in User settings, it's a text, both on front-end and back-end

https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/frontend/src/app/pages/users/UserDialog.tsx#L122-L123

https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/backend/src/Notifo/Areas/Api/Controllers/Users/Dtos/ProfileDto.cs#L28-L31

On the other hand, when it comes to IntegrationProperty, it's usually as number, e.g. here

https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/backend/src/Notifo.Domain.Integrations/MessageBird/MessageBirdSmsIntegration.cs#L31-L37

Therefore, on the front-end side, it's rendered as normal number input - which definitely shouldn't be a case - for example it has that arrow on the right that lets the user change the values by 1, so they could accidently click on it and change the number, or, what's even more extreme, put a negative phone number (screenshot for reference) - and there is no validation of those properties, neither on the front-end and back-end side (the error comes out later, not immediately when saving).

In my opinion, we should normalize the phone number to always be a text property and then make use of my newly created PropertyFormat option - also because some people would prefer to write numbers with spaces, e.g. +48 123 456 789, and the numeric input doesn't allow them to do so.

Also, I'd consider globally refactoring the whole Properties of both user and each integration itself - to add some validation. Right now the back-end code for saving those properties doesn't have any. https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/backend/src/Notifo.Domain/Apps/UpsertAppIntegration.cs#L49

https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/backend/src/Notifo.Domain/Apps/UpsertAppIntegration.cs#L60-L71

That's because it's a <string, string> dictionary. https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/backend/src/Notifo.Domain/Apps/UpsertAppIntegration.cs#L32

Same thing goes with user properties.

What do you think?

Have a lovely day!

[SebastianStehle]

Totally agree about the normalization. But phone numbers are a beast ;) ... therefore I avoided them whenever possible.

It it is true, that on this place there is no validation. But the validation properties are validated when you call GetString() etc. So it makes sense to leverage that.

[Arciiix]

Okay, thank you for your reply. Therefore - so I'll just make a PR solving this issue + this phone number thing, without the refactoring of the properties. And then you can review it and I'll make some changes if needed.

Do you think the refactoring of properties should be done later, in the (near) future?

[SebastianStehle]

I would make 3 PRs:

1. Introduce new format:
2. Fix phone numbers.

I have just seen that we have validation there: https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/backend/src/Notifo.Domain/Integrations/IntegrationManager.cs#L100

[Arciiix]

Okay, thank you!

Regarding to the validation method that you sent - yes, it validates whether the property is of the given type, but this only works for data types, not specific values. For example you can still put -4 as the phone number and no validation error will occur. The earliest time the validation error will occur is when Notifo will try to send a notification, which is a bit too late (I'm not talking about API key - depending on the integration, it can be validated in the OnConfiguredAsync method that they inherit and is called after this universal one you mentioned). So my proposal would be to either completely refactor the properties or validate them in that OnConfiguredAsync in regard to their actual format (phone number/email/etc).

What do you think?

[SebastianStehle]

I think you are not correct. See the following method: Everything is validated (I think there are also tests for it).

https://github.com/notifo-io/notifo/blob/a10a0ee90ae722dfd47bf4ea9b201a2d0924083c/backend/src/Notifo.Domain.Integrations.Abstractions/IntegrationProperty.cs#L112

The problem is how the properties are defined in the concrete integrations. E.g. you could just fix your problem by having a minimum value of 100 or so (110 is the smallest phone number I can think about right now ;))

[Arciiix]

Oh yes, you're right, I'm so sorry, my brain is sometimes not braining haha 😅 I'll look at your comments from the PR and implement the changes as soon as I find some time :)

[Arciiix]

It can be in a few days though, but I'll do it :)