fix(ftwhttp): reject raw CR/LF characters in header values

[fzipi]

Summary

• Header.Write wrote header name/value pairs straight onto the wire with no validation. A header value containing a raw CR or LF — for example from an unquoted YAML block scalar (|), which silently appends a trailing newline — broke the request's header framing without producing any error.
• The broken framing caused the target to misinterpret the request (headers after the broken one are lost), while go-ftw reported the test as passing, since no protocol-level error occurred anywhere in the send path.
• Header.Write now rejects header names/values containing a raw CR or LF, pointing at encoded_request/encoded_data as the documented way to send control characters intentionally (that path bypasses Header.Write entirely and is unaffected).
• While adding an end-to-end regression test, found and fixed a related bug: Run()'s defer cleanLogs(logLines) was registered after the test loop, so any early-return error from inside the loop (which the new regression test is the first in the suite to trigger) skipped cleanup and leaked the log-file watcher. On Windows this held the log file open and broke the test's TempDir cleanup in CI.
• Also includes an unrelated, pre-existing go.mod/go.sum tidy (stale ftw-tests-schema/v3 entry) needed to satisfy the go-mod-tidy pre-commit hook, split into its own commit.

Fixes #662

Test plan

• Added TestWriteRejectsCRLFInValue / TestWriteRejectsCRLFInName in ftwhttp/header_test.go
• Added an end-to-end regression test (runner/testdata/TestHeaderCRLFInjectionRun.yaml + TestHeaderCRLFInjectionRun in runner/run_test.go) reproducing the exact header from the issue and asserting Run() now surfaces a clear error instead of trivially passing
• go test ./... passes
• golangci-lint run reports 0 issues

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Advanced

Run ID: eb93ddb6-44c8-481f-89e5-164ed26fdc3b

📥 Commits

Reviewing files that changed from the base of the PR and between 7f24117 and 8a3646f.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (5)

• ftwhttp/header.go
• ftwhttp/header_test.go
• go.mod
• runner/run_test.go
• runner/testdata/TestHeaderCRLFInjectionRun.yaml

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• coreruleset/coreruleset (manual)
• coreruleset/go-ftw (manual)
• coreruleset/crs-toolchain (manual)
• coreruleset/crs-linter (manual)
• coreruleset/documentation (manual)

💤 Files with no reviewable changes (1)

• go.mod

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Changes

Header.Write now rejects raw CR or LF characters in header names and values. Unit and runner tests cover the rejection, including YAML block-scalar input that adds a trailing newline.

CRLF Header Validation

Layer / File(s)	Summary
Header write validation ftwhttp/header.go, ftwhttp/header_test.go	Header.Write returns an error for raw CR or LF characters in header names and values. Unit tests cover both cases.
Runner regression coverage runner/run_test.go, runner/testdata/TestHeaderCRLFInjectionRun.yaml, go.mod	Runner coverage verifies that invalid YAML header input returns an error. The schema dependency changes from v3.0.1 to v2.3.0.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested labels: release:fix

Merge Risk: ⚪ Minimal · up to 8a364

The CR/LF validation change has no remaining supported merge-blocking risk in the reviewed request path.

🚥 Pre-merge checks | ✅ 16 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The go.mod change downgrades github.com/coreruleset/ftw-tests-schema from v3.0.1 to v2.3.0. The supplied issue and implementation concern invalid header input and silent request drops. The depende…	Remove the unrelated go.mod and corresponding dependency-tidy changes, or provide issue-linked evidence that this dependency change is required to implement or test #662.
Ai Contribution Disclosure	⚠️ Warning	The PR violates the disclosure policy. The supplied PR body has no ## ai disclosure section and also omits the required lowercase ## what, ## why, and ## refs sections. The reviewed commit mes…	Add a lowercase ## ai disclosure section with concrete **tools used** including the model name and version, **assisted with** describing the generated work, and **review performed** describing specific verification. Add the required…

