rproxy: read the rest of a Connection: close visitor's body after an early answer (closes #373)

Go's server reads the body left unread once the handler returns, but not
for a request with Connection: close: it closes the connection with the
rest unread, and the visitor, still sending, gets a reset. rproxy now
reads that rest itself once it has answered, up to the 256 KiB Go's
server reads, until the deadline already set on the connection.

Model: opus-5-5

SPEC.md

@@ -1800,11 +1800,20 @@ Code injection skips these rules:
 - 921130, 941100, 941160 and 941270, which refused `<meta>` and `<link>`
   elements.
 
-`<script>alert(1)</script>` and `javascript:alert(1)` are still refused in the
-articles and code injection (941110, 941170 and 941210). Path traversal and SQL
-injection are still refused in every field above. Script injection is too,
-outside the articles. `; wget http://example.com/x.sh` is still refused in the
-query fields, the plain text and the addresses.
+A `<script>` element skips 941110 in posts, pages and code injection, so an HTML
+card can embed a widget and code injection can load analytics. These are the
+fields, on the Admin API:
+
+- `lexical`, `mobiledoc` and `html` of posts and pages;
+- `codeinjection_head` and `codeinjection_foot` of posts and pages;
+- `value` of settings.
+
+`<script>alert(1)</script>` is still refused in snippets and newsletters.
+`javascript:alert(1)` is still refused in the articles and code injection
+(941170 and 941210). Path traversal and SQL injection are still refused in every
+field above. Script injection is too, outside the articles and code injection.
+`; wget http://example.com/x.sh` is still refused in the query fields, the plain
+text and the addresses.
 
 Coraza names a staff user's `profile_image` field `json.users.0.profile_image`.
 That name holds `.profile`, a file on the list of system files, so the name

internal/coreruleset/ghost.conf

@@ -153,8 +153,8 @@ SecRule REQUEST_FILENAME "@rx ^/ghost/api/admin/users/[0-9a-f]{24}/$" \
 #   snippets.<n>.lexical and snippets.<n>.mobiledoc, a part of a post saved for
 #   reuse
 #   newsletters.<n>.footer_content, the footer of a newsletter
-# A <script> element (941110) and a javascript: link (941170, 941210) are still
-# refused in them.
+# A javascript: link (941170, 941210) is still refused in them. A <script>
+# element (941110) passes only in posts and pages, by 900366.
 SecRule REQUEST_FILENAME "@beginsWith /ghost/api/admin/" \
     "id:900355,phase:1,pass,nolog,chain"
     SecRule &ARGS_GET:/^json\.(?:(?:posts|pages)\.\d+\.(?:lexical|mobiledoc|html)|snippets\.\d+\.(?:lexical|mobiledoc)|newsletters\.\d+\.footer_content)$/ "@eq 0" \
@@ -184,7 +184,8 @@ SecRule REQUEST_FILENAME "@beginsWith /ghost/api/admin/" \
 # These fields hold it, and skip the rules anywhere on the Admin API:
 #   posts.<n>.codeinjection_head and _foot, and the same fields of pages
 #   settings.<n>.value, the value of a setting
-# A <script> element and a javascript: link are still refused in them.
+# A javascript: link is still refused in them, and a <script> element passes
+# by 900366.
 SecRule REQUEST_FILENAME "@beginsWith /ghost/api/admin/" \
     "id:900356,phase:1,pass,nolog,chain"
     SecRule &ARGS_GET:/^json\.(?:(?:posts|pages)\.\d+\.codeinjection_(?:head|foot)|settings\.\d+\.value)$/ "@eq 0" \
@@ -200,6 +201,21 @@ SecRule REQUEST_FILENAME "@beginsWith /ghost/api/admin/" \
         ctl:ruleRemoveTargetById=941160;ARGS:/^json\.(?:(?:posts|pages)\.\d+\.codeinjection_(?:head|foot)|settings\.\d+\.value)$/,\
         ctl:ruleRemoveTargetById=941270;ARGS:/^json\.(?:(?:posts|pages)\.\d+\.codeinjection_(?:head|foot)|settings\.\d+\.value)$/"
 
