mirror of
https://github.com/renorris/openfsd
synced 2026-08-13 13:05:41 +08:00
fix: address review feedback for airport-editor shell
Parse form before CSRF so oversized bodies get size flash not 403; add body-size and POST authz PE tests; slog.Debug; AIR attachment assert.
This commit is contained in:
@@ -70,13 +70,12 @@ func (s *Server) handleFrontendAirportEditorDownloadAIR(c *gin.Context) {
|
||||
// handleAirportEditorDownload is the shared pure-echo download path.
|
||||
// bodyField is apt_text or air_text; defaultName/requiredExt define disposition.
|
||||
func (s *Server) handleAirportEditorDownload(c *gin.Context, bodyField, defaultName, requiredExt string) {
|
||||
// Cap before CSRF form parse so oversized bodies fail closed early.
|
||||
// Cap before form parse so oversized bodies fail closed early.
|
||||
// Parse form first (not validateCSRF-first) so a pure oversize wire body
|
||||
// surfaces as the size flash rather than a misleading 403: validateCSRF
|
||||
// would call PostForm, hit MaxBytesReader, and look like a CSRF miss.
|
||||
// Form contents are not trusted until CSRF succeeds below.
|
||||
c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, airportEditorWebMaxBody+4096)
|
||||
if !s.validateCSRF(c) {
|
||||
c.AbortWithStatus(http.StatusForbidden)
|
||||
return
|
||||
}
|
||||
|
||||
if err := c.Request.ParseForm(); err != nil {
|
||||
if isRequestTooLarge(err) {
|
||||
s.redirectAirportEditorFlash(c, "err", "Payload too large (max 2 MiB)")
|
||||
@@ -85,6 +84,10 @@ func (s *Server) handleAirportEditorDownload(c *gin.Context, bodyField, defaultN
|
||||
s.redirectAirportEditorFlash(c, "err", "Invalid form")
|
||||
return
|
||||
}
|
||||
if !s.validateCSRF(c) {
|
||||
c.AbortWithStatus(http.StatusForbidden)
|
||||
return
|
||||
}
|
||||
|
||||
text := c.PostForm(bodyField)
|
||||
if len(text) > airportEditorWebMaxBody {
|
||||
@@ -99,12 +102,12 @@ func (s *Server) handleAirportEditorDownload(c *gin.Context, bodyField, defaultN
|
||||
|
||||
filename := safeAirportEditorFilename(c.PostForm("filename"), defaultName, requiredExt)
|
||||
|
||||
// slog allowlist only — never log body text.
|
||||
// slog allowlist only — never log body text. Debug per design observability table.
|
||||
cid := 0
|
||||
if claims := getJwtContext(c); claims != nil {
|
||||
cid = claims.CID
|
||||
}
|
||||
slog.Info("airport-editor echo-download",
|
||||
slog.Debug("airport-editor echo-download",
|
||||
"cid", cid,
|
||||
"path", c.Request.URL.Path,
|
||||
"content_length", len(text),
|
||||
|
||||
@@ -123,6 +123,51 @@ func TestAirportEditorDownloadRequiresCSRF(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestAirportEditorDownloadUnauthRedirect(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
form := url.Values{}
|
||||
form.Set("apt_text", "icao=KBTV\n")
|
||||
form.Set("filename", "test.apt")
|
||||
form.Set("csrf_token", "not-a-real-token")
|
||||
req := httptest.NewRequest(http.MethodPost, "/airport-editor/download-apt", strings.NewReader(form.Encode()))
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
w := httptest.NewRecorder()
|
||||
ts.engine.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusSeeOther {
|
||||
t.Fatalf("status %d want 303", w.Code)
|
||||
}
|
||||
if loc := w.Header().Get("Location"); loc != "/login" {
|
||||
t.Fatalf("Location=%q want /login", loc)
|
||||
}
|
||||
if cd := w.Header().Get("Content-Disposition"); strings.Contains(cd, "attachment") {
|
||||
t.Fatalf("unauth must not get attachment, Content-Disposition=%q", cd)
|
||||
}
|
||||
}
|
||||
|
||||
func TestAirportEditorDownloadObserverRedirect(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
obs := createTestUser(t, ts, "pw", int(protocol.NetworkRatingObserver))
|
||||
cookies := formLogin(t, ts, obs.CID, "pw")
|
||||
|
||||
form := url.Values{}
|
||||
form.Set("apt_text", "icao=KBTV\n")
|
||||
form.Set("filename", "test.apt")
|
||||
// formPOST injects CSRF when present; observer should still be rating-gated.
|
||||
w, _ := formPOST(t, ts, "/airport-editor/download-apt", form, cookies)
|
||||
if w.Code != http.StatusSeeOther {
|
||||
t.Fatalf("status %d want 303", w.Code)
|
||||
}
|
||||
if loc := w.Header().Get("Location"); loc != "/dashboard" {
|
||||
t.Fatalf("Location=%q want /dashboard", loc)
|
||||
}
|
||||
if cd := w.Header().Get("Content-Disposition"); strings.Contains(cd, "attachment") {
|
||||
t.Fatalf("observer must not get attachment, Content-Disposition=%q", cd)
|
||||
}
|
||||
if strings.Contains(w.Body.String(), "icao=KBTV") {
|
||||
t.Fatal("observer must not receive echo body")
|
||||
}
|
||||
}
|
||||
|
||||
func TestAirportEditorDownloadAPTContentDisposition(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
admin := createTestUser(t, ts, "pw", int(protocol.NetworkRatingAdministator))
|
||||
@@ -172,7 +217,7 @@ func TestAirportEditorDownloadAIRContentDisposition(t *testing.T) {
|
||||
t.Fatalf("status %d want 200 body %s", w.Code, w.Body.String())
|
||||
}
|
||||
cd := w.Header().Get("Content-Disposition")
|
||||
if !strings.Contains(cd, `filename="scenario.air"`) {
|
||||
if !strings.Contains(cd, `attachment`) || !strings.Contains(cd, `filename="scenario.air"`) {
|
||||
t.Fatalf("Content-Disposition=%q", cd)
|
||||
}
|
||||
if got := w.Body.String(); got != payload {
|
||||
@@ -181,6 +226,65 @@ func TestAirportEditorDownloadAIRContentDisposition(t *testing.T) {
|
||||
assertWorkdirUnchanged(t, before, after)
|
||||
}
|
||||
|
||||
func TestAirportEditorDownloadOversizedBody(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
admin := createTestUser(t, ts, "pw", int(protocol.NetworkRatingAdministator))
|
||||
cookies := formLogin(t, ts, admin.CID, "pw")
|
||||
|
||||
// Wire body exceeds MaxBytesReader (2 MiB + 4 KiB slack). Fail closed with size flash.
|
||||
oversized := strings.Repeat("A", airportEditorWebMaxBody+8192)
|
||||
form := url.Values{}
|
||||
form.Set("apt_text", oversized)
|
||||
form.Set("filename", "big.apt")
|
||||
|
||||
before := workdirSnapshot(t)
|
||||
w, cookies := formPOST(t, ts, "/airport-editor/download-apt", form, cookies)
|
||||
after := workdirSnapshot(t)
|
||||
|
||||
if w.Code != http.StatusSeeOther {
|
||||
t.Fatalf("status %d want 303 (size flash), body=%s", w.Code, clip(w.Body.String(), 200))
|
||||
}
|
||||
loc := w.Header().Get("Location")
|
||||
if !strings.Contains(loc, "flash=err") {
|
||||
t.Fatalf("Location=%q want flash=err", loc)
|
||||
}
|
||||
if cd := w.Header().Get("Content-Disposition"); strings.Contains(cd, "attachment") {
|
||||
t.Fatalf("oversized must not attach, Content-Disposition=%q", cd)
|
||||
}
|
||||
w, _ = authedGET(t, ts, loc, cookies)
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("follow flash status %d", w.Code)
|
||||
}
|
||||
if !strings.Contains(w.Body.String(), "Payload too large") {
|
||||
t.Fatalf("expected size flash, body=%s", clip(w.Body.String(), 500))
|
||||
}
|
||||
assertWorkdirUnchanged(t, before, after)
|
||||
}
|
||||
|
||||
func TestAirportEditorDownloadLargeButUnderLimit(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
admin := createTestUser(t, ts, "pw", int(protocol.NetworkRatingAdministator))
|
||||
cookies := formLogin(t, ts, admin.CID, "pw")
|
||||
|
||||
// Comfortably under 2 MiB after form encoding; exercises large-payload success path.
|
||||
payload := strings.Repeat("icao=TEST\n", 32*1024) // ~288 KiB
|
||||
form := url.Values{}
|
||||
form.Set("apt_text", payload)
|
||||
form.Set("filename", "large.apt")
|
||||
|
||||
w, _ := formPOST(t, ts, "/airport-editor/download-apt", form, cookies)
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("status %d want 200 body %s", w.Code, clip(w.Body.String(), 200))
|
||||
}
|
||||
cd := w.Header().Get("Content-Disposition")
|
||||
if !strings.Contains(cd, `attachment`) || !strings.Contains(cd, `filename="large.apt"`) {
|
||||
t.Fatalf("Content-Disposition=%q", cd)
|
||||
}
|
||||
if got := w.Body.String(); got != payload {
|
||||
t.Fatalf("body length mismatch: got %d want %d", len(got), len(payload))
|
||||
}
|
||||
}
|
||||
|
||||
func TestAirportEditorDownloadUnsafeFilenameFallsBack(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
admin := createTestUser(t, ts, "pw", int(protocol.NetworkRatingAdministator))
|
||||
|
||||
Reference in New Issue
Block a user