curiousdannii / parchment

The Interactive Fiction web app
https://iplayif.com
MIT License
429 stars 60 forks source link

Add an option to disable CORS proxy #71

Closed juhana closed 2 years ago

juhana commented 4 years ago

Parchment automatically uses a proxy to download the story file if it detects that it's not on the same domain. There are two cases where the proxy makes things worse rather than better:

  1. The file is on a different domain, but the server sets the correct CORS headers so it works without the proxy. The file might be inaccessible to the proxy (localhost, internal network etc.)
  2. The URL is a data URL (data:...)

I've been able to get around it with default_story: "data:text/javascript,processBase64Zcode('....'); // .js" which tricks Parchment into thinking that it's a legacy .js file, but that's a very hacky solution.

Arguably case 2 is a bug and it should detect data URLs, but for case 1 we'd need either that setting the proxy URL option to null would disable it altogether, or a separate setting that disables the proxy. Preferably with options auto-detection (current behavior), always enabled or always disabled. HugoJS has this setting:

// use a CORS proxy to load game files?
// "always" (or true), "never" (or false), or "auto".
// The "auto" option uses proxy only if the story URL points to another domain.
use_proxy: "auto"
curiousdannii commented 4 years ago

I'll keep this in mind for the rewrite I was intending to start tomorrow ;).

I wonder how common the CORS headers are now. I also don't remember what happens if you request something you can't access, if the browser will actually download the whole file (which could be quite large) only to raise an error later.

What's the use case for a data URL?

juhana commented 4 years ago

Thanks! I would consider disabling the proxy an "I know what I'm doing" move so whoever does it would be responsible for any side effects that might occur.

What's the use case for a data URL?

I have Parchment running in an iframe where the main app passes the story file from memory so there's no actual URL it could load.

curiousdannii commented 4 years ago

Probably the best way would be to just pass in an arraybuffer/uint8array. I'll have a think about whether that should be an overloaded option for default_story or a separate option.

curiousdannii commented 2 years ago

@juhana Sorry it's taken so long.

I remember you saying that you'd like to be able to just pass in a data array, is that still the case? Would that be instead of using a data URL, or would you also need support for a data URL?

I'm now thinking there will be a parchment_options.auto_launch option to disable autolaunching, and then a function like parchment.load_data("zcode", Uint8Array(...)) which you could use to specify the story format and pass in the data file directly. Do you think this would fit what you want to do with it?

In regards to the proxy server, I'm thinking that it would just try loading the URL, and only if it fails would it try the proxy server. Do you think it really needs an option if that's how it works?

juhana commented 2 years ago

I remember you saying that you'd like to be able to just pass in a data array, is that still the case? Would that be instead of using a data URL, or would you also need support for a data URL? I'm now thinking there will be a parchment_options.auto_launch option to disable autolaunching, and then a function like parchment.load_data("zcode", Uint8Array(...)) which you could use to specify the story format and pass in the data file directly. Do you think this would fit what you want to do with it?

Sounds good! No need for a data URL if a data array can be passed in.

In regards to the proxy server, I'm thinking that it would just try loading the URL, and only if it fails would it try the proxy server. Do you think it really needs an option if that's how it works?

That system would work for my use cases, but if it's certain that the proxy won't work (e.g. localhost URLs) it'd be cleaner if you could just tell it to not even bother trying the proxy.

curiousdannii commented 2 years ago

I implemented the above as parchment.load:

https://github.com/curiousdannii/parchment/blob/c8eaa6e80b11980e947fabea22fc7eb369f01e09/src/common/launcher.js#L72-L75

I did change it so that it will try all URLs without the proxy, it no longer assumes other domains don't have proper CORS support. I guess I could add an option to disable the proxy (perhaps by setting the proxy_url to null?) but I doubt it would be useful. If you're in a position where you know the proxy won't work then can't you also ensure the url is correct? Actually, even without changing the code, simply setting the proxy_url to an invalid domain should in effect mean it is never used. It's such a niche use case that I think that might be enough.

juhana commented 2 years ago

If you're in a position where you know the proxy won't work then can't you also ensure the url is correct?

In theory yes, but as long as you can pass the URL as a query parameter you can always have old links pointing to deleted files.

If setting the proxy url to null would disable the proxy, that would be the most intuitive way for it to work. (It's the first thing I tried when I tried to disable it.)

curiousdannii commented 2 years ago

As it is now, I think setting it to null won't work, but setting it to "http://fake.invalid/" would. It would still call fetch(), it would just instantly fail.

curiousdannii commented 2 years ago

If you set

parchment_options = {auto_launch: 0}

Then Parchment won't launch automatically. You can then call parchment.load() passing a Uint8Array and a format name:

parchment.load(data, 'glulx')

I think your concerns here have basically been handled by now, but if you have any more just make a new issue.

curiousdannii commented 2 years ago

It's probably not necessary, but I've added a use_proxy option to disable use of the proxy.

juhana commented 2 years ago

Thanks, that'll certainly make things more straightforward!