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

Implement security best practices using Flask-Talisman #845

Merged
merged 1 commit into from Oct 10, 2021

Conversation

zorun
Copy link
Collaborator

@zorun zorun commented Oct 10, 2021

No description provided.

@almet
Copy link
Member

almet commented Oct 10, 2021

That's nice :-)

app,
# Forcing HTTPS is the job of a reverse proxy
force_https=False,
# This is handled separately through the SESSION_COOKIE_SECURE Flask setting
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the comment. It's kinda counter-intuitive.

Copy link
Member

@almet almet left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks, didn't know about Flask-Talisman, it's long overdue… :-)

@zorun
Copy link
Collaborator Author

zorun commented Oct 10, 2021

The contentious point is whether to enable cookie security by default. If yes (what I did here), it will silently break any HTTP setup (dev, prod): browsers will refuse to send the session cookie, so the user can't login to a project, change language, etc.

After discussing on IRC, I'm leaning towards leaving it disabled by default, and document the setting so that admins can enable it if they know they are using HTTPS. It's a bit sad, but we have no way to know if we're behind a reverse proxy doing HTTPS or not.

@zorun
Copy link
Collaborator Author

zorun commented Oct 10, 2021

Looks good to me. Thanks, didn't know about Flask-Talisman, it's long overdue… :-)

Hey, thanks for your review :) Yes, Flask-Talisman is kind of magical, but fortunately simple enough to trust it...

What do you think about the default setting?

@almet
Copy link
Member

almet commented Oct 10, 2021

I think it's okay to break things during a major upgrade, as will be the case for the next release. So, I would be advocating for having a setup which uses secure cookies by default, especially as it's mentioned in the upgrade docs.

That being said, I'll let you decide, I don't have a strong opinion about it.

@Glandos
Copy link
Member

Glandos commented Oct 10, 2021

I would like to agree with @almet but we can have a warning in the release notes saying that it'll be defaulted to True in the next major release.

@zorun
Copy link
Collaborator Author

zorun commented Oct 10, 2021

I was concerned about the dev environment, because it's using plain HTTP. But browsers apparently have exceptions and they accept to send secure cookies to localhost over HTTP, which should cover most dev cases. I have added a note in the documentation for the few people not developping over localhost.

So, let's merge this!

@zorun zorun merged commit e626a1c into spiral-project:master Oct 10, 2021
@almet
Copy link
Member

almet commented Oct 10, 2021

Looks good, thanks <3

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.

None yet

3 participants