fix https://linear.app/ghost/issue/ENG-1105/settingscacheget-is-slow
- through profiling and flamegraphs, we can see that `_doGet` is one of
the bottlenecks during high traffic times, sometimes taking up to 20%
of the CPU time when hammering Ghost with `wrk`
- this is because, for the majority of settings cache lookup, we're
running `JSON.parse`, which blocks the main thread
- whilst we're only parsing small strings, we're doing it a LOT,
sometimes hundreds of times per request, which adds up
- this code just throws most deserializing at `JSON.parse`, so if we can
stop it from doing that, it'd be a huge win
- my initial attempts here were to convert the _doGet function to a
smarter deserializing, by looking up `cacheEntry.type` and acting
accordingly
- however, it became a bit of a logical nightmare, and difficult to
reason about for now (i still think we should do it)
- therefore, I'm just doing to add a hotpath fix to catch 99% of
usecases, which is checking the type of the cache entry and returning
the value if it's a string
- on a trivial benchmark locally, this causes Ghost to return 30% more
requests per second!!
ref PLG-220
- Improved `getBestComments` service to paginate correctly since we're
using a custom query to determine the top comments that goes beyond the
scope of what `findPage` is capable of.
- Updated CommentsController and CommentsService to support custom order
parameters.
- Added tests
no issue
- filtering was previously added to breadcrumbs but that wasn't enough to clean up Sentry reports
- added filtering to the `beforeSend` hook too so reports don't get cluttered with unhelpful XHR noise
ref https://app.incident.io/ghost/incidents/117
- the authenticate call made as part of signup was missed as part of the update when we adjusted the params for `cookie` authenticator's `authenticate` method in Admin so it could switch behaviour for 2fa
- fixed the authenticate call params and updated our mocked `/session` endpoint to check for expected POST data which would have let tests catch this error
closes https://linear.app/ghost/issue/ENG-1658
- switched to using a task to match patterns elsewhere and have better cancellation behaviour if code is re-used in a short-lived component
- added `drop: true` task modifier to our main tasks so they can't be triggered again whilst we're waiting on an API request
- removed confusing countdown in button text
- restored forced "text" data type for resend API request to match API behavior
- added acceptance tests for resend behaviour
This adds a `content_api_url` helper, returning the url for Ghost's
Content API. By default it will return an absolute URL but can be
passed `absolute=false` if a relative URL is wanted.
This works in tandem with the `content_api_key` helper to
facilitate third party integrations with the Content API, for
example - custom Portal or Search implementations.
no issue
- Fixed a flaky [publishing
test](https://github.com/TryGhost/Ghost/actions/runs/11509561903/job/32039943951)
that was suffering from a race condition. It was trying to copy the
bookmark link shown on the publishing complete modal, but it was
sometimes already closed by that point.
- It seemed to pass consistently locally, but in CI it would frequently
fail. This commit should wait to copy the link before closing the modal.
no issue
- Browser tests in CI were yielding a passing result even if one or more
tests failed (including retries).
- The `yarn dev` command that triggers the browser tests in CI was
catching any errors and exiting with code 0, resulting in a ✅ in CI.
- This commit changes `yarn dev` to exit with code 1 if the browser
tests fail, so that CI will correctly fail if any of the browser tests
fail.
no issue
- Dev Containers let you work on Ghost in a consistent, isolated
environment with all the necessary development dependencies
pre-installed. VSCode (or Cursor) can effectively run _inside_ the
container, providing a local quality development environment while
working in a well-defined, isolated environment.
- For now the default setup only works with "Clone repository in
Container Volume" or "Clone PR in Container Volume" — this allows for a
super quick and simple setup. We can also introduce another
configuration to allow opening an existing local checkout in a Dev
Container, but that's not quite ready yet.
- This PR also added the `yarn clean:hard` command which: deletes all
node_modules, cleans the yarn cache, and cleans the NX cache. This will
be necessary for opening a local checkout in a Dev Container.
- To learn more about Dev Containers, read this guide from VSCode:
https://code.visualstudio.com/docs/devcontainers/containers#_personalizing-with-dotfile-repositories
---------
Co-authored-by: Joe Grigg <joe@ghost.org>
Co-authored-by: Steve Larson <9larsons@gmail.com>
- Adding custom fonts for themes behind a feature flag
- Introduces new `@tryghost/custom-fonts` module to manage custom fonts
- UI updates for Branding and Theme settings
---------
Co-authored-by: Fabien O'Carroll <fabien@allou.is>
Co-authored-by: Sodbileg Gansukh <sodbileg.gansukh@gmail.com>
Co-authored-by: Peter Zimon <peter.zimon@gmail.com>
Co-authored-by: Sanne de Vries <sannedv@protonmail.com>
Co-authored-by: Daniël van der Winden <danielvanderwinden@ghost.org>
- replaced a couple of uses of lodash.each in favor of native for loops
- tidied up `debug` statements and spacing
- pulled out common statements into variables
closes https://linear.app/ghost/issue/ONC-254closes#20771
The portal script, which is responsible for handling the recommendations popup,
was only loaded into the front end if either members or donations are enabled.
We're adding an extra condition to load it if recommendations are enabled.
We may want to consider splitting out this functionality into several scripts,
so that we don't have to load _everything_ if only one feature is enabled, but
that is outside the scope of this issue.
refs https://linear.app/ghost/issue/ONC-469
Hidden comments were not being purged from the cache, which resulted
in stale data being served, and hidden comments being visible.
refs https://linear.app/ghost/issue/ONC-469
Hidden comments were not being purged from the cache, which resulted
in stale data being served, and hidden comments being visible.
no ref.
The default routing for the portal app is a signup page. When a site has
no recommendations, the result of triggering that page (by following a
link that goes there) is a prompt to sign up, or a notification that one
cannot sign up, if membership is disabled.
This patch adds a "No recommendations" message, which will be shown if a
user follows a recommendations link (#/portal/recommendations) on a site
without recommendations. While we shouldn't end up there very often,
it'll make a lot more sense when it does!
closes https://linear.app/ghost/issue/ENG-1672
- removed input on-blur validation because it can be triggered when clicking reset button giving a misleading error state
- added client-side validation for 6-digit code
- added validation when submitting the form
- added error reset when typing in the code field, including removal of button failure state, so it's clearer you're in a new submit state
no issue
- previously we determined any 403 response was an indication that we should switch to the 2fa input screen during sign-in
- added a custom error that explicitly looks for an error with our `2FA_TOKEN_REQUIRED` code so we don't have any confusion when a non-2fa 403 is received for any reason and to have the option of moving away from the 403 if needed without breaking the client
- test to ensure our error 2fa-required error detection works correctly
- extracted duplicate steps in the authentication tests into a helper function
- fixed authentication tests so they better represent our API output of `errors` being an array
closes https://linear.app/tryghost/issue/ENG-1652/
- returning `undefined` from a task is equivalent to failing
- switched to returning `true` when we get the 2fa required error so the button stays in the neutral/success state
- added `SUCCESS` and `FAILURE` consts to better reflect control flow when returning from tasks and ensured we always return a value
ref ENG-1629
Use separate protection for the 2 endpoints as one can resend an
email, and the other is used to login -- each presents its own
security challenges.
After migrations run, any sessions made with the labs flag turned off
will have the verified flag set. We also need new sessions made after
that to gain the verified flag, so that they aren't logged out at the
point that the labs flag is enabled (or removed).
ref ENG-1641
Using `getUserFromSession` requires the cookie header to be set, but
at this point we may still be constructing the session. Instead we can
get the user id from the session itself
More typical in TOTP setups for each token to last 1 minute, and to
allow some older tokens.
Also moved the options setting out of the generate scope in case
verify is called first (unlikely but possible).
closes https://linear.app/tryghost/issue/ENG-1617/
closes https://linear.app/tryghost/issue/ENG-1619/
- updated cookie authenticator's `authenticate` method to accept an `{identification, pasword, token}` object
- if `token` is provided, hit our `PUT /session/verify/` endpoint passing through the token instead of hitting the `POST /session/` endpoint
- added `signin/verify` route
- displays a 2fa code input field, including required attributes for macOS auto-fill from email/messages to work
- uses `session.authenticate({token})` when submitted
- updated signin routine to detect token-required state
- detects a `403` response with a `2FA_TOKEN_REQUIRED` code property when authenticating
- if detected transitions to the `signin/verify` route
refs ENG-1622
Currently unused by the API, this session variable will be used to
confirm whether the user has authenticated their session with an email
OTP. The verified status is not removed on logout, so sessions are now
retained instead of being destroyed.
ref 324211f
- this includes changes to improve package size
Package size was found to be bloated due to expanding i18n strings. We
were packing all i18n strings instead of just the ones relevant to the
package. Thanks to @cathysarisky for identifying this!
ref https://linear.app/tryghost/issue/ENG-1653
- we were always setting a `style="background-color: #123456"` attribute on the buttons but that didn't allow for different button states such as the red failure state to correctly override meaning there was some odd behaviour when hovering
- removed the fixed `style` attribute and adjusted `<GhTaskButton>`
- added `@useAccentColor` prop
- when `@useAccentColor` is true, add the necessary `style` attribute except when showing the failure state
no issue
- The `test:*` commands in `ghost/core` are all implicitly dependent on
the TS packages in the whole monorepo being built, but we hadn't
explicitly declared this dependency to NX.
- Now if you run `yarn nx run ghost:test:e2e` (or any other `test:*`
commands in ghost), NX will know that it needs to rebuild the TS
packages, unless they are cached and haven't changed.
- With this, you should be able to directly clone the repo and run `yarn
nx run ghost:test:e2e` to run e2e tests, without running `yarn dev` or
`yarn nx run-many -t build:ts` first.
- This is especially useful for getting tests to run properly in docker
no issue
- I apparently never added @tryghost/metrics-server as a dependency to
ghost/core/package.json. It worked in most cases as a 'phantom
dependency' — yarn installs all node_modules in a flat structure, so
even though it wasn't a dependency in package.json, it still resolved to
the correct package, as long as the typescript packages were all built
first.
- This passed CI because we explicitly run ts:build on all packages
before running tests, and it worked in production because we build the
TS packages as part of the docker build. However, when trying to run
tests locally, it would sometimes fail unless you explicitly ran nx
run-many -t build:ts at the top level before running the tests.
- Adding it as a dependency in package.json fixes this problem.
ref https://linear.app/tryghost/issue/ONC-387/
With some recent changes, we added validation to unsubscribe URLs to verify the source, allowing us to cut down on spam and improving security, as the underlying key could be re-generated should the need arise. This had the side effect of making unsubscribe URLs difficult to reconstruct when using third-party/downstream integrations, such as ActiveCampaign, which fills a gap in the current Ghost feature set.
Now any authenticated query to `/api/members` will return an `unsubscribe_url` field that can be used directly.
no ref
When running tests, occasionally we'll see some varying sort in the
members api response because members are generally all created with the
same timestamp. While `ObjectId` should be progressive, and our defalut
sort is `ORDER BY created_at desc, id desc`, we still would sometimes
see issues. This ought to remove any flakiness.
no ref
Stubbed expected test errors. In general, we should be expecting these
errors in the tests as we write them as that is the expected behavior
(or that behavior should change).
closes#16748
The members/:member_id/signin_urls endpoint currently only does
cookie-based authentication. When #21249 is merged, turning on 2FA is
going to break any 3rd party processes that use it (including my social
sign-in offering).
This patch gives admin API keys 'read' permission on this endpoint, and
enables 3rd party processes to handle user logins the right way, instead
of via a staff member's email/password.
Migration included. Feedback appreciated.
I have the wrong name on my migration. I can see it doesn't follow the
naming convention, but I'm not sure how the names are generated.
---------
Co-authored-by: Michael Barrett <mike182uk@gmail.com>
ref https://linear.app/tryghost/issue/ONC-433
- due to a regression introduced in commit 871d21a, incoming
recommendations were not rendering in Admin Settings anymore, as they
were marked as deleted
- this commit updates the refresh logic of incoming recommendations on
boot: previously deleted incoming recommendations are refetched, and if
now available, restored
- when a recommendation is restored, we don't send a staff email
notification
ref https://linear.app/tryghost/issue/ONC-433
- due to a regression introduced in commit 871d21a, incoming
recommendations were not rendering in Admin Settings anymore, as they
were marked as deleted
- this commit updates the refresh logic of incoming recommendations on
boot: previously deleted incoming recommendations are refetched, and if
now available, restored
- when a recommendation is restored, we don't send a staff email
notification
- concat is too heavy of a function to call on the hotpath, so we can
just replace it with a native spread, which is much faster
- this cuts ~1.5% from boot time for sites with a lot of posts
No issue
Wrapped a missing /month and /year string. Should work with and without
trials now.
Added a few additional translations for Japanese and French.
Adjusted German - some strings were too long to fit in the layout.
Changed (the German equivalents of) "Start a X day free trial" to
"Select", because the German is just not going to fit on the button.
ref https://linear.app/tryghost/issue/DEV-32/remove-migratejs-script
- we want to switch to using this code path instead of our separate
migrate.js script on Pro
- the main things we're missing are metrics + monitoring for when things
go wrong, so this adds that to the DatabaseStateManager
- this allows us to eventually delete the script without losing
functionality
- the code kept an array of IDs, and would check new entries against the
values of this array
- this algorithm is O(n^2) and became quite slow when the site had a lot
of redirects
- we can do away with this entirely, and just compute the keys of the
redirects to get the IDs
- this speeds up loading redirects by 3x or so
- this will prevent the `ready` variable from being set to true if there
is an error with minification, as we have not correctly generated the
assets yet
- we don't need any of these fields to do URL service calculations, so
we can exclude them from being fetched, which improves performance of
URL service init
- ultimately, we should switch this to an include list to make this more
explicit
- this code is on the hotpath for the URL service and has shown to be
slow for sites with a lot of posts
- this is due to the overhead of the lodash functions we use here
- we can take advantage of how JS executes if-statements and move the
variable into the if-statement, which lazy evaluates it (for the URL
service, this branch is not hit, so it's a big win)
- this cuts about 2% from CPU time
- this code is a hotpath for the URL service and has shown to be slow
for sites with a lot of posts
- this is because of the overhead of lodash
- we can just do away with lodash and use native JS, which has
a negligible performance cost
- this cuts about 5% CPU time during boot of large sites
- moment calls are unbelievably heavy and we should do away with it
where possible
- this code doesn't need moment and we can just use native JS Date here
- this saves about 5% CPU time when booting sites with a lot of posts
- this code has shown to be chronically slow, due to the `Object.assign`
- we don't really need this, as we can just use a normal assign in this
case
- this cuts 15% CPU from boot time for sites with a lot of resources
(posts)
- it turns out we it the false case of this if-statement quite a lot,
and _generateUrl is heavy enough that we should try and do it less
- by moving it into the if-statement, we cut 4% CPU time from boot on
heavy sites
- lodash adds non-negligible runtime to this loop, so we can just
replace it with native JS and cut 3% CPU time from boot for sites
with a large number of posts
- we have to deserialize the values from the DB to turn them into moment + boolean values
- the use of lodash adds unnecessary overhead to the function, and writing it in native JS
is a low faster
- also fixes the naming of the functions to make it clearer in flamegraphs
no issue
- config helpers are required early during boot and it requiring lodash added some unnecessary require+compile time
- switched to using an inlined escapeRegExp function in place of requiring lodash
no issue
- config utils are required early during boot and it requiring lodash added some unnecessary require+compile time
- switched to using native JS for the few lodash methods we used
no issue
- bumped gscan version to provide `skipChecks` flag to `check` function
- added `optimization:themes:skipBootChecks` config flag defaulting to `false` to maintain current behaviour
- updated theme service initialization to use `skipChecks: true` when the config flag is set
- we only want to skip the checks during boot in specific cases to improve performance, they are still useful for general development and any production use-cases where themes get edited directly on the server
- updated our theme validate module to accept and pass through `skipChecks` option
- switched the `isZip` positional argument of `validate.check()` to an options object property to make usage cleaner
- right now, we minify the assets on boot. This is wasteful because they're not even needed
- this commit implements a change which lazy-minifies these assets and
allows for cache invalidation when the theme changes
- it also introduces some middleware that each asset calls to ensure
that the assets are minified before serving
