OCPBUGS-88729: Fix HSTS annotation regex to reject newlines and accept trailing semicolons

[Thealisyed]

Summary

• Replace \s* with [ \t]* in the HSTS regex pattern to reject newline characters (\n, \r, \f) that break HAProxy config on reload and can cause cluster-wide router unavailability
• Allow an optional trailing semicolon in HSTS annotation values, which were previously silently rejected causing the Strict-Transport-Security header to not be set
• Add 6 new unit tests covering newline rejection (3 cases) and trailing semicolon acceptance (3 cases)

The Issue

The HSTS regex in haproxy-config.template:31 used \s* to match whitespace between directives. In Go, \s matches [\t\n\f\r ] — including newlines. An annotation like max-age=31536000\n;\npreload would pass the regex, render literal newlines into the HAProxy config, and cause HAProxy to reject the entire config on reload — taking down all routes cluster-wide.

Solution

Two changes to the same regex:

1. \s* → [ \t]* — restricts whitespace matching to horizontal whitespace only (spaces and tabs)
2. Append (?:[ \t]*;)? — allows an optional trailing semicolon, fixing a silent failure that has existed since 2018 (openshift/origin#19860)

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of HSTS header values with extra whitespace, tabs, newlines, and optional trailing semicolons.
  • More valid header formats are now accepted, while malformed values are correctly ignored.

• Tests

  • Added coverage for additional HSTS header formatting edge cases to verify expected routing behavior and rendered config output.

[openshift-ci-robot]

@Thealisyed: This pull request references Jira Issue OCPBUGS-88729, which is invalid:

• expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Summary

• Replace \s* with [ \t]* in the HSTS regex pattern to reject newline characters (\n, \r, \f) that break HAProxy config on reload and can cause cluster-wide router unavailability
• Allow an optional trailing semicolon in HSTS annotation values, which were previously silently rejected causing the Strict-Transport-Security header to not be set
• Add 6 new unit tests covering newline rejection (3 cases) and trailing semicolon acceptance (3 cases)

Bug

The HSTS regex in haproxy-config.template:31 used \s* to match whitespace between directives. In Go, \s matches [\t\n\f\r ] — including newlines. An annotation like max-age=31536000\n;\npreload would pass the regex, render literal newlines into the HAProxy config, and cause HAProxy to reject the entire config on reload — taking down all routes cluster-wide.

Fix

Two changes to the same regex:

1. \s* → [ \t]* — restricts whitespace matching to horizontal whitespace only (spaces and tabs)
2. Append (?:[ \t]*;)? — allows an optional trailing semicolon, fixing a silent failure that has existed since 2018 (openshift/origin#19860)

Test plan

• New unit tests for newline rejection (\n, \r\n, \r)
• New unit tests for trailing semicolon acceptance
• New unit test for tab whitespace (regression)
• All existing HSTS tests continue to pass
• CGO_ENABLED=1 go test -race -run TestConfigTemplate ./pkg/router/ passes
• E2E: Create edge route with HSTS annotation containing newlines → verify router remains healthy
• E2E: Create edge route with trailing semicolon HSTS annotation → verify Strict-Transport-Security header is set

🤖 Assisted with Claude Code

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

The $hstsPattern regex in haproxy-config.template is updated to use a printf-constructed pattern that permits flexible whitespace and optional trailing semicolons. Corresponding tests are added to TestConfigTemplate covering newline, CRLF, carriage return (rejected), trailing semicolon, and tab-whitespace (accepted) HSTS header variants.

Changes

HSTS Header Pattern Fix and Tests

Layer / File(s)	Summary
HSTS regex update and edge-case test coverage images/router/haproxy/conf/haproxy-config.template, pkg/router/router_test.go	$hstsPattern is changed to a printf-constructed pattern tolerating optional trailing semicolons and flexible spacing. Test cases are added for newline, CRLF, and carriage return variants (all notFound: true) and for trailing-semicolon and tab-whitespace variants (matched against expected set-header Strict-Transport-Security strings).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• grzpiotrowski
• ironcladlou

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
Ote Binary Stdout Contract	❌ Error	pkg/router/router_test.go's TestMain writes to stdout with fmt.Println/fmt.Printf before tests start, violating the OTE JSON stdout contract.	Send those messages to os.Stderr (or remove them) and ensure klog/log output is directed to stderr before any startup logging in TestMain.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main change: tightening the HSTS regex to reject newlines and allow trailing semicolons.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The added test titles are static map keys like 'HSTS with newline' and are passed unchanged to t.Run; no dynamic or generated values appear in titles.
Test Structure And Quality	✅ Passed	The new HSTS cases are isolated table-driven subtests; cleanup is deferred via cleanUpRoutes, and no cluster waits or missing timeouts were introduced.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the HSTS coverage is plain Go unit tests using only Route APIs, which MicroShift supports.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The added HSTS cases are Go unit tests in TestConfigTemplate using fake clients; no Ginkgo/e2e or multi-node/SNO assumptions found.
Topology-Aware Scheduling Compatibility	✅ Passed	Only the HSTS regex and unit tests changed; no manifests, controllers, or scheduling constraints were added.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	Only unit tests in pkg/router/router_test.go changed; no Ginkgo e2e tests or external connectivity assumptions were added.
No-Weak-Crypto	✅ Passed	The PR only changes the HSTS regex and tests; no new crypto APIs, weak algorithms, or secret comparisons are introduced.
Container-Privileges	✅ Passed	Diff only changes the HSTS template and unit tests; no privileged/hostPID/hostNetwork/hostIPC/SYS_ADMIN/allowPrivilegeEscalation changes appear.
No-Sensitive-Data-In-Logs	✅ Passed	Diff only changes HSTS regex and unit tests; no new logging or sensitive fields are added, and the added test strings are HSTS values, not secrets.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign alebedev87 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@Thealisyed: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-agnostic	174ae48	link	true	/test e2e-agnostic
ci/prow/e2e-aws-fips	174ae48	link	true	/test e2e-aws-fips
ci/prow/e2e-aws-serial-2of2	174ae48	link	true	/test e2e-aws-serial-2of2

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.