Added config-driven store selection for redirects#27894
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors the custom redirects storage system from a hardcoded FileStore dependency into an adapter-managed architecture. A new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@ghost/core/core/server/services/custom-redirects/make-store.ts`:
- Line 30: The current assignment for active uses the logical OR fallback which
treats empty string as falsey and coerces '' to 'file'; change the fallback to a
nullish default so only undefined/null fall back. Locate the active declaration
that reads config.get('storage:redirects:active') and replace the || 'file'
behavior with a nullish coalescing default (so empty string remains an explicit
value and will trigger the unknown-value handling elsewhere).
🪄 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: 5e050373-08cb-487b-a864-989c453a74d7
📒 Files selected for processing (4)
ghost/core/core/server/services/custom-redirects/index.jsghost/core/core/server/services/custom-redirects/make-store.tsghost/core/test/integration/services/custom-redirects/make-store.test.tsghost/core/test/unit/server/services/custom-redirects/make-store.test.ts
vershwal
commented
May 14, 2026
vershwal
force-pushed
the
GCSStoreForRedirects
branch
from
50ae95c to
57bed7a
Compare
vershwal
force-pushed
the
config-driven-store-selection
branch
2 times, most recently
from
2179a3b to
abb180f
Compare
Contributor
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 `@ghost/core/core/server/adapters/redirects/GCSStore.ts`:
- Around line 58-63: The validation currently only checks accessKeyId vs
secretAccessKey and allows a standalone sessionToken; update the credential
validation in GCSStore to also consider options.sessionToken so that if
sessionToken is provided without both accessKeyId and secretAccessKey it throws
errors.IncorrectUsageError (using tpl(messages.partialCredentials)); modify both
places where hasAccessKey/hasSecretKey are computed (the blocks around the
current checks) to compute hasSessionToken = Boolean(options.sessionToken) and
require that if hasSessionToken is true then hasAccessKey && hasSecretKey must
also be true (otherwise throw the same partialCredentials error).
🪄 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: 96113f5e-e29a-4edb-b555-c65514c4c631
📒 Files selected for processing (11)
ghost/core/core/server/adapters/redirects/FileStore.tsghost/core/core/server/adapters/redirects/GCSStore.tsghost/core/core/server/adapters/redirects/RedirectsStoreBase.d.tsghost/core/core/server/adapters/redirects/RedirectsStoreBase.jsghost/core/core/server/services/adapter-manager/config.jsghost/core/core/server/services/adapter-manager/index.jsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/shared/config/defaults.jsonghost/core/test/integration/adapters/redirects/gcs-store.test.tsghost/core/test/unit/server/adapters/redirects/file-store.test.tsghost/core/test/unit/server/adapters/redirects/gcs-store.test.ts
✅ Files skipped from review due to trivial changes (3)
- ghost/core/core/server/adapters/redirects/RedirectsStoreBase.d.ts
- ghost/core/core/server/adapters/redirects/RedirectsStoreBase.js
- ghost/core/core/server/services/adapter-manager/config.js
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 26029295641 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
vershwal
force-pushed
the
config-driven-store-selection
branch
from
319f2e0 to
02fba9a
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
vershwal
force-pushed
the
config-driven-store-selection
branch
from
02fba9a to
537da55
Compare
Member
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
allouis
reviewed
May 19, 2026
vershwal
force-pushed
the
config-driven-store-selection
branch
from
140a341 to
939967d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/integration/adapters/redirects/gcs-store.test.ts (1)
26-28: 💤 Low valueConsider escaping the prefix parameter in regex construction.
The static analysis tool flagged this as a potential ReDoS vulnerability because the
prefixparameter is directly interpolated into the regex pattern. While the risk is very low here (test code with controlled string literal inputs like'tenant-abc'), you could eliminate the warning by escaping the prefix using a library likeescape-string-regexpor manually escaping regex special characters.🔒 Optional fix to escape the prefix
+// Helper to escape regex special chars +const escapeRegex = (str: string) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const backupKeyPattern = (prefix = '') => new RegExp( - `^${prefix ? `${prefix}/` : ''}redirects-\\d{4}-\\d{2}-\\d{2}-\\d{2}-\\d{2}-\\d{2}\\.json$` + `^${prefix ? `${escapeRegex(prefix)}/` : ''}redirects-\\d{4}-\\d{2}-\\d{2}-\\d{2}-\\d{2}-\\d{2}\\.json$` );🤖 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 `@ghost/core/test/integration/adapters/redirects/gcs-store.test.ts` around lines 26 - 28, The regex in backupKeyPattern directly interpolates the prefix parameter which can introduce a ReDoS/regex-injection warning; update the function backupKeyPattern to escape prefix before building the RegExp (e.g., import and call escapeStringRegexp(prefix) from the escape-string-regexp package or replace with a small utility that escapes regex metacharacters) and then interpolate the escapedPrefix into the pattern so the generated RegExp is safe.
🤖 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 `@ghost/core/core/server/adapters/redirects/GCSStore.ts`:
- Around line 110-127: The CopyObjectCommand in replaceAll uses CopySource:
`${this.bucket}/${key}` which must be URL-encoded; update replaceAll so
CopySource is built as `${this.bucket}/${encodeURIComponent(key)}` (keep
getBackupRedirectsFilePath, buildKey, PutObjectCommand and other logic
unchanged) to ensure the key portion is properly encoded when calling
CopyObjectCommand.
---
Nitpick comments:
In `@ghost/core/test/integration/adapters/redirects/gcs-store.test.ts`:
- Around line 26-28: The regex in backupKeyPattern directly interpolates the
prefix parameter which can introduce a ReDoS/regex-injection warning; update the
function backupKeyPattern to escape prefix before building the RegExp (e.g.,
import and call escapeStringRegexp(prefix) from the escape-string-regexp package
or replace with a small utility that escapes regex metacharacters) and then
interpolate the escapedPrefix into the pattern so the generated RegExp is safe.
🪄 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: b3fcb8fa-af4a-431a-859a-b1dfeccd7d76
📒 Files selected for processing (15)
ghost/core/core/server/adapters/redirects/FileStore.tsghost/core/core/server/adapters/redirects/GCSStore.tsghost/core/core/server/adapters/redirects/RedirectsStoreBase.d.tsghost/core/core/server/adapters/redirects/RedirectsStoreBase.jsghost/core/core/server/services/adapter-manager/config.jsghost/core/core/server/services/adapter-manager/index.jsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/shared/config/defaults.jsonghost/core/test/integration/adapters/redirects/gcs-store.test.tsghost/core/test/integration/services/custom-redirects/gcs-store.test.tsghost/core/test/unit/server/adapters/redirects/file-store.test.tsghost/core/test/unit/server/adapters/redirects/gcs-store.test.tsghost/core/test/unit/server/adapters/storage/index.test.jsghost/core/test/unit/server/services/custom-redirects/gcs-store.test.tsghost/core/test/utils/minio.ts
💤 Files with no reviewable changes (2)
- ghost/core/test/integration/services/custom-redirects/gcs-store.test.ts
- ghost/core/test/unit/server/services/custom-redirects/gcs-store.test.ts
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/unit/server/adapters/redirects/file-store.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ghost/core/core/server/adapters/redirects/RedirectsStoreBase.js
- ghost/core/core/shared/config/defaults.json
- ghost/core/core/server/adapters/redirects/RedirectsStoreBase.d.ts
- ghost/core/core/server/adapters/redirects/FileStore.ts
- ghost/core/core/server/services/custom-redirects/index.js
- ghost/core/test/unit/server/adapters/redirects/gcs-store.test.ts
- ghost/core/core/server/services/adapter-manager/index.js
vershwal
force-pushed
the
config-driven-store-selection
branch
from
939967d to
f9ef392
Compare
allouis
reviewed
May 19, 2026
vershwal
force-pushed
the
config-driven-store-selection
branch
from
f9ef392 to
7b7656d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ghost/core/core/server/adapters/redirects/FileStore.ts`:
- Around line 49-50: The FileStore.getAll logging currently emits the full
redirects payload (JSON.stringify(parsed)) at info level which risks leaking
sensitive URL/query data and large noisy logs; update FileStore.getAll (and the
analogous log in the nearby method at lines 55-56) to stop logging the entire
parsed/redirects object — instead log a safe, minimal summary such as the number
of redirects or switch to a lower verbosity (debug/trace) and/or sanitize URLs
before logging so that sensitive query/path data is not recorded.
In `@ghost/core/core/server/adapters/redirects/S3RedirectsStore.ts`:
- Around line 115-116: The info log in S3RedirectsStore.getAll currently
serializes and logs the full parsed redirect objects (logging.info(`[redirects]
S3RedirectsStore.getAll: contents=${JSON.stringify(parsed)}`)), which can expose
sensitive URLs and bloat logs; change this to avoid logging full objects —
instead log a safe summary such as the number of redirects and/or a
non-sensitive identifier (e.g., logging.info(`[redirects]
S3RedirectsStore.getAll: count=${parsed.length}`) or list only sanitized
fields), and apply the same change to the similar log at the other location
(lines 122-123) so no full redirect payload is written to info logs.
🪄 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: 216eab63-aa75-4d45-921b-d9c04f5e81e7
📒 Files selected for processing (17)
ghost/core/core/server/adapters/redirects/FileStore.tsghost/core/core/server/adapters/redirects/RedirectsStoreBase.d.tsghost/core/core/server/adapters/redirects/RedirectsStoreBase.jsghost/core/core/server/adapters/redirects/S3RedirectsStore.tsghost/core/core/server/services/adapter-manager/config.jsghost/core/core/server/services/adapter-manager/index.jsghost/core/core/server/services/custom-redirects/gcs-store.tsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/server/services/custom-redirects/redirects-service.tsghost/core/core/shared/config/defaults.jsonghost/core/test/integration/adapters/redirects/s3-redirects-store.test.tsghost/core/test/integration/services/custom-redirects/gcs-store.test.tsghost/core/test/unit/server/adapters/redirects/file-store.test.tsghost/core/test/unit/server/adapters/redirects/s3-redirects-store.test.tsghost/core/test/unit/server/adapters/storage/index.test.jsghost/core/test/unit/server/services/custom-redirects/gcs-store.test.tsghost/core/test/utils/minio.ts
💤 Files with no reviewable changes (3)
- ghost/core/test/unit/server/services/custom-redirects/gcs-store.test.ts
- ghost/core/core/server/services/custom-redirects/gcs-store.ts
- ghost/core/test/integration/services/custom-redirects/gcs-store.test.ts
✅ Files skipped from review due to trivial changes (4)
- ghost/core/core/server/adapters/redirects/RedirectsStoreBase.d.ts
- ghost/core/core/server/services/custom-redirects/redirects-service.ts
- ghost/core/core/shared/config/defaults.json
- ghost/core/test/unit/server/adapters/redirects/file-store.test.ts
vershwal
force-pushed
the
config-driven-store-selection
branch
from
9e54791 to
62e3540
Compare
ref https://linear.app/ghost/issue/HKG-1701 - Adapter-manager requires a runtime base class for `instanceof` validation and a `requiredFns` array for method-shape checks before it will hand back an adapter instance. - Marker-only — `FileStore` and `GCSStore` share zero implementation, so the base class is the cost of pattern alignment rather than a place for shared logic.
ref https://linear.app/ghost/issue/HKG-1701 - Adapter-manager resolves via `require(<paths>/<type>/<class>)`, so FileStore has to live at `adapters/redirects/` for the registration to find it. - Now `extends RedirectsStoreBase` with `super()` to pass the `instanceof` gate. The .d.ts shim alongside the JS base class lets the TS adapter default-import it under `esModuleInterop` (TS won't resolve a `.js` module without `allowJs`). - Test relocated to `test/unit/server/adapters/redirects/` to match the layout `S3Storage` already follows. Contract helper stays put — moving it would drag the in-memory-store test along with no real benefit.
ref https://linear.app/ghost/issue/HKG-1701 - Constructor now takes config primitives plus an optional `s3Client` for test injection — same hybrid as `S3Storage.ts`. Adapter-manager passes a plain config object to the constructor and can't build dependencies, so the store has to own client construction. - Restored partial-credential validation we dropped when the client became required — both `accessKeyId` and `secretAccessKey` must be supplied together so a misconfigured operator gets a clear error instead of silent anonymous S3 calls. - Switched to `export default` so the file clears the filename lint rule via the same exemption `S3Storage.ts` uses. - Tests relocated to `test/{unit,integration}/server/adapters/redirects/`. Unit drops the `missingClient` case and restores partial-cred cases; integration is unchanged behaviourally.
ref https://linear.app/ghost/issue/HKG-1701 - Registered `redirects` as an adapter type backed by `RedirectsStoreBase`. Operators now pick `FileStore` (default) or `GCSStore` via `adapters.redirects.active`, with per-class config under `adapters.redirects.<ClassName>`. - Injected `basePath` into the FileStore config at runtime in `adapter-manager/config.js` — mirrors the `scheduling.schedulerUrl` precedent, since the path comes from Ghost's content directory and can't be a static value. Guarded on `paths:contentPath` so unit tests that replace the `paths` config (e.g. storage's bad-adapter test) don't trip on an unrelated TypeError. - Switched FileStore to `export default` so adapter-manager's `resolveAdapterExport` can find it via the same `.default` branch `S3Storage` and `GCSStore` already rely on. Named-only ESM exports fall through every branch and crash with `Adapter is not a constructor`. - `custom-redirects/index.js` now resolves the store via `adapterManager.getAdapter('redirects')`; direct `new FileStore(...)` is gone and the service is store-agnostic.
- Resulting key layout per site:
s3://<bucket>/<tenantPrefix>/redirects.json
s3://<bucket>/<tenantPrefix>/redirects-YYYY-MM-DD-HH-mm-ss.json
Deliberately flat under the tenant prefix (no extra `data/`
subdir) — redirects has no CDN-facing URL prefix to mirror, and
the local `<contentPath>/data/redirects.json` location is a
filesystem-organisation choice that has no operational meaning
on a shared bucket
- `tenantPrefix` is optional and defaults to empty, so existing
self-hosted GCS deployments and Ghost's own tests keep writing
to `redirects.json` at the bucket root unchanged
- Integration tests cover scoped reads/writes, scoped backup
keys, multi-tenant isolation against a shared bucket, and
slash normalisation
ref https://linear.app/ghost/issue/HKG-1701 - Removed `s3Client` and `getBackupKey` from `GCSStoreOptions`. Both were test-only escape hatches that gave the constructor two input shapes; it now takes config primitives only, matching the production config shape. - Simplified the FileStore basePath block in `adapter-manager/config.js` to a single `||=` per allouis's suggestion. The previous four-condition guard existed only to dodge `storage/index.test.js`'s "broken storage" case — fixed that test instead by setting `paths:storage` via the colon-key form so the rest of `paths` stays intact. - Rewrote the integration test against the same pattern as `adapter-cache-redis.test.js`: direct adapter construction, shared bucket per suite, no config manipulation. Backup-key tests match the per-second timestamp the store emits in production, with real waits between writes where distinct timestamps are needed. - Extracted `getMinioConfig()` in `test/utils/minio.ts` so the admin S3Client (fixture setup) and the GCSStore under test share the same primitives.
ref https://linear.app/ghost/issue/HKG-1701 - The adapter is a generic S3-compatible client (AWS via IAM roles, GCS-HMAC, R2, MinIO, …), not GCS-specific. The old name implied otherwise and was a confusion vector during PR review. - `S3RedirectsStore` keeps the purpose-in-the-name pattern used by `S3Storage` (storage) while disambiguating from it. Sibling `FileStore` keeps its name; only the GCS-flavoured class moved. - Config key under `adapters.redirects.active` becomes `"S3RedirectsStore"` — Zuul will need a matching update (follow-up). - No behaviour change; pure rename + symbol/messages updates.
vershwal
force-pushed
the
config-driven-store-selection
branch
from
62e3540 to
10a2e4f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/HKG-1701
Made the redirects store selectable via Ghost's existing
adapter-manager.Selection now flowed through the same path as
storage,sso,cache, andscheduling.adapter-managernow knew about aredirectstype, validated against a runtimeRedirectsStoreBase.adapters.redirects.active = "FileStore" | "GCSStore", with per-class config underadapters.redirects.<ClassName>. It defaulted toFileStore.RedirectsServicegot its store viaadapterManager.getAdapter('redirects'); it no longer knew which implementation backed it.core/server/adapters/redirects/.Notes
GCSStoreconstructor was reshaped (vs Added GCSStore for redirects with timestamped backups #27891): It took config primitives (bucket,region,endpoint,accessKeyId, …) plus an optionals3Clientfor test injection — same hybrid asS3Storage.ts. This reversed Added GCSStore for redirects with timestamped backups #27891's decision to require an injecteds3Client, because adapter-manager passed a plain config object to constructors and couldn't build dependencies. The store had to own client construction.accessKeyIdandsecretAccessKeyhad to be supplied together once the store built the S3Client.RedirectsStoreBasewas a marker class —FileStoreandGCSStoreshared zero implementation, so the base only declaredrequiredFns. This trade-off was accepted for adapter-manager pattern alignment.basePathwas injected at runtime inadapter-manager/config.jsbecause it was computed from Ghost's content directory. This mirrored thescheduling.schedulerUrlprecedent.Tests
test/integration/adapters/redirects/using the existing precedent fromtest/integration/adapters/redis/. It ran against MinIO viapnpm dev:storagelocally, or the GHA MinIO container from HKG-1699 in CI.