mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-10 09:14:24 +00:00
feat(viewer): add defense-in-depth security headers (#735)
* feat(viewer): add defense-in-depth security headers Wrap the local viewer with a middleware that sets a strict Content-Security-Policy (default-src 'self', no unsafe-inline) plus X-Content-Type-Options, X-Frame-Options, Referrer-Policy, and Permissions-Policy on every response. HSTS is intentionally omitted since the viewer serves plain HTTP on loopback. To keep the CSP strict without an 'unsafe-inline' relaxation, the formerly-inline session script is externalized to static/session.js (it uses no template variables). Update the assurance case with a CWE-79 countermeasure row documenting these headers. Adds tests covering header presence, HSTS omission, and CSP strictness. * fix: Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This commit is contained in:
parent
927b710df3
commit
d1008b8f3b
6 changed files with 120 additions and 31 deletions
|
|
@ -88,6 +88,7 @@ The following maps [OWASP Top 10](https://owasp.org/www-project-top-ten/) and [C
|
|||
| Weakness | Applicability | Countermeasure |
|
||||
|----------|---------------|----------------|
|
||||
| **A03:2021 Injection** (CWE-78 OS Command Injection) | All `exec.Command` calls use `git` with explicit argument lists — no shell interpolation. `--end-of-options` prevents flag injection. | Mitigated |
|
||||
| **A03:2021 Injection** (CWE-79 Cross-site Scripting) | Viewer template output is HTML-escaped by `html/template`. Defense-in-depth: every viewer response carries a strict Content-Security-Policy (`default-src 'self'`, no `unsafe-inline`) plus `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `Referrer-Policy`, and `Permissions-Policy` (`internal/viewer/securityheaders.go`). | Mitigated |
|
||||
| **A01:2021 Broken Access Control** (CWE-22 Path Traversal) | Agent file-read tool validates paths with `pathutil.WithinBase()` before and after symlink resolution (`internal/tool/filereader.go:91-112`). | Mitigated |
|
||||
| **A02:2021 Cryptographic Failures** | All API communication uses HTTPS/TLS 1.2+. Go's default TLS configuration is used without weakening. `InsecureSkipVerify` is never set. | Mitigated |
|
||||
| **A07:2021 Auth Failures** (CWE-798 Hard-coded Credentials) | API keys are read exclusively from environment variables, never embedded in code or config files, never logged. | Mitigated |
|
||||
|
|
|
|||
35
internal/viewer/securityheaders.go
Normal file
35
internal/viewer/securityheaders.go
Normal file
|
|
@ -0,0 +1,35 @@
|
|||
package viewer
|
||||
|
||||
import "net/http"
|
||||
|
||||
// contentSecurityPolicy locks the viewer down to first-party resources only.
|
||||
// The viewer loads no third-party scripts, styles, fonts, or frames, so a
|
||||
// strict same-origin policy holds without any 'unsafe-inline' relaxation
|
||||
// (the previously-inline session script now lives in static/session.js).
|
||||
// This mitigates injection of active content should any user- or LLM-supplied
|
||||
// value ever escape HTML escaping in a template.
|
||||
const contentSecurityPolicy = "default-src 'self'; " +
|
||||
"script-src 'self'; " +
|
||||
"style-src 'self'; " +
|
||||
"img-src 'self' data:; " +
|
||||
"object-src 'none'; " +
|
||||
"base-uri 'none'; " +
|
||||
"frame-ancestors 'none'; " +
|
||||
"form-action 'none'"
|
||||
|
||||
// securityHeaders wraps a handler and sets defense-in-depth response headers on
|
||||
// every reply. These harden the local viewer's browser-facing surface (the
|
||||
// session JSONL exposed here contains reviewed source code and the LLM's
|
||||
// analysis of it). HSTS is intentionally omitted: the viewer serves plain HTTP
|
||||
// on loopback, where HSTS is meaningless and would wrongly pin localhost.
|
||||
func securityHeaders(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
h := w.Header()
|
||||
h.Set("Content-Security-Policy", contentSecurityPolicy)
|
||||
h.Set("X-Content-Type-Options", "nosniff")
|
||||
h.Set("X-Frame-Options", "DENY")
|
||||
h.Set("Referrer-Policy", "no-referrer")
|
||||
h.Set("Permissions-Policy", "geolocation=(), camera=(), microphone=()")
|
||||
next.ServeHTTP(w, r)
|
||||
})
|
||||
}
|
||||
54
internal/viewer/securityheaders_test.go
Normal file
54
internal/viewer/securityheaders_test.go
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
package viewer
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestSecurityHeadersSetsAllHeaders(t *testing.T) {
|
||||
inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusOK)
|
||||
_, _ = w.Write([]byte("ok"))
|
||||
})
|
||||
|
||||
rec := httptest.NewRecorder()
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
securityHeaders(inner).ServeHTTP(rec, req)
|
||||
|
||||
want := map[string]string{
|
||||
"Content-Security-Policy": contentSecurityPolicy,
|
||||
"X-Content-Type-Options": "nosniff",
|
||||
"X-Frame-Options": "DENY",
|
||||
"Referrer-Policy": "no-referrer",
|
||||
"Permissions-Policy": "geolocation=(), camera=(), microphone=()",
|
||||
}
|
||||
for k, v := range want {
|
||||
if got := rec.Header().Get(k); got != v {
|
||||
t.Errorf("header %q = %q, want %q", k, got, v)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The viewer serves plain HTTP on loopback; HSTS would wrongly pin localhost.
|
||||
func TestSecurityHeadersOmitsHSTS(t *testing.T) {
|
||||
inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
|
||||
rec := httptest.NewRecorder()
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
securityHeaders(inner).ServeHTTP(rec, req)
|
||||
|
||||
if got := rec.Header().Get("Strict-Transport-Security"); got != "" {
|
||||
t.Errorf("HSTS should not be set on the loopback viewer, got %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
// The CSP must stay strict: no 'unsafe-inline'/'unsafe-eval' relaxation, since
|
||||
// the formerly-inline session script now lives in static/session.js.
|
||||
func TestContentSecurityPolicyIsStrict(t *testing.T) {
|
||||
for _, bad := range []string{"unsafe-inline", "unsafe-eval", "*"} {
|
||||
if strings.Contains(contentSecurityPolicy, bad) {
|
||||
t.Errorf("CSP unexpectedly contains %q: %s", bad, contentSecurityPolicy)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -11,7 +11,7 @@ import (
|
|||
"time"
|
||||
)
|
||||
|
||||
//go:embed templates/*.html static/style.css
|
||||
//go:embed templates/*.html static/style.css static/session.js
|
||||
var assets embed.FS
|
||||
|
||||
func StartServer(addr string) error {
|
||||
|
|
@ -54,9 +54,12 @@ func StartServer(addr string) error {
|
|||
allowed := resolveAllowedHostsFromEnv(addr)
|
||||
guarded := hostGuard(allowed, mux)
|
||||
|
||||
// Outermost layer: set defense-in-depth security headers on every response.
|
||||
handler := securityHeaders(guarded)
|
||||
|
||||
srv := &http.Server{
|
||||
Addr: addr,
|
||||
Handler: guarded,
|
||||
Handler: handler,
|
||||
}
|
||||
|
||||
fmt.Printf("\nOpen browser: http://%s\n", DisplayAddr(addr))
|
||||
|
|
|
|||
24
internal/viewer/static/session.js
Normal file
24
internal/viewer/static/session.js
Normal file
|
|
@ -0,0 +1,24 @@
|
|||
document.querySelectorAll('.response-text').forEach(function(el) {
|
||||
const text = el.textContent;
|
||||
const esc = function(s) {
|
||||
return s.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>')
|
||||
.replace(/"/g, '"').replace(/'/g, ''');
|
||||
};
|
||||
const codeBlocks = [];
|
||||
let html = esc(text);
|
||||
html = html.replace(/
|
||||
html = html
|
||||
.replace(/`([^`]+)`/g, '<code class="inline-code">$1</code>')
|
||||
.replace(/\*\*(.+?)\*\*/g, '<strong>$1</strong>')
|
||||
.replace(/^### (.+)$/gm, '<div class="md-h3">$1</div>')
|
||||
.replace(/^## (.+)$/gm, '<div class="md-h2">$1</div>')
|
||||
.replace(/^# (.+)$/gm, '<div class="md-h1">$1</div>')
|
||||
.replace(/^[-*] (.+)$/gm, '<div class="md-li">• $1</div>')
|
||||
.replace(/\n{2,}/g, '<br><br>')
|
||||
.replace(/\n/g, '<br>');
|
||||
codeBlocks.forEach(function(code, i) {
|
||||
html = html.replace('%%CODEBLOCK_' + i + '%%',
|
||||
'<pre class="code-block"><code>' + code + '</code></pre>');
|
||||
});
|
||||
el.innerHTML = html;
|
||||
});
|
||||
|
|
@ -231,35 +231,7 @@
|
|||
{{end}}
|
||||
</div>
|
||||
|
||||
<script>
|
||||
document.querySelectorAll('.response-text').forEach(function(el) {
|
||||
var text = el.textContent;
|
||||
var esc = function(s) {
|
||||
return s.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>')
|
||||
.replace(/"/g, '"').replace(/'/g, ''');
|
||||
};
|
||||
var codeBlocks = [];
|
||||
var html = esc(text);
|
||||
html = html.replace(/```(\w*)\n([\s\S]*?)```/g, function(_, lang, code) {
|
||||
codeBlocks.push(code.replace(/^\n|\n$/g, ''));
|
||||
return '%%CODEBLOCK_' + (codeBlocks.length - 1) + '%%';
|
||||
});
|
||||
html = html
|
||||
.replace(/`([^`]+)`/g, '<code class="inline-code">$1</code>')
|
||||
.replace(/\*\*(.+?)\*\*/g, '<strong>$1</strong>')
|
||||
.replace(/^### (.+)$/gm, '<div class="md-h3">$1</div>')
|
||||
.replace(/^## (.+)$/gm, '<div class="md-h2">$1</div>')
|
||||
.replace(/^# (.+)$/gm, '<div class="md-h1">$1</div>')
|
||||
.replace(/^[-*] (.+)$/gm, '<div class="md-li">• $1</div>')
|
||||
.replace(/\n{2,}/g, '<br><br>')
|
||||
.replace(/\n/g, '<br>');
|
||||
codeBlocks.forEach(function(code, i) {
|
||||
html = html.replace('%%CODEBLOCK_' + i + '%%',
|
||||
'<pre class="code-block"><code>' + code + '</code></pre>');
|
||||
});
|
||||
el.innerHTML = html;
|
||||
});
|
||||
</script>
|
||||
<script src="/static/session.js"></script>
|
||||
</main>
|
||||
</body>
|
||||
</html>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue