feat(ui): Enable CSP, HSTS, X-Frame-Options. Fixes #2760, #1376, #2761 #2971
Conversation
alexec
changed the title
server/static/static.go
Outdated
| @@ -26,6 +27,12 @@ func (s *FilesServer) ServerFiles(w http.ResponseWriter, r *http.Request) { | |||
| w = &responseRewriter{ResponseWriter: w, old: []byte(`<base href="/">`), new: []byte(fmt.Sprintf(`<base href="%s">`, s.baseHRef))} | |||
| } | |||
|
|
|||
| w.Header().Set("X-Frame-Options", "DENY") | |||
| w.Header().Set("Content-Security-Policy", "default-src 'self' 'unsafe-inline'") | |||
| if s.secure { | |||
There was a problem hiding this comment.
IMHO, HSTS should be set from the final endpoint to the client (i.e. the LB or Ingress), because of several reasons. For one, once set, the browser will not access the URL on http endpoint anymore but will restrict to using https connection. Might lead to edge-case problems, i.e. Client -> (http) -> LB and/or Ingress -> (https) -> Argo)
But the more obvious reason might be, that the connection is Client -> (https) -> LB and/or Ingress -> (http) -> Argo). In this case, the HSTS header would not be set (because Argo server is serving plain HTTP), and then this might have no effect at all.
I think a better solution would be to document that users should set HSTS header on their Load Balancer or Ingress, if they want it.
There was a problem hiding this comment.
good point - and you can front this with HTTP LB, in which case you would never want HSTS
There was a problem hiding this comment.
I note your second case cannot apply - if we serve on HTTP - this flag won't be added - but I think we should make it optional - default on.
There was a problem hiding this comment.
ok, so new logic is this - --hsts is on by default - but only effected if --secure
|
Kudos, SonarCloud Quality Gate passed!
|
You should hope I have already tested it! Which I have. |
Checklist:
"fix(controller): Updates such and such. Fixes #1234".Fixes #2760
Fixes #1376
Fixes #2761