-
Notifications
You must be signed in to change notification settings - Fork 69
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Missing URL validation that can lead to internal error #247
Comments
Good catch. You could just assign a pattern to the URLProperty in the HttpIntegration. See: 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. |
Sure, I'll do it soon. Thank you! |
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. |
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. |
Hey, For example, in User settings, it's a text, both on front-end and back-end notifo/frontend/src/app/pages/users/UserDialog.tsx Lines 122 to 123 in a10a0ee
On the other hand, when it comes to IntegrationProperty, it's usually as number, e.g. here notifo/backend/src/Notifo.Domain.Integrations/MessageBird/MessageBirdSmsIntegration.cs Lines 31 to 37 in a10a0ee
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 Also, I'd consider globally refactoring the whole
notifo/backend/src/Notifo.Domain/Apps/UpsertAppIntegration.cs Lines 60 to 71 in a10a0ee
That's because it's a
Same thing goes with user properties. What do you think? Have a lovely day! |
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. |
Okay, thank you for your reply. Do you think the refactoring of properties should be done later, in the (near) future? |
I would make 3 PRs:
I have just seen that we have validation there:
|
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 What do you think? |
I think you are not correct. See the following method: Everything is validated (I think there are also tests for it). notifo/backend/src/Notifo.Domain.Integrations.Abstractions/IntegrationProperty.cs Line 112 in a10a0ee
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 ;)) |
Oh yes, you're right, I'm so sorry, my brain is sometimes not braining haha 馃槄 |
It can be in a few days though, but I'll do it :) |
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
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!
The text was updated successfully, but these errors were encountered: