Skip to content

[security][core] escape values at dashboard html sinks the api escaper does not cover (24.05) - #7965

Closed
ar2rsawseen wants to merge 2 commits into
release.24.05from
backport/dashboard-html-sink-escaping-2405
Closed

[security][core] escape values at dashboard html sinks the api escaper does not cover (24.05)#7965
ar2rsawseen wants to merge 2 commits into
release.24.05from
backport/dashboard-html-sink-escaping-2405

Conversation

@ar2rsawseen

Copy link
Copy Markdown
Member

Backport of #7954 to release.24.05.

Not a clean cherry-pick: one of the three sites needs a different fix on this branch, because the localized strings differ. Details below.

Same as master

compliance-hub export history actions column. The datatable builds an HTML string in onReady and the template renders it. Most of what goes into that string came through common.returnOutput, so escape_html_entities already escaped it and it is deliberately left alone: escaping twice would surface the entities literally in the UI. One value comes from countlyGlobal instead, which express-expose serializes for the JavaScript string context only, so the API's HTML escaping never applied to it. It is now escaped where it is interpolated.

countlyCommon.encodeHtml and returnOutput's use of escape_html_entities(k, v, true) are both identical on this branch, so the reasoning transfers unchanged.

populator template delete confirmation. Body converted to text interpolation. populator.delete-template-description carries no markup in any of the 26 locale files on this branch.

Different on this branch

populator environment delete confirmation. On master the body converts to text interpolation too. Here it cannot: the string is

populator.environment-delete-warning-description = Are you sure you want to delete <b>{0}</b> environment?

so converting it would render <b> literally. The body keeps v-html and the environment name is escaped where it is substituted instead. Same outcome, different mechanism, chosen by what the locale file actually contains rather than by what master did.

The name reaching both dialogs was HTML-decoded on the way in — deliberately, since the dropdown label needs the decoded form — so the API's escaping no longer held at render time. That is what made these reachable.

plugins dependency confirmation stays on v-html, as on master. plugins.confirm carries a <br/><br/> in all translated locale files, and the values interpolated into it are plugin titles from package metadata, not anything a dashboard account can write.

Verification

Ran in a dedicated worktree of release.24.05.

  • new tests against unpatched code on this branch: 3 failing, 2 passing. Against patched: 5 passing. The three failures are exactly the three security assertions.
  • full unit suite: 101 passing before, 106 after (+5, the new tests). Baseline established by re-running with the new file excluded.
  • the same 5 pre-existing failures appear before and after: 3 Common API utility functions ObjectID validation cases and 2 network-dependent Countly Request cases. Worth flagging that the 3 ObjectID ones are most likely an artifact of running this branch against a newer checkout's node_modules rather than anything on the branch itself, since master's suite does not show them. Either way they are identical either side of the change.
  • eslint clean on all changed files.

🤖 Generated with Claude Code

ar2rsawseen and others added 2 commits August 19, 2026 09:56
…istory actions column

Backport of the master change.

The export/purge history datatable builds an html string in onReady and the
template renders it, so every value interpolated into it has to be html-safe
before it gets there.

Values taken from the row arrive through common.returnOutput, which
escape_html_entities has already escaped, so they are deliberately left alone:
escaping them a second time would surface the entities literally in the ui.
One value in that string comes from countlyGlobal instead. That object is
serialized into the dashboard by express-expose, whose escaping is for the
javascript string context and is value-preserving by design, so the api's html
escaping never applied to it. It is now escaped where it is interpolated.

Adds test/unit-tests/plugins.compliance-hub.actions-escaping.js, which loads
the real module in a sandbox and exercises the actual onReady builder. It pins
both directions: a value carrying markup is neutralized, and an already-escaped
api value is not double-escaped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ete confirmations

Backport of the master change, adjusted for this branch: the two populator
delete confirmations need different fixes here, because their localized strings
differ from master's.

Both dialogs substitute a name that was html-decoded on the way in, so the
escaping the api applied no longer held by the time it was rendered.

- the template delete confirmation carries no markup in any of the 26 locale
  files, so its body is now text interpolation, which removes the sink
- the environment delete confirmation cannot do that on this branch. Its string
  is "Are you sure you want to delete <b>{0}</b> environment?", so the body has
  to stay v-html and the name is escaped where it is substituted instead. On
  master the same string has no markup, which is why that branch converts the
  template and this one does not.

Left the plugins plugin's dependency confirmation on v-html, as on master:
plugins.confirm carries a <br/><br/> in all translated locale files, and the
values interpolated into it are plugin titles from package metadata rather than
anything a dashboard account can write.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ar2rsawseen

Copy link
Copy Markdown
Member Author

Superseded by #7971, which consolidates the frontend HTML-sink hardening into a single per-repo PR. The same change (same authorship) is included there. Closing this one.

@ar2rsawseen
ar2rsawseen deleted the backport/dashboard-html-sink-escaping-2405 branch August 19, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant