feat(dream/server): add CORS headers

[mgcrea]

I have been building a alternative webui that polls the dream server API as I wanted to save generation history and quickly work with seeds/variants (demo).

It requires a CORS header to properly work when started (development mode) on another port.

[lstein]

@bakkot You identified a cross-site scripting vulnerability in a previous PR that attempted to add CORS headers. Could you check this one out as well? It just a one line fix.

[mgcrea]

Hum, did not see the other PR, indeed I think this is something that we may want to have behind a flag. Will look into it.

[bakkot]

This has the same problem as the previous one. cf #371.

[tildebyte]

Definitely must be non-default opt-in with a warning. Browsers don't even allow this behavior for 'localhost' any more.

[TesseractCat]

Instead of adding CORS support, maybe we should separate out the API and frontend components of the WebUI (this should be helpful for other use cases-like a UI redesign in the future). Right now the POST /, and GET /cancel endpoints are the API surface, and server.py should be reduced to handling those. Separately, frontends can be created which interface with those endpoints.

[tildebyte]

@TesseractCat; That work is actually ongoing in this repo (i.e. redesign). I guess the question is; how much effort do we want to put into the existing web ui?

[TesseractCat]

@TesseractCat; That work is actually ongoing in this repo (i.e. redesign). I guess the question is; how much effort do we want to put into the existing web ui?

Well, when I made the original UI, my goal was to make something simple without any dependencies. I guess as we start thinking about more complex frontends, we should improve the modularity. I think we should separate the API layer and the frontend, and then move the frontends to separate repositories. Then either the existing web UI could be moved to it's own repository, or could be kept as a simple default.

[lstein]

What do you say we anounce a freeze to major changes to the WebUI until @psychedelicious has completed their redesign?

They seem to be moving along quite rapidly and I don't want contributors to spend a lot of time working on what was initially a quick-and-dirty implementation (with much gratitude to @TesseractCat )

[lstein]

I have been building a alternative webui that polls the dream server API as I wanted to save generation history and quickly work with seeds/variants (demo).

It requires a CORS header to properly work when started (development mode) on another port.

The demo's quite nice, BTW. I like the style of building the query up bit by bit.

[tildebyte]

What do you say we anounce a freeze to major changes to the WebUI until @psychedelicious has completed their redesign?

Agreed

[psychedelicious]

I'm trying to finish the core features for the redesign today, as I may be busy for the next couple days - have had nothing else to work on the past couple days so I've been cranking it out.

Regarding CORS, agree we should have a switch on server-y scripts to allow all origins.

@TesseractCat , thanks for making the web UI, I've used it as a jumping off point for a more involved UI made in React.

[mgcrea]

@lstein @bakkot just updated the PR adding a couple of flags to properly enable CORS mode:

Defaults to Access-Control-Allow-Origin: * when enabled:

scripts/dream.py --web --cors
scripts/dream.py --web --cors https://mgcrea.github.io 

[tildebyte]

I think having —cors simply take an optional arg is a better pattern, i.e.

• —cors # i.e. '*'
• —cors https://mgcrea.github.io

[mgcrea]

I think having —cors simply take an optional arg is a better pattern, i.e.

• —cors # i.e. '*'
• —cors https://mgcrea.github.io

Yep, I agree, not fluent in Python so I initially took the easy road, but just updated the PR to do just that.

[bakkot]

I don't think * is a good default - it's fundamentally not safe. The easiest thing should not be the unsafe thing. And I would worry that some UIs would suggest people start with --cors instead of --cors this_specific_origin, because it's easier, and thus leave their users vulnerable.

That is, I would make the argument mandatory, and reject * as an option even if it's explicitly specified.

[mgcrea]

@bakkot In my opinion it's good enough for now, as we are still in the early stages, the risk profile is in my opinion relatively low and acceptable behind a flag. It will still be possible to revisit at a later date.

Forbidding the wildcard * do seem a bit extreme to me. Every server development framework out there does allow the use of a wildcard so I'm not sure why this stable-diffusion backend should be treated differently. At the end of the day you are running untrusted server code on your computer and that can already be considered to be a pretty big risk.

Also it would require more complex code to handle multiple allowed origins.

But I'm willing to change the PR if there is a consensus to do so.

[bakkot]

I am strongly opposed to making * the default and mostly opposed to allowing it at all. The benefits are not commensurate with the risks. allow-origin * is something you'd use for public-facing servers which have been duly hardened against untrusted visitors, not for internal services.

At the end of the day you are running untrusted server code on your computer and that can already be considered to be a pretty big risk.

There is a very large difference between running untrusted code you downloaded from a specific, active GitHub repository and running untrusted code from every single website you visit.

[psychedelicious]

Every server development framework out there does allow the use of a wildcard so I'm not sure why this stable-diffusion backend should be treated differently.

Difference being this project is not a server development framework (clearly targeted at developers), but a user-facing application. If somebody knows enough to edit the scripts themselves, then they can set CORS to "*", fine.

An unsuspecting user may have trouble accessing the SD service due to a CORS issue and set CORS to "*" without understanding anything more than it fixed the problem. Instead, they should come to this repo and ask for help so they can get things set up securely.

[lstein]

Why not just have a flag that the user can set at launch time and a little warning message that alerts the user to the risks of this and directs them to appropriate background reading material? Lincoln

…

On Wed, Sep 7, 2022 at 11:30 AM psychedelicious ***@***.***> wrote: Every server development framework out there does allow the use of a wildcard so I'm not sure why this stable-diffusion backend should be treated differently. Difference being this project is not a server development framework (clearly targeted at developers), but a user-facing application. If somebody knows enough to edit the scripts themselves, then they can set CORS to "*", fine. An unsuspecting user may have trouble accessing the SD service due to a CORS issue and set CORS to "*" without understanding anything more than it fixed the problem. Instead, they should come to this repo and ask for help so they can get things set up securely. — Reply to this email directly, view it on GitHub <#408 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA3EVKPMPUAZKTYRDKBWOTV5CYJRANCNFSM6AAAAAAQGFW3YQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- *Lincoln Stein* Head, Adaptive Oncology, OICR Senior Principal Investigator, OICR Professor, Department of Molecular Genetics, University of Toronto Tel: 416-673-8514 Cell: 416-817-8240 ***@***.*** *E**xecutive Assistant* Michelle Xin Tel: 647-260-7927 ***@***.*** ***@***.***>* *Ontario Institute for Cancer Research* MaRS Centre, 661 University Avenue, Suite 510, Toronto, Ontario, Canada M5G 0A3 @OICR_news <https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Foicr_news&data=04%7C01%7CMichelle.Xin%40oicr.on.ca%7C9fa8636ff38b4a60ff5a08d926dd2113%7C9df949f8a6eb419d9caa1f8c83db674f%7C0%7C0%7C637583553462287559%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PS9KzggzFoecbbt%2BZQyhkWkQo9D0hHiiujsbP7Idv4s%3D&reserved=0> | www.oicr.on.ca *Collaborate. Translate. Change lives.* This message and any attachments may contain confidential and/or privileged information for the sole use of the intended recipient. Any review or distribution by anyone other than the person for whom it was originally intended is strictly prohibited. If you have received this message in error, please contact the sender and delete all copies. Opinions, conclusions or other information contained in this message may not be that of the organization.

[bakkot]

There's not really much reason to allow *. The motivation for this, as I understand it, is that you're trying to allow a specific site to make requests to your local server so that it can be an alternative UI. For that you only need the ability to let that specific site make requests to the server. There's no particular need to also allow every other site you visit to make requests.

[mgcrea]

@lstein quite like the warning idea (could be something big in caps) to educate end-users about the risks.

@bakkot you might want to test several alternative frontends + a local dev version, etc, in that case '*' can be quite useful.

I do really think that we should let users decide what they want to do, and allow any value. I don't like blacklisting features for the sake of perceived users' own good. Cases could be made against NSFW-generated materials, it's pretty much endless. etc.

I think a good middle ground would be to remove the wildcard default, and require something to be specified, and add a big warning message (with maybe a link with more details) if you try to pass a wildcard. But still allow people to do what they want.

[bakkot]

It's open source. Someone who actually knows what they're doing can trivially set it to *. Someone who doesn't know what they're doing shouldn't be setting it to *.

[tildebyte]

mcgrea: I agree with

I do really think that we should let users decide what they want to do, and allow any value

when the thing in question has a low risk of immediate harm

OTOH, bakkot's

Someone who actually knows what they're doing can trivially set it to *. Someone who doesn't know what they're doing shouldn't be setting it to *

is the right way to think about it IMO

ALL web browsers disallow connections to localhost by default. There are JILLIONS of questions on SO, Reddit, etc. from brand-new web devs who don't understand why they can't just "load the thing". I don't believe I've ever seen a consensus of "yeah, here's how to turn that off but be careful ' (though there's always at least one person...) - it's always "here's how you set up a local dev server, and here's why".

TL;DR - taking away users' choice is bad. Handing a complete noob a loaded firearm with no safety (and a round in the chamber) is utterly irresponsible (just to strain that metaphor - we're not a firing range, so I don't think that education is our job in this instance).

[nderscore]

There is absolutely no good reason to allow * in the CORS field unless you're running a public service for anyone to use (which this project isn't currently equipped for). It's a huge security liability otherwise.

you might want to test several alternative frontends + a local dev version, etc, in that case '*' can be quite useful.

@mgcrea it's not needed for local development at all

Every modern JS dev server (webpack, vite, etc) can be configured to proxy requests in order to get around this.

Here's an example with vite.

[mgcrea]

Well, looks like you all have strong opinions on this and I don't, not sure we really need the same security profile as a public browser used by billions of people but let's not diverge any further!

I did remove the default to * for now and force users to set a value + add a big warning if you pass the wildcard.

If you want to forbid the wildcard altogether, feel free to push an extra commit on top of this. I'll be fine with it.

[lstein]

This has been superseded by subsequent changes to the WebGUI. The new GUI does allow CORS permissions to be set on the command line at launch time.