Added support for gating access to blocks of post content #22069
Conversation
|
Warning Rate limit exceeded@kevinansfield has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces functionality for managing gated content within Ghost's membership system. Key changes include the addition of functions to parse gated block parameters, check access permissions for members, and strip out gated content based on user access rights. The implementation affects multiple files, enhancing the ability to conditionally render content based on membership status. Tests have been added or modified to ensure the new functionalities operate correctly, validating both the access logic and the content rendering processes. Changes
Suggested Labels
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
kevinansfield
force-pushed
the
visibility-alpha
branch
from
26643f5 to
b3c7404
Compare
![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
🧹 Nitpick comments (8)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (4)
9-21: Consider clarifying the loop assignment.
While the static analysis tool flags the assignment within thewhilecondition (line 14) as suspicious, this is a common pattern in JavaScript to iteratively extract regex matches. If your linter’s rules allow, you can ignore or disable this rule. Otherwise, you could refactor to aforordo...whileapproach, though it’s not strictly necessary.🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
28-40: Optionally preserve or indicate removed content.
Stripping the gated content entirely might lead to layout shifts or confusion for readers. Consider replacing gated sections with a placeholder like “Content for members only” instead of returning an empty string, if that aligns with your UX requirements.
Line range hint
53-69: Unify gating logic if desired.
The “members-only” marker logic resembles gating; consider consolidating both gating approaches if that improves code maintainability or clarity.
79-85: Well-handled gating detection.
The extra pass usingHAS_GATED_BLOCKS_REGEXis cleanly integrated. Just be aware this gating approach is distinct from the “members-only” marker above—unifying them may reduce duplication.ghost/core/test/unit/server/services/members/content-gating.test.js (1)
94-127: Thorough test coverage for gated blocks.
You’ve covered various scenarios (nonMember, empty or invalidmemberSegment, free vs. paid, etc.). This is comprehensive. For completeness, consider testing edge cases like combiningnonMember:truewith a validmemberSegmentto confirm expected behavior.ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (3)
Line range hint
12-119: Consider adding edge cases to visibility tests.The visibility tests cover the main scenarios well. Consider adding tests for:
- Posts with mixed visibility content
- Edge cases around member status transitions
- Invalid visibility values
184-224: Add negative test cases for parameter parsing.While the happy path cases are well covered, consider adding tests for:
- Invalid parameter formats
- Malformed segment strings
- Empty or null inputs
226-286: Add error handling test cases.Consider adding tests for:
- Malformed block syntax
- Nested gated blocks
- Incomplete block tags
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js(3 hunks)ghost/core/core/server/services/members/content-gating.js(1 hunks)ghost/core/test/e2e-api/content/posts.test.js(1 hunks)ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js(6 hunks)ghost/core/test/unit/server/services/members/content-gating.test.js(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Database tests (Node 20.11.1, sqlite3)
ghost/core/test/e2e-api/content/posts.test.js
[warning] 446-446:
Retried 'Posts Content API Strips out gated blocks not viewable by anonymous viewers ' due to 'The "string" argument must be of type string. Received type object (null)'
🪛 Biome (1.9.4)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js
[error] 14-14: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Browser tests
- GitHub Check: Ghost-CLI tests
🔇 Additional comments (8)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (4)
4-7: Regex approach is valid.
The introducedHAS_GATED_BLOCKS_REGEXandGATED_BLOCK_REGEXare aligned with the gating block syntax. Using regular expressions here is straightforward and appropriate for the specified HTML comment markers.
42-46: No issues found.
The_updatePlaintexthelper is straightforward and consistent with the logic used for updating the HTML excerpt.
48-52: Plaintext strategy looks consistent.
This preserves the existing approach of generating a truncated excerpt for non-custom excerpts. No concerns here.
93-97: Exports are well-structured.
Exporting the new functions alongsideforPostis neatly organized and consistent.ghost/core/core/server/services/members/content-gating.js (1)
64-88: Gated block access logic seems robust.
The conditional checks fornonMember,memberSegment, and the usage of NQL queries align with your design. Consider documenting how conflicting rules (e.g.,nonMember:trueplus a specificmemberSegment) are resolved if that scenario arises. Otherwise, this is a solid implementation.ghost/core/test/unit/server/services/members/content-gating.test.js (1)
2-2: Good practice to test new exports.
Importing bothcheckPostAccessandcheckGatedBlockAccessensures you can thoroughly test integration and avoid missing coverage.ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (2)
1-9: LGTM! Well-structured test setup.The test setup follows best practices with proper cleanup of stubs and use of strict assertions.
121-155: LGTM! Good optimization testing.Tests effectively verify that stripGatedBlocks is only called when necessary, which is important for performance.
ref https://linear.app/ghost/issue/PLG-327 - updated post output serializer's gating functions to add gating of specific content blocks - uses regex to look for specific strings in the HTML for speed compared to fully parsing the HTML - content gating blocks look like `<!--kg-gated-block:begin nonMembers:true/false memberSegment:"status:free,status:-free"-->...gated content...<!--kg-gated-block:end-->` - occurs at the API level so that content is correctly gated in Content API output and front-end website - added `checkGatedBlockAccess()` to members-service content-gating methods to keep the underlying member checks co-located - bumped Koenig packages to add support for new `visibility` card property format when rendering
kevinansfield
force-pushed
the
visibility-alpha
branch
from
b3c7404 to
beaf57f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
ghost/core/core/server/services/members/content-gating.js (1)
64-88: Add input validation for gatedBlockParams.The function handles the logic well, but it should validate the input parameters to ensure they match the expected types and structure.
Consider adding validation:
function checkGatedBlockAccess(gatedBlockParams, member) { + if (!gatedBlockParams || typeof gatedBlockParams !== 'object') { + return BLOCK_ACCESS; + } const {nonMember, memberSegment} = gatedBlockParams; + if (nonMember !== undefined && typeof nonMember !== 'boolean') { + return BLOCK_ACCESS; + } + if (memberSegment !== undefined && typeof memberSegment !== 'string') { + return BLOCK_ACCESS; + } const isLoggedIn = !!member;ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (3)
6-7: Consider improving regex maintainability.While the regex patterns are correct, they could be more maintainable by:
- Breaking down complex patterns into named components
- Adding detailed documentation about the pattern structure
Consider this approach:
+// Regex components for better maintainability +const BLOCK_START = '<!--\\s*kg-gated-block:begin'; +const BLOCK_END = '<!--\\s*kg-gated-block:end\\s*-->'; +const PARAMS_CAPTURE = '([^\\n]+?)'; +const CONTENT_CAPTURE = '([\\s\\S]*?)'; + -const HAS_GATED_BLOCKS_REGEX = /<!--\s*kg-gated-block:begin/; -const GATED_BLOCK_REGEX = /<!--\s*kg-gated-block:begin\s+([^\n]+?)\s*-->\s*([\s\S]*?)\s*<!--\s*kg-gated-block:end\s*-->/g; +// Quick check for presence of gated blocks +const HAS_GATED_BLOCKS_REGEX = new RegExp(BLOCK_START); + +// Full pattern for extracting params and content +const GATED_BLOCK_REGEX = new RegExp( + `${BLOCK_START}\\s+${PARAMS_CAPTURE}\\s*-->\\s*${CONTENT_CAPTURE}\\s*${BLOCK_END}`, 'g' +);
9-21: Add error handling for malformed parameters.The function should handle malformed input gracefully.
Consider adding error handling:
const parseGatedBlockParams = function (paramsString) { + if (typeof paramsString !== 'string') { + return {}; + } const params = {}; // Match key-value pairs with optional quotes around the value const paramsRegex = /\b(?<key>\w+):["']?(?<value>[^"'\s]+)["']?/g; let match; while ((match = paramsRegex.exec(paramsString)) !== null) { const key = match.groups.key; const value = match.groups.value; + // Validate key format + if (!/^[a-zA-Z]\w*$/.test(key)) { + continue; + } // Convert "true"/"false" strings to booleans for `nonMember` params[key] = value === 'true' ? true : value === 'false' ? false : value; } return params; };🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
28-40: Consider performance optimization for large HTML content.For large HTML content with many gated blocks, the current implementation processes each block individually. Consider batching the access checks.
Consider this optimization:
const stripGatedBlocks = function (html, member) { + // Cache access check results for identical params + const accessCache = new Map(); + return html.replace(GATED_BLOCK_REGEX, (match, params, content) => { const gatedBlockParams = module.exports.parseGatedBlockParams(params); - const checkResult = membersService.contentGating.checkGatedBlockAccess(gatedBlockParams, member); + + // Cache key for identical params + const cacheKey = JSON.stringify(gatedBlockParams); + let checkResult; + + if (accessCache.has(cacheKey)) { + checkResult = accessCache.get(cacheKey); + } else { + checkResult = membersService.contentGating.checkGatedBlockAccess(gatedBlockParams, member); + accessCache.set(cacheKey, checkResult); + } if (checkResult === PERMIT_ACCESS) { return content; } else { return ''; } }); };ghost/core/test/e2e-api/content/posts.test.js (1)
434-448: Add test cases for comprehensive coverage.Consider adding the following test cases to ensure complete coverage of gated blocks functionality:
- Test with multiple gated blocks in a single post
- Test with nested gated blocks
- Test with different member segments
- Test with malformed gated block syntax
Example test case for multiple gated blocks:
it('Handles multiple gated blocks correctly for anonymous viewers', async function () { const post = await models.Post.add({ title: 'multiple blocks', status: 'published', slug: 'multiple-gated-blocks', lexical: JSON.stringify({ root: { children: [ { type: 'html', version: 1, html: '<p>Block 1: Members only</p>', visibility: { web: { nonMember: false, memberSegment: 'status:free,status:-free' } } }, { type: 'html', version: 1, html: '<p>Public content</p>', visibility: { web: { nonMember: true, memberSegment: '' } } }, { type: 'html', version: 1, html: '<p>Block 2: Members only</p>', visibility: { web: { nonMember: false, memberSegment: 'status:free,status:-free' } } } ], type: 'root', version: 1 } }) }, {context: {internal: true}}); const response = await agent .get(`posts/${post.id}/`) .expectStatus(200); const html = response.body.posts[0].html; // Verify both gated blocks are removed assert.doesNotMatch(html, /Block 1: Members only/); assert.doesNotMatch(html, /Block 2: Members only/); // Verify public content remains assert.match(html, /Public content/); // Verify HTML structure const $ = cheerio.load(html); assert.equal($('p').length, 1, 'Should only contain one paragraph'); });ghost/core/test/unit/server/services/members/content-gating.test.js (1)
100-126: Consider adding more edge cases for robust testing.While the current test cases cover the main scenarios, consider adding tests for:
- Multiple segment combinations (e.g., "status:free,status:paid")
- Invalid segment syntax
- Special characters in segment values
+ it('memberSegment with multiple segments permits access when any segment matches', function () { + testCheckGatedBlockAccess({ + params: {memberSegment: 'status:free,status:paid'}, + member: {status: 'free'}, + expectedAccess: true + }); + }); + + it('handles special characters in segment values', function () { + testCheckGatedBlockAccess({ + params: {memberSegment: 'status:special!@#$'}, + member: {status: 'special!@#$'}, + expectedAccess: true + }); + });ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (1)
157-181: Add test cases for nested gated blocks.While the current tests cover basic scenarios, consider adding tests for:
- Nested gated blocks (blocks within blocks)
- Malformed block tags
- HTML entities within block parameters
+ it('handles nested gated blocks correctly', function () { + const attrs = { + visibility: 'public', + html: ` + <!--kg-gated-block:begin nonMember:true--> + <p>Outer block.</p> + <!--kg-gated-block:begin nonMember:false--> + <p>Inner block.</p> + <!--kg-gated-block:end--> + <!--kg-gated-block:end--> + ` + }; + + const frame = { + options: {}, + original: { + context: {} + } + }; + + gating.forPost(attrs, frame); + assert.match(attrs.html, /<p>Outer block\.<\/p>/); + assert.notMatch(attrs.html, /<p>Inner block\.<\/p>/); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
apps/admin-x-settings/package.json(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js(3 hunks)ghost/core/core/server/services/members/content-gating.js(1 hunks)ghost/core/package.json(1 hunks)ghost/core/test/e2e-api/content/posts.test.js(1 hunks)ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js(6 hunks)ghost/core/test/unit/server/services/members/content-gating.test.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js
[error] 14-14: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (7)
ghost/core/core/server/services/members/content-gating.js (1)
76-85: LGTM! Robust handling of member segments.The implementation correctly:
- Uses NQL for parsing member segments
- Handles empty queries appropriately
- Maintains co-location of member checks
ghost/core/package.json (1)
109-111: Verify package versions exist and check for vulnerabilities.The updated package versions need verification to ensure they exist in the registry and are free from known vulnerabilities.
ghost/core/test/e2e-api/content/posts.test.js (1)
434-448: Address test stability and enhance assertions.The test needs improvements for stability and assertion coverage.
apps/admin-x-settings/package.json (1)
39-39: Verify the necessity of this dependency update.The update to
@tryghost/kg-unsplash-selectorfrom0.2.7to0.2.8appears unrelated to the content gating functionality. Please confirm if this update is intentional or necessary for this PR.ghost/core/test/unit/server/services/members/content-gating.test.js (1)
94-98: LGTM! Well-structured test helper function.The
testCheckGatedBlockAccesshelper function provides a clean and reusable way to test different access scenarios.ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (2)
1-9: LGTM! Good test setup with proper cleanup.The switch to
assertand the addition ofafterEachfor cleanup improves test reliability.
226-285: Add performance tests for large content blocks.The current tests verify functionality but don't address performance concerns with large content or multiple gated blocks.
ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (3)
11-12: Consider restricting the value pattern in parameter regex.The current pattern (
[^"'\s]+) allows any non-whitespace characters in parameter values. Consider restricting this to a specific set of allowed characters to prevent injection attacks.-const GATED_BLOCK_PARAM_REGEX = /\b(?<key>\w+):["']?(?<value>[^"'\s]+)["']?/g; +const GATED_BLOCK_PARAM_REGEX = /\b(?<key>\w+):["']?(?<value>[\w,:-]+)["']?/g;
54-64: Optimize HTML processing for plaintext and excerpt.The functions potentially process the same HTML content twice. Consider caching the plaintext result when both plaintext and excerpt are needed.
function _updatePlaintext(attrs) { if (attrs.html) { - attrs.plaintext = htmlToPlaintext.excerpt(attrs.html); + // Cache the result for potential reuse in _updateExcerpt + attrs._cachedPlaintext = htmlToPlaintext.excerpt(attrs.html); + attrs.plaintext = attrs._cachedPlaintext; } } function _updateExcerpt(attrs) { if (!attrs.custom_excerpt && attrs.excerpt) { - attrs.excerpt = htmlToPlaintext.excerpt(attrs.html).substring(0, 500); + // Reuse cached plaintext if available + const plaintext = attrs._cachedPlaintext || htmlToPlaintext.excerpt(attrs.html); + attrs.excerpt = plaintext.substring(0, 500); + // Clean up cache + delete attrs._cachedPlaintext; } }
91-96: Consider extracting gated block processing into a separate function.The gated block processing logic is mixed with paywall processing. Extracting it would improve maintainability and make the code easier to test.
+function _processGatedBlocks(attrs, member) { + const hasGatedBlocks = HAS_GATED_BLOCKS_REGEX.test(attrs.html); + if (hasGatedBlocks) { + attrs.html = module.exports.stripGatedBlocks(attrs.html, member); + _updatePlaintext(attrs); + _updateExcerpt(attrs); + } + return hasGatedBlocks; +} + // In forPost function - const hasGatedBlocks = HAS_GATED_BLOCKS_REGEX.test(attrs.html); - if (hasGatedBlocks) { - attrs.html = module.exports.stripGatedBlocks(attrs.html, frame.original.context.member); - _updatePlaintext(attrs); - _updateExcerpt(attrs); - } + _processGatedBlocks(attrs, frame.original.context.member);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (4)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (4)
7-7: LGTM! Performance optimization with fast pre-check.The use of a simple regex for pre-checking is a good performance optimization, avoiding the more expensive full regex when unnecessary.
105-109: LGTM! Clear and well-organized exports.The module exports are clean and follow best practices.
35-52: 🛠️ Refactor suggestionAdd error handling for parameter parsing failures.
The function should gracefully handle parameter parsing errors to prevent content from being incorrectly displayed or hidden.
const stripGatedBlocks = function (html, member) { return html.replace(GATED_BLOCK_REGEX, (match, params, content) => { - const gatedBlockParams = module.exports.parseGatedBlockParams(params); - const checkResult = membersService.contentGating.checkGatedBlockAccess(gatedBlockParams, member); + try { + const gatedBlockParams = module.exports.parseGatedBlockParams(params); + const checkResult = membersService.contentGating.checkGatedBlockAccess(gatedBlockParams, member); - if (checkResult === PERMIT_ACCESS) { - // return content rather than match to avoid rendering gated block wrapping comments - return content; - } else { + if (checkResult === PERMIT_ACCESS) { + // return content rather than match to avoid rendering gated block wrapping comments + return content; + } return ''; + } catch (err) { + // Log error for monitoring but don't expose to output + console.error(`Error processing gated block: ${err.message}`); + return ''; } }); };Let's verify the test coverage for this function:
9-10: Consider adding input validation for gated block parameters.While the regex correctly captures the structure, it doesn't validate the parameter values. Consider adding validation to prevent potential security issues with malformed or malicious input.
Run this script to check for any potentially dangerous parameter values in existing content:
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js
Outdated
Show resolved
Hide resolved
- limited known params, param types, and param values to our small supported set
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (1)
20-51:⚠️ Potential issueAdd input validation and error handling.
The function needs additional safeguards:
- Validate input string length to prevent DoS
- Add error handling for malformed input
- Log invalid parameters for debugging
Apply this improved implementation:
const parseGatedBlockParams = function (paramsString) { + const MAX_PARAMS_LENGTH = 1000; + + if (!paramsString || typeof paramsString !== 'string') { + return {}; + } + + if (paramsString.length > MAX_PARAMS_LENGTH) { + return {}; + } + const params = {}; const matches = paramsString.matchAll(GATED_BLOCK_PARAM_REGEX); for (const match of matches) { const key = match.groups.key; let value = match.groups.value; if (!ALLOWED_GATED_BLOCK_KEYS.includes(key)) { + console.warn(`Invalid gated block parameter key: ${key}`); continue; } // Convert "true"/"false" strings to booleans, otherwise keep as string if (value === 'true') { value = true; } else if (value === 'false') { value = false; } if (typeof value !== ALLOWED_GATED_BLOCK_PARAMS[key].type) { + console.warn(`Invalid type for parameter ${key}: ${typeof value}`); continue; } if (ALLOWED_GATED_BLOCK_PARAMS[key].allowedValues && !ALLOWED_GATED_BLOCK_PARAMS[key].allowedValues.includes(value)) { + console.warn(`Invalid value for parameter ${key}: ${value}`); continue; } params[key] = value; } return params; };🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Invalid
typeofcomparison value: this expression is not a string literalnot a string literal
(lint/suspicious/useValidTypeof)
🧹 Nitpick comments (4)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (2)
58-70: Consider caching the regex replacement results.The
stripGatedBlocksfunction processes the entire HTML content on each call. For large posts with many gated blocks, this could be performance-intensive.Consider caching the results using a weak map keyed by the HTML content and member ID:
+const gatedBlockCache = new WeakMap(); + const stripGatedBlocks = function (html, member) { + const cacheKey = `${html}-${member?.id || 'anonymous'}`; + if (gatedBlockCache.has(cacheKey)) { + return gatedBlockCache.get(cacheKey); + } + - return html.replace(GATED_BLOCK_REGEX, (match, params, content) => { + const result = html.replace(GATED_BLOCK_REGEX, (match, params, content) => { const gatedBlockParams = module.exports.parseGatedBlockParams(params); const checkResult = membersService.contentGating.checkGatedBlockAccess(gatedBlockParams, member); if (checkResult === PERMIT_ACCESS) { return content; } else { return ''; } }); + gatedBlockCache.set(cacheKey, result); + return result; };
109-114: Consider early return for performance optimization.The code could be optimized by checking for gated blocks before processing.
- const hasGatedBlocks = HAS_GATED_BLOCKS_REGEX.test(attrs.html); - if (hasGatedBlocks) { + if (attrs.html && HAS_GATED_BLOCKS_REGEX.test(attrs.html)) { attrs.html = module.exports.stripGatedBlocks(attrs.html, frame.original.context.member); _updatePlaintext(attrs); _updateExcerpt(attrs); }ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (2)
184-241: Add tests for empty and null inputs.The test suite for
parseGatedBlockParamsis comprehensive but missing edge cases.Add these test cases:
const testCases = [{ + input: '', + output: {} +}, { + input: null, + output: {} +}, { input: 'nonMember:true', output: {nonMember: true} },
306-321: Add more malformed input test cases.The malformed gated block test could be expanded to cover more edge cases.
Add these test cases:
it('handles nested gated blocks', function () { const checkGatedBlockAccessStub = stubCheckGatedBlockAccess(true); const html = ` <!--kg-gated-block:begin nonMember:true--> <p>outer block</p> <!--kg-gated-block:begin nonMember:true--> <p>inner block</p> <!--kg-gated-block:end--> <!--kg-gated-block:end--> `; const result = gating.stripGatedBlocks(html, null); assert.match(result, /<p>outer block<\/p>/); assert.match(result, /<p>inner block<\/p>/); }); it('handles unclosed gated blocks', function () { const checkGatedBlockAccessStub = stubCheckGatedBlockAccess(true); const html = ` <!--kg-gated-block:begin nonMember:true--> <p>unclosed block</p> <p>Non-gated content</p> `; const result = gating.stripGatedBlocks(html, null); assert.equal(result, html); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js(3 hunks)ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js
[error] 39-39: Invalid typeof comparison value: this expression is not a string literal
not a string literal
(lint/suspicious/useValidTypeof)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Browser tests
- GitHub Check: Signup-form tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Comments-UI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
- GitHub Check: i18n
- GitHub Check: Lint
🔇 Additional comments (4)
ghost/core/core/server/api/endpoints/utils/serializers/output/utils/post-gating.js (3)
6-12: LGTM! Well-documented regex patterns.The regex patterns are well-documented with clear examples and purpose. The pre-check optimization using
HAS_GATED_BLOCKS_REGEXis a good performance consideration.
72-82: LGTM! Clear and focused helper functions.The helper functions
_updatePlaintextand_updateExcerptare well-scoped and handle their specific tasks correctly.
14-18: Consider adding validation for memberSegment format.While the
allowedValueslist is good, consider adding a regex pattern to validate the format of custommemberSegmentvalues to prevent injection attacks.Let's verify if there are any security implications:
ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (1)
7-9: LGTM! Good test hygiene with afterEach cleanup.The
afterEachhook ensures proper cleanup of sinon stubs between tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (3)
Line range hint
12-133: Consider adding edge cases to the test suite.While the test coverage is good, consider adding these scenarios:
- Handling of empty HTML content
- Multiple nested gated blocks
- Special characters in gated content
- Very large gated blocks
Example test case:
+ it('should handle empty HTML content gracefully', function () { + const attrs = { + visibility: 'members', + plaintext: '', + html: '' + }; + frame.original.context.member = {status: 'paid'}; + gating.forPost(attrs, frame); + assert.equal(attrs.html, ''); + assert.equal(attrs.plaintext, ''); + });
141-186: Add descriptive comments for test cases.The test cases are comprehensive but would benefit from documentation explaining the purpose of each case. Consider grouping them logically with comments:
const testCases = [ + // Valid nonMember boolean variations { input: 'nonMember:true', output: {nonMember: true} }, + // Valid memberSegment variations { input: 'memberSegment:"status:free"', output: {memberSegment: 'status:free'} }, + // Error cases { input: 'nonMember:invalid', output: {} },
195-272: Add tests for complex HTML structures.The test suite handles basic cases well, but consider adding tests for:
- HTML with complex nested structures (tables, lists)
- Multiple gated blocks within HTML elements
- Blocks with HTML comments inside them
Example test:
+ it('handles gated blocks within complex HTML structures', function () { + const checkGatedBlockAccessStub = stubCheckGatedBlockAccess(true); + const html = ` + <table> + <tr> + <td><!--kg-gated-block:begin nonMember:true--> + <ul> + <li>Secret list item</li> + </ul> + <!--kg-gated-block:end--></td> + </tr> + </table> + `; + const result = gating.stripGatedBlocks(html, {}); + sinon.assert.calledOnce(checkGatedBlockAccessStub); + assert.match(result, /<ul>\s*<li>Secret list item<\/li>\s*<\/ul>/); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Browser tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Signup-form tests
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Comments-UI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Lint
🔇 Additional comments (1)
ghost/core/test/unit/api/canary/utils/serializers/output/utils/post-gating.test.js (1)
1-9: LGTM! Well-structured test setup.Good practices observed:
- Using strict assertions for more reliable testing
- Proper test cleanup with
sinon.restore()in afterEach hook
ref https://linear.app/ghost/issue/PLG-327
<!--kg-gated-block:begin nonMembers:true/false memberSegment:"status:free,status:-free"-->...gated content...<!--kg-gated-block:end-->checkGatedBlockAccess()to members-service content-gating methods to keep the underlying member checks co-located