🔒 [security fix] Implement URL encoding for mailto: and tel: links#24
🔒 [security fix] Implement URL encoding for mailto: and tel: links#24SmolSoftBoi wants to merge 2 commits into
Conversation
- Use `fn:encode-for-uri()` for dynamic content in `mailto:` links (email addresses and subject query parameters). - Use `fn:encode-for-uri()` for dynamic content in `tel:` links. - Fix bug in `CompanyUrl` link where it was missing Attribute Value Template curly braces. 🎯 What: Missing URL encoding in URI schemes.⚠️ Risk: Potential for URI injection or broken links if dynamic content contains special characters. 🛡️ Solution: Applied standard XSLT `fn:encode-for-uri()` function to dynamic components of URIs. Co-authored-by: SmolSoftBoi <491681+SmolSoftBoi@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
This comment was marked as low quality.
This comment was marked as low quality.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds URI encoding to dynamic mailto: and tel: links in SDSComXSL.xsl and fixes an incorrectly hard-coded CompanyUrl href to use the XSLT value, improving security and correctness of generated links. Flow diagram for URI-encoding dynamic mailto and tel linksflowchart TD
A[SDS XML data
- Email
- EmailCompetentPerson
- CompanyUrl
- EmergencyPhone No
- Title
- ProductNo] --> B[XSLT template SDSComXSL]
B --> C1[Apply fn:encode-for-uri to Email]
B --> C2[Apply fn:encode-for-uri to EmailCompetentPerson]
B --> C3[Apply fn:encode-for-uri to Title]
B --> C4[Apply fn:encode-for-uri to ProductNo]
B --> C5[Apply fn:encode-for-uri to EmergencyPhone No]
B --> C6[Use CompanyUrl as AVT in href]
C1 --> D1[Build mailto href<br/>mailto:{encoded Email}]
C2 --> D2[Build mailto href<br/>mailto:{encoded EmailCompetentPerson}]
C3 --> D1
C4 --> D1
C3 --> D2
C4 --> D2
C5 --> D3[Build tel href<br/>tel:{encoded No}]
C6 --> D4[Build http/https href<br/>{CompanyUrl}]
D1 --> E[HTML a elements with
encoded mailto links
including subject with
encoded Title and ProductNo]
D2 --> E
D3 --> F[HTML a elements with
encoded tel links]
D4 --> G[HTML a element with
correct CompanyUrl href]
E --> H[Browser renders safe mailto links]
F --> H
G --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- For the
mailto:links, consider encoding the subject as a single value (e.g.subject={fn:encode-for-uri(concat($Title, ' ', ProductNo))}) rather than mixing a literal%20with separately encoded pieces to avoid awkward composition and potential double-encoding issues if this evolves. - You’re applying
fn:encode-for-uri()to the entire email address and phone number; while this is safe, it also encodes characters like@and+that are valid in themailto:/tel:schemes—if readability or interoperability with some clients matters, you might prefer encoding only the query parameters and leaving the address/number unencoded.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the `mailto:` links, consider encoding the subject as a single value (e.g. `subject={fn:encode-for-uri(concat($Title, ' ', ProductNo))}`) rather than mixing a literal `%20` with separately encoded pieces to avoid awkward composition and potential double-encoding issues if this evolves.
- You’re applying `fn:encode-for-uri()` to the entire email address and phone number; while this is safe, it also encodes characters like `@` and `+` that are valid in the `mailto:`/`tel:` schemes—if readability or interoperability with some clients matters, you might prefer encoding only the query parameters and leaving the address/number unencoded.
## Individual Comments
### Comment 1
<location path="SDSComXSL.xsl" line_range="466" />
<code_context>
<xsl:for-each select="EmergencyPhone">
<dt class="col-4">
- <a href="tel:{No}"><xsl:value-of select="No"/></a>
+ <a href="tel:{fn:encode-for-uri(No)}"><xsl:value-of select="No"/></a>
</dt>
<dd class="col-8">
</code_context>
<issue_to_address>
**issue (bug_risk):** Encoding the entire phone number for a `tel:` URI can break valid numbers (e.g., leading `+`).
`fn:encode-for-uri` will turn `+` into `%2B`, but `tel:` URIs expect a literal leading `+` (and some clients may not handle the encoded form). Consider leaving the number unencoded and only normalizing/stripping spaces or formatting characters instead of encoding the entire value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <xsl:for-each select="EmergencyPhone"> | ||
| <dt class="col-4"> | ||
| <a href="tel:{No}"><xsl:value-of select="No"/></a> | ||
| <a href="tel:{fn:encode-for-uri(No)}"><xsl:value-of select="No"/></a> |
There was a problem hiding this comment.
issue (bug_risk): Encoding the entire phone number for a tel: URI can break valid numbers (e.g., leading +).
fn:encode-for-uri will turn + into %2B, but tel: URIs expect a literal leading + (and some clients may not handle the encoded form). Consider leaving the number unencoded and only normalizing/stripping spaces or formatting characters instead of encoding the entire value.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eb86e177d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <xsl:for-each select="EmergencyPhone"> | ||
| <dt class="col-4"> | ||
| <a href="tel:{No}"><xsl:value-of select="No"/></a> | ||
| <a href="tel:{fn:encode-for-uri(No)}"><xsl:value-of select="No"/></a> |
There was a problem hiding this comment.
Encoding No with fn:encode-for-uri() turns delimiter characters like +, ;, =, and ? into %2B, %3B, etc., but these delimiters are significant in tel: URIs (for global numbers and parameters such as extensions). For emergency numbers that include a leading + or URI parameters, the generated link can become non-conformant and may not dial correctly in some clients.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens generated mailto: and tel: links in the main SDS HTML XSLT by URL-encoding dynamic URI components, and fixes the CompanyUrl anchor so it actually uses the XML-provided URL.
Changes:
- Apply
fn:encode-for-uri()to dynamic pieces ofmailto:links (address + subject parts). - Apply
fn:encode-for-uri()totel:links. - Fix
CompanyUrlanchorhrefto use an Attribute Value Template ({CompanyUrl}).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </dt> | ||
| <dd class="col-8"> | ||
| <a href="CompanyUrl" target="_blank"><xsl:value-of select="CompanyUrl"/></a> | ||
| <a href="{CompanyUrl}" target="_blank"><xsl:value-of select="CompanyUrl"/></a> |
| <br/> | ||
| </xsl:if> | ||
| <a href="mailto:{.}?subject={$Title}%20{ProductNo}"><xsl:value-of select="."/></a> | ||
| <a href="mailto:{fn:encode-for-uri(.)}?subject={fn:encode-for-uri($Title)}%20{fn:encode-for-uri(ProductNo)}"><xsl:value-of select="."/></a> |
| </dt> | ||
| <dd class="col-8"> | ||
| <a href="mailto:{Email}?subject={$Title}%20{ProductNo}"><xsl:value-of select="Email"/></a> | ||
| <a href="mailto:{fn:encode-for-uri(Email)}?subject={fn:encode-for-uri($Title)}%20{fn:encode-for-uri(ProductNo)}"><xsl:value-of select="Email"/></a> |
| </dt> | ||
| <dd class="col-8"> | ||
| <a href="mailto:{EmailCompetentPerson}?subject={$Title}%20{ProductNo}"><xsl:value-of select="EmailCompetentPerson"/></a> | ||
| <a href="mailto:{fn:encode-for-uri(EmailCompetentPerson)}?subject={fn:encode-for-uri($Title)}%20{fn:encode-for-uri(ProductNo)}"><xsl:value-of select="EmailCompetentPerson"/></a> |
| <xsl:for-each select="EmergencyPhone"> | ||
| <dt class="col-4"> | ||
| <a href="tel:{No}"><xsl:value-of select="No"/></a> | ||
| <a href="tel:{fn:encode-for-uri(No)}"><xsl:value-of select="No"/></a> |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking feedback
tel:links should not URI-encode the entire subscriber.fn:encode-for-uri(No)encodes structural delimiters like+,;,=, and?, which can break valid global numbers and parameters (for example+...or;ext=) in RFC 3966 — SDSComXSL.xsl#L466
If you'd like me to apply a fix for this, reply with please fix 1.
| <xsl:for-each select="EmergencyPhone"> | ||
| <dt class="col-4"> | ||
| <a href="tel:{No}"><xsl:value-of select="No"/></a> | ||
| <a href="tel:{fn:encode-for-uri(No)}"><xsl:value-of select="No"/></a> |
There was a problem hiding this comment.
tel: URIs are structured, so encoding the entire subscriber with fn:encode-for-uri(No) is risky here: it percent-encodes delimiter characters that RFC 3966 expects to stay literal (+, ;, =, ?). That can break valid forms like global numbers (+...) and parameters (;ext=, ;phone-context=).
Suggested fix: normalize/validate No, then compose the tel: URI structurally without full URI-component encoding; only encode parameter values when needed.
- Use `fn:encode-for-uri()` for dynamic content in `mailto:` links (email addresses and subject query parameters). - Use `fn:encode-for-uri()` for dynamic content in `tel:` links. - Fix bug in `CompanyUrl` link where it was missing Attribute Value Template curly braces. - Update GitHub Actions workflow to use `actions/checkout@v4` and `actions/setup-node@v4` with `cache: 'yarn'`. 🎯 What: Missing URL encoding in URI schemes and incorrect CI configuration.⚠️ Risk: Potential for URI injection or broken links. CI pipeline failing due to incorrect cache settings and deprecated actions. 🛡️ Solution: Applied standard XSLT `fn:encode-for-uri()` function, corrected Attribute Value Templates, and updated CI workflow. Co-authored-by: SmolSoftBoi <491681+SmolSoftBoi@users.noreply.github.com>
The vulnerability involved missing URL encoding when placing dynamic content into
mailto:link parameters inSDSComXSL.xsl. This could lead to URI injection or broken links if fields like the document title or product number contained special characters (e.g., spaces, ampersands).To address this, I have:
fn:encode-for-uri()to the dynamic parts of allmailto:links, including the email address and thesubjectquery parameter.fn:encode-for-uri()totel:links for consistency and URI safety.CompanyUrllink where thehrefattribute was incorrectly set to a literal string instead of using an Attribute Value Template.All changes were verified to be syntactically correct and follow XSLT 3.0 best practices.
PR created automatically by Jules for task 16311954826983525945 started by @SmolSoftBoi
Summary by Sourcery
Apply URI encoding to dynamic contact links to harden against malformed or unsafe URLs.
Bug Fixes: