-
Notifications
You must be signed in to change notification settings - Fork 45
chore: escape raw html #2395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: escape raw html #2395
Conversation
|
bugbot run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: EJS Template Change Breaks HTML and URL Escaping
The change from <%- ... %> to <%= ... %> in EJS templates introduces two bugs:
- The
include('partial-masthead')function's HTML output is now HTML-escaped, causing the masthead to display as literal text instead of rendered HTML elements. This breaks the layout on the transition and 404 error pages. - The
escapedLongUrlvariable, already URL-escaped, is now HTML-escaped again. This double-escaping corrupts the URL in thedata-hrefattribute, potentially breaking the redirect functionality on the transition page.
src/server/views/transition-page.ejs#L19-L26
GoGovSG/src/server/views/transition-page.ejs
Lines 19 to 26 in 0b22570
| <% if(assetVariant === 'gov') { %> | |
| <%= include('partial-masthead') %> | |
| <% } %> | |
| <div class="top-half"> | |
| <div class="top-half-content"> | |
| <h3>Check your address bar</h3> | |
| <a href="https://go.gov.sg/go-antiphishing" target="_blank" rel="noreferrer noopener">Beware of phishing! <br class="mobile-break">Make sure your link starts with <%= displayHostname.toLowerCase() %></a> | |
| <p id="url" data-href="<%= escapedLongUrl %>">You will be redirected in <span id="countdown-seconds">6</span> second<span id="s">s</span></p> |
src/server/views/404.error.ejs#L19-L20
GoGovSG/src/server/views/404.error.ejs
Lines 19 to 20 in 0b22570
| <% if(assetVariant === 'gov') { %> | |
| <%= include('partial-masthead') %> |
BugBot free trial expires on July 22, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
|
when will ai replace me :kekw: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates EJS templates to escape dynamic values, replacing unescaped output tags (<%- … %>) with escaped tags (<%= … %>) to prevent raw HTML injection.
- Replace unescaped EJS tags with escaped tags in the transition page template
- Replace unescaped EJS tags with escaped tags in the 404 error page template
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/server/views/transition-page.ejs | Swapped <%- to <%= for title, assets, and links |
| src/server/views/404.error.ejs | Swapped <%- to <%= for title, assets, and images |
Comments suppressed due to low confidence (1)
src/server/views/transition-page.ejs:26
- The
escapedLongUrlis currently rendered with<%- %>(unescaped) in thedata-hrefattribute, which may allow injection or break HTML if the URL contains special characters. Switch to<%= escapedLongUrl %>to ensure proper HTML escaping.
<p id="url" data-href="<%- escapedLongUrl %>">You will be redirected in <span id="countdown-seconds">6</span> second<span id="s">s</span></p>
Problem
Go uses raw html for templating without escaping them. This PR adds that in.
Solution
add escaping into the html template (ejs docs)