Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,56 +3,138 @@ import {renderWithVisibility} from '../../utils/visibility';

// TODO - this is a placeholder for the cta card web template
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed now 😄

function ctaCardTemplate(dataset) {
const backgroundAccent = dataset.backgroundColor === 'accent' ? 'kg-style-accent' : '';
// Add validation for buttonColor
if (!dataset.buttonColor || !dataset.buttonColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) {
dataset.buttonColor = 'accent';
}
console.log(dataset);
const buttonAccent = dataset.buttonColor === 'accent' ? 'kg-style-accent' : '';
const buttonStyle = dataset.buttonColor !== 'accent' ? `background-color: ${dataset.buttonColor};` : '';

const buttonStyle = dataset.buttonColor === 'accent'
? `style="color: ${dataset.buttonTextColor};"`
: `style="background-color: ${dataset.buttonColor}; color: ${dataset.buttonTextColor};"`;
return `
<div class="cta-card ${backgroundAccent}" data-layout="${dataset.layout}" style="background-color: ${dataset.backgroundColor};">
${dataset.hasImage ? `<img src="${dataset.imageUrl}" alt="CTA Image">` : ''}
<div>
${dataset.textValue}
</div>
${dataset.showButton ? `
<a href="${dataset.buttonUrl}" class="kg-cta-button ${buttonAccent}"
style="${buttonStyle} color: ${dataset.buttonTextColor};">
${dataset.buttonText}
</a>
` : ''}
<div class="kg-card kg-cta-card kg-cta-bg-${dataset.backgroundColor} kg-cta-${dataset.layout}" data-layout="${dataset.layout}">
${dataset.hasSponsorLabel ? `
<div class="kg-sponsor-label">
<div class="kg-cta-sponsor-label">
${dataset.sponsorLabel}
</div>
` : ''}
<div class="kg-cta-content">
${dataset.hasImage ? `
<div class="kg-cta-image-container">
<img src="${dataset.imageUrl}" alt="CTA Image">
</div>
` : ''}
<div class="kg-cta-content-inner">
<div class="kg-cta-text">
${dataset.textValue}
</div>
${dataset.showButton ? `
<a href="${dataset.buttonUrl}" class="kg-cta-button ${buttonAccent}"
${buttonStyle}>
${dataset.buttonText}
</a>
` : ''}
</div>
</div>
</div>
`;
}

// TODO - this is a placeholder for the email template
function emailCTATemplate(dataset) {
const buttonStyle = dataset.buttonColor !== 'accent' ? `background-color: ${dataset.buttonColor};` : '';
const buttonStyle = dataset.buttonColor === 'accent'
? `color: ${dataset.buttonTextColor};`
: `background-color: ${dataset.buttonColor}; color: ${dataset.buttonTextColor};`;
const backgroundStyle = `background-color: ${dataset.backgroundColor};`;

const renderContent = () => {
if (dataset.layout === 'minimal') {
return `
<tr>
<td class="kg-cta-content">
<table border="0" cellpadding="0" cellspacing="0" width="100%" class="kg-cta-content-wrapper">
<tr>
${dataset.hasImage ? `
<td class="kg-cta-image-container" width="64">
<img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image" width="64">
</td>
` : ''}
<td class="kg-cta-content-inner">
<div class="kg-cta-text">
${dataset.textValue}
</div>
${dataset.showButton ? `
<a href="${dataset.buttonUrl}"
class="kg-cta-button ${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}"
style="${buttonStyle}">
${dataset.buttonText}
</a>
` : ''}
</td>
</tr>
</table>
</td>
</tr>
`;
}

return `
<tr>
<td class="kg-cta-content">
<table border="0" cellpadding="0" cellspacing="0" width="100%" class="kg-cta-content-wrapper">
${dataset.hasImage ? `
<tr>
<td class="kg-cta-image-container">
<table border="0" cellpadding="0" cellspacing="0" width="100%">
<tr>
<td>
<img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image">
</td>
</tr>
</table>
</td>
</tr>
` : ''}
<tr>
<td class="kg-cta-text">
${dataset.textValue}
</td>
</tr>
${dataset.showButton ? `
<tr>
<td>
<table border="0" cellpadding="0" cellspacing="0" width="100%">
<tr>
<td>
<a href="${dataset.buttonUrl}"
class="kg-cta-button ${dataset.buttonColor === 'accent' ? 'kg-style-accent' : ''}"
style="${buttonStyle}">
${dataset.buttonText}
</a>
</td>
</tr>
</table>
</td>
</tr>
` : ''}
</table>
</td>
</tr>
`;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor email template for maintainability and security.

The email template has several issues:

  1. Same security vulnerabilities as the web template (unsanitized URLs and colors)
  2. Complex nested table structure with duplicated code between layouts

Consider these improvements:

  1. Extract common table structures into reusable functions:
const renderImageSection = (imageUrl, altText, minimal = false) => `
    <tr>
        <td class="kg-cta-image-container"${minimal ? ' width="64"' : ''}>
            <img src="${sanitizeUrl(imageUrl)}" 
                 alt="${altText || 'CTA Image'}" 
                 class="kg-cta-image"
                 ${minimal ? ' width="64"' : ''}>
        </td>
    </tr>
`;
  1. Apply the same security measures as the web template:
-<img src="${dataset.imageUrl}" alt="CTA Image" class="kg-cta-image">
+<img src="${sanitizeUrl(dataset.imageUrl)}" alt="${dataset.imageAltText || 'CTA Image'}" class="kg-cta-image">

-<a href="${dataset.buttonUrl}" 
+<a href="${sanitizeUrl(dataset.buttonUrl)}" 


return `
<div class="cta-card-email" style="${backgroundStyle} padding: 16px; text-align: center; border-radius: 8px;">
${dataset.hasImage ? `<img src="${dataset.imageUrl}" alt="CTA Image" style="max-width: 100%; border-radius: 4px;">` : ''}
<div class="cta-text" style="margin-top: 12px; color: ${dataset.textColor};">
${dataset.textValue}
</div>
${dataset.showButton ? `
<a href="${dataset.buttonUrl}" class="cta-button"
style="display: inline-block; margin-top: 12px; padding: 10px 16px;
${buttonStyle} color: ${dataset.buttonTextColor}; text-decoration: none;
border-radius: 4px;">
${dataset.buttonText}
</a>
` : ''}
<table class="kg-card kg-cta-card kg-cta-bg-${dataset.backgroundColor} kg-cta-${dataset.layout}" border="0" cellpadding="0" cellspacing="0" width="100%" style="${backgroundStyle}">
${dataset.hasSponsorLabel ? `
<div class="sponsor-label" style="margin-top: 8px; font-size: 12px; color: #888;">
${dataset.sponsorLabel}
</div>
<tr>
<td class="kg-cta-sponsor-label">
<p>Sponsored</p>
</td>
</tr>
` : ''}
</div>
${renderContent()}
</table>
`;
}

Expand All @@ -72,10 +154,14 @@ export function renderCallToActionNode(node, options = {}) {
backgroundColor: node.backgroundColor,
sponsorLabel: node.sponsorLabel,
hasImage: node.hasImage,
imageUrl: node.imageUrl,
textColor: node.textColor
imageUrl: node.imageUrl
};

// Add validation for backgroundColor
if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) {
dataset.backgroundColor = 'white';
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Align backgroundColor validation with buttonColor validation.

The validation pattern for backgroundColor is more restrictive than buttonColor. Consider using the same pattern for consistency.

-if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) {
+if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Add validation for backgroundColor
if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+$/)) {
dataset.backgroundColor = 'white';
}
// Add validation for backgroundColor
if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) {
dataset.backgroundColor = 'white';
}


if (options.target === 'email') {
const emailDoc = options.createDocument();
const emailDiv = emailDoc.createElement('div');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ describe('CallToActionNode', function () {

const html = element.outerHTML.toString();
html.should.containEql('data-layout="minimal"');
html.should.containEql('background-color: green');
html.should.containEql('kg-cta-bg-green');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add test coverage for color validation.

The test suite should include cases for color validation, including:

  • Valid hex colors
  • Invalid color formats
  • Default color fallbacks
it('validates buttonColor format', editorTest(function () {
    const testCases = [
        {input: '#123', expected: '#123'},
        {input: '#123456', expected: '#123456'},
        {input: 'invalid<script>', expected: 'accent'},
        {input: undefined, expected: 'accent'}
    ];
    
    testCases.forEach(({input, expected}) => {
        const node = new CallToActionNode({...dataset, buttonColor: input});
        const {element} = node.exportDOM(exportOptions);
        if (expected === 'accent') {
            element.outerHTML.should.containEql('kg-style-accent');
        } else {
            element.outerHTML.should.containEql(`background-color: ${expected}`);
        }
    });
}));

html.should.containEql('background-color: #F0F0F0');
html.should.containEql('Get access now');
html.should.containEql('http://someblog.com/somepost');
Expand Down
Loading