🔒 Fix broken CompanyUrl link and improve security attributes - #22
🔒 Fix broken CompanyUrl link and improve security attributes#22SmolSoftBoi wants to merge 2 commits into
Conversation
- Use attribute value template `{CompanyUrl}` for href to make the link dynamic.
- Add `rel="noopener noreferrer"` to the link with `target="_blank"` to mitigate tabnabbing risks.
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 GuideFixes the broken CompanyUrl hyperlink in the SDSCom XSLT template and hardens the anchor tag against tabnabbing by adding appropriate security attributes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This comment was marked as low quality.
This comment was marked as low quality.
![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 left some high level feedback:
- Consider normalizing or validating
CompanyUrl(e.g., ensuring it has a proper scheme likehttps://and no dangerous protocols) before using it inhrefto avoid generating unusable or unsafe links.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing or validating `CompanyUrl` (e.g., ensuring it has a proper scheme like `https://` and no dangerous protocols) before using it in `href` to avoid generating unusable or unsafe links.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR fixes the rendered “Company URL” anchor in the main SDSComXSL.xsl transform so it actually links to the company URL value from the input XML, and hardens the external link against tabnabbing when opened in a new tab.
Changes:
- Updated the
<a>taghreffrom a hardcoded string to an XSLT attribute value template that uses the XMLCompanyUrlvalue. - Added
rel="noopener noreferrer"alongsidetarget="_blank"for safer external link behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Non-blocking feedback (1)
- Validate and normalize
CompanyUrlbefore writing it intohref— SDSComXSL.xsl#L402
This change correctly fixes the hardcodedhref, but it now emits any incoming URI scheme as-is. If a feed includesjavascript:/data:(or a scheme-less value likeexample.com), the rendered link can be unsafe or still broken.
Consider normalizing withnormalize-space()and allowing onlyhttp/https(optionally prependinghttps://for scheme-less hostnames) before settinghref.
If you want Charlie to apply fixes, reply with the item number (for example: please fix 1).
- Use attribute value template `{CompanyUrl}` for href in `SDSComXSL.xsl`.
- Add `rel="noopener noreferrer"` to the link with `target="_blank"`.
- Update GitHub Actions workflow to use `v4` actions and `yarn` cache.
Co-authored-by: SmolSoftBoi <491681+SmolSoftBoi@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SDSComXSL.xsl`:
- Line 402: The current anchor writes CompanyUrl directly into href which allows
harmful schemes; change the template around the <a href="{CompanyUrl}"> and
xsl:value-of select="CompanyUrl" to validate and only emit the href when
CompanyUrl begins with a safe scheme (allow-list http:// and https://).
Implement an XSLT conditional (xsl:choose/xsl:when or matches()/starts-with
checks) that tests CompanyUrl for ^https?:// and only sets href="{CompanyUrl}"
when it passes; otherwise render the CompanyUrl as plain text (or omit the link)
to avoid emitting javascript: or other unsafe schemes. Ensure you update the
same element where xsl:value-of select="CompanyUrl" is used so displayed text
remains but the clickable link is only created for validated URLs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae173db6-b5db-4dc3-8323-d2280c7f95a8
📒 Files selected for processing (2)
.github/workflows/node.js.ymlSDSComXSL.xsl
📜 Review details
🔇 Additional comments (1)
.github/workflows/node.js.yml (1)
23-29: Good CI alignment and action upgrades.Using
@v4actions and switching cache toyarnat Lines 23, 25, and 28 is consistent with the install/build/test steps and improves workflow hygiene.
| </dt> | ||
| <dd class="col-8"> | ||
| <a href="CompanyUrl" target="_blank"><xsl:value-of select="CompanyUrl"/></a> | ||
| <a href="{CompanyUrl}" target="_blank" rel="noopener noreferrer"><xsl:value-of select="CompanyUrl"/></a> |
There was a problem hiding this comment.
Validate CompanyUrl before writing it into href.
Line 402 now trusts input directly in href; a crafted value like javascript:... would create an executable link in the rendered HTML. Please allow-list safe schemes (for example http:// and https://) before emitting an <a>.
Suggested fix
- <a href="{CompanyUrl}" target="_blank" rel="noopener noreferrer"><xsl:value-of select="CompanyUrl"/></a>
+ <xsl:variable name="companyUrl" select="normalize-space(CompanyUrl)"/>
+ <xsl:variable name="companyUrlLower" select="lower-case($companyUrl)"/>
+ <xsl:choose>
+ <xsl:when test="starts-with($companyUrlLower, 'http://') or starts-with($companyUrlLower, 'https://')">
+ <a href="{$companyUrl}" target="_blank" rel="noopener noreferrer">
+ <xsl:value-of select="$companyUrl"/>
+ </a>
+ </xsl:when>
+ <xsl:otherwise>
+ <xsl:value-of select="$companyUrl"/>
+ </xsl:otherwise>
+ </xsl:choose>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SDSComXSL.xsl` at line 402, The current anchor writes CompanyUrl directly
into href which allows harmful schemes; change the template around the <a
href="{CompanyUrl}"> and xsl:value-of select="CompanyUrl" to validate and only
emit the href when CompanyUrl begins with a safe scheme (allow-list http:// and
https://). Implement an XSLT conditional (xsl:choose/xsl:when or
matches()/starts-with checks) that tests CompanyUrl for ^https?:// and only sets
href="{CompanyUrl}" when it passes; otherwise render the CompanyUrl as plain
text (or omit the link) to avoid emitting javascript: or other unsafe schemes.
Ensure you update the same element where xsl:value-of select="CompanyUrl" is
used so displayed text remains but the clickable link is only created for
validated URLs.
🎯 What: The
hrefattribute of theCompanyUrllink inSDSComXSL.xslwas hardcoded to the string"CompanyUrl", rendering the link broken. This PR fixes the link to use the dynamic value from the XML and addsrel="noopener noreferrer"for enhanced security.target="_blank"withoutrel="noopener noreferrer"exposes users to potential tabnabbing attacks, where a malicious linked site could gain control over the original tab.🛡️ Solution:
hrefattribute to{CompanyUrl}using XSLT attribute value templates.rel="noopener noreferrer"to the<a>tag to follow security best practices for external links.PR created automatically by Jules for task 5361912156639603957 started by @SmolSoftBoi
Summary by Sourcery
Fix the supplier website link in SDSComXSL to use the dynamic CompanyUrl value and harden the external link with appropriate security attributes.
Bug Fixes:
Enhancements: