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

Set secure flag on session cookie when running on https #3877

Merged
merged 2 commits into from
May 17, 2024

Conversation

knolleary
Copy link
Member

Part of https://github.com/FlowFuse/security/issues/88

Description

If we know we're running with https configured, we should set the secure flag on the session cookie. We cannot set it always on as that will break localfs installs without https configured.

I believe the test on base_url is the right one to do here. I've checked our helm chart sets that with a https prefix if the https flag is set.

@knolleary knolleary requested a review from hardillb May 16, 2024 16:52
@knolleary
Copy link
Member Author

Going to verify this works as I expect it to on the prestaging setup as I don't have a full https enabled localfs setup.

Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 79.25%. Comparing base (05a4c0d) to head (3611e77).
Report is 13 commits behind head on main.

Current head 3611e77 differs from pull request most recent head a7f8d88

Please upload reports for the commit a7f8d88 to get more accurate results.

Files Patch % Lines
forge/routes/auth/index.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3877      +/-   ##
==========================================
- Coverage   79.26%   79.25%   -0.01%     
==========================================
  Files         281      281              
  Lines       12723    12722       -1     
  Branches     2838     2838              
==========================================
- Hits        10085    10083       -2     
- Misses       2638     2639       +1     
Flag Coverage Δ
backend 79.25% <50.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hardillb
Copy link
Contributor

I'll have another look at how hard it would be to do HTTPs on localfs, in theory we should be able to share a single certificate between the forge app and the all the instances, since the certs don't care about port numbers.

Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Approved as logically it looks good, but not merged while waiting on pre-staging testing results.

@knolleary
Copy link
Member Author

Have verified the cookie has the secure flag set in the prestaging env. Will merge in the morning.

@knolleary knolleary enabled auto-merge May 17, 2024 08:00
@knolleary knolleary merged commit 201392a into main May 17, 2024
11 checks passed
@knolleary knolleary deleted the sec88-cookie-settings branch May 17, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants