[scanner] fix: extend LogString to strip ANSI escapes, null bytes, and Unicode line separators#20857
Conversation
…and Unicode line separators (part of hive advisory)
pkg/sanitize LogString previously only replaced ASCII CR/LF with the visible
placeholder ⏎. Attackers could still inject forged log lines using:
- ANSI/VT100 terminal escape sequences (CSI, OSC) — cursor movement, color
resets, window-title overwrite (CWE-117)
- Unicode line/paragraph separators (U+2028, U+2029) — treated as newlines
by many log aggregators and JavaScript engines
- Null bytes (\x00) — truncate entries in C-based log pipelines
- Other C0 control characters (backspace, bell, DEL, etc.)
Changes:
- Add ansiEscapeRe regexp to strip CSI, OSC, and 2-char ESC sequences
- Add logC0Re regexp to strip remaining C0/DEL control characters
- Extend logInjectionReplacer with U+2028, U+2029, and null byte entries
- Update tests: flip "documents current broken behavior" assertions to
require neutralization; rename Preserves* tests to Strips*/Neutralizes*
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Copilot <copilot@github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
🐝 Hi @clubanderson! I'm Trusted users — org members and contributors with write access — can mention Automation may take a moment to start, and follow-up happens through workflow activity rather than chat replies. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
Bot
added
the
size/L
There was a problem hiding this comment.
Pull request overview
This PR hardens pkg/sanitize.LogString to better neutralize log-injection vectors (CWE-117) by stripping/normalizing additional control and escape sequences beyond ASCII CR/LF, and updates unit tests to enforce the stronger contract.
Changes:
- Extend log sanitization to replace Unicode line/paragraph separators (U+2028/U+2029) with
⏎and strip null bytes. - Strip ANSI/VT100 escape sequences and additional C0/DEL control characters prior to replacement.
- Update/rename log-sanitization tests from “documents current behavior” to “requires neutralization”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/sanitize/prompt.go | Extends LogString sanitization to strip terminal escapes/control chars and normalize additional newline-like separators. |
| pkg/sanitize/prompt_test.go | Tightens tests to assert the hardened LogString behavior against more injection vectors. |
| // - CSI sequences: ESC [ <params> <final-byte> | ||
| // - OSC sequences: ESC ] <text> BEL | ||
| // - Two-char Fe/Fs: ESC <byte in 0x40–0x5F> | ||
| var ansiEscapeRe = regexp.MustCompile(`\x1b(?:\[[0-?]*[ -/]*[@-~]|\][^\x07]*\x07|[@-_])`) |
| {"CSI clear screen", "normal\x1b[2Jtext", "normaltext"}, | ||
| {"CSI color change", "normal\x1b[31mred text\x1b[0m", "normalred text"}, | ||
| {"OSC title change", "normal\x1b]0;pwned\x07rest", "normalrest"}, | ||
| {"cursor movement", "data\x1b[5Aoverwrite", "dataoverwrite"}, |
clubanderson
left a comment
clubanderson
left a comment
There was a problem hiding this comment.
[quality] This directly implements the hardening I was tracking in advisory bead 46bb0e2d-78b (referenced across #20808, #20828, #20840). Nice to see it land.
Implementation looks correct:
ansiEscapeRe = \x1b(?:\[[0-?]*[ -/]*[@-~]|\][^\x07]*\x07|[@-_])covers CSI, BEL-terminated OSC, and two-char Fe/Fs sequences.logC0Re = [\x01-\x08\x0b\x0c\x0e-\x1a\x1c-\x1f\x7f]correctly excludes\x00(handled by replacer),\x09TAB (intentionally kept),\x0a/\x0d(handled),\x1bESC (handled by ansi regex). No overlap, no gaps in the C0 range.- Replacer additions for U+2028 / U+2029 → ⏎ and
\x00→ "" match the finding.
Tests properly flipped: the former TestLogString_*Passthrough / Preserves* cases that documented the gap are renamed to Neutralizes* / Strips* and now assert exact expected output (e.g. "normal\x1b[2Jtext" → "normaltext"). Coverage of surrounding-content preservation is retained.
Two small edge cases worth an optional follow-up (non-blocking):
-
OSC terminated by ST (
ESC \) instead of BEL — the OSC branch only matches BEL-terminated sequences. AnESC ]without BEL is caught by the two-char branch (since]= 0x5D ∈[@-_]), which strips theESC ]prefix but leaves the OSC payload text visible as plain characters. Not a security regression (the ESC is gone, so no terminal control), but the payload text passes through. If you want tighter coverage, add an ST-terminated OSC alt:\][^\x07]*(?:\x07|\x1b\\). -
DCS (
ESC P), APC (ESC _), PM (ESC ^) — same story. First byte after ESC is stripped by the two-char branch; payload text remains as plain characters. Again, no ESC = no terminal control, but tighter parsing would remove the whole sequence.
Both are minor and can be follow-ups. LGTM.
Bead will be closed once merged.
Filed by quality agent (ACMM L4/L6 — full mode)
…ions The logC0Re comment was incomplete: it did not document that \x00 (null byte) is also excluded because the logInjectionReplacer already handles it, and that \x09 (tab) is intentionally left through as safe. This clarifies the full set of excluded bytes so reviewers can verify the ranges are correct. All 19 sanitize package tests pass (TestLogString_Strips*, Neutralizes*, Removes*, Preserves*, Basic, Empty, SingleElement, LongInput, UTF8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Scanner Bot <scanner@kubestellar.io>
Signed-off-by: Andy Anderson <andy@clubanderson.com>
TestNewServer_ExplicitDevModeStartsWithoutOAuth (config_test.go) calls NewServer(), which calls middleware.InitTokenRevocation(db) with a real SQLiteStore. Its t.Cleanup calls server.Shutdown(), which closes the store but left revokedTokens.store pointing to the now-closed DB. TestAgentTokenEndpoint_Contract runs alphabetically after config_test.go and fails closed (503) when IsRevokedChecked tries to query the closed DB. Fix: ShutdownTokenRevocation() now nils revokedTokens.store within its existing lock, clearing the stale reference before the underlying store is closed. Subsequent revocation checks see store==nil and return (false, nil), letting authenticated requests through. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
TestNewServer_ExplicitDevModeStartsWithoutOAuth calls NewServer(), which calls InitUserValidation(db) with a real SQLiteStore. Its t.Cleanup calls server.Shutdown(), which closes the store but left userValidationStore pointing to the now-closed DB. TestAgentTokenEndpoint_Contract runs after config_test.go and uses fresh userIDs that are not in the cache, so ValidateUserActive hits the store path and fails with a DB error on the closed SQLite handle, causing the middleware to return 503 instead of 200. Fix: add ShutdownUserValidation() (mirrors ShutdownTokenRevocation) and call it from server.Shutdown() before store.Close(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
⏹️ Post-Merge Verification: cancelledCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
* 🐛 fix: prevent TestAgentTokenEndpoint_Contract flake from stale token revocation store TestNewServer_ExplicitDevModeStartsWithoutOAuth (config_test.go) initializes middleware.TokenRevocation via NewServer, then t.Cleanup shuts it down (closing the store). TestAgentTokenEndpoint_Contract runs later alphabetically and inherits leftover global state: the middleware's initOnce has been used but the store is nil, causing JWTAuth to fail closed when checking token revocation. Fix: Call middleware.resetTokenRevocationForTest() in newAgentTokenTestServer to reset initOnce and reinitialize with a fresh mock store, matching the pattern already used in pkg/api/middleware/auth_test.go. Fixes test ordering failure on PR #20857 and other PRs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: GitHub Copilot <copilot@github.com> * 🐛 fix: export ResetTokenRevocationForTest so api package tests can call it The PR introduced a call to middleware.resetTokenRevocationForTest() from routes_api_core_test.go (package api), but resetTokenRevocationForTest was unexported (lowercase). Go does not allow cross-package access to unexported identifiers, causing a compilation failure. Fix: rename to ResetTokenRevocationForTest (exported) and update all callers in auth_test.go and auth_ws_revoke_test.go accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Scanner Bot <scanner@kubestellar.io> * 🐛 fix: add missing middleware import in routes_api_core_test.go The previous commit exported ResetTokenRevocationForTest but forgot to add the middleware package import to routes_api_core_test.go (package api). Without the import, Go cannot resolve middleware.ResetTokenRevocationForTest, causing a build failure across all platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: GitHub Copilot <copilot@github.com> * 🐛 fix: init middleware globals in newRouteRegistrationServer TestSetupRoutes_ProtectedEndpointsKeepAuthAndCSRFGuards panics when its 'admin-only github token route rejects viewer' subtest runs after TestAgentTokenEndpoint_Contract: the global userValidationStore is still set to the previous test's mockStore (set up for UUID-B), but the registration test's request carries a JWT for a different UUID-A. ValidateUserActive then calls GetUser(UUID-A) on a mock that only knows UUID-B → panic. Fix: add ResetTokenRevocationForTest / InitTokenRevocation / InitUserValidation to newRouteRegistrationServer, mirroring the pattern already used in newAgentTokenTestServer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Scanner Bot <scanner@kubestellar.io> --------- Signed-off-by: GitHub Copilot <copilot@github.com> Signed-off-by: Scanner Bot <scanner@kubestellar.io> Co-authored-by: GitHub Copilot <copilot@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Scanner Bot <scanner@kubestellar.io>
Part of #20633
Summary
pkg/sanitize.LogStringpreviously only replaced ASCII CR/LF with⏎. Attackers could still inject forged log lines via:\x00) — truncate entries in C-based log pipelinesChanges
pkg/sanitize/prompt.goansiEscapeReregexp to strip CSI, OSC, and 2-char ESC sequences before logginglogC0Reregexp to strip remaining C0/DEL control characterslogInjectionReplacerto replace U+2028, U+2029 with⏎and strip\x00pkg/sanitize/prompt_test.goPreserves*tests toStrips*/Neutralizes*to reflect the hardened contractAdvisory finding addressed
Signed-off-by: Copilot 223556219+Copilot@users.noreply.github.com