✅ Passed checks (16 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #662 requires an explicit error when invalid header input causes a request to be dropped. Header.Write now rejects raw \\r and \\n in header names and values. Unit tests cover both affected …
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Regex Assembly Is The Source Of Truth	✅ Passed	Passed — not applicable. The pull request changes only ftwhttp, runner test data, go.mod, and go.sum. It does not modify an @rx pattern in rules/*.conf or any file under regex-assembly/.
Rule Change Requires Go-Ftw Test Coverage	✅ Passed	Passed: not applicable. The authoritative pull-request diff changes only ftwhttp/header.go, ftwhttp/header_test.go, go.mod, go.sum, runner/run_test.go, and runner/testdata/TestHeaderCRLFInjectionRun.y…
Redos Risk & Re2 Compatibility	✅ Passed	Passed: not applicable. The scoped diff changes only ftwhttp, runner tests/test data, and Go module files. It does not modify rules/.conf or regex-assembly/.ra, and it adds or modifies no regexp.Mus…
False Positive Risk & Existing Coverage	✅ Passed	Not applicable. The pull request changes Go HTTP header handling, tests, test data, and module metadata. It does not add or widen detection patterns or add rules under rules/*.conf, plugins/*.conf…
Crs Rule Metadata & Id Conventions	✅ Passed	Passed — not applicable. The review-scoped diff changes only Go files, go.mod/go.sum, and runner test data. It does not modify rules/.conf, plugins/.conf, or crs-setup.conf.example, and it adds or m…
Rule & Config Breaking Changes	✅ Passed	PASS — The authoritative diff changes ftwhttp.Header.Write validation, tests, and dependency metadata only. Header.Write(io.Writer) error keeps the same exported signature. The change does not rem…
Owasp Security (Web, Api & Llm)	✅ Passed	No OWASP failure condition is introduced. The changed Header.Write path rejects raw CR/LF in tuple.Name and tuple.Value before flushing or sending the request, which mitigates the header-injecti…
Unpinned Dependencies & Actions	✅ Passed	Go is applicable because this PR changes go.mod and go.sum. The only go.mod dependency change removes the stale exact entry github.com/coreruleset/ftw-tests-schema/v3 v3.0.1; the remaining require ent…
Secrets, Payloads & Pii In Logs	✅ Passed	No changed line logs a credential, token, request value, full request, response, or real-traffic PII. The new error includes only the quoted header name and static remediation text; it does not includ…
New Dependency Scrutiny	✅ Passed	PASS — The PR adds no dependency entry, Action, or Buildkite plugin. The authoritative diff only removes github.com/coreruleset/ftw-tests-schema/v3 v3.0.1 from go.mod and its go.sum record. `ftw…
Install & Build-Time Code Execution	✅ Passed	PASS — The PR changes only Go source/tests, YAML test data, and dependency metadata. The authoritative diff introduces no pipe-to-shell installer, remote binary fetch, Docker build fetch, Python build…
Renovate: Config Present And Valid	✅ Passed	PASS. The PR modifies root files, so the scan scope applies. No Renovate config file is changed. renovate.json exists at the PR head, and it is valid JSON with the exact required $schema and `gith…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: rejecting raw CR/LF characters in HTTP header values. It omits header names, but it remains specific and directly related to the primary fix.

Full details: Out of Scope Changes check

Explanation

The go.mod change downgrades github.com/coreruleset/ftw-tests-schema from v3.0.1 to v2.3.0. The supplied issue and implementation concern invalid header input and silent request drops. The dependency downgrade has no demonstrated connection to those objectives. The excluded go.sum prevents verification of the corresponding checksum change, but it does not establish scope relevance.

Full details: Ai Contribution Disclosure

Explanation

The PR violates the disclosure policy. The supplied PR body has no ## ai disclosure section and also omits the required lowercase ## what, ## why, and ## refs sections. The reviewed commit messages contain Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>, which the policy explicitly forbids. The diff is small and incremental, but it adds generated-looking explanatory comments and regression-test scaffolding, so the missing disclosure is not excused as a trivial change.

Resolution

Add a lowercase ## ai disclosure section with concrete **tools used** including the model name and version, **assisted with** describing the generated work, and **review performed** describing specific verification. Add the required lowercase ## what, ## why, and ## refs sections. Remove the Co-Authored-By trailer and any other AI-tool signature from every commit message and the PR body, then amend or recreate the commits.

• Fix all pre-merge checks with AI

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}