+# A <script> element skips 941110 in posts, pages and code injection, on the
+# Admin API, which refused:
+#   an HTML card that embeds a widget with its script
+#   code injection that loads analytics,
+#   <script defer src="https://analytics.example.com/script.js"></script>
+# These fields take a <script> element on purpose:
+#   posts.<n>.lexical, mobiledoc and html, and the same fields of pages
+#   posts.<n>.codeinjection_head and _foot, and the same fields of pages
+#   settings.<n>.value, the value of a setting
+# Snippets and a newsletter's footer still refuse it.
+SecRule REQUEST_FILENAME "@beginsWith /ghost/api/admin/" \
+    "id:900366,phase:1,pass,nolog,chain"
+    SecRule &ARGS_GET:/^json\.(?:(?:posts|pages)\.\d+\.(?:lexical|mobiledoc|html|codeinjection_(?:head|foot))|settings\.\d+\.value)$/ "@eq 0" \
+        "ctl:ruleRemoveTargetById=941110;ARGS:/^json\.(?:(?:posts|pages)\.\d+\.(?:lexical|mobiledoc|html|codeinjection_(?:head|foot))|settings\.\d+\.value)$/"
+
 # Saving a staff user's profile sends the field profile_image, whose name holds
 # .profile, a file the list of system files names (930120).
 SecRule REQUEST_FILENAME "@rx ^/ghost/api/admin/users/[0-9a-f]{24}/$" \

internal/coreruleset/ghost_test.go

@@ -143,10 +143,11 @@ var ghostAddressFields = []ghostField{
 	{http.MethodPut, ghostUser, `{"users":[{"email":"VALUE"}]}`},
 }
 
-// ghostArticleFields hold an article and skip the rules of 900355.
+// ghostPostFields hold a post or a page, skip the rules of 900355, and take a
+// <script> element by 900366.
 //
 //nolint:gochecknoglobals // a constant cannot be a list
