Download card: Add download=true param to differentiate cross site requests#1737
Download card: Add download=true param to differentiate cross site requests#1737
download=true param to differentiate cross site requests#1737Conversation
WalkthroughThe file card component in Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…requests ref ONC-1478 - This helps us differentiate when a file is being linked via a download card vs just hot linking and wanting to open in-browser - I've tested this via plain Nginx + via Ghost and the param doesn't change linking the file in normal operations
jloh
force-pushed
the
modify_download_card
branch
from
7111b2b to
8ae9b16
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-default-cards/lib/cards/file.js`:
- Line 44: The anchor tag unconditionally appends "?download=true" to
payload.src which breaks URLs that already contain a query string or fragment;
fix by computing a fullDownloadUrl local variable from payload.src that: 1)
strips or preserves fragments properly, 2) appends "?download=true" if there is
no existing query string or "&download=true" if there is one (ensuring the
fragment remains after the query), and then pass the fully-assembled
fullDownloadUrl into escapeHtml and use that escaped value in the href attribute
of the kg-file-card-container anchor; update the code that builds the href (and
any title/download attributes) to use this computed variable instead of escaping
payload.src directly.
| let html = ` | ||
| <div class="kg-card kg-file-card ${classNames}"> | ||
| <a class="kg-file-card-container" href="${escapeHtml(payload.src)}" title="Download" download> | ||
| <a class="kg-file-card-container" href="${escapeHtml(payload.src)}?download=true" title="Download" download> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the actual file to understand the context
cat -n packages/kg-default-cards/lib/cards/file.js | head -80Repository: TryGhost/Koenig
Length of output: 4619
🏁 Script executed:
# Search for tests related to file card or download=true
fd -e test.js -o -e spec.js -o -e tests.js | xargs rg -l "file.*card\|download" 2>/dev/null || echo "No test files found"Repository: TryGhost/Koenig
Length of output: 212
🏁 Script executed:
# Look for test files in common locations
find . -type f \( -name "*.test.js" -o -name "*.spec.js" -o -name "*test*.js" \) -path "*/test/*" 2>/dev/null | head -20Repository: TryGhost/Koenig
Length of output: 1143
🏁 Script executed:
# Search for any download=true references in the codebase
rg -n "download.*true\|download=true" --type=js -B2 -A2Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Search for URL handling utilities or helpers
rg -n "escapeHtml|formatUrl|appendQuery|setQuery" --type=js -A3 | head -50Repository: TryGhost/Koenig
Length of output: 4160
🏁 Script executed:
# Check the file card test to see if there are tests for download=true or URLs with query params
cat packages/kg-default-cards/test/cards/file.test.jsRepository: TryGhost/Koenig
Length of output: 140
🏁 Script executed:
# Search for any URL with query params in test files
rg -n "\\?" packages/kg-default-cards/test/cards/file.test.js -C2Repository: TryGhost/Koenig
Length of output: 148
🏁 Script executed:
# Check if there are any CDN or S3 URL examples in the codebase
rg -n "s3\|cdn\|cloudinary\|X-Amz" --type=js | head -20Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check how payload.src is typically set/used in examples
rg -n "payload\.src" packages/kg-default-cards --type=js -B1 -A1Repository: TryGhost/Koenig
Length of output: 8980
🏁 Script executed:
# Look for any test files that might test the file card
find . -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "file.*card\|kg-file" 2>/dev/null | head -10Repository: TryGhost/Koenig
Length of output: 416
🏁 Script executed:
# Check the list of test files in kg-default-cards/test/cards to see what's available
ls -la packages/kg-default-cards/test/cards/Repository: TryGhost/Koenig
Length of output: 1341
🏁 Script executed:
# Look at how the image card handles URLs with query params for comparison
cat packages/kg-default-cards/lib/cards/image.js | sed -n '110,115p'Repository: TryGhost/Koenig
Length of output: 370
🏁 Script executed:
# Check if URL API is used elsewhere in the codebase for query param handling
rg -n "new URL\|searchParams\|append\|URLSearchParams" packages/kg-default-cards/lib/cards --type=js -B1 -A1Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check the file card test at kg-default-nodes
cat packages/kg-default-nodes/test/nodes/file.test.js | head -100Repository: TryGhost/Koenig
Length of output: 4623
🏁 Script executed:
# Check the e2e test for file card
cat packages/koenig-lexical/test/e2e/cards/file-card.test.js | head -150Repository: TryGhost/Koenig
Length of output: 5957
🏁 Script executed:
# Search specifically in these test files for download or href testing
rg -n "download\|href.*src\|payload\.src" packages/kg-default-nodes/test/nodes/file.test.js packages/koenig-lexical/test/e2e/cards/file-card.test.js -C2Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check the actual FileNode implementation to see what it does
find . -name "*.js" -type f | xargs grep -l "class FileNode\|export.*FileNode" | head -5Repository: TryGhost/Koenig
Length of output: 283
🏁 Script executed:
# Look at the full file.test.js to see if the exportDOM test is checking for ?download=true
grep -n "download" packages/kg-default-nodes/test/nodes/file.test.js -C3Repository: TryGhost/Koenig
Length of output: 4378
🏁 Script executed:
# Check if there are multiple file renderers or if the test file and card file diverge
cat packages/kg-default-nodes/test/nodes/file.test.js | grep -A 20 "exportDOM"Repository: TryGhost/Koenig
Length of output: 4353
🏁 Script executed:
# Look for any recent changes or comments about download parameter
rg -n "download.*true\|download=true" --type=js -C3Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check the file-renderer.js which is used by the Lexical nodes
cat packages/kg-default-nodes/lib/nodes/file/file-renderer.jsRepository: TryGhost/Koenig
Length of output: 6431
🏁 Script executed:
# Compare the two renderers - check file.js from kg-default-cards again
cat packages/kg-default-cards/lib/cards/file.js | sed -n '40,60p'Repository: TryGhost/Koenig
Length of output: 1444
🏁 Script executed:
# Understand the relationship between these two packages
cat packages/kg-default-cards/package.json | grep -A5 -B5 "kg-default-nodes\|description"
cat packages/kg-default-nodes/package.json | grep -A5 -B5 "kg-default-cards\|description"Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check what version or context the kg-default-cards package is used in
cat packages/kg-default-cards/package.jsonRepository: TryGhost/Koenig
Length of output: 1067
🏁 Script executed:
# Look for any tests specifically for the card (not node) version
find . -type f -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "kg-default-cards" 2>/dev/null | head -10Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Verify if there are tests in kg-default-cards itself
ls -la packages/kg-default-cards/test/ 2>/dev/null || echo "No test directory"Repository: TryGhost/Koenig
Length of output: 388
🏁 Script executed:
# Check if there are references to ?download=true in any actual codebase usage
rg -n "payload\.src.*download\|download.*true" packages/kg-default-cards --type=jsRepository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check git history to understand when ?download=true was added
git log --oneline packages/kg-default-cards/lib/cards/file.js 2>/dev/null | head -10Repository: TryGhost/Koenig
Length of output: 144
🏁 Script executed:
# Check when the ?download=true was added
git log -p packages/kg-default-cards/lib/cards/file.js 2>/dev/null | grep -A 10 -B 10 "download=true" | head -40Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check if kg-default-cards is actually used/imported elsewhere
rg -n "kg-default-cards" --type=js | grep -E "import|require" | head -20Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Check if there are any other renderers or if kg-default-cards is deprecated
rg -n "from.*kg-default-cards\|require.*kg-default-cards" --type=jsRepository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
# Verify the actual test for file card in kg-default-cards
cat packages/kg-default-cards/test/cards/file.test.js 2>/dev/null || echo "File not found - checking ls"
ls packages/kg-default-cards/test/cards/ | grep fileRepository: TryGhost/Koenig
Length of output: 87
🏁 Script executed:
# Check the status and usage of kg-default-cards - is it active or deprecated?
cat packages/kg-default-cards/README.md 2>/dev/null || echo "No README"Repository: TryGhost/Koenig
Length of output: 637
🏁 Script executed:
# See the full git commit that added ?download=true
git log --format="%H %s" packages/kg-default-cards/lib/cards/file.js 2>/dev/null | head -1 | cut -d' ' -f1 | xargs -I {} git show {} --stat 2>/dev/null | head -30Repository: TryGhost/Koenig
Length of output: 1717
🏁 Script executed:
# Check if kg-default-nodes package is the newer/preferred one
cat packages/kg-default-nodes/README.md 2>/dev/null | head -20Repository: TryGhost/Koenig
Length of output: 368
🏁 Script executed:
# Confirm: does the kg-default-nodes file-renderer set ?download=true anywhere?
grep -n "download" packages/kg-default-nodes/lib/nodes/file/file-renderer.jsRepository: TryGhost/Koenig
Length of output: 307
Malformed URL when payload.src already contains query parameters.
The suffix is unconditionally appended with ?, so any payload.src containing a query string (e.g., CDN-signed URLs from S3/GCS, or any URL with ?v=...) produces a malformed URL:
https://bucket.s3.amazonaws.com/file.pdf?X-Amz-Signature=abc?download=true
^^ invalid
Browsers are lenient but specs-wise this is malformed — the second ? is treated as a literal character in the final query-param value, preventing download from being cleanly parsed. A URL fragment (#anchor) in payload.src has the same problem: the appended ?download=true lands inside the fragment where the server never sees it.
Additionally, escapeHtml should wrap the fully-assembled URL rather than only payload.src for consistency as the code evolves.
🐛 Proposed fix
- <a class="kg-file-card-container" href="${escapeHtml(payload.src)}?download=true" title="Download" download>
+ <a class="kg-file-card-container" href="${escapeHtml(payload.src.includes('?') ? `${payload.src}&download=true` : `${payload.src}?download=true`)}" title="Download" download>Or, for clarity, compute it as a local variable before the template literal:
let html = `
+ `;
+ const downloadSrc = payload.src.includes('?')
+ ? `${payload.src}&download=true`
+ : `${payload.src}?download=true`;
+ html += `
<div class="kg-card kg-file-card ${classNames}">
- <a class="kg-file-card-container" href="${escapeHtml(payload.src)}?download=true" title="Download" download>
+ <a class="kg-file-card-container" href="${escapeHtml(downloadSrc)}" title="Download" download>📝 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.
| <a class="kg-file-card-container" href="${escapeHtml(payload.src)}?download=true" title="Download" download> | |
| <a class="kg-file-card-container" href="${escapeHtml(payload.src.includes('?') ? `${payload.src}&download=true` : `${payload.src}?download=true`)}" title="Download" download> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-cards/lib/cards/file.js` at line 44, The anchor tag
unconditionally appends "?download=true" to payload.src which breaks URLs that
already contain a query string or fragment; fix by computing a fullDownloadUrl
local variable from payload.src that: 1) strips or preserves fragments properly,
2) appends "?download=true" if there is no existing query string or
"&download=true" if there is one (ensuring the fragment remains after the
query), and then pass the fully-assembled fullDownloadUrl into escapeHtml and
use that escaped value in the href attribute of the kg-file-card-container
anchor; update the code that builds the href (and any title/download attributes)
to use this computed variable instead of escaping payload.src directly.
kevinansfield
left a comment
kevinansfield
left a comment
There was a problem hiding this comment.
It would be preferable to build the src URL before being used in the template so we can add the query param correctly in cases where files already have query params.
|
We aren't going forward with this in the end, cancelling. |
2 participants
ref ONC-1478