CDPT-2079 AJAX search fix & refactor #738
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const resultsHtml = posts.map((props) => t.renderHtml(props)); | ||
|
|
||
| // Append the html to the content section. | ||
| $("#content").append(resultsHtml); |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 2 months ago
To fix the problem, we need to ensure that any HTML content generated from the template is properly sanitized before being inserted into the DOM. We can use a library like DOMPurify to sanitize the HTML content. This will remove any potentially malicious scripts or tags from the HTML content.
- Install the
DOMPurifylibrary. - Import
DOMPurifyin the relevant files. - Use
DOMPurify.sanitizeto sanitize the HTML content before appending it to the DOM.
| @@ -1,2 +1,3 @@ | ||
| import AjaxTemplating from "./ajax-templating.js"; | ||
| import DOMPurify from "dompurify"; | ||
|
|
||
| @@ -89,4 +90,7 @@ | ||
|
|
||
| // Sanitize the html content. | ||
| const sanitizedHtml = resultsHtml.map(html => DOMPurify.sanitize(html)); | ||
|
|
||
| // Append the html to the content section. | ||
| $("#content").append(resultsHtml); | ||
| $("#content").append(sanitizedHtml); | ||
|
|
| @@ -30,3 +30,4 @@ | ||
| "jquery": "^3.7.1", | ||
| "node-notifier": "^10.0.1" | ||
| "node-notifier": "^10.0.1", | ||
| "dompurify": "^3.1.7" | ||
| }, |
| Package | Version | Security advisories |
| dompurify (npm) | 3.1.7 | None |
There was a problem hiding this comment.
I think this is OK as I do see a way that an attacker could inject malicious code into resultsHtml.
Would you mind offering a second opinion please @wilson1000 ?
There was a problem hiding this comment.
Can we test this? It looks good 😄
| totalPages: !totalResults ? 1 : Math.ceil(totalResults / resultsPerPage), | ||
| }); | ||
|
|
||
| $(".c-pagination").html(paginationHtml); |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 2 months ago
To fix the problem, we need to ensure that any dynamic content inserted into the DOM is properly sanitized or escaped to prevent XSS attacks. The best way to fix this is to use a library like DOMPurify to sanitize the HTML content before inserting it into the DOM.
- Install the
DOMPurifylibrary. - Import
DOMPurifyin the relevant files. - Use
DOMPurify.sanitizeto sanitize the HTML content before inserting it into the DOM.
| @@ -1,2 +1,3 @@ | ||
| import AjaxTemplating from "./ajax-templating.js"; | ||
| import DOMPurify from 'dompurify'; | ||
|
|
||
| @@ -137,3 +138,3 @@ | ||
|
|
||
| $(".c-pagination").html(paginationHtml); | ||
| $(".c-pagination").html(DOMPurify.sanitize(paginationHtml)); | ||
|
|
| @@ -1 +1,2 @@ | ||
| import DOMPurify from 'dompurify'; | ||
| /** | ||
| @@ -85,3 +86,3 @@ | ||
|
|
||
| return parts.join(""); | ||
| return DOMPurify.sanitize(parts.join("")); | ||
| } |
| @@ -30,3 +30,4 @@ | ||
| "jquery": "^3.7.1", | ||
| "node-notifier": "^10.0.1" | ||
| "node-notifier": "^10.0.1", | ||
| "dompurify": "^3.1.7" | ||
| }, |
| Package | Version | Security advisories |
| dompurify (npm) | 3.1.7 | None |
There was a problem hiding this comment.
Hi @EarthlingDavey , it looks like you ventured down a rabbit hole here 😄
Can you walk me through the update tomorrow? It would be good to get your view of the improvements you've made. Thank you!
| @@ -120,6 +119,9 @@ | |||
| new MOJ\Intranet\WPOffloadMedia(); | |||
| new MOJ\Intranet\WPElasticPress(); | |||
|
|
|||
| $search = new MOJ\Intranet\Search(); | |||
| $search->hooks(); | |||
There was a problem hiding this comment.
Do we have to notate it like this? Can we follow the pattern of include and or do we need other $search variables in the functions.php script?
This PR includes a refactor of archive pages.
AJAX filter/search results are pulled in via ajax.
The underlying WP_Query for the AJAX results is the same as the WP_Query used for loading the first page of results (without AJAX).
Some bugs have been ironed out and unnecessary code has been removed.
There are still a lot of potential improvements to be made, e.g.
get-news-rest-api.phpand re-write functions that rely on it.pagination.phpand modify templates that use it, as a 2nd WP_Query should not be run just for pagination.^ having removed the file
get-blog-rest-api.phpand refactoring pagination onpage_news.phpandpage_blog.phpshows that the above improvements are feasible.