-var ghostArticleFields = []ghostField{
+var ghostPostFields = []ghostField{
 	{http.MethodPut, ghostPost, `{"posts":[{"lexical":"VALUE"}]}`},
 	{http.MethodPut, ghostPage, `{"pages":[{"lexical":"VALUE"}]}`},
 	{
@@ -155,6 +156,13 @@ var ghostArticleFields = []ghostField{
 		`{"posts":[{"html":"VALUE"}]}`,
 	},
 	{http.MethodPost, ghostPosts, `{"posts":[{"mobiledoc":"VALUE"}]}`},
+}
+
+// ghostSnippetAndNewsletterFooterFields hold a snippet or a newsletter's
+// footer, skip the rules of 900355, and still refuse a <script> element.
+//
+//nolint:gochecknoglobals // a constant cannot be a list
+var ghostSnippetAndNewsletterFooterFields = []ghostField{
 	{
 		http.MethodPost,
 		"/ghost/api/admin/snippets/",
@@ -167,8 +175,16 @@ var ghostArticleFields = []ghostField{
 	},
 }
 
-// ghostCodeFields hold code injection or a setting and skip the rules of
-// 900356.
+// ghostArticleFields hold an article and skip the rules of 900355.
+//
+//nolint:gochecknoglobals // a constant cannot be a list
+var ghostArticleFields = slices.Concat(
+	ghostPostFields,
+	ghostSnippetAndNewsletterFooterFields,
+)
+
+// ghostCodeFields hold code injection or a setting, skip the rules of 900356,
+// and take a <script> element by 900366.
 //
 //nolint:gochecknoglobals // a constant cannot be a list
 var ghostCodeFields = []ghostField{
@@ -335,11 +351,41 @@ func TestGhostOrdinaryValuesPassInTheExcludedFields(t *testing.T) {
 		url.QueryEscape("label:[powershell-users]+name:~'ls -la'"), false)
 }
 
-// ghostAttacks are an SQL injection, a path traversal and a script
-// injection, not escaped.
+// ghostWidget is an HTML card that embeds a widget with its script.
+const ghostWidget = `<div id="signup"></div>` +
+	`<script src="https://widgets.example.com/form.js" async></script>`
+
+// ghostWidgetCard is ghostWidget as the editor saves it in lexical.
+const ghostWidgetCard = `{"root":{"children":[{"type":"html","version":1,` +
+	`"html":"<div id=\"signup\"></div><script ` +
+	`src=\"https://widgets.example.com/form.js\" async></script>"}],` +
+	`"direction":null,"format":"","indent":0,"type":"root","version":1}}`
+
+// ghostAnalytics is code injection that loads analytics.
+const ghostAnalytics = `<script defer ` +
+	`src="https://analytics.example.com/script.js"></script>`
+
+func TestGhostScriptsPassInPostsAndCodeInjection(t *testing.T) {
+	t.Parallel()
+
+	// An HTML card in a post, as the editor saves it, and as integrations
+	// send it.
+	wantGhostField(t, ghostPostFields[0], ghostWidgetCard, false)
+
+	for _, f := range ghostPostFields {
+		wantGhostField(t, f, ghostWidget, false)
+	}
+
+	// Site code injection, saved in settings, and a post's or a page's own.
+	for _, f := range ghostCodeFields {
+		wantGhostField(t, f, ghostAnalytics, false)
+	}
+}
+
+// ghostAttacks are an SQL injection and a path traversal, not escaped.
 //
 //nolint:gochecknoglobals // a constant cannot be a list
-var ghostAttacks = []string{injection, pathTraversal, scriptTag}
+var ghostAttacks = []string{injection, pathTraversal}
 
 // commandAfterASeparator is a shell command after a ";", not escaped.
 const commandAfterASeparator = "; wget http://example.com/x.sh"
@@ -358,6 +404,17 @@ func TestGhostAttacksAreRefusedInTheExcludedFields(t *testing.T) {
 		}
 	}
 
+	// A script injection stays refused in every field that does not take a
+	// <script> element.
+	for _, f := range slices.Concat(ghostTextFields, ghostAddressFields,
+		ghostSnippetAndNewsletterFooterFields) {
+		wantGhostField(t, f, scriptTag, true)
+	}
+
+	for _, target := range ghostQueryFields {
+		wantGhostQuery(t, target+url.QueryEscape(scriptTag), true)
+	}
+
 	// A command after a separator stays refused wherever its rules do not
 	// misfire: in the query fields, the plain text and the addresses.
 	for _, f := range slices.Concat(ghostTextFields, ghostAddressFields) {
@@ -372,13 +429,17 @@ func TestGhostAttacksAreRefusedInTheExcludedFields(t *testing.T) {
 		)
 	}
 
-	// The articles and code injection carry HTML on purpose, but a script
-	// element and a javascript: link are still refused there.
+	// The articles and code injection carry HTML on purpose, but a
+	// javascript: link is still refused there, and a script element in
+	// snippets and a newsletter's footer.
 	for _, f := range slices.Concat(ghostArticleFields, ghostCodeFields) {
-		wantGhostField(t, f, `<p>Hi</p><script src="/x.js"></script>`, true)
 		wantGhostField(t, f, `<a href="javascript:alert(1)">click</a>`, true)
 	}
 
+	for _, f := range ghostSnippetAndNewsletterFooterFields {
+		wantGhostField(t, f, `<p>Hi</p><script src="/x.js"></script>`, true)
+	}
+
 	// An address keeps 932250, which refuses a listing of files.
 	for _, f := range ghostAddressFields {
 		wantGhostField(t, f, "ls -la /root", true)
@@ -459,6 +520,44 @@ func TestGhostExclusionsAreNarrow(t *testing.T) {
 	}
 }
 
+func TestGhostScriptExclusionIsNarrow(t *testing.T) {
+	t.Parallel()
+
+	crs := readingBodies(t)
+
+	// A <script> element keeps 941110 off the Admin API, in fields that do
+	// not take one, and when the query has a field of the same name. 941100
+	// and 941160 refuse each request too, so the test looks for 941110
+	// itself.
+	const (
+		script           = `<script defer src=\"/script.js\"></script>`
+		scriptInPost     = `{"posts":[{"lexical":"` + script + `"}]}`
+		scriptInSettings = `{"settings":[{"value":"` + script + `"}]}`
+	)
+
+	for _, refused := range []struct{ target, body string }{
+		{"/ghost/api/content/posts/", scriptInPost},
+		{ghostPosts + "?json.posts.0.lexical=x", scriptInPost},
+		{ghostPosts, `{"posts":[{"feature_image_caption":"` + script + `"}]}`},
+		{"/ghost/api/admin/users/me/", `{"users":[{"bio":"` + script + `"}]}`},
+		{"/api/settings/", scriptInSettings},
+		{ghostSettings + "?json.settings.0.value=x", scriptInSettings},
+	} {
+		r := request{http.MethodPut, refused.target, []string{jsonBody}}
+
+		got, _ := inspectBody(t, crs, r, refused.body)
+		if got.Score < ghostThreshold ||
+			!slices.Contains(got.RuleIDs, 941110) {
+			t.Errorf(
+				"PUT %s %s: %+v, want 941110 among them",
+				refused.target,
+				refused.body,
+				got,
+			)
+		}
+	}
+}
+
 func TestGhostQueryExclusionsAreNarrow(t *testing.T) {
 	t.Parallel()
 

internal/rproxy/limits_test.go

@@ -131,6 +131,67 @@ func TestADenyOutranksTheBodySizeLimit(t *testing.T) {
 	}
 }
 
+func TestAVisitorSendingConnectionCloseReadsAnAnswerBeforeTheRestOfItsBody(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name   string
+		waf    http.HandlerFunc
+		status int
+	}{
+		{"over the size limit", allow, http.StatusRequestEntityTooLarge},
+		{"denied", deny, http.StatusForbidden},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			standIn := startWAF(t, tc.waf)
+			app := startApp(t, func(http.ResponseWriter, *http.Request) {
+				t.Error("the app was asked")
+			})
+			addr, _ := startRProxy(t, standIn.URL, app.URL,
+				map[string]string{maxRequestSize: "1K"})
+
+			const size = 129 << 10
+
+			conn := dial(t, addr)
+			send(t, conn, post+"Connection: close\r\n"+
+				announcedLength(size)+
+				strings.Repeat("x", questionBodyMaxBytes))
+
+			reader := bufio.NewReader(conn)
+
+			res, err := http.ReadResponse(reader, nil)
+			if err != nil {
+				t.Fatalf("read the answer: %v", err)
+			}
+
+			_, _ = io.Copy(io.Discard, res.Body)
+			_ = res.Body.Close()
+
+			if res.StatusCode != tc.status {
+				t.Errorf("status %d, want %d", res.StatusCode, tc.status)
+			}
+
+			// The rest goes in small parts, so that a reset fails a later one.
+			for sent := questionBodyMaxBytes; sent < size; sent += 1 << 10 {
+				send(t, conn, strings.Repeat("x", 1<<10))
+			}
+
+			rest, err := io.ReadAll(reader)
+			if err != nil || len(rest) != 0 {
+				t.Errorf(
+					"after the answer, read %q and %v, want the end",
+					rest,
+					err,
+				)
+			}
+		})
+	}
+}
+
 func TestABodyNotAnnouncedOverItsLimitIsRefused(t *testing.T) {
 	t.Parallel()
 

internal/rproxy/request.go

@@ -38,6 +38,10 @@ const plainText = "text/plain; charset=utf-8"
 // and the visitor then gets a closed connection and no answer.
 const minSendTime = 10 * time.Second
 
+// maxBodyReadAfterAnswer is the most of a body left unread that Go's server
+// reads once the handler returns, for a visitor without Connection: close.
+const maxBodyReadAfterAnswer = 256 << 10
+
 // errResponseTooLate refuses an app's answer that arrives once
 // SWW_RPROXY_MAX_RESPONSE_TIME_MS has run out.
 var errResponseTooLate = errors.New(
@@ -338,8 +342,8 @@ func (rq *request) answer(status int) {
 	}
 
 	// A visitor found too slow is read no more; any other may go on sending until
-	// its time is up, so that Go's server can read the rest of the body and end
-	// the request cleanly.
+	// its time is up, so that the rest of the body is read and the request ends
+	// cleanly: by Go's server, or by readRestOfBody.
 	deadline := rq.requestDeadline()
 	if status == http.StatusRequestTimeout {
 		deadline = time.Now()
@@ -523,6 +527,22 @@ func (rq *request) stopReadingBody(deadline time.Time) {
 	_ = rq.rc.SetReadDeadline(deadline)
 }
 
+// readRestOfBody reads, once the visitor has been answered, the rest of a body
+// that has not arrived whole, when the visitor sent Connection: close. Go's
+// server reads it for any other visitor, but closes this visitor's connection
+// with the rest unread: the visitor, still sending, then gets a reset and may
+// never read the answer. The read ends at the deadline already set on the
+// connection, and after as much as Go's server reads.
+func (rq *request) readRestOfBody() {
+	if !rq.in.Close || rq.body == nil || rq.body.received.Load() ||
+		rq.out.status == 0 ||
+		rq.out.status == http.StatusSwitchingProtocols {
+		return
+	}
+
+	_, _ = io.CopyN(io.Discard, rq.in.Body, maxBodyReadAfterAnswer)
+}
+
 // stopPassingOnBody ends the transport's reading of a visitor's body that has
 // not arrived whole, and lets Go's server read the rest until deadline. Go's
 // server reads it once the handler returns, but with no deadline at all if

internal/rproxy/rproxy.go

@@ -169,6 +169,7 @@ func newWAFTransport() *http.Transport {
 // it writes the request's log line and sends its report.
 func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	rq := h.newRequest(w, r)
+	defer rq.readRestOfBody()
 	defer rq.finish(r.Context())
 
 	if r.Method == http.MethodGet && r.URL.Path == waf.HealthPath {