-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Allow plugin settings to add connect-src CSP sources #7166
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
base: develop
Are you sure you want to change the base?
Changes from 4 commits
c87106f
119c512
c3ebc85
c77bef4
cf532bf
70ded0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import ( | |
| "io" | ||
| "io/fs" | ||
| "net/http" | ||
| "net/url" | ||
| "os" | ||
| "path" | ||
| "path/filepath" | ||
|
|
@@ -552,6 +553,37 @@ func isURL(s string) bool { | |
| return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://") | ||
| } | ||
|
|
||
| const cspSettingPrefix = "csp_" | ||
|
|
||
| // cspConnectSrcFromSettings returns validated http(s) connect-src URLs from | ||
| // the plugin's settings, plus the keys that were skipped as invalid. | ||
| // Settings keys beginning with "csp_" are treated as connect-src sources. | ||
| func cspConnectSrcFromSettings(settings map[string]interface{}) (valid []string, skippedKeys []string) { | ||
| for k, v := range settings { | ||
| if !strings.HasPrefix(k, cspSettingPrefix) { | ||
| continue | ||
| } | ||
| s, ok := v.(string) | ||
| if !ok || !isValidConnectSrcURL(s) { | ||
| skippedKeys = append(skippedKeys, k) | ||
| continue | ||
| } | ||
| valid = append(valid, s) | ||
| } | ||
| return valid, skippedKeys | ||
| } | ||
|
|
||
| func isValidConnectSrcURL(s string) bool { | ||
| if strings.ContainsAny(s, " \t\r\n;\"'") { | ||
| return false | ||
| } | ||
| u, err := url.Parse(s) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return (u.Scheme == "http" || u.Scheme == "https") && u.Host != "" && u.Hostname() != "" && u.User == nil && !strings.Contains(u.Host, "*") | ||
| } | ||
|
|
||
| func setPageSecurityHeaders(w http.ResponseWriter, r *http.Request, plugins []*plugin.Plugin) { | ||
| c := config.GetInstance() | ||
|
|
||
|
|
@@ -609,6 +641,15 @@ func setPageSecurityHeaders(w http.ResponseWriter, r *http.Request, plugins []*p | |
| } | ||
|
|
||
| connectSrcSlice = append(connectSrcSlice, ui.CSP.ConnectSrc...) | ||
|
|
||
| if settings := c.GetPluginConfiguration(plugin.ID); settings != nil { | ||
| valid, skippedKeys := cspConnectSrcFromSettings(settings) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I think the prefix you went with will collide with the generic plugin-settings namespace. I think this would inject or degub on every page load and there's no opt in. Not sure the best way to handle this tbh.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, this was a real concern. Added an opt-in mechanism: plugins must now set This prevents accidental namespace collisions. The flag is plumbed through Also updated Plugins.md to document the opt-in requirement. |
||
| connectSrcSlice = append(connectSrcSlice, valid...) | ||
| for _, key := range skippedKeys { | ||
| logger.Debugf("skipping invalid csp_ setting %q for plugin %q", key, plugin.ID) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this be better as a warn?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Changed |
||
| } | ||
| } | ||
|
|
||
| scriptSrcSlice = append(scriptSrcSlice, ui.CSP.ScriptSrc...) | ||
| styleSrcSlice = append(styleSrcSlice, ui.CSP.StyleSrc...) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| package api | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestCspConnectSrcFromSettings(t *testing.T) { | ||
| for _, tt := range []struct { | ||
| name string | ||
| settings map[string]interface{} | ||
| valid []string | ||
| skippedKeys []string | ||
| }{ | ||
| { | ||
| name: "no csp keys", | ||
| settings: map[string]interface{}{"foo": "bar", "other_setting": "https://x.com"}, | ||
| valid: nil, | ||
| }, | ||
| { | ||
| name: "valid https url", | ||
| settings: map[string]interface{}{"csp_x": "https://api.example.com"}, | ||
| valid: []string{"https://api.example.com"}, | ||
| }, | ||
| { | ||
| name: "valid http url", | ||
| settings: map[string]interface{}{"csp_x": "http://localhost:7860"}, | ||
| valid: []string{"http://localhost:7860"}, | ||
| }, | ||
| { | ||
| name: "disallowed scheme", | ||
| settings: map[string]interface{}{"csp_x": "ftp://example.com"}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "not a url", | ||
| settings: map[string]interface{}{"csp_x": "not a url"}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "non string value", | ||
| settings: map[string]interface{}{"csp_x": 123}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "wildcard host", | ||
| settings: map[string]interface{}{"csp_x": "http://*:7860"}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "csp directive breakout via semicolon in path", | ||
| settings: map[string]interface{}{"csp_x": "https://evil.com/; script-src 'none'"}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "whitespace in path", | ||
| settings: map[string]interface{}{"csp_x": "https://evil.com/a b"}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "degenerate host port only", | ||
| settings: map[string]interface{}{"csp_x": "https://:7860"}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "userinfo in url", | ||
| settings: map[string]interface{}{"csp_x": "https://user@attacker.com"}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "empty string", | ||
| settings: map[string]interface{}{"csp_x": ""}, | ||
| skippedKeys: []string{"csp_x"}, | ||
| }, | ||
| { | ||
| name: "mixed valid and invalid", | ||
| settings: map[string]interface{}{"csp_a": "https://api.example.com", "csp_b": "javascript:alert(1)", "csp_c": "http://localhost:7860", "other": "ignored"}, | ||
| valid: []string{"https://api.example.com", "http://localhost:7860"}, | ||
| skippedKeys: []string{"csp_b"}, | ||
| }, | ||
| } { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| valid, skippedKeys := cspConnectSrcFromSettings(tt.settings) | ||
| assert.ElementsMatch(t, tt.valid, valid) | ||
| assert.ElementsMatch(t, tt.skippedKeys, skippedKeys) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are commas filtered here? Could that potentially break headers if someone was misconfigured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Added
,to the blocked character list inisValidConnectSrcURL. Commas are valid in URLs but could cause confusion if someone uses them as CSP list separators. No harm in rejecting them defensively.Resolved in the latest commit.