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

feat: Detect URLs in monitor descriptions #5576

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

eden881
Copy link

@eden881 eden881 commented Jan 25, 2025

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #5575

Adds a minimal logic to the description field to detect URL schemes and make them clickable.
It also sanitizes the URL to prevent XSS attacks.

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Screenshot 2025-01-25 at 23-46-35 Uptime Kuma

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

This would open us to an XSS, as the content is just v-html'ed..

@CommanderStorm CommanderStorm marked this pull request as draft January 26, 2025 11:01
@CommanderStorm CommanderStorm added area:status-page Everything related to the status page pr:please address review comments this PR needs a bit more work to be mergable labels Jan 26, 2025
@eden881
Copy link
Author

eden881 commented Jan 26, 2025

@CommanderStorm That's the reason I've sanitized the input from the description and not processed it blindly.
I can try and change it in a way that doesn't involve v-html - do you know a way to create links without <a> tags?

@CommanderStorm
Copy link
Collaborator

I think there might be a misunderstanding.

My concern isn’t about creating links but about the rest of the input. The current approach doesn’t sanitize the description to prevent an XSS.

For example, if a <script> tag is included, it would be rendered as regular HTML without proper sanitization.

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Jan 27, 2025

Here is how we do this on the status page side.
I don't know if this renders links or if marked can be configured to do so.

descriptionHTML() {
if (this.config.description != null) {
return DOMPurify.sanitize(marked(this.config.description));
} else {
return "";
}
},

@eden881
Copy link
Author

eden881 commented Jan 27, 2025

I managed to avoid v-html completely with a component :)

Not sure what failed the test though. It says Error: Connect Timeout Error.
Maybe something spontaneous, can you trigger a rerun?

Edit: I've synced my branch and it triggered a rerun ✅

@eden881 eden881 marked this pull request as ready for review January 27, 2025 02:27
@CommanderStorm
Copy link
Collaborator

Have you investigated if a similar solution as on the status page could work?
I think rendering markdown would be much simpler and better maintainable than splitting the codebase..

(What you came up with looks quite hacky..)

@eden881
Copy link
Author

eden881 commented Jan 27, 2025

Sure, I'll give it a shot.

@eden881 eden881 marked this pull request as draft January 27, 2025 13:24
@eden881
Copy link
Author

eden881 commented Jan 27, 2025

@CommanderStorm I've used the approach you suggested from StatusPage.vue.
It works, but:

  • It still uses v-html (although this time it's passed through DOMPurify.sanitize())
  • The <a> tag doesn't contain target="_blank" rel="noopener noreferrer" and so it opens in the current tab, replacing the status page, instead of creating a new tab

It's still better than nothing and I have no problem with this solution, but I do think the component approach is slightly better functionality-wise.

What do you think? If it looks good to you feel free to merge :)

@eden881 eden881 marked this pull request as ready for review January 27, 2025 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:status-page Everything related to the status page pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect links in monitor descriptions
2 participants