-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Allow embedding of CORS proxy in fetch relay_url #1189
Conversation
Extends the syntax of relay_url for fetch to: fetch[:CORS-PROXY-URL] where CORS-PROXY-URL is a full URL pointing to the CORS proxy server, for example: fetch:https://example.com/?url=
src/browser/starter.js
Outdated
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
I don't understand how the latest build error relates to this PR. |
I think this could be expanded better for a syntax like this for the relay url: UPDATE: while this seemed like a good idea at first, I realize now that if we just make a proxy that handles requests and just passes all the request data to the actual internet, keeps track of them, then returns the response, it would work better. We could make the `FetchNetworkAdapter just pass them along to server as a stream of data. Only problem with this is that all the v86 VMs configured with this would have the same ip address as the server if we don't keep track of it and simulate a network. But then this basically acts the same way as the websocket proxy we already have. Making a fetch-based network would only provide an advantage if making fetch request is faster than websocket connections(which we don't know if it is or not). |
No, it's not related to your PR, most likely because Rust has been updated, in my fork it also fails on recent 298da23: https://github.com/SuperMaxusa/v86/actions/runs/12121425872/job/33792327522#step:16:1
My opinion, that looks a little "messier" in options: net_device: {
relay_url: "fetch:http://proxy-for.get/:http://localhost:1234:https://another-proxy:404..."
} Although there is no need to have a proxy for each HTTP method (currently). Feel free to correct me if I'm wrong. |
@SuperMaxusa I don't think your wrong about this. It does seem excessive now that I think about it. I'm just wondering if fetch based networks that work like the websocket do are/might be faster(but then we're basically doing the exact same thing as the websocket). |
@copy I'll rephrase what I wrote eralier. In this.network_adapter = new WispNetworkAdapter(relay_url, this.bus, options); Note that the This is how this.id = config.id || 0;
this.router_mac = new Uint8Array((config.router_mac || "52:54:0:1:2:3").split(":").map(function(x) { return parseInt(x, 16); }));
this.router_ip = new Uint8Array((config.router_ip || "192.168.86.1").split(".").map(function(x) { return parseInt(x, 10); }));
this.vm_ip = new Uint8Array((config.vm_ip || "192.168.86.100").split(".").map(function(x) { return parseInt(x, 10); }));
this.masquerade = config.masquerade === undefined || !!config.masquerade; My only question is whether this is correct or if this.network_adapter = new WispNetworkAdapter(relay_url, this.bus, options.net_device); Are |
@SuperMaxusa Thank you for the info about rust. In my opinion it should only be a single, optional CORS proxy in the fetch URL. |
Yes, |
- aligned construction of WispNetworkAdapter and FetchNetworkAdapter classes - supported members in object options.net_device (all optional): id, router_mac, router_ip, vm_ip, masquerade, dns_method, doh_server, cors_proxy - completed support for free choice between fake network DNS methods "static" and "doh"
@chschnell Good to merge? |
Sure! |
Extends the syntax of relay_url for fetch to:
where CORS-PROXY-URL is a full URL pointing to the CORS proxy server, for example:
Fixes issue #1188.