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 Websocket Upgrade Test #5613

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

Conversation

PoleTransformer
Copy link

@PoleTransformer PoleTransformer commented Feb 10, 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 #805
Uses native Websocket API to upgrade to Websocket connection. If the closing response code is 1000(Normal Closure), returns success. Gives user the choice to test non compliant Websocket servers that don't respond with the correct headers. By default conforms to RFC6455.
Uses curl to send websocket headers and look at the response. If the response is 101(switching protocols), returns success. I couldn't find a super elegant way to close the connection after upgrade, so I set a timeout that closes the connection after 1 second. This breaks the ping feature, which will always return 1024ms all the time. If anyone knows of a better way to close the websocket connection, while keeping the ping feature, please let me know. I initially wanted to use the builtin js websocket library, but some non compliant websocket servers don't return the Sec-WebSocket-Accept header, which causes websocket to error out. I left my implementation commented out in case someone wants to work on it. @louislam

Type of change

Please delete any options that are not relevant.

  • 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)

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.

@CommanderStorm
Copy link
Collaborator

This breaks the ping feature

If the impelentation of a new monitor breaks an existing monitor, this is a merge blocker.
I don't see how it could though.
Could you share specifics?

@CommanderStorm
Copy link
Collaborator

I initially wanted to use the builtin js websocket library, but some non compliant websocket servers don't return the Sec-WebSocket-Accept header, which causes websocket to error out.

Executing curl seems a bit like a bodge.. unsure if this is the way to go.

If the service that is being monitored is non compliant, then we should throw an error notifying the user.

I would like for this monitor to be the least amount of "special" that we can get away with

@CommanderStorm
Copy link
Collaborator

Please also make sure that you add a short little testcase to ensure that this monitor keeps working here.
Regressions in monioting types which are not often used are hard to spot

Please also update the docs (the readme) with the new monitoring type

@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Feb 16, 2025
@masim05
Copy link

masim05 commented Feb 17, 2025

@PoleTransformer really appreciate your effort, I was about to report the same request and prepare a PR =)
I see you are using curl, as was mentioned above, it introduces an indirect dependency (let's way, curl might not be installed on the host system, unlikely, but possible). Did you have a chance to consider using nodejs native modules: http(s)?

@PoleTransformer
Copy link
Author

PoleTransformer commented Feb 18, 2025

This breaks the ping feature

If the impelentation of a new monitor breaks an existing monitor, this is a merge blocker. I don't see how it could though. Could you share specifics?

In my previous implementation, I forced a timeout on curl that closes the connection after 1 second. The ping will only show around 1000ms. I fixed this in my latest implementation.

I initially wanted to use the builtin js websocket library, but some non compliant websocket servers don't return the Sec-WebSocket-Accept header, which causes websocket to error out.

Executing curl seems a bit like a bodge.. unsure if this is the way to go.

If the service that is being monitored is non compliant, then we should throw an error notifying the user.

I would like for this monitor to be the least amount of "special" that we can get away with

I agree. After rethinking my ideas, I decided to ditch using curl. It is not native and adds unnecessary dependencies.

Please also make sure that you add a short little testcase to ensure that this monitor keeps working here. Regressions in monioting types which are not often used are hard to spot

Please also update the docs (the readme) with the new monitoring type

I have added a test file that currently tests:

  1. Secure websocket(wss)
  2. Insecure websocket(ws)
  3. Non compliant servers that don't send Sec-WebSocket-Accept header
  4. Non websocket servers

@PoleTransformer
Copy link
Author

@PoleTransformer really appreciate your effort, I was about to report the same request and prepare a PR =) I see you are using curl, as was mentioned above, it introduces an indirect dependency (let's way, curl might not be installed on the host system, unlikely, but possible). Did you have a chance to consider using nodejs native modules: http(s)?

@masim05 Thank you. I switched to using ws instead of curl. Now I need to figure out why the insecure websocket test fails on macos... Passes all tests on my machine.

@PoleTransformer
Copy link
Author

@CommanderStorm It seems like Macos, Ubuntu, and Windows are unable to establish a insecure ws connection. These platforms fail the insecure testcase, everything else passes. For some reason generic ARM64 passes the test. Do you know why this is?

@CommanderStorm
Copy link
Collaborator

It seems like Macos, Ubuntu, and Windows are unable to establish a insecure ws connection. These platforms fail the insecure testcase, everything else passes. For some reason generic ARM64 passes the test. Do you know why this is?

I have no clue.
If you have the time (I currently have exams => am a bit tight on investigation time), looking through nodejs or websocket issues about platform support would be appreciated.

If insecure connections don't work on some platforms, we at least have to put up a warning (otherwise, supporting issues caused the monitor would be horrible ^^)

@PoleTransformer
Copy link
Author

It seems like Macos, Ubuntu, and Windows are unable to establish a insecure ws connection. These platforms fail the insecure testcase, everything else passes. For some reason generic ARM64 passes the test. Do you know why this is?

I have no clue. If you have the time (I currently have exams => am a bit tight on investigation time), looking through nodejs or websocket issues about platform support would be appreciated.

If insecure connections don't work on some platforms, we at least have to put up a warning (otherwise, supporting issues caused the monitor would be horrible ^^)

I am very confused about this. I know browsers won't let you establish insecure connections over an existing secure connection ie http on a https website. Could this be related to security policy set in CI? Im still stumped at why generic arm64 passes the test.

I will do further testing and research. I have midterms coming up as well and wish you the best in your studies.

@PoleTransformer
Copy link
Author

@CommanderStorm Thanks for all your inputs. Got rid of wsurl. I also figured out why the insecure websocket test was failing. The server I configured was a cloudflare server, which had a ip geolocation rule that was blocked on Github's CI infrastructure. It returned a 403 error instead of 101. So lesson learned, use a single assert statement instead of 2 for each test case.

For now, I simply spin up a ws server in the test file and perform the test that way locally, which passes without issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Web Socket (websocket/socket.io) Monitor
3 participants