Skip to content

fix: render insight text as nodes, not innerHTML, and drop stale requests (#1053) - #1058

Merged
Aditya8369 merged 1 commit into
Aditya8369:mainfrom
MOHITKOURAV01:fix/1053-analytics-insights-xss
Aug 27, 2026
Merged

fix: render insight text as nodes, not innerHTML, and drop stale requests (#1053)#1058
Aditya8369 merged 1 commit into
Aditya8369:mainfrom
MOHITKOURAV01:fix/1053-analytics-insights-xss

Conversation

@MOHITKOURAV01

Copy link
Copy Markdown
Contributor

Fixes #1053

1. The insight body was written as raw HTML

dangerouslySetInnerHTML={{
  __html: insight.description.replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>')
}}

The replace only rewrites the ** markers — but the whole string is then handed to innerHTML, so every other character in it is interpreted as markup too.

insight.description is not a constant. aiInsightsService builds it by interpolation:

description: `Based on the past year in ${cityName || 'this location'}, **${formatMonth(highestMonth)}** had the highest average pollution ...`

cityName reaches the component from Dashboard, which gets it from the location search — the geocoder's answer to text the visitor typed. Third-party data on a path to innerHTML. A place name (or a doctored/cached geocoder response) containing <img src=x onerror="..."> executed in the page, on the origin that holds community reports, the exposure log and leaderboard identity.

React escapes by default, and the rest of this component relies on that: {insight.title}, {insight.source}, {insight.confidence} were all already safe. This one line opted out.

The fix is src/utils/boldMarkup.jsxrenderBoldMarkup(text) splits the string and returns React nodes:

<p>{renderBoldMarkup(insight.description)}</p>

React escapes the text between the markers, and inside them, the same way it escapes everything else in the component. There is no string of HTML at any point, so there is nothing to get the escaping of wrong. CommunityHub reached the same conclusion after #497 — its comment now reads "component never uses dangerouslySetInnerHTML".

Two details worth naming:

  • An unclosed ** is left as literal text. Guessing would bold the entire rest of the string, and a lone ** in a sentence is far more likely to be punctuation.
  • The regex is rebuilt per call. A shared /g instance keeps lastIndex between calls, so the second insight would start matching from wherever the first stopped.

insight.description.replace also threw on a missing description. A non-string now renders as empty.

2. The insight request was never cancelled

useEffect(() => {
  if (lat != null && lon != null) {
    generateAIInsights(lat, lon, cityName).then(result => { ... })
  }
}, [lat, lon, cityName]);

No cleanup, no guard. generateAIInsights goes through fetchHistoricalData — a year of hourly archive data, slow on a cold cache. Search Delhi, then Mumbai: two requests in flight, and whichever resolves last wins. If Delhi's resolves second, the panel shows Delhi's insights under Mumbai's heading with loadingInsights already false, so nothing signals it. It was also the source of the setState-after-unmount warning.

A current flag in the effect's cleanup drops any resolution that is no longer the live request. The previous city's insights are also cleared when a new one starts, rather than lingering behind the spinner.

Verification

src/utils/boldMarkup.test.jsx — 11 tests. The ones that matter are about what does not happen: no element is built from the text, <script> and <img onerror> survive as characters, and markup inside a bold span is escaped too. A test that only checked the bold segments would have passed against the innerHTML version.

src/components/AnalyticsInsights.insights.test.jsx — 6 tests at the component level, with aiInsightsService mocked:

  • a hostile cityName produces no <img> and no <script>, and window.__xss stays undefined
  • an insight with no description does not take the panel down
  • Mumbai resolves, then Delhi's slower request lands → Mumbai's insights stay
  • a late error from the previous city does not replace the current city's insights
  • no coordinates → no fetch at all

The 22 existing AnalyticsInsights.test.jsx tests pass (verified with #1054 applied locally — that file cannot collect on main because #1049 makes historicalDataService throw on import, which is unrelated to this change). Both new test files pass on plain main, since mocking aiInsightsService keeps the broken module out of the graph entirely.

npx eslint is clean on all four changed files.

…ests (Aditya8369#1053)

AnalyticsInsights turned `**bold**` into `<strong>` by writing HTML:

  dangerouslySetInnerHTML={{
    __html: insight.description.replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>')
  }}

The replace only rewrites the `**` markers, but the whole string is then handed
to innerHTML, so every other character in it is interpreted as markup too.
`insight.description` is built in aiInsightsService by interpolating the
location name, which comes from the geocoder's answer to text the visitor typed
— third-party data on a path to innerHTML. A place name containing an `<img
src=x onerror=...>` executed in the page, on the origin that holds the
community reports, exposure log and leaderboard identity.

Adds `renderBoldMarkup`, which splits the string and returns React nodes, so
React escapes the text between the markers and inside them the same way it
escapes the title, source and confidence right next to it. There is no string of
HTML at any point. A missing or non-string description now renders as empty
rather than throwing on `.replace`.

The insights effect also had no cleanup. `generateAIInsights` reads a year of
hourly archive data, so searching Delhi and then Mumbai left two requests in
flight and whichever resolved last won — Delhi's insights under Mumbai's
heading, with loadingInsights already false so nothing signalled it. A guard
flag drops a resolution that is no longer current, which also stops the three
setState calls firing after unmount.

Fixes Aditya8369#1053
@vercel

vercel Bot commented Aug 26, 2026

Copy link
Copy Markdown

@MOHITKOURAV01 is attempting to deploy a commit to the Aditya Mahajan's projects Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

Copy link
Copy Markdown

Thank You for Your Contribution! 🎉

Hi @MOHITKOURAV01,

Thank you for opening this Pull Request and contributing to our project. We truly appreciate your efforts.

Please make sure that:

  • Your code follows the project's guidelines.
  • You have linked the appropriate issue (if applicable).
  • Screenshots are added for UI/UX changes.
  • Your PR is ready for review.

The maintainer @Aditya8369 will review your PR shortly!

Happy Contributing! 🚀

@github-actions github-actions Bot added the ECSoC26 Contributions considered under ECSoC'26 label Aug 26, 2026
@Aditya8369
Aditya8369 merged commit fadd9ea into Aditya8369:main Aug 27, 2026
3 of 13 checks passed
@github-actions

Copy link
Copy Markdown

🎉 Your PR just got merged, @MOHITKOURAV01 — thank you for contributing to Pollution Control Hub!

Your work is now part of the project. Here's what to do next:

  • ⭐ If you haven't already, consider giving the repo a star — it helps us grow.
  • 📢 Share your contribution on LinkedIn, Twitter, or wherever you hang out. You shipped open source!
  • 🔍 Browse other open issues if you want to keep contributing.

We really appreciate you taking the time. See you in the next PR! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ECSoC26-L3 ECSoC26 Contributions considered under ECSoC'26

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AnalyticsInsights renders geocoder-supplied text through dangerouslySetInnerHTML, and never cancels a stale insight request

2 participants