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.

Sign up for GitHub

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

Allow embedding of CORS proxy in fetch relay_url #1189

Merged
merged 5 commits into from
Dec 11, 2024
Merged

Allow embedding of CORS proxy in fetch relay_url #1189

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
690c46b
Allow embedding of CORS proxy in fetch relay_url
chschnell Nov 27, 2024
f66e39c
moved fetch URL decoding from starter.js into main.js
chschnell Dec 1, 2024
32f0cb2
fixed eslint error
chschnell Dec 1, 2024
27fc39d
pass options.net_device to NetworkAdapter constructors
chschnell Dec 8, 2024
fdc2f1f
Update src/browser/main.js
copy Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/browser/starter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,9 +311,12 @@ V86.prototype.continue_init = async function(emulator, options)
if(relay_url)
{
// TODO: remove bus, use direct calls instead
if(relay_url === "fetch")
{
this.network_adapter = new FetchNetworkAdapter(this.bus);
if(relay_url.startsWith("fetch")) {
const fetch_options = {};
if(relay_url.length > 6 && relay_url[5] === ":") {
fetch_options.cors_proxy = relay_url.slice(6);
}
this.network_adapter = new FetchNetworkAdapter(this.bus, fetch_options);
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking about this again, I think this code belongs in browser/main.js. The V86 constructor is supposed to be the high-level api, while browser/main.js implements the more stringly-typed frontend.

Copy link
Contributor Author

@chschnell chschnell Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Forgive me if I'm wrong, but isn't main.js the entry point for your web page and V86 embedding? In my own embedding I call the V86 constructor directly without using anything from main.js (to my knowledge), so I wouldn't gain anything from my own PR, or would I? :)

How about this: We currently have this in starter.js:

if(relay_url.startsWith("fetch"))
{
    const fetch_options = {};
    if(relay_url.length > 6 && relay_url[5] === ":") {
        fetch_options.cors_proxy = relay_url.slice(6);
    }
    this.network_adapter = new FetchNetworkAdapter(this.bus, fetch_options);
}
else if(relay_url.startsWith("wisp://") || relay_url.startsWith("wisps://"))
{
    this.network_adapter = new WispNetworkAdapter(relay_url, this.bus, options);
}
else
{
    this.network_adapter = new NetworkAdapter(relay_url, this.bus);
}

Note that relay_url is passed as the first argument to the constructors of WispNetworkAdapter and NetworkAdapter.

Why not do the same with FetchNetworkAdapter and do away with its fetch_options alltogether (EDIT: remove it only here, keep it as the 3rd, optional constructor argument as in FetchNetworkAdapter(fetch_url, bus, config) ).

The rule would then be that it is the NetworkAdapter's responsibility to decode its relay_url according to its internal rules, this knowledge doesn't need to leak outside the respective NetworkAdapter classes.

And the code from above would simplify to (nice and clean):

if(relay_url.startsWith("fetch"))
{
    this.network_adapter = new FetchNetworkAdapter(relay_url, this.bus);
}
else if(relay_url.startsWith("wisp://") || relay_url.startsWith("wisps://"))
{
    this.network_adapter = new WispNetworkAdapter(relay_url, this.bus, options);
}
else
{
    this.network_adapter = new NetworkAdapter(relay_url, this.bus);
}

Copy link
Owner

Choose a reason for hiding this comment

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

Your second example looks worse to me than the current version in this PR, because it splits the parsing over multiple files.

You're making a valid point though, which is that the the existing implementation already does parsing to a certain degree.

I'm mostly worried that we end up with:

    net_device: {
        relay_url: "ws://.../?ipv4=off&dns=1.2.3.4&other-config=..."
    }

When, in my opinion, a nicer JavaScript API would look like this:

    net_device: {
        type: "relay",
        relay_url: "ws://.../",
        ipv4: false,
        dns: "1.2.3.4",
    }

Forgive me if I'm wrong, but isn't main.js the entry point for your web page and V86 embedding? In my own embedding I call the V86 constructor directly without using anything from main.js (to my knowledge), so I wouldn't gain anything from my own PR, or would I? :)

The V86 constructor should provide an option to specify the cors proxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean now.

So I moved the fetch URL decoding out into main.js (and also into my own embedding).

I am not entirely sure where to put cors_proxy though, I've put it under net_device:

let v86_config = {
    net_device: {
        type: "ne2k" | "virtio",
        relay_url: "fetch" | "ws://..." | "wisp://...",
        cors_proxy: undefined | "https://..."
    },
    // ...
};
let v86 = new V86(v86_config);

In starter.js, I decided to pass options.net_device as the options argument to the FetchNetworkAdapter constructor, meaning this constructor expects an object like (all members optional):

options = {
    id: undefined | <int>,
    router_mac: undefined | <str>,
    router_ip: undefined | <str>,
    vm_ip: undefined | <str>,
    cors_proxy: undefined | "https://...",
}

I am pointing this out because this is not how it is implemented for WispNetworkAdapter, the whole options object and not just its member options.net_device is passed to its constructor in starter.js.

I'd suggest to also pass options.net_device instead of options to the WispNetworkAdapter constructor.

Here's the combined set of all possible members for options.net_device that we would have then (for both constructors):

let v86_config = {
    net_device: {
        type: "ne2k" | "virtio",
        relay_url: "fetch" | "ws://..." | "wisp://...",
        id: undefined | <int>,
        router_mac: undefined | <str>,
        router_ip: undefined | <str>,
        vm_ip: undefined | <str>,
        cors_proxy: undefined | "https://..."
        dns_method: undefined | "static" | "doh",
        doh_server: undefined | <str>,
    },
    // ...
};
let v86 = new V86(v86_config);

}
else if(relay_url.startsWith("wisp://") || relay_url.startsWith("wisps://"))
{
Expand Down