Improved string escaping for NQL filters and email From headers#28463
Improved string escaping for NQL filters and email From headers#284639larsons wants to merge 2 commits into
Conversation
WalkthroughThis PR adds least-privilege security hardening to four GitHub Actions workflows (CI, Release, Stale, Stale i18n) by introducing explicit 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 docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ghost/core/core/server/services/email-address/email-address-parser.js (1)
49-49: ⚡ Quick winAdd a regression test for mixed backslash+quote display names.
This escaping-order fix is good, but it should be pinned with a unit case that includes both
\and"innameto prevent future reintroductions of double/under-escaping.🤖 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/services/email-address/email-address-parser.js` at line 49, Add a regression unit test for email-address-parser that exercises mixed backslash+quote display names to lock in the escaping-order fix: construct an email object whose name contains both backslashes and double quotes (e.g. contains "\" and \\), call the parsing/formatting function that uses escapedName (the code path that does email.name.replace(/\\/g, '\\\\').replace(/"/g, '\\"')), and assert the output matches the expected RFC-safe escaped display name; put the test alongside existing tests for the email-address-parser module so future changes to escapedName or the replace order will fail the test if broken.ghost/admin/app/components/gh-resource-select.js (1)
14-21: ⚡ Quick winConsider alignment with
apps/postsescaping behavior.The
escapeNqlStringhelper here only escapes single quotes (' → \'), but theapps/postsimplementation (test shown in context) also escapes backslashes when they precede quotes to handle inputs liketest\'valuecorrectly. This inconsistency could cause issues if search terms flow between subsystems or if developers expect uniform behavior across the codebase.If the decision to skip backslash-before-quote escaping is intentional for search UX reasons (to avoid corrupting benign terms), consider adding an explicit note in the comment explaining this tradeoff and that it differs from the
apps/postsescaping contract.🤖 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/admin/app/components/gh-resource-select.js` around lines 14 - 21, The escapeNqlString helper currently only replaces single quotes but should match the apps/posts behavior: update function escapeNqlString to also escape backslashes that precede quotes (so inputs like test\'value become safe) and update the top comment to explain the escaping rule and why it matches apps/posts; alternatively, if you intentionally want different behavior, explicitly note that in the comment and describe the tradeoff versus apps/posts’ escaping contract so the inconsistency is documented.ghost/core/core/server/services/members/members-api/controllers/router-controller.js (1)
1091-1094: 💤 Low valueClarify why both quote types are escaped here.
The comment mentions "single-quoted NQL filter," but the code escapes both double and single quotes (
replace(/("|')/g, '\\$1')), which differs from the other NQL escaping sites in this PR that only escape single quotes. While this is more defensive and matches the test expectations (context snippet), the comment should explain the rationale—for example, that newsletter names may contain either quote type or that thename:[...]array syntax has stricter requirements than the~'...'search operator.🤖 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/services/members/members-api/controllers/router-controller.js` around lines 1091 - 1094, Update the comment above the requestedNewsletterNamesFilter creation to explain why both single and double quotes are escaped: state that newsletter names can contain either quote character and that the NQL name:[...] array syntax (used here) requires escaping both quote types to safely embed names inside single-quoted array items, which is why replace(/("|')/g, '\\$1') is used instead of only escaping single quotes; reference the requestedNewsletterNamesFilter variable and the replace(...) call in the comment so future readers understand the defensive choice and its relation to the name:[...] vs ~'...' operator differences.
🤖 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 `@apps/admin-x-framework/src/hooks/use-filterable-api.ts`:
- Around line 15-22: The function escapeNqlString currently returns an already
quoted string which collides with another implementation that returns only an
escaped value; rename this implementation to escapeAndQuoteNqlString (and update
its callers in use-filterable-api.ts to call the new name) so behavior is
explicit, or alternatively extract a single shared helper (e.g., export
escapeNql and escapeAndQuoteNql from a new common util) and update both this
module and gh-resource-select.js to import and use the appropriate helper; make
sure the symbol names and usages (escapeNqlString, escapeAndQuoteNqlString,
escapeNql) are consistent across callers.
In `@ghost/admin/app/services/labels-manager.js`:
- Around line 76-80: The inline escaping in labels-manager.js (creating safeTerm
via term.replace(/'/g, `\\'`) before using it in the NQL filter should be
replaced with the shared escapeNqlString helper to avoid duplicated logic;
extract escapeNqlString from gh-resource-select.js into a small shared util
(e.g., export function escapeNqlString(term)), import that util into
labels-manager.js, and call escapeNqlString(term) where safeTerm is currently
created so the query uses the shared, single-source escaping logic.
In `@ghost/admin/app/services/tags-manager.js`:
- Around line 26-30: Replace the inline escaping in tags-manager.js with the
shared escapeNqlString helper: remove the manual term.replace(/'/g, `\\'`) and
instead import escapeNqlString from the shared utility (the function currently
used in gh-resource-select.js after you extract it to a shared module), call
const safeTerm = escapeNqlString(term) and pass that into
this.store.query('tag', {filter: `tags.name:~'${safeTerm}'`, limit: 100, page,
order: 'name asc'}); ensure the helper is exported from the new shared util and
the import is added at the top of this file so all NQL escaping is centralized.
---
Nitpick comments:
In `@ghost/admin/app/components/gh-resource-select.js`:
- Around line 14-21: The escapeNqlString helper currently only replaces single
quotes but should match the apps/posts behavior: update function escapeNqlString
to also escape backslashes that precede quotes (so inputs like test\'value
become safe) and update the top comment to explain the escaping rule and why it
matches apps/posts; alternatively, if you intentionally want different behavior,
explicitly note that in the comment and describe the tradeoff versus apps/posts’
escaping contract so the inconsistency is documented.
In `@ghost/core/core/server/services/email-address/email-address-parser.js`:
- Line 49: Add a regression unit test for email-address-parser that exercises
mixed backslash+quote display names to lock in the escaping-order fix: construct
an email object whose name contains both backslashes and double quotes (e.g.
contains "\" and \\), call the parsing/formatting function that uses escapedName
(the code path that does email.name.replace(/\\/g, '\\\\').replace(/"/g,
'\\"')), and assert the output matches the expected RFC-safe escaped display
name; put the test alongside existing tests for the email-address-parser module
so future changes to escapedName or the replace order will fail the test if
broken.
In
`@ghost/core/core/server/services/members/members-api/controllers/router-controller.js`:
- Around line 1091-1094: Update the comment above the
requestedNewsletterNamesFilter creation to explain why both single and double
quotes are escaped: state that newsletter names can contain either quote
character and that the NQL name:[...] array syntax (used here) requires escaping
both quote types to safely embed names inside single-quoted array items, which
is why replace(/("|')/g, '\\$1') is used instead of only escaping single quotes;
reference the requestedNewsletterNamesFilter variable and the replace(...) call
in the comment so future readers understand the defensive choice and its
relation to the name:[...] vs ~'...' operator differences.
🪄 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: 2ed67494-bf88-444d-a51c-16e5dfa28bda
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/stale-i18n.yml.github/workflows/stale.ymlapps/admin-x-framework/src/hooks/use-filterable-api.tsghost/admin/app/components/gh-resource-select.jsghost/admin/app/services/labels-manager.jsghost/admin/app/services/tags-manager.jsghost/core/core/server/services/email-address/email-address-parser.jsghost/core/core/server/services/email-service/email-renderer.jsghost/core/core/server/services/members/members-api/controllers/router-controller.js
| // Escape a value for safe embedding in a single-quoted NQL filter string. | ||
| // The NQL lexer only treats `\'`/`\"` as escape sequences and reads a lone | ||
| // backslash as a literal character, so escaping every single quote (globally) | ||
| // is sufficient to prevent filter breakout. Backslashes must NOT be doubled — | ||
| // doing so corrupts benign terms that legitimately contain a backslash. | ||
| const escapeNqlString = (value: string) => { | ||
| return '\'' + value.replace(/'/g, '\\\'') + '\''; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Function name collision with different behavior.
The escapeNqlString function here returns a single-quoted, escaped string ('<escaped>'), but the identically named function in ghost/admin/app/components/gh-resource-select.js returns only the escaped value without quotes (caller adds quotes separately). Having the same function name with different signatures and behavior across the codebase creates confusion and maintenance risk.
Consider one of these approaches:
- Extract a shared helper into a common utility module (e.g.,
@tryghost/nql-utils) that both files import - Use distinct names (e.g.,
escapeNqlStringvsescapeAndQuoteNqlString) to clarify the behavioral difference - Standardize on one approach (escaped-only or escaped-and-quoted) across all implementations
🤖 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 `@apps/admin-x-framework/src/hooks/use-filterable-api.ts` around lines 15 - 22,
The function escapeNqlString currently returns an already quoted string which
collides with another implementation that returns only an escaped value; rename
this implementation to escapeAndQuoteNqlString (and update its callers in
use-filterable-api.ts to call the new name) so behavior is explicit, or
alternatively extract a single shared helper (e.g., export escapeNql and
escapeAndQuoteNql from a new common util) and update both this module and
gh-resource-select.js to import and use the appropriate helper; make sure the
symbol names and usages (escapeNqlString, escapeAndQuoteNqlString, escapeNql)
are consistent across callers.
| // Escape every single quote so the term is safely embedded in the | ||
| // single-quoted NQL filter below. NQL reads a lone backslash literally | ||
| // (only `\'`/`\"` are escapes), so quotes alone need escaping. | ||
| const safeTerm = term.replace(/'/g, `\\'`); | ||
| const labels = yield this.store.query('label', {filter: `name:~'${safeTerm}'`, limit: PAGE_SIZE, page, order: 'name asc'}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consolidate escaping logic to use the shared helper.
This inline escaping (term.replace(/'/g, '\\'))) duplicates the logic now available in the escapeNqlString helper defined in ghost/admin/app/components/gh-resource-select.js (lines 14-21). For maintainability, extract escapeNqlString into a shared utility module and import it here.
This ensures:
- Single source of truth for NQL string escaping rules
- Easier updates if escaping requirements change (e.g., handling backslashes before quotes)
- Consistent behavior across label, tag, and resource search filters
🤖 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/admin/app/services/labels-manager.js` around lines 76 - 80, The inline
escaping in labels-manager.js (creating safeTerm via term.replace(/'/g, `\\'`)
before using it in the NQL filter should be replaced with the shared
escapeNqlString helper to avoid duplicated logic; extract escapeNqlString from
gh-resource-select.js into a small shared util (e.g., export function
escapeNqlString(term)), import that util into labels-manager.js, and call
escapeNqlString(term) where safeTerm is currently created so the query uses the
shared, single-source escaping logic.
| // Escape every single quote so the term is safely embedded in the | ||
| // single-quoted NQL filter below. NQL reads a lone backslash literally | ||
| // (only `\'`/`\"` are escapes), so quotes alone need escaping. | ||
| const safeTerm = term.replace(/'/g, `\\'`); | ||
| return yield this.store.query('tag', {filter: `tags.name:~'${safeTerm}'`, limit: 100, page, order: 'name asc'}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consolidate escaping logic to use the shared helper.
This inline escaping duplicates the escapeNqlString helper now defined in ghost/admin/app/components/gh-resource-select.js. Extract the helper into a shared utility module and import it here to maintain a single source of truth for NQL escaping rules and ensure consistent behavior across all search filters.
🤖 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/admin/app/services/tags-manager.js` around lines 26 - 30, Replace the
inline escaping in tags-manager.js with the shared escapeNqlString helper:
remove the manual term.replace(/'/g, `\\'`) and instead import escapeNqlString
from the shared utility (the function currently used in gh-resource-select.js
after you extract it to a shared module), call const safeTerm =
escapeNqlString(term) and pass that into this.store.query('tag', {filter:
`tags.name:~'${safeTerm}'`, limit: 100, page, order: 'name asc'}); ensure the
helper is exported from the new shared util and the import is added at the top
of this file so all NQL escaping is centralized.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #28463 +/- ##
==========================================
+ Coverage 73.74% 73.75% +0.01%
==========================================
Files 1541 1541
Lines 132180 132289 +109
Branches 15784 15804 +20
==========================================
+ Hits 97476 97576 +100
+ Misses 33737 33724 -13
- Partials 967 989 +22
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:
|
no ref - CodeQL (js/incomplete-sanitization) flagged several filter/header builders with incomplete escaping - gh-resource-select escaped only the first quote (String.replace with a string argument); it now escapes every quote via a shared helper - the email From name was escaped in email-renderer AND again in EmailAddressParser.stringify, double-escaping any title containing a double quote; the renderer now passes the raw name so stringify is the single escaping point (this also covers the previously unescaped newsletter sender_name) - email-address-parser now escapes backslashes before quotes - the NQL sites that already escaped all quotes are left functionally unchanged: NQL reads a lone backslash literally (verified against @tryghost/nql), so doubling backslashes would corrupt legitimate search terms; added comments explaining this
9larsons
force-pushed
the
claude/condescending-heyrovsky-49fb43
branch
from
26fc991 to
6d16fda
Compare
9larsons
changed the title
no ref - gh-resource-select escapeNqlString now returns the quoted string, matching the escapeNqlString contract used in apps/posts and admin-x-framework (it previously returned the unquoted value, a same-name/different-behavior collision flagged in review) - added a regression test for email-address-parser covering a name containing both a backslash and a double quote, locking in the escape-once ordering - expanded the router-controller comment to explain why the name:[...] array syntax escapes both quote characters
|
Thanks for the review — pushed Escaping backslashes (the
|
|
On the suggestion to consolidate the NQL escaping into a shared helper (the inline I kept these inline and individually documented rather than extracting a shared util. It's a single intentional line per site, and a helper imported across an Ember component plus two services adds indirection for little gain. With Happy to extract a (Replying here rather than inline — the REST review-reply endpoint is returning 401 for me at the moment.) |
| // every quote prevents breakout, and backslashes must NOT be doubled (doubling | ||
| // corrupts terms containing a backslash, e.g. `a\b` would be searched as `a\\b`). | ||
| function escapeNqlString(term) { | ||
| return '\'' + String(term).replace(/'/g, '\\\'') + '\''; |
2 participants
Summary
Resolves the
js/incomplete-sanitizationCodeQL alerts around NQL filter and email-header string building. (Split out from the broader code-scanning triage — the GitHub Actions permissions changes are in a separate PR so they can be reviewed in isolation.)gh-resource-selectescaped only the first quote in NQL search filters (String.replacewith a string argument) — now escapes every quote via a shared helper.Fromname was escaped inemail-rendererand again inEmailAddressParser.stringify, double-escaping any title containing a". The renderer now passes the raw name sostringifyis the single escaping point — which also covers the previously unescaped newslettersender_name.email-address-parsernow escapes backslashes before quotes.@tryghost/nql), so doubling backslashes would corrupt legitimate search terms. Added explanatory comments.Notes
Fromheaders for sites whose title or sender name contains a double quote.Test plan
ghost/coreemail-address-parser+email-rendererunit suites pass; lint-staged eslint clean on all changed files.Fromdouble-escape fix verified empirically againstEmailAddressParser.stringify(raw → stringify yields correct single-escaping).@tryghost/nql: breakout attempts parse as a singlenamecondition, benign terms (O'Brien,a\b) round-trip.