Conversation
WalkthroughAdded an 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/adapters/storage/LocalStorageBase.js (1)
120-132:⚠️ Potential issue | 🟡 MinorUse the normalized path when building the public URL.
Line 121 normalizes the on-disk path, but Line 131 still uses raw
targetPath. Inputs likefoo/../bar.pngnow writebar.pngand return/content/media/foo/../bar.png, so the public URL is no longer canonical.🩹 Proposed fix
async saveRaw(buffer, targetPath) { - const storagePath = path.join(this.storagePath, this._normalizeStorageRelativePath(targetPath)); + const normalizedTargetPath = this._normalizeStorageRelativePath(targetPath); + const storagePath = path.join(this.storagePath, normalizedTargetPath); const targetDir = path.dirname(storagePath); @@ urlUtils.urlJoin('/', urlUtils.getSubdir(), this.staticFileURLPrefix, - targetPath) + normalizedTargetPath) ).replace(new RegExp(`\\${path.sep}`, 'g'), '/');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js` around lines 120 - 132, The public URL is being built from the raw targetPath instead of the normalized on-disk path, causing non-canonical URLs for inputs like "foo/../bar.png"; in saveRaw, compute and reuse the normalized relative path (the result of this._normalizeStorageRelativePath(targetPath)) when building fullUrl (rather than targetPath) so the URL matches the actual stored file path and keep the existing urlJoin and path.sep replacement logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js`:
- Around line 173-176: The code currently trusts targetDir and only normalizes
fileName, allowing path traversal; create a shared helper (e.g.,
_resolveAndValidateStoragePath(targetDir, relativeOrAbsolutePath)) that resolves
the full candidate path with path.resolve(targetDir || this.storagePath,
this._normalizeStorageRelativePath(fileName)) and then verifies the resolved
path is inside path.resolve(this.storagePath) (allowing the case where the
caller passed an absolute path that already begins with the storage root).
Replace the inline joins at the save() call sites and the code around the
current path.join usage (referenced near the block using
_normalizeStorageRelativePath and the logic around line 251) to call this helper
and throw/return an error if validation fails, avoiding startsWith and ensuring
you always validate the fully resolved path.
---
Outside diff comments:
In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js`:
- Around line 120-132: The public URL is being built from the raw targetPath
instead of the normalized on-disk path, causing non-canonical URLs for inputs
like "foo/../bar.png"; in saveRaw, compute and reuse the normalized relative
path (the result of this._normalizeStorageRelativePath(targetPath)) when
building fullUrl (rather than targetPath) so the URL matches the actual stored
file path and keep the existing urlJoin and path.sep replacement logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a4282e3-e3ee-4fcd-8b10-a31ef66842cd
📒 Files selected for processing (3)
ghost/core/core/server/adapters/storage/LocalStorageBase.jsghost/core/test/unit/server/adapters/storage/local-base-storage.test.jsghost/core/test/unit/server/lib/image/image-size.test.js
kevinansfield
force-pushed
the
storage-path-validation
branch
3 times, most recently
from
6d6c080 to
f902ae9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js (1)
165-166: Test titles should match actualexistsbehavior (returns false, notrejects).Both tests assert
exists === falserather than promise rejection, so renaming improves accuracy and future readability.✏️ Proposed rename diff
- it('exists rejects when targetDir resolves outside storage root via traversal', async function () { + it('exists returns false when targetDir resolves outside storage root via traversal', async function () { - it('exists rejects when targetDir prefix-matches but is not inside storage root', async function () { + it('exists returns false when targetDir prefix-matches but is not inside storage root', async function () {Also applies to: 185-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js` around lines 165 - 166, The test title incorrectly says "rejects" but the assertions check that storage.exists resolves to false; update the descriptive strings for the two specs in local-base-storage.test.js (the one referencing traversal and the other at the same block) to say "returns false when targetDir resolves outside storage root via traversal" (and the analogous rename for the second spec), so the test names accurately reflect that exists returns false rather than rejecting; leave the assertions and stubbing logic unchanged and only modify the it(...) description strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js`:
- Around line 165-166: The test title incorrectly says "rejects" but the
assertions check that storage.exists resolves to false; update the descriptive
strings for the two specs in local-base-storage.test.js (the one referencing
traversal and the other at the same block) to say "returns false when targetDir
resolves outside storage root via traversal" (and the analogous rename for the
second spec), so the test names accurately reflect that exists returns false
rather than rejecting; leave the assertions and stubbing logic unchanged and
only modify the it(...) description strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 124a6d25-1f21-4fcc-8de4-07dd57ed4ce0
📒 Files selected for processing (3)
ghost/core/core/server/adapters/storage/LocalStorageBase.jsghost/core/test/unit/server/adapters/storage/local-base-storage.test.jsghost/core/test/unit/server/lib/image/image-size.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/adapters/storage/LocalStorageBase.js
- ghost/core/test/unit/server/lib/image/image-size.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ghost/core/core/server/adapters/storage/LocalStorageBase.js (2)
96-103: Consider simplifying the null/undefined check.The combined
!== undefined && !== nullcheck can be simplified to!= null, which covers both cases in JavaScript due to loose equality coercion.♻️ Suggested simplification
// If fileName provided, normalize and resolve let resolvedPath; - if (fileName !== undefined && fileName !== null) { + if (fileName != null) { const normalizedFileName = this._normalizeStorageRelativePath(fileName); resolvedPath = path.resolve(resolvedBase, normalizedFileName); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js` around lines 96 - 103, The null/undefined check around building resolvedPath can be simplified: in the block that declares resolvedPath (using resolvedBase and this._normalizeStorageRelativePath), replace the explicit `fileName !== undefined && fileName !== null` test with the concise `fileName != null` check so both null and undefined are handled; keep the existing branches (call this._normalizeStorageRelativePath(fileName) and resolve with path.resolve or fall back to resolvedBase) and ensure variable names resolvedPath, resolvedBase and method _normalizeStorageRelativePath are used as-is.
164-178: Minor inconsistency: URL uses originaltargetPathwhile storage uses normalized.The storage path (line 165) correctly normalizes
targetPath, but the returned URL (line 175) uses the originaltargetPath. If the caller provides a path with backslashes or redundant separators, the URL may not exactly match the normalized storage location.Consider using the normalized path for URL construction as well for full consistency:
♻️ Suggested fix
async saveRaw(buffer, targetPath) { - const storagePath = path.join(this.storagePath, this._normalizeStorageRelativePath(targetPath)); + const normalizedPath = this._normalizeStorageRelativePath(targetPath); + const storagePath = path.join(this.storagePath, normalizedPath); const targetDir = path.dirname(storagePath); await fs.mkdirs(targetDir); await fs.writeFile(storagePath, buffer); // For local file system storage can use relative path so add a slash const fullUrl = ( urlUtils.urlJoin('/', urlUtils.getSubdir(), this.staticFileURLPrefix, - targetPath) + normalizedPath) ).replace(new RegExp(`\\${path.sep}`, 'g'), '/'); return fullUrl; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js` around lines 164 - 178, In saveRaw, the stored file path is created from this._normalizeStorageRelativePath(targetPath) but the returned fullUrl still uses the original targetPath; change the method to compute and reuse a normalized relative path (e.g., call this._normalizeStorageRelativePath(targetPath) once into a variable like normalizedRelativePath) and use that variable both when building storagePath (path.join(this.storagePath, normalizedRelativePath)) and when constructing the fullUrl via urlUtils.urlJoin('/', urlUtils.getSubdir(), this.staticFileURLPrefix, normalizedRelativePath); ensure you still replace path.sep with '/' for the final URL.ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js (1)
204-215: Test correctly validates rejection, but error message could be misleading.The test validates that
delete('file.txt', '../../etc')is rejected, which is correct. However, the error message reports"file.txt"as invalid when the actual problem is thetargetDirescaping the storage root. This matches the current implementation behavior (which usesfileName || targetDirfor the message), but could make debugging harder.Consider updating
_resolveAndValidateStoragePathto provide more context about whetherfileName,targetDir, or the combination is the problem. Not blocking, as the security validation is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js` around lines 204 - 215, The test shows the validation rejects correctly but the error message can be misleading because _resolveAndValidateStoragePath currently uses fileName || targetDir in the error text; update _resolveAndValidateStoragePath to determine which input caused the escape (fileName, targetDir, or their resolved combination) and throw a clearer error message indicating the specific offending component (e.g., "The targetDir '../../etc' resolves outside the storage root" or "The fileName 'file.txt' resolves outside the storage root" or a combined message when both are involved) so callers and tests can see the true cause.ghost/core/test/unit/server/lib/image/image-size.test.js (1)
677-678: Consider rejecting with Error objects in test mocks.Static analysis flags that
Promise.reject({})should reject with anErrorinstance. While this works for these tests, using proper Error objects would be more realistic and align with best practices.♻️ Example fix
- request: () => Promise.reject({}) + request: () => Promise.reject(new Error('simulated request failure'))This pattern appears at lines 677, 698, 740, and 758.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/lib/image/image-size.test.js` around lines 677 - 678, Replace mock rejections that use Promise.reject({}) with real Error instances to satisfy static analysis and better simulate real errors; locate each occurrence of the object rejection pattern (e.g., the mock entry "request: () => Promise.reject({})" in the image-size tests) and change them to reject with an Error (for example Promise.reject(new Error('mock request error')) or a specific Error subtype) so all four occurrences are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js`:
- Around line 96-103: The null/undefined check around building resolvedPath can
be simplified: in the block that declares resolvedPath (using resolvedBase and
this._normalizeStorageRelativePath), replace the explicit `fileName !==
undefined && fileName !== null` test with the concise `fileName != null` check
so both null and undefined are handled; keep the existing branches (call
this._normalizeStorageRelativePath(fileName) and resolve with path.resolve or
fall back to resolvedBase) and ensure variable names resolvedPath, resolvedBase
and method _normalizeStorageRelativePath are used as-is.
- Around line 164-178: In saveRaw, the stored file path is created from
this._normalizeStorageRelativePath(targetPath) but the returned fullUrl still
uses the original targetPath; change the method to compute and reuse a
normalized relative path (e.g., call
this._normalizeStorageRelativePath(targetPath) once into a variable like
normalizedRelativePath) and use that variable both when building storagePath
(path.join(this.storagePath, normalizedRelativePath)) and when constructing the
fullUrl via urlUtils.urlJoin('/', urlUtils.getSubdir(),
this.staticFileURLPrefix, normalizedRelativePath); ensure you still replace
path.sep with '/' for the final URL.
In `@ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js`:
- Around line 204-215: The test shows the validation rejects correctly but the
error message can be misleading because _resolveAndValidateStoragePath currently
uses fileName || targetDir in the error text; update
_resolveAndValidateStoragePath to determine which input caused the escape
(fileName, targetDir, or their resolved combination) and throw a clearer error
message indicating the specific offending component (e.g., "The targetDir
'../../etc' resolves outside the storage root" or "The fileName 'file.txt'
resolves outside the storage root" or a combined message when both are involved)
so callers and tests can see the true cause.
In `@ghost/core/test/unit/server/lib/image/image-size.test.js`:
- Around line 677-678: Replace mock rejections that use Promise.reject({}) with
real Error instances to satisfy static analysis and better simulate real errors;
locate each occurrence of the object rejection pattern (e.g., the mock entry
"request: () => Promise.reject({})" in the image-size tests) and change them to
reject with an Error (for example Promise.reject(new Error('mock request
error')) or a specific Error subtype) so all four occurrences are updated
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b36db6b-d7bb-462b-b711-ce175ab19694
📒 Files selected for processing (3)
ghost/core/core/server/adapters/storage/LocalStorageBase.jsghost/core/test/unit/server/adapters/storage/local-base-storage.test.jsghost/core/test/unit/server/lib/image/image-size.test.js
kevinansfield
force-pushed
the
storage-path-validation
branch
from
f902ae9 to
c946de8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/adapters/storage/LocalStorageBase.js (1)
211-231: Returningfalsefor invalid paths may mask configuration errors.The
existsmethod now returnsfalseforIncorrectUsageErrorinstead of throwing. While this maintains backward compatibility, callers cannot distinguish between "file doesn't exist" and "path is invalid."Per context snippet 3, code like:
if (mediaStorage.exists(frame.file.name, targetDir)) { await mediaStorage.delete(frame.file.name, targetDir); }will silently skip deletion for invalid paths rather than surfacing the configuration error.
Consider whether this trade-off aligns with the debugging experience you want. An alternative would be logging a warning before returning
false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js` around lines 211 - 231, The exists(fileName, targetDir) method currently swallows IncorrectUsageError from _resolveAndValidateStoragePath and returns false; instead, keep the backward-compatible false return but emit a warning so configuration/path errors are visible: catch IncorrectUsageError in exists, log a clear warning including fileName and targetDir (use the adapter's logger or the project logger used elsewhere in this class) and then return Promise.resolve(false); do not change the return value or throw, only add the warning before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/adapters/storage/LocalStorageBase.js`:
- Around line 211-231: The exists(fileName, targetDir) method currently swallows
IncorrectUsageError from _resolveAndValidateStoragePath and returns false;
instead, keep the backward-compatible false return but emit a warning so
configuration/path errors are visible: catch IncorrectUsageError in exists, log
a clear warning including fileName and targetDir (use the adapter's logger or
the project logger used elsewhere in this class) and then return
Promise.resolve(false); do not change the return value or throw, only add the
warning before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f2cf3ca-db58-4329-adae-97b1e51a7ee0
📒 Files selected for processing (3)
ghost/core/core/server/adapters/storage/LocalStorageBase.jsghost/core/test/unit/server/adapters/storage/local-base-storage.test.jsghost/core/test/unit/server/lib/image/image-size.test.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js
kevinansfield
force-pushed
the
storage-path-validation
branch
2 times, most recently
from
3a92983 to
084ebdb
Compare
allouis
left a comment
allouis
left a comment
There was a problem hiding this comment.
Overall improvements look good, but I think we can clean this up a little
| try { | ||
| normalizedPath = this._normalizeStorageRelativePath(options.path); | ||
| } catch (err) { | ||
| return Promise.reject(err); |
There was a problem hiding this comment.
If we make this an async function we can remove the try/catch
| filePath = this._resolveAndValidateStoragePath(targetDir, fileName); | ||
| } catch (err) { | ||
| if (err instanceof errors.IncorrectUsageError) { | ||
| return Promise.resolve(false); |
There was a problem hiding this comment.
Why do we want to return false here instead of erroring?
There was a problem hiding this comment.
exists() is intended to return a boolean, if we detect path traversal outside of the storage base then by definition the file doesn't exist and false makes sense and means callers don't have to handle errors. The other errors types re-thrown on 221 are from genuinely catastrophic errors from the path validation so re-throwing those makes sense.
| * @returns {string} | ||
| */ | ||
| _normalizeStorageRelativePath(filePath, options = {}) { | ||
| const { |
There was a problem hiding this comment.
I find this injection of which error message to throw kinda weird/confusing, I would prefer to see the callers handle this themselves.
We could have the contract so that it returns string | null and then callers can do if (result === null) throw ...
There was a problem hiding this comment.
Returning string | null feels far to implicit to me, I like the explicit error on incorrect usage but I agree passing the error details into the function feels off 🤔
There was a problem hiding this comment.
Updated so the helper function always throws the default error, then for the single URL case the caller catches and re-throws its custom error.
kevinansfield
force-pushed
the
storage-path-validation
branch
2 times, most recently
from
28ead5b to
3f3a7e7
Compare
ref https://linear.app/ghost/issue/ONC-1595/ - shared path normalization across local storage reads and URL conversion so image path handling stays consistent - reduced setup duplication in image-size tests
kevinansfield
force-pushed
the
storage-path-validation
branch
from
3f3a7e7 to
a2c2bad
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/lib/image/image-size.test.js (1)
538-540: Consider rejecting with an Error instance.SonarCloud flags
Promise.reject({})as a code smell. Rejecting with a properError(even a generic one) ensures the test better reflects production error handling behavior.♻️ Suggested fix
- request: () => Promise.reject({}) + request: () => Promise.reject(new Error('request failed'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/lib/image/image-size.test.js` around lines 538 - 540, The test is rejecting with a plain object which SonarCloud flags; update the mock passed to createImageSize so its request() returns a rejected Error instance instead (e.g., Promise.reject(new Error(...))) to mirror real error behavior and satisfy the linter, referring to the createImageSize call and its request mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/unit/server/lib/image/image-size.test.js`:
- Around line 538-540: The test is rejecting with a plain object which
SonarCloud flags; update the mock passed to createImageSize so its request()
returns a rejected Error instance instead (e.g., Promise.reject(new Error(...)))
to mirror real error behavior and satisfy the linter, referring to the
createImageSize call and its request mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d1f3381-9483-4ff5-ac70-3a325ec48c3e
📒 Files selected for processing (3)
ghost/core/core/server/adapters/storage/LocalStorageBase.jsghost/core/test/unit/server/adapters/storage/local-base-storage.test.jsghost/core/test/unit/server/lib/image/image-size.test.js
2 participants
ref https://linear.app/ghost/issue/ONC-1595/