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

Add custom html code to head #3693

Closed
wants to merge 1 commit into from
Closed

Add custom html code to head #3693

wants to merge 1 commit into from

Conversation

mueller-ma
Copy link
Contributor

@mueller-ma mueller-ma commented Sep 5, 2023

⚠️⚠️⚠️ 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

With this PR a new text field will be added that allows setting custom html code to the <head> of a status page.
The implementation will be similar to
https://github.com/louislam/uptime-kuma/pull/2567/files, but with a multi-line text field and without escaping any special chars.

This will allow tracking with most analytic platforms and has been requested several times.
Closes #2818

Type of change

Please delete any options that are not relevant.

  • 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 generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

@mueller-ma

This comment was marked as resolved.

@louislam
Copy link
Owner

I personally ok with this, but:

@mueller-ma
Copy link
Contributor Author

For the XSS attack: In many cases the person with login to the status page might also edit the html files on the server itself. For the other cases a env variable could be added that disables the new setting, so the server admin can restrict the kuma admin.

@mueller-ma mueller-ma changed the title [empty commit] Add custom html code to head Add custom html code to head Oct 13, 2023
@mueller-ma

This comment was marked as resolved.

@louislam

This comment was marked as resolved.

@mueller-ma
Copy link
Contributor Author

mueller-ma commented Oct 18, 2023

Using npm run dev the new setting and the migration are working! I tried to break the page with invalid code, but the browsers (latest Chrome and Firefox) do their best to fix it:

  • <p>Hi</p><foo: Browsers don't add the tag <foo, because it's invalid.
  • <p>Hi</p><script>: Browsers add the closing tag for script: <p>Hi</p><script></script>

From my side this is ready to merge.

@mueller-ma mueller-ma marked this pull request as ready for review October 23, 2023 07:41
@CommanderStorm

This comment was marked as resolved.

@CommanderStorm CommanderStorm mentioned this pull request Nov 10, 2023
1 task
@mueller-ma

This comment was marked as resolved.

@chakflying chakflying added help wanted May need your help to test or answer area:status-page Everything related to the status page labels Dec 2, 2023
@mueller-ma
Copy link
Contributor Author

Can I do something to get this merged?

@mueller-ma
Copy link
Contributor Author

I've addressed the security concerns mentioned in the previous review. Could you please take another look at the PR? Thanks!

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.

Currently, setting it to false or 0 would activate this feature.
=> lets not check the environment variable this way ^^

I have also left some remarks about how we communicate this feature.
I think the communication of this is not quite where it needs to be mergable.

src/lang/en.json Outdated Show resolved Hide resolved
server/model/status_page.js Outdated Show resolved Hide resolved
<!-- Custom HTML -->
<div class="my-3">
<div class="mb-1">{{ $t("Custom HTML") }}</div>
<prism-editor v-model="config.customHtml" class="css-editor" :highlight="highlighter" line-numbers></prism-editor>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the frontend should be disabled if said environment variable is not present and enabled when it is.

This would also enable baving a better helptext, communicating that adding stuff here is inherently kind of dangerous as said code is not vetted by security reviews.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To do this would most likely involve creating a setting on server, then the status page edit page would have to fetch that setting and enable/disable accordingly. A bit complicated IMO.

Maybe having a remark in the description is good enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since vite also supports environment variables, I am not convinced that would be necessary.
Needs figuring out if this linked correctly though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Haven't tested this but I think these only work at build time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also have not tested, but arent we using SSR for this feature?
=> the environment variables should be covered...

With this PR a new text field will be added that allows setting custom
html code to the `<head>` of a status page.
The implementation will be similar to
https://github.com/louislam/uptime-kuma/pull/2567/files, but with a
multi-line text field and without escaping any special chars.

For security reasons the env var `UPTIME_KUMA_ALLOW_CUSTOM_HTML` must be set to `1` to enable this feature.

This will allow tracking with most analytic platforms and has been
requested several times.

Closes #2818
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label May 19, 2024
@CommanderStorm
Copy link
Collaborator

Reworked the PR to adress my concerns: #4817

@CommanderStorm
Copy link
Collaborator

Thanks a lot for the contribution.
Next step is a review of #4817 and if found good, this is merged in to the next release (dependent on reviewer availability) ^^

@chakflying chakflying removed the help wanted May need your help to test or answer label Jun 4, 2024
@mueller-ma mueller-ma deleted the custom-head branch June 4, 2024 06:11
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.

Add custom header option to status page
4 participants