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(vncscreen component): added missing handling for serververificat… #62

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

blackcow1987
Copy link

@blackcow1987 blackcow1987 commented Feb 18, 2025

…ion event

Looking at the example code of novnc, it was developed to handle the serververification event when using RA2ne authentication, but react-vnc lacks the corresponding code, so it was added.

        UI.rfb.addEventListener("serververification", UI.serverVerify);

https://github.com/novnc/noVNC/blob/master/app/ui.js#L1085

    async serverVerify(e) {
        const type = e.detail.type;
        if (type === 'RSA') {
            const publickey = e.detail.publickey;
            let fingerprint = await window.crypto.subtle.digest("SHA-1", publickey);
            // The same fingerprint format as RealVNC
            fingerprint = Array.from(new Uint8Array(fingerprint).slice(0, 8)).map(
                x => x.toString(16).padStart(2, '0')).join('-');
            document.getElementById('noVNC_verify_server_dlg').classList.add('noVNC_open');
            document.getElementById('noVNC_fingerprint').innerHTML = fingerprint;
        }
    },

https://github.com/novnc/noVNC/blob/master/app/ui.js#L1214

Description

Describe your PR here.

Resolved issues

Closes #1

Before submitting the PR, please take the following into consideration

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. If you don't have an issue, please create one.
  • Prefix your PR title with feat: , fix: , chore: , docs:, or refactor:.
  • The description should clearly illustrate what problems it solves.
  • Ensure that the commit messages follow our guidelines.
  • Resolve merge conflicts (if any).
  • Make sure that the current branch is upto date with the main branch.

…ion event

Looking at the example code of novnc, it was developed to handle the serververification event when
using RA2ne authentication, but react-vnc lacks the corresponding code, so it was added.
Copy link
Owner

@roerohan roerohan left a comment

Choose a reason for hiding this comment

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

Current PR looks good, but an additional feature was requested. Not sure if that is needed though, please let me know accordingly.

}

if (e.detail.type === 'RSA') {
const fingerprint = await window.crypto.subtle.digest("SHA-1", e.detail.publickey)
Copy link
Owner

Choose a reason for hiding this comment

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

Currently the fingerprint is only logged, it would be better if we could expose the fingerprint (if present) by forwarding the ref to the parent component, so that the fingerprint can be programmatically verified if needed.

The easiest way to do it in my opinion is to assign it to the RFB object (if it has a rfb.fingerprint field?) or just on the VNCScreenHandle object.

P.S. I'm not sure the fingerprint needs to be used by a client in any scenario, but I assume that's the purpose of it - to make sure the fingerprint is in accordance with the VNC server

Copy link
Owner

Choose a reason for hiding this comment

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

I added support for react 19, which has caused some conflicts in the PR. Sorry for the inconvenience, it would be great if you could resolve the merge conflicts.

@blackcow1987
Copy link
Author

I have found that RSA type connections fail if this event is not handled. I found this issue while testing with RealVNC. I will solve the conflicts :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants