Repository navigation
Stats: Report uncaught dashboard JavaScript errors to Logstash - #54669
Draft
adamwoodnz wants to merge 37 commits into
Draft
adamwoodnz wants to merge 37 commits into
adamwoodnz wants to merge 37 commits into
Conversation
The JavaScript error reporter needs to know, before it installs any window listener, whether the host qualifies, which package version to tag events with, and which stack frames are dashboard code. Deciding the host in one PHP condition means widening reporting to self-hosted sites later is a change to that line alone. The origin list is filterable so sibling packages that ship their own build can add their base URL.
Reporting must skip errors that did not come from dashboard code. Matching the first stack frame against the allowlisted script origins gives a cheap, robust test that works on minified V8 and Firefox stacks and also yields the module and widget for the report.
A lazy-load failure message such as 'Failed to fetch dynamically imported module: <build url>' sits on the Error header line and matched the origin, so network failures were reported as dashboard crashes. Require the V8 or Firefox/Safari frame shape before matching, and cover the empty-origin guard.
Frames such as 'module code@<url>' carry a space before the @. The previous shape check rejected them, so a load-time crash could lose its only dashboard frame. Allow any label up to the first colon, which still rejects an 'Error: message' header.
Error reports carry the page's query string. Only parameters the dashboard defines keep their value; every other one is reported by name with its value redacted.
A parameter name is part of the payload and can carry what a site owner typed, such as an email address with no equals sign. Names outside a safe pattern now collapse into one [unreportable] key, and the result has no prototype so names like __proto__ are reported like any other.
The js-error endpoint bans an IP for 60 minutes after 30 requests in an hour, and that ban also silences the other Jetpack reporters on the same IP, Comments included. A render error that loops must therefore cost at most one request per message and frame, 10 per browser per hour, and 3 per page when storage is unavailable. Any non-2xx response stops reporting for the rest of the hour. Window errors and unhandled rejections report only when the stack has a dashboard frame, because wp-admin also runs other plugins' and extensions' scripts. A boundary catch always reports, since React already pinned the error to our tree.
A stack frame for code in the page document carries the page URL, and an error message can embed it. That text bypassed redactQuery, so the query string and hash reached the log even though the url field drops them. Both are now replaced by exact string in the message, trace and component stack. The endpoint rejects an oversized keepalive request, which costs a report and a budget slot for nothing. The message is capped at 1000 characters and the trace and component stack at 8000 each.
A budget record that failed to parse threw before the write, so that browser stayed on the 3 per page cap on every reload and the record was never repaired. The parse now has its own guard and a corrupt record counts as absent. Error text and stack frames can embed other absolute URLs, such as a failed REST request that carries a nonce. Every http(s) URL in the message, trace and component stack is now cut at its first query or hash, keeping a trailing stack position. The payload now has a type that names the only top-level keys the endpoint keeps, because an extra key is dropped silently. The thrown value is described once per report so the dedupe key and the payload cannot disagree. New tests cover the stop flag across an hour boundary, duplicates not spending budget, and the stored record guards.
Uncaught errors and unhandled rejections outside React never reach the error boundary. Installing the listeners at the top of init() also catches a failure later in init() itself. The listeners only choose candidates; reportError does the filtering and budgeting.
Rejections count only when the reason is an Error with a stack, so handled API failures such as apiFetch's { code, message } objects stay out of the log.
The host boundaries log nothing, so a render crash never reached the team. This boundary reports the error, then rethrows the same object so the host fallback UI still renders unchanged.
A thrown null or 0 made the caught check fail, so children rendered again, threw again and looped until React's update limit hid the real error. Wrapping the value distinguishes presence from falsiness. Also drops a test that could not fail.
An empty message gets a 400 from the endpoint, and any non-2xx response stops reporting for the rest of the hour. A single throw of an empty string would therefore have silently switched error reporting off for that browser. The primitive path now falls back the way the object path already does, for an empty or whitespace-only result.
Nothing reported a React render crash until the error boundary was mounted. The seven route stages and WidgetRoot together cover every surface the dashboard renders, including widgets other plugins ship through the SDK. The boundary rethrows after reporting, so the host fallback is unchanged.
…cement A hand-listed route test stays green when someone adds a route whose stage() is unwrapped, which is the bug it exists to catch. The test now reads routes/ and checks every stage. A second WidgetRoot test makes a provider throw, so moving the boundary inside the providers fails.
…discovery A boundary moved between the two providers still passed the ChartsProvider test, leaving a crash in AnalyticsQueryClientProvider unreported. The discovery test also skipped a route whose stage lives in stage.ts.
A window error thrown by a widget another package ships has no stack frame under the Premium Analytics build, so the dashboard's error reporter dropped it. Each widget package now appends its own build URL to the origin filter from register_widget_types(), the one entry point both the init hook and the WordPress.com Simple mu-plugin call.
The package origins are added while the widget type registry hydrates. The reporter now triggers that itself, so the order no longer depends on generated build code. The has_filter guard went too: add_filter already dedupes, and the origin test now checks the origin prefixes a real asset URL.
The registry hydration test nulled the Widget_Type_Registry singleton and set a widget-manifest global, then undid both after calling get_config(). WorDBless's BaseTestCase restores $wp_filter from an #[After] method, so the hook removals were redundant, but it cannot restore a $GLOBALS entry or a nulled singleton. A failed assertion inside get_config() therefore skipped that cleanup and left the registry hydrated from an empty manifest, which fails later tests that read it for reasons they do not cause.
…eporter Deleting the filter registration in register_admin_page() left every test green while the script data key vanished and the reporter silently turned off, so one wiring test now pins it. The README records the rules a reader cannot get from the code: the PHP-owned gate, the origin filter other packages extend, and why the hourly budget exists (the endpoint's IP ban would silence other Jetpack reporters).
Three claims did not match the code. The budget is a clock-hour bucket, not a rolling window, so a reader planning around it would expect a report to free up sixty minutes after it was sent. The window handlers are installed by installGlobalErrorHandlers, not installGlobalHandlers, which is the name a reader would grep for and fail to find. The origin list already holds this package's own build URL before any filter runs, which the filter section read as though it started empty. Also records that the boundary rethrows after reporting, since nothing else says the host still renders its own fallback.
The reporter held its dedupe set, page counter and stop flag at module scope, but the package was bundled into each build unit that imported it: init, widgets-toolkit and all seven route stages. Each copy kept its own state, so the per-page-load fallback cap counted per build unit and a route change could reset the dedupe set. Declaring the module exports makes wp-build externalize it, the way the eight other stateful internal packages are, leaving one instance. The package installs window listeners and holds state, so it is not side-effect free either.
Any non-2xx used to disable the browser for the rest of the hour, so a systematic 4xx would silence every browser after its first report, and zero reports in the log reads exactly like a healthy dashboard. The endpoint's IP block is what the budget guards against and it answers 403, so only 403 and 429 stop the hour now; a 400 means a payload to fix, which the hourly budget already bounds. Every refusal is named once in the console outside production, so this cannot go quiet again unnoticed. The dedupe key used the raw message, so a message embedding the page query string produced a new key on every date-range change and one repeating bug could spend the budget that protects every reporter behind the same IP. It now keys on the message that goes into the payload. A missing origins array would have thrown inside the reporter's catch and lost every report with no signal. PHP publishes the array with the gate, so it cannot happen today, but the failure is total and the guard is free. The endpoint drops an unknown top-level key silently, so a test now pins the posted keys to the names it keeps, and the two the payload never set are gone from the interface rather than inviting someone to fill them in.
Site owners read this entry, and the name of the pipeline the reports end up in means nothing to them. Where the reports go is what they can act on.
The filter result went straight to array_values, so a third party returning a non-array fataled the dashboard request, an entry without a trailing slash produced module paths with a leading slash and no widget name, and a bare slash matched every stack line, which would attribute every error on the page to dashboard code. The package's own origin is kept first either way.
Each package's existing origin test took a path that clears the guard, so moving the add_filter call below it still passed in all three. That placement is the whole point: WordPress.com Simple calls the registrant directly, and the origin has to reach the dashboard on the runs where the registrant registers nothing. Moving the call now fails the new case.
An origin filter returning "https://" or "/build/" passed every check: trailingslashit leaves them alone, and neither is the bare slash the guard looked for. Matched with a substring test, each then flagged every frame of that shape as dashboard code, which is the blast radius the guard exists to prevent. Origins now need a host. Dropping "sideEffects": false from the manifest did not have the effect it looked like. wp-build reads moduleSideEffects as a boolean cast of the field, so an absent field is still false, and a future side-effect-only import of this package could be elided along with the listeners it installs. Declared true. The console warning for a refused report fired once per page load, so a harmless 400 permanently hid a later 403 or 429, the two statuses that stop reporting for the rest of the hour and the ones an operator needs to see. It now fires once per status. The bare-slash rationale sat in both the function and its test. The function owns it.
Each parameter name in the query object became a field in a log index that every other reporter shares, so one dashboard URL could widen the index for everybody. Reporting the query as a single masked string keeps it to one field. An unsafe parameter name is now dropped rather than used as a key: in string form the name is the leak, and there is no key left to hide it behind. A single marker records that something was removed. `properties.route` read `p` from that object, so it reads the page query directly instead. It is what a Logstash search groups on and must not regress.
An error message or a stack can carry the address of whoever was signed in, and no personal data may leave the browser. The pass is deliberately broad, because under-matching leaks an address while over-matching costs a few characters, and it runs before the length cap so a redaction cannot be cut in half.
…ials A cross-origin fetch sends no cookies today, so this changes nothing now. It stops a future change to that default from attaching a signed-in identity to a report that is meant to be anonymous. Unlike a custom header, it triggers no CORS preflight, which this endpoint would never answer.
The top-level `jetpack_version` conventionally carries the Jetpack plugin version, so filling it with the Premium Analytics package version makes the shared field disagree with every other reporter that sets it. The comments reporter already puts its own package version in `properties.version`, so follow it. The endpoint contract constant keeps `jetpack_version`: it records what the endpoint accepts, not what we send.
Narrowing the stop to the two rate-limit statuses assumed a refusal of another kind costs nothing. It does: a request sent while a ban is already active restarts the ban, so continuing to try extends the block that reaches every other reporter sharing the IP. Any refusal is now a reason to stop for the hour. The per-status console warning stays. It is what keeps a silent stall from looking like a healthy dashboard.
…ude it A tab left open across a deploy asks for a module file that is gone. React.lazy turns that into a render error, so the boundary reports it even though it is not a bug of ours, and it crowds out real crashes in a search. Tagging the report lets a search drop them without losing the record that it happened. The window path already ignores these, because the messages carry no stack frames. Reloading the page would be the other answer, and a user-visible change is an anti-goal here.
The README described the stop behaviour that has since been reverted, and said nothing about why the query is a string, why the version is not in the field whose name suggests it, or what the chunk-load tag is for. Those are the parts a reader cannot recover from the code.
The chunk-load tag matched the error message only, which catches how Chrome, Firefox and Safari word a failed dynamic import. webpack says it differently: the message is "Loading chunk 7 failed." and the only sign is the name, ChunkLoadError. Those reports would have landed in the feature's search untagged, which is the deploy-window noise the tag exists to let a search exclude.
A translation tool or an extension that reparents React-managed text nodes crashes the commit phase with a NotFoundError from insertBefore. The boundary reports it like any other render error, so without a marker those land in this feature's search indistinguishable from our own bugs, and there is no way to tell from the stack that the page was rewritten under React. The probes key on artefacts the agents leave behind and run only while a report is being built. They are collapsed into one properties field naming the agents found, rather than one field per agent, because the log index is shared with every other reporter posting to this endpoint.
adamwoodnz
added
Enhancement
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const url = line.slice( start ).split( /[\s)]/ )[ 0 ]; | ||
| const position = POSITION.exec( url ); | ||
| const withoutPosition = position ? url.slice( 0, position.index ) : url; | ||
| const path = withoutPosition.slice( origin.length ).replace( /[?#].*$/, '' ); |
Code Coverage SummaryCoverage changed in 7 files. Only the first 5 are listed here.
8 files are newly checked for coverage. Only the first 5 are listed here.
|
This branch has not been deployed
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
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.
Fixes STATS-582
Why
Odyssey (Classic Stats) sent uncaught JavaScript errors to Sentry. That loader was removed for security, and Stats v2 has no error reporting of its own, so a crash in the Premium Analytics dashboard is invisible to us: we only learn about it if a user reports it, and we cannot reproduce most of them. This sends uncaught dashboard exceptions to WordPress.com's JS error log under their own feature, so we can see them.
Proposed changes
packages/error-reporting, owns the reporter: payload assembly, stack-frame matching against a PHP-supplied origin allowlist, redaction, dedupe, an hourly budget, and the transport.premium_analyticsscript data. Reporting is on only for WordPress.com Simple and Atomic sites.packages/initinstalls the windowerrorandunhandledrejectionlisteners. A rejection is reported only when its reason is anErrorwith a matching stack, so anapiFetchfailure object is not an error report.StatsErrorBoundarymounts inside every routestage()and insideWidgetRoot. It reports fromcomponentDidCatch, then rethrows the original error so the host's own boundary renders its normal fallback.packages/ads,packages/videopressandpackages/woocommerce-statseach add their own build URL through a new filter, so a window error from any registered widget is recognised rather than dropped as foreign.Does this pull request change what data or activity we track or use?
Yes. This is the reason for the
[Status] Needs Privacy Updateslabel.On WordPress.com Simple and Atomic sites only, an uncaught dashboard exception now sends one request to
public-api.wordpress.com/rest/v1.1/js-errorcontaining: the error message, its stack, its name, the React component stack for a render crash, the page URL as origin plus pathname, the WordPress.com blog ID, the Premium Analytics package version, the route and widget names, and the page's query parameters as one masked string.What is deliberately excluded, and the code that excludes it:
urlisorigin + pathname. The message, stack and component stack have the page's own query and hash stripped, and the query of any absolute URL in them stripped, which covers a_wpnonceappearing inside a stack.credentials: 'omit', so no signed-in identity is attached to an anonymous report.Terms_Of_Service::has_agreed().Rate limiting is deliberate, because the endpoint limits by IP and a block there reaches every other Jetpack reporter sharing that IP, Comments included: reports are deduped for the life of the page, capped at 10 per browser per hour, and stop entirely for the rest of the hour on any refusal from the endpoint.
Related product discussion/links
jetpack_statsto the endpoint's feature allowlist: 257838-ghe-Automattic/wpcomBlocked on
Do not merge before 257838-ghe-Automattic/wpcom has deployed. Until then the endpoint rewrites an unknown
featuretocalypso, so reports would land in the wrong search rather than failing visibly.Testing instructions
The gate answers only to Simple and Atomic, so a self-hosted dev site reports nothing by design. To exercise it locally, filter
jetpack_admin_js_script_dataat priority 21 from an mu-plugin and setpremium_analytics.error_reporting.enabledto true.On the dashboard (
?page=jetpack-premium-analytics-wp-admin):JetpackScriptData.premium_analytics.error_reporting.script_originsagainst thesrcof the dashboard's loaded module scripts. Each loaded script URL must start with one of the origins. There should be four origins: Premium Analytics, woocommerce-stats, videopress and ads.properties.source: boundary, and that the widget's normal fallback UI still appears, unchanged.properties.source: error.Errorthrown from dashboard code, and confirmproperties.source: unhandledrejection. Reject with a plain object and confirm nothing is sent._wpnonceand a fragment, and confirm neither reaches the payload.script_originsis still published.Watch the request in the Network panel rather than Logstash until the wpcom PR deploys.
What still needs checking on Simple or Atomic
The origin allowlist is derived from
plugins_url(), and the scripts are served as WordPress Script Modules. Those resolve to the same string on a self-hosted site, which I verified below, but if WordPress.com rewrites script-module URLs through a CDN and notplugins_url(), everyerrorandunhandledrejectionreport would be silently dropped there. Please confirm step 1 on a Simple sandbox or an Atomic site. Render crashes are unaffected either way, because a boundary catch skips the origin filter.Verification
Checks run, all green:
premium-analytics4901 JS tests and 837 PHP tests,ads9,woocommerce-stats144, monorepotypecheck,lint-changed, and PHPCS on every changed PHP file.Two notes on what I could not verify locally:
wordpress-stubs.phpunder this machine's PHP 8.5, identically ontrunkfor untouched packages, and the only other local PHP has noastextension. CI covers it.videopresshas 2 pre-existing test failures inAdmin_UI_TestandChannel_Blocks_Test, in a built checkout with local absolute paths. Neither test nor its subject is touched by this branch, which changes onlyclass-analytics-dashboard.phpand its two test files there.Exercised in a browser on a dev site with the gate forced on, which confirmed: the real loaded script URL starts with the published origin (the property the whole origin design rests on, and the one no unit test can prove); a
_wpnonceand fragment embedded in an error message are both absent from the payload; the payload's nine top-level keys are all within the eleven the endpoint keeps, with no headers andkeepaliveset; and with the gate off, a dashboard error produces no request whilescript_originsis still published.The
boundarysource and the host fallback were not exercised by a real render crash in the browser. They rest on 11 mount-point tests plus the host boundaries in@wordpress/bootand@wordpress/widget-dashboard, which wrap every stage and every widget.Follow-ups
Terms_Of_Service::has_agreed().🤖 Generated with Claude Code