Skip to content

Add escapeHtml function for HTML context escaping - #1263

Open
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/add-escape-html-function
Open

Add escapeHtml function for HTML context escaping#1263
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/add-escape-html-function

Conversation

@pavankumar-vh

Copy link
Copy Markdown

Overview

Add a dedicated escapeHtml function for escaping strings in HTML contexts, separate from the existing escapeString function.

Problem

The previous PR (#1231) modified escapeString to escape HTML-unsafe characters, but this could break existing callers that depend on the un-escaped characters passing through (e.g., building strings for JSON parsing or non-HTML contexts).

Fix

Added a separate escapeHtml function that explicitly escapes HTML-unsafe characters (<, >, &, ') to prevent XSS attacks and HTML injection. The existing escapeString function remains unchanged for backward compatibility.

export const escapeHtml = (str: string): string => {
  return JSON.stringify(str)
    .slice(1, -1)
    .replace(/</g, '\\u003c')
    .replace(/>/g, '\\u003e')
    .replace(/&/g, '\\u0026')
    .replace(/'/g, '\\u0027')
}

Testing

Added comprehensive test coverage for both functions:

  • escapeString tests verify it still escapes JSON special characters but NOT HTML-unsafe ones
  • escapeHtml tests verify it properly escapes <script>, &, ', > to prevent XSS
  • Both functions tested with empty strings and regular characters

All 24 tests pass (19 existing + 5 new).

Files Changed

  • common/src/util/string.ts - Added escapeHtml function
  • common/src/util/__tests__/string.test.ts - Added test coverage for both functions

Scope

This change only touches common/ which is an approved contribution area per the Contributing Guide.

The escapeString function is used for generic string escaping (likely for
embedding JS/JSON string literals in generated code), not for HTML output.
Overloading it with HTML escaping would break existing callers that depend
on the un-escaped characters passing through.

Added a separate escapeHtml function that explicitly escapes HTML-unsafe
characters (<, >, &, ') to prevent XSS attacks and HTML injection when
embedding user-controlled strings in HTML contexts.

Also added comprehensive test coverage for both functions to document the
distinction and prevent regressions.
@codebuff-team

Copy link
Copy Markdown
Contributor

Thanks for the tests and the clear intent to separate JSON-escaping from HTML-escaping — that's the right instinct, and the docstring is clear.

The problem is this PR only adds escapeHtml to common/src/util/string.ts; it doesn't change any call site to actually use it. If the concern is that escapeString was modified by #1231 to escape HTML-unsafe characters and this broke non-HTML callers, the fix needs to (a) show the diff reverting/adjusting escapeString back to its original behavior if it changed, and (b) update the actual place(s) that build HTML output to call escapeHtml instead of escapeString. As it stands, this just adds a dead export with no consumers — nothing in the app is safer or more correct after this merges, since no XSS-vulnerable code path is touched.

Also: there's a stray double-blank-line introduced in string.test.ts (before the new describe('escapeString', ...) block) that a linter would likely flag.

To make this portable, please grep for where user-controlled strings are actually interpolated into HTML/script tags in this codebase, and change those call sites to use escapeHtml. Without that, there's no bug being fixed — just a speculative utility function sitting unused.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants