fix: remove partial HTML entity in excerpt#22513
fix: remove partial HTML entity in excerpt#22513im-adithya wants to merge 1 commit intoTryGhost:mainfrom
Conversation
WalkthroughThe changes modify the logic within the excerpt function to handle improperly formatted HTML entities. Instead of immediately returning the result as a SafeString, the function now stores the output of the excerpt retrieval in a variable. It then checks the stored result for any ampersand that is not immediately followed by a semicolon. If such a case is detected, the code truncates the excerpt at the last occurrence of the ampersand to avoid returning a malformed HTML entity. Finally, the cleaned result is wrapped in a new SafeString and returned. No changes were made to any exported or public entity declarations. Assessment against linked issues
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/frontend/helpers/excerpt.js (1)
43-50: Implementation to remove partial HTML entities looks good!This change prevents issues with incomplete HTML entities by checking if the excerpt ends with an ampersand that has no matching semicolon, and truncating the result to exclude it. This is a solid fix for the issue mentioned in the PR objectives.
One minor suggestion: Consider expanding this logic to handle cases where other common HTML entity patterns might be incomplete (not just ampersands). For example,
&#xor&#without completing numeric references.
|
@vershwal can you please have a look :D |
|
Hi @im-adithya, I'm aware you made several updates a few months ago, but most of them didn't have tests. As I explained in this comment we don't accept PRs without tests. I've left this open for a bit in case you wanted to update it but am having a bit of a clear out today. You're welcome to re-open the PR with tests anytime and we can look to review it! |
Fixes #21955
Description
We cannot remove the escape since it has been added for preventing XSS attacks here: #17190
So I just added a check to see if the text ends with ampersand and only consider till there.
Screenshots
Checklist
yarn test:allandyarn lint)