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

[WIP] Websocket authentication for preview #1093

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tmistele
Copy link

This adds authentication so that users don't have to worry about a) third parties being able to connect to the tinymist servers on 127.0.0.1 (browsers allow any website to do that) and b) third parties impersonating the tinymist server, potentially making the frontend html/javascript leak information (e.g. on multi-user systems).

This is not ready yet. But I thought I'd post it so you can comment on the overall approach.

This seems to work for me but I haven't tested everything. Especially the VSCode extension I have tested only very superficially. And I have not at all tested the "compat" integration with typst-preview.

Some random notes and questions:

  • I guess typst-preview will not be updated to add support for authentication? For now, I've built in an escape hatch to avoid authentication for the "compat" preview mode (completely untested!). How do you think this should be handled? It would be easiest to just not support the typst-preview compat mode but I guess you don't want to do that yet?
  • For now I've used a somewhat non-standard challenge response authentication. As you said, @Myriad-Dreamin , it may be good to look into more standard alternatives.
  • How would one actually test the "control plane" server? Where is it used? This is not clear to me.
  • I've added authentication only to the websocket part, not to the static html server. So any website can load the preview forntend's html. That's probably okay if the frontend html does not contain anything interesting to an attacker. So I've restructured the code a bit to avoid replace(...)ing ports and user settings directly into the html. These settings are now passed in as part of URL that's opened in the browser (like http://127.0.0.1:23625/#param=value). That code restructuring makes it more immediately obvoius that there's nothing interesting in the html for an attacker.
    Unfortunately, the vscode webview panel doesn't seem to support setting a location hash (or maybe it does and I didn't figure out how?), so I've built in an escape hatch for the VSCode webview panel where the settings are now replace(...)ed into the html. That's somewhat unfortunate, but not a problem from a security point of view because the vscode webview panel cannot be accessed from outside vscode I think.
  • Maybe off-topic: I noticed that the vscode extension replaces occurences of typst-webview-assets/ in the frontend html. That didn't seem to have any effect in my tests. Does this code ever do anything?

cc @Myriad-Dreamin

@Myriad-Dreamin
Copy link
Owner

I have pinged Enter-tainer. He will give first review when he schedule a time.

@Enter-tainer
Copy link
Collaborator

I guess typst-preview will not be updated to add support for authentication? For now, I've built in an escape hatch to avoid authentication for the "compat" preview mode (completely untested!). How do you think this should be handled? It would be easiest to just not support the typst-preview compat mode but I guess you don't want to do that yet?

I haven't read code yet. Does compat mode means the tinymist preview in tinymist cli? I suppose it is still being used by nvim users? (Pls correct me if i get it wrong)

Unfortunately, the vscode webview panel doesn't seem to support setting a location hash (or maybe it does and I didn't figure out how?), so I've built in an escape hatch for the VSCode webview panel where the settings are now replace(...)ed into the html.

I cannot remember it clearly. Typst-preview starts from vscode webview and then it goes into browser and finally integrated into tinymist. I guess this explains why we do replaces.

crates/tinymist/src/tool/preview/auth.rs Show resolved Hide resolved
crates/tinymist/src/tool/preview/auth.rs Show resolved Hide resolved
///
/// This function takes an owned `HyperWebsocketStream` to avoid accidental misuse elsewhere before authentication.
///
/// TODO: This is a basic challenge response authentication with nonces. Is there something more standard we could use?
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

crates/tinymist/src/tool/preview/auth.rs Outdated Show resolved Hide resolved
@Enter-tainer
Copy link
Collaborator

Enter-tainer commented Dec 31, 2024

I just remembered that we actually have 2 websockets here(at least in typst-preview). The "control plane" and the "data plane".

This exists because in the very first, typst-preview is not a lsp, so it cannot directly receive buffer updates in vscode. Therefore a websocket server is for sending control messages between editor(vscode) and typst-preview binary. And the data plane server send typst document data to browser/webview for previewing.

https://enter-tainer.github.io/typst-preview/arch.html

https://enter-tainer.github.io/typst-preview/editor.html

@tmistele
Copy link
Author

I haven't read code yet. Does compat mode means the tinymist preview in tinymist cli? I suppose it is still being used by nvim users? (Pls correct me if i get it wrong)

By compat I meant typst-preview. At least I think the code in editors/vscode/src/features/preview-compat.ts is meant to support users that use typst-preview instead of the preview build into tinymist? I haven't actually tested this, so I'm not 100% sure.

The websocket authentication should work fine with the tinymist preview cli command.

