Change Details

Few moments ago, I filled tasks for a couple of XSS vulnerabilities found in #growthexperiments. SInce there's at least three of them (and I could probably find more, as it didn't take much effort after I spotted the initial one, T289063), I'm also filling an auditing task to systematically look for similar pattern in the rest of GrowthExperiments codebase, to make sure nothing was missed. The general pattern is that `DashboardModule` abstract class, that's used for creating modularized dashboard special pages calls methods like `getHeaderText`, which is then displayed in a standard place of the module. For `getHeaderText()`, the output ends up in the header. The caveat is that those method are expected to //output raw HTML//, rather than strings. It's possible to output raw messages, but in that case, `->parse()` or `->escaped()` must be used. This caused several XSS vulns in the codebase. IMO, that pattern is not ideal -- it's very easy to make a mistake. I think that instead, something like `HtmlArmor` should be used to mark HTML that's intentionally raw HTML, treating everything else as text that needs to be escaped.

Few moments ago, I filled tasks for a couple of XSS vulnerabilities found in #growthexperiments. SInce there's at least three of them (and I could probably find more, as it didn't take much effort after I spotted the initial one, T289063), I'm also filling an auditing task to systematically look for similar pattern in the rest of GrowthExperiments codebase, to make sure nothing was missed. The general pattern is that `DashboardModule` abstract class, that's used for creating modularized dashboard special pages calls methods like `getFooter`, which is then displayed in a standard place of the module. For `getFooter()`, the output ends up in the footer. The caveat is that those method are expected to //output raw HTML//, rather than strings. It's possible to output raw messages, but in that case, `->parse()` or `->escaped()` must be used. This caused several XSS vulns in the codebase. IMO, that pattern is not ideal -- it's very easy to make a mistake. I think that instead, something like `HtmlArmor` should be used to mark HTML that's intentionally raw HTML, treating everything else as text that needs to be escaped.

Few moments ago, I filled tasks for a couple of XSS vulnerabilities found in #growthexperiments. SInce there's at least three of them (and I could probably find more, as it didn't take much effort after I spotted the initial one, T289063), I'm also filling an auditing task to systematically look for similar pattern in the rest of GrowthExperiments codebase, to make sure nothing was missed. The general pattern is that `DashboardModule` abstract class, that's used for creating modularized dashboard special pages calls methods like `getHeaderText`Footer`, which is then displayed in a standard place of the module. For `getHeaderText()`Footer()`, the output ends up in the headerfooter. The caveat is that those method are expected to //output raw HTML//, rather than strings. It's possible to output raw messages, but in that case, `->parse()` or `->escaped()` must be used. This caused several XSS vulns in the codebase. IMO, that pattern is not ideal -- it's very easy to make a mistake. I think that instead, something like `HtmlArmor` should be used to mark HTML that's intentionally raw HTML, treating everything else as text that needs to be escaped.