Adopt browse/ensure/create nomenclature for the gift-links API and drop revoked_at#28837
Merged
Conversation
github-actions
Bot
added
the
migration
Contributor
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
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:
WalkthroughThe pull request renames gift-links controller and service actions to Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 38s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-06-24 14:07:03 UTC
rob-ghost
force-pushed
the
chore/gift-links-api-nomenclature
branch
from
0df1b72 to
254daba
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| router.put('/pages/:id/gift_link', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.issue)); | ||
| router.post('/pages/:id/gift_link', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.reissue)); | ||
| router.put('/gift_links/revoke_all', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.revokeAll)); | ||
| router.get('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); |
| router.post('/pages/:id/gift_link', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.reissue)); | ||
| router.put('/gift_links/revoke_all', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.revokeAll)); | ||
| router.get('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); | ||
| router.put('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.ensure)); |
| router.put('/gift_links/revoke_all', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.revokeAll)); | ||
| router.get('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); | ||
| router.put('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.ensure)); | ||
| router.post('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.create)); |
| router.get('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); | ||
| router.put('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.ensure)); | ||
| router.post('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.create)); | ||
| router.get('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); |
| router.put('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.ensure)); | ||
| router.post('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.create)); | ||
| router.get('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); | ||
| router.put('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.ensure)); |
| router.post('/posts/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.create)); | ||
| router.get('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); | ||
| router.put('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.ensure)); | ||
| router.post('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.create)); |
| router.get('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.browse)); | ||
| router.put('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.ensure)); | ||
| router.post('/pages/:id/gift_links', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.create)); | ||
| router.put('/gift_links/remove_all', mw.authAdminApi, labs.enabledMiddleware('giftLinks'), http(api.giftLinks.removeAll)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
73-73: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
/gift_links/remove_allis a global, destructive, unauthenticated-by-rate-limit mutation.This single PUT wipes every
post_gift_linksrow across the whole site. It's gated byauthAdminApi+ the labs flag + theremoveAllpermission (consistent with the rest of the Admin API), but CodeQL also flags the route as not rate-limited. Given the blast radius of a global delete, consider whether the bulk endpoint warrants additional protection (e.g., brute/rate-limit middleware) beyond standard auth. The other gift_links routes (67-72) follow the same auth pattern as the rest of the file and don't need separate treatment.🤖 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/core/server/web/api/endpoints/admin/routes.js` at line 73, The `/gift_links/remove_all` admin mutation in the routes setup is a global destructive endpoint and currently lacks any rate-limiting protection. Update the `router.put('/gift_links/remove_all', ...)` entry in the admin routes to add the same kind of brute/rate-limit middleware used elsewhere in the API, while keeping the existing `mw.authAdminApi`, `labs.enabledMiddleware('giftLinks')`, and `http(api.giftLinks.removeAll)` wiring intact.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@ghost/core/core/server/web/api/endpoints/admin/routes.js`:
- Line 73: The `/gift_links/remove_all` admin mutation in the routes setup is a
global destructive endpoint and currently lacks any rate-limiting protection.
Update the `router.put('/gift_links/remove_all', ...)` entry in the admin routes
to add the same kind of brute/rate-limit middleware used elsewhere in the API,
while keeping the existing `mw.authAdminApi`,
`labs.enabledMiddleware('giftLinks')`, and `http(api.giftLinks.removeAll)`
wiring intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 756b0e12-7e4c-4559-ac3e-88eb05f16707
📒 Files selected for processing (12)
ghost/core/core/server/api/endpoints/gift-links.tsghost/core/core/server/api/endpoints/utils/serializers/output/gift-links.tsghost/core/core/server/data/migrations/versions/6.46/2026-06-23-00-00-00-drop-gift-links-revoked-at.jsghost/core/core/server/data/migrations/versions/6.46/2026-06-23-00-00-01-rename-gift-links-revoke-all-permission-to-remove-all.jsghost/core/core/server/data/schema/fixtures/fixtures.jsonghost/core/core/server/data/schema/schema.jsghost/core/core/server/services/gift-links/database.tsghost/core/core/server/services/gift-links/service.tsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/test/e2e-api/admin/gift-links.test.tsghost/core/test/integration/services/gift-links.test.tsghost/core/test/utils/fixtures/fixtures.json
rob-ghost
force-pushed
the
chore/gift-links-api-nomenclature
branch
4 times, most recently
from
a565fce to
fb4593e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28837 +/- ##
==========================================
- Coverage 74.23% 73.12% -1.11%
==========================================
Files 1563 1563
Lines 134939 134925 -14
Branches 16362 16369 +7
==========================================
- Hits 100166 98658 -1508
- Misses 33791 35336 +1545
+ Partials 982 931 -51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ed revoked_at no ref Adopts the agreed gift-links vocabulary - browse / ensure / create / removeAll - across the controller, service, serializer and routes, and pluralises the per-post routes to /gift_links. The POST is named create, not the BREAD add: the framework reserves add for body-driven creates and this create is body-less, so POST /gift_links carries the add semantics at the REST layer. Drops the revoked_at validity timestamp and its handling (a forthcoming action audit log will own the who/what/when) via a drop-column migration. The bulk-remove permission and route are renamed from revoke_all to remove_all. Test names and suites follow the new verbs.
rob-ghost
force-pushed
the
chore/gift-links-api-nomenclature
branch
from
fb4593e to
15ce3ba
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
2 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.
Problem
The gift-links admin API used issue / reissue / revokeAll verbs and a singular
/gift_linkroute. "Reissue" implied a dyad that does not exist - the operation just creates a new gift link - and the singular route did not match the collection the response already returns. The table also carried arevoked_atvalidity timestamp.Solution
Adopt the agreed vocabulary across the controller, service, serializer and routes - browse / ensure / create / removeAll - and pluralise the per-post routes to the collection (
/posts/:id/gift_links). The POST is namedcreaterather thanaddbecause the framework reservesaddfor body-driven creates and this create is body-less; POST to the collection carries the "add" semantics at the REST layer.Drop
revoked_atand all its handling: a forthcoming action audit log will own the who/what/when of changes, so a redundant per-row validity timestamp is removed via a drop-column migration. The bulk-remove permission and route are renamed fromrevoke_alltoremove_allto match.Validated: integration + e2e suites green (the e2e boots Ghost, exercising both migrations, the routes, and the renamed permission).
Stacked on #28783. Retargets to
mainonce that merges.