I just remembered that we actually have 2 websockets here(at least in typst-preview). The "control plane" and the "data plane".

Right. For typst-preview (the "compat" mode) this PR does not add any authentication (see here here and here ). So things should just continue to work for typst-preview users, though with worse security compared to the tinymist built-in preview. But I think that's unavoidable unless we also want to update typst-preview to support authentication (or drop typst-preview support in tinymist)?

I actually have a related question:

There are also two websockets in the tinymist preview cli commeand (data plane + control plane). This PR requires authentication for both. But I'm not sure where the "control plane" part is used. Is it used by e.g. neovim? I haven't tested the control plane websocket.

@Enter-tainer
Copy link
Collaborator

Sorry but I completely forget what does typst-preview compat mode do. @Myriad-Dreamin could you please give more input on this? Thanks! From what I understand so far, people should be using tinymist preview in neovim and lsp based(json rpc) in vscode.

But I'm not sure where the "control plane" part is used. Is it used by e.g. neovim?

I believe so. typst-preview.nvim use websocat to connect to control plane. This is documented in https://enter-tainer.github.io/typst-preview/editor.html

@tmistele
Copy link
Author

tmistele commented Jan 2, 2025

I believe so. typst-preview.nvim use websocat to connect to control plane. This is documented in https://enter-tainer.github.io/typst-preview/editor.html

Ah, so the neovim part lives in a different repository. Requiring authentication for the control plane websocket is a breaking change for that neovim package then, I guess. How should we handle this?

Looking at that code, it also seems to read the server urls to connect to from the log::info!(...) that tinymist prints to stderr. So I probably should also use log::info to print the secret for the websocket authentication to stderr (edit: did that).

As a possible alternative: @SylvanFranklin recently said to be working on using lsp workspace commands to start the preview etc. for neovim. In that case, neovim would no longer need the control plane server.

(cc @chomosuke)

mirroring HTTP digest authentication
So users can infer how to connect to the control plane server when using
`typst preview` cli.
@Enter-tainer
Copy link
Collaborator

As a possible alternative: @ SylvanFranklin tmistele/helix-tinymist#3 to be working on using lsp workspace commands to start the preview etc. for neovim. In that case, neovim would no longer need the control plane server.

That would be cool. Previously we use websocket because we don't have a good editor/server connection. But as typst-preview has been integrated into tinymist, we could use lsp commands to achieve the same thing.

@Myriad-Dreamin
Copy link
Owner

Sorry but I completely forget what does typst-preview compat mode do.

It is specific to compatible with the old typst-preview extension. They are still in the repository and built per CI and release.

@chomosuke
Copy link

chomosuke commented Jan 2, 2025

typst-preview.nvim is happy to take this breaking change to the face and upgrade to using lsp commands (or whatever the new way of starting preview is). Can probably make a new plugin called tinymist.nvim and deprecate the old plugin, or just a new version.

@Myriad-Dreamin
Copy link
Owner

How should we handle this?

@tmistele We can add an option to enable/disable the authentication. It should be good to continue warn if the authentication is not enabled. I believe some user will finally find and report the warning log in the next few years. Of course, we could also open an issue to some known downstream, like typst-preview.nvim and typst-preview.el (emacs).

@SylvanFranklin
Copy link
Contributor

As a possible alternative: @SylvanFranklin recently said to be working on using lsp workspace commands to start the preview etc. for neovim. In that case, neovim would no longer need the control plane server.

Hey @tmistele I have paused working on this because I wasn't sure about the status of typst-preview.nvim relating to tinymist.

@tmistele
Copy link
Author

tmistele commented Jan 3, 2025

@tmistele We can add an option to enable/disable the authentication. It should be good to continue warn if the authentication is not enabled. I believe some user will finally find and report the warning log in the next few years. Of course, we could also open an issue to some known downstream, like typst-preview.nvim and typst-preview.el (emacs).

I would be fine with a configuration option as long as the default is to turn authentication on.

That would of course still break downstream packages. But such downstream packages then have a very easy and quick temporary fix (just add a command line flag which turns authentication off). This should give downstream packages a good way to transition (add the command line flag until they added proper authentication support). Does that sound good to you @Myriad-Dreamin ?

I agree we should open issues to downstream packages so they're aware.

@Enter-tainer
Copy link
Collaborator

Sorry but I completely forget what does typst-preview compat mode do.

It is specific to compatible with the old typst-preview extension. They are still in the repository and built per CI and release.

thanks for clarification. i think we can safely remove them when it's time.

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.

5 participants