chore(security): standardise the default response headers - #7885
Open
ar2rsawseen wants to merge 5 commits into
Open
chore(security): standardise the default response headers#7885ar2rsawseen wants to merge 5 commits into
ar2rsawseen wants to merge 5 commits into
Conversation
Brings the shipped header set in line with current guidance, and makes the two default strings in this repo agree with each other and with countly-platform. Removed: - X-XSS-Protection. Deprecated and removed from every current browser, so it does nothing, and while it existed it could be turned against the page: steered into disabling one of the page's own scripts, and with mode=block the aborted load is observable cross-origin, which makes it an oracle for reading page contents. It is on the list of headers to remove rather than to send. - The `preload` token on Strict-Transport-Security. Preload needs the domain submitted to the browser preload list and is painful to reverse, and with includeSubDomains on a shared parent domain it can affect a customer's other services, so it should be opted into rather than shipped. max-age and includeSubDomains are unchanged. Added: Referrer-Policy and Permissions-Policy (both already in countly-platform), X-Content-Type-Options to the api set, which only the dashboard set had, and Cross-Origin-Opener-Policy and Cross-Origin-Resource-Policy on the dashboard set. Not added: Content-Security-Policy. The dashboard inlines countlyGlobal per request through res.expose, so its hash is not stable and a static policy would need 'unsafe-inline', which removes the protection CSP is there for. It needs that inline script dealt with first, or per-request nonces, neither of which a flat config string can express. Cross-Origin-Resource-Policy is deliberately not on the api set: that set carries Access-Control-Allow-Origin:* because web SDKs post from arbitrary customer origins, and the two work against each other. The isolation headers are applied globally, as the others are, and removed on the routes that are embedded from customer origins, which is what those routes already do for X-Frame-Options. Cross-Origin-Opener-Policy matters there because the SDK may open the popup as a window rather than an iframe, where same-origin would sever window.opener and any callback the SDK expects. Existing installs need the upgrade script. Config defaults only seed keys that are absent: checkConfigs uses getObjectDiff, which copies a default across only when the stored value is undefined, so an install that already has these keys would otherwise keep its original values forever and a change here would reach new installs only. The script edits rather than overwrites, so anything an operator added by hand survives, and it is idempotent. 12 unit tests over the rewrite, covering what it removes, what it adds, what it preserves, ordering, CRLF input and non-string input. Full unit suite 185 passing before, 197 after, same 2 pre-existing failures (Countly Request, network dependent). eslint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two corrections after walking the routes that would be affected. Cross-Origin-Resource-Policy is dropped entirely. It blocks any cross-origin subresource load, so every asset a customer site fetches from the Countly host would need an exemption, and the exemptions form a denylist that has to be complete to be safe. Walking it turned up more than the widget routes: /pixel.png is the no-JS tracking pixel, embedded as an <img> from customer pages and emails, and in countly-platform the surveys and content plugins each serve their own asset routes. A missed exemption there fails silently, with no console error and no failed request anyone would notice, only missing analytics data. That is the wrong risk to take for a header whose benefit is narrow, so it is left out until it can be applied to the dashboard routes as an allowlist rather than subtracted from everything as a denylist. Cross-Origin-Opener-Policy is relaxed from same-origin to same-origin-allow-popups. The dashboard opens popups from several places, including crashes-jira opening a cross-origin Jira URL, and plain same-origin severs the opener for any popup that navigates cross-origin. The relaxed value keeps the protection that matters, refusing an opener reference from a cross-origin document that opens us, without touching popups we open ourselves. The CORP removal on the star-rating routes goes with it, since nothing sets that header now. The COOP removal on the popup stays: an SDK may open it as a window rather than an iframe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing the CORP comment blocks left the two res.writeHead calls mis-indented. Those routes are now byte-identical to master again, which is the intent: nothing sets Cross-Origin-Resource-Policy any more, so they need no exemption. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The feedback widget routes remove X-Frame-Options so the widget can be embedded in customer pages, which leaves nothing describing where the widget may be framed. X-Frame-Options cannot express a domain list, but CSP frame-ancestors takes a source list, so the routes now send one built from the app's own configuration. The source list comes from app.plugins.allow_access_control_origin, the per-app newline separated origin list that already drives Access-Control-Allow-Origin in api/utils/common.js. One list now configures both, so a customer names their domains once. Behaviour: - An app with no usable origin configured gets no header at all. Apps that never filled the list in keep embedding exactly as before, so frame-ancestors 'none' is never used as a fallback. - X-Frame-Options is still removed. Browsers that see both apply the stricter one, which would block the embed regardless of the CSP. - The value is computed per request from the app behind the widget id, so it cannot live in security.dashboard_additional_headers, which is a static string. - Only well-formed origins (scheme, host, optional port) are emitted. Malformed entries are skipped individually, because a browser discards a whole directive containing one source it cannot parse. - 'self' is always included, since the dashboard previews these routes in an iframe of its own and nobody lists their Countly origin in a list meant for customer facing sites. app_domain is deliberately not used: it is optional, free text, never validated as an origin, and only used for display, audit logging and building view URLs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
allow_access_control_origin is labelled in the app management UI as "Allow Access-Control-Origin by listing separate origin (including https://) per line". Nothing tells a customer it governs framing, so an app that listed only the origin its SDK makes XHR from would have a working embed on another host blocked the moment the directive is enforced. Emit it through Content-Security-Policy-Report-Only instead, which blocks nothing and surfaces the violations, and add security. widget_frame_ancestors_enforce (default false) so an operator can promote it once the reports from a real deployment come back clean. Everything else is unchanged: X-Frame-Options still comes off, an empty or unusable list still means no header at all, and the enforced path is covered by tests so the promotion is a config change rather than a code change.
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
1 participant
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.
Standardises the shipped security response headers so the three repos agree, and drops the ones that should no longer be sent.
Removed
X-XSS-Protection. Deprecated and removed from every current browser, so it does nothing. While it existed it could be turned against the page: steered into disabling one of the page's own scripts, and withmode=blockthe aborted load is observable cross-origin, which makes it an oracle for reading page contents. It belongs on the remove list, and scanners flag its presence.The
preloadtoken on HSTS. Preload requires submitting the domain to the browser preload list and is painful to reverse; withincludeSubDomainson a shared parent domain it can affect a customer's other services.max-ageandincludeSubDomainsare unchanged — this only stops us shipping the irreversible part as a default.Added
Referrer-Policy,Permissions-Policy,Cross-Origin-Opener-Policy,Cross-Origin-Resource-Policy, andX-Content-Type-Optionswhere it was missing.Not added, and why
Content-Security-Policy. The classic dashboard inlinescountlyGlobalper request viares.expose, so its hash is not stable and a static policy would need'unsafe-inline'— which removes the protection CSP exists for. It needs that inline script dealt with, or per-request nonces, and a flat config string cannot express a nonce. This is why a CSP attempt here previously broke things rather than helping.Cross-Origin-Resource-Policyon the api set. That set carriesAccess-Control-Allow-Origin: *because web SDKs post from arbitrary customer origins. The two work against each other.The embeddable-route pattern
The isolation headers are applied globally, like the rest, and removed on the routes customers embed from their own origin — which is exactly what those routes already do for
X-Frame-Options.Cross-Origin-Opener-Policymatters there specifically: an SDK may open a widget as a window rather than an iframe, andsame-originwould severwindow.openeralong with any callback the SDK expects.Existing installs need the upgrade script
Config defaults only seed keys that are absent —
checkConfigsusesgetObjectDiff, which copies a default across only when the stored value isundefined. So an install that already has these keys keeps its original values forever, and a change here would reach new installs only. Without the migration this PR would be cosmetic for the existing fleet.The script edits rather than overwrites, so anything an operator added by hand survives: it drops the deprecated header, strips only the
preloadtoken from HSTS while leavingmax-age/includeSubDomains, and appends what is missing. It is idempotent.12 unit tests cover the rewrite: what it removes, what it adds, what it preserves, ordering, CRLF input, blank lines, case-insensitive header matching, and non-string input. It edits customer configuration, so both directions are asserted.
🤖 Generated with Claude Code