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.

Sign up for GitHub

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

Use secure cookie for HTTPS sites #26999

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Use secure cookie for HTTPS sites #26999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9d0ec9
fix
wxiaoguang Sep 10, 2023
aee808f
Merge branch 'main' into use-cookie-secure
lafriks Sep 10, 2023
53f066e
Merge branch 'main' into use-cookie-secure
GiteaBot Sep 11, 2023
7e3cdef
Merge branch 'main' into use-cookie-secure
GiteaBot Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions custom/conf/app.example.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1746,8 +1746,8 @@ LEVEL = Info
;; Session cookie name
;COOKIE_NAME = i_like_gitea
;;
;; If you use session in https only, default is false
;COOKIE_SECURE = false
;; If you use session in https only: true or false. If not set, it defaults to `true` if the ROOT_URL is an HTTPS URL.
;COOKIE_SECURE =
;;
;; Session GC time interval in seconds, default is 86400 (1 day)
;GC_INTERVAL_TIME = 86400
Expand Down
2 changes: 1 addition & 1 deletion docs/content/administration/config-cheat-sheet.en-us.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ and

- `PROVIDER`: **memory**: Session engine provider \[memory, file, redis, redis-cluster, db, mysql, couchbase, memcache, postgres\]. Setting `db` will reuse the configuration in `[database]`
- `PROVIDER_CONFIG`: **data/sessions**: For file, the root path; for db, empty (database config will be used); for others, the connection string. Relative paths will be made absolute against _`AppWorkPath`_.
- `COOKIE_SECURE`: **false**: Enable this to force using HTTPS for all session access.
- `COOKIE_SECURE`:**_empty_**: `true` or `false`. Enable this to force using HTTPS for all session access. If not set, it defaults to `true` if the ROOT_URL is an HTTPS URL.
- `COOKIE_NAME`: **i\_like\_gitea**: The name of the cookie used for the session ID.
- `GC_INTERVAL_TIME`: **86400**: GC interval in seconds.
- `SESSION_LIFE_TIME`: **86400**: Session life time in seconds, default is 86400 (1 day)
Expand Down
2 changes: 1 addition & 1 deletion docs/content/administration/config-cheat-sheet.zh-cn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ Gitea 创建以下非唯一队列：

- `PROVIDER`: **memory**：会话存储引擎 \[memory, file, redis, redis-cluster, db, mysql, couchbase, memcache, postgres\]。设置为 `db` 将会重用 `[database]` 的配置信息。
- `PROVIDER_CONFIG`: **data/sessions**：对于文件，为根路径；对于 db，为空（将使用数据库配置）；对于其他引擎，为连接字符串。相对路径将根据 _`AppWorkPath`_ 绝对化。
- `COOKIE_SECURE`: **false**：启用此选项以强制在所有会话访问中使用 HTTPS。
- `COOKIE_SECURE`: **_empty_**：`true` 或 `false`。启用此选项以强制在所有会话访问中使用 HTTPS。如果没有设置，当 ROOT_URL 是 https 链接的时候默认设置为 true。
- `COOKIE_NAME`: **i\_like\_gitea**：用于会话 ID 的 cookie 名称。
- `GC_INTERVAL_TIME`: **86400**：GC 间隔时间，以秒为单位。
- `SESSION_LIFE_TIME`: **86400**：会话生命周期，以秒为单位，默认为 86400（1 天）。
Expand Down
2 changes: 1 addition & 1 deletion modules/setting/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func loadSessionFrom(rootCfg ConfigProvider) {
}
SessionConfig.CookieName = sec.Key("COOKIE_NAME").MustString("i_like_gitea")
SessionConfig.CookiePath = AppSubURL + "/" // there was a bug, old code only set CookePath=AppSubURL, no trailing slash
SessionConfig.Secure = sec.Key("COOKIE_SECURE").MustBool(false)
SessionConfig.Secure = sec.Key("COOKIE_SECURE").MustBool(strings.HasPrefix(strings.ToLower(AppURL), "https://"))
Copy link
Member

Choose a reason for hiding this comment

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

On second thought, I would actually drop reading setting key at all and remove it from configs and leaving just this assignment based on app url. From security point of view there is no point in making it use unsecured cookie over https

Copy link
Contributor Author

@wxiaoguang wxiaoguang Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

See #26953

Theoretically yes. But:

  1. There is no "dynamic cookie secure" support in the go-chi package (it could be improved separately)
  2. "dynamic cookie secure" will leave security problems if the site admin doesn't configure the X-Forwarded-Proto correctly, because Gitea self might be running in HTTP mode behind an HTTPS proxy.
  3. Why the COOKIE_SECURE is still kept because in case the user really would like to run Gitea on HTTP and HTTPS at the same time and they know what they are doing.

Maybe we can use a new (breaking) approach in 1.21 or 1.22 and drop COOKIE_SECURE then

Copy link
Member

Choose a reason for hiding this comment

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

points 1 and 2 are not really relevant for this solution but as for 3 I don't really see such use-case so in name of security I would propose to drop this setting in future

Copy link
Contributor Author

@wxiaoguang wxiaoguang Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, the change has been backported to 1.20, let's break it for 1.21 later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

points 1 and 2 are not really relevant for this solution but as for 3 I don't really see such use-case so in name of security I would propose to drop this setting in future

The case: #29451

SessionConfig.Gclifetime = sec.Key("GC_INTERVAL_TIME").MustInt64(86400)
SessionConfig.Maxlifetime = sec.Key("SESSION_LIFE_TIME").MustInt64(86400)
SessionConfig.Domain = sec.Key("DOMAIN").String()
Expand Down