[Alerting] Alert flyout: labeled enums, tz-aware timestamps, named source, linkified runbooks (SRE2/OBS1)

[lezzago]

What & why

The alert-detail flyout contradicted the alerts table it was opened from, for the same alert: State/Severity shown as raw enums (active/critical), timestamps via toLocaleString() with no timezone, and the 'Source' row repeating the action label 'Open rule' instead of naming the rule/SLO. Now: Title-cased chips, zone-labelled timestamps (… PDT), the source row names the actual rule/SLO, URL-shaped annotations (runbooks) linkify as external links (SRE2), and SLO burn-rate alerts pivot to the SLO detail page (OBS1).

Findings: SRE2, OBS1, plus flyout↔table parity (State/Severity/timestamps/empty-glyph).

Before / after

Before / after

Flow

Testing

alert_detail_flyout.test.tsx + linkify_annotation.test.tsx: 27 tests pass. Centrally green.

Review

Independent review agent: APPROVE — verified the protocol-based XSS gate on linkify (only http/https), the cross-app SLO pivot, and full flyout↔table parity.

Regression check

The before/after above is a full-viewport capture, so adjacent components on the same surface are visible and unchanged — the diff is scoped to what's called out. Cross-component safety is also verified centrally: this change is file-disjoint from the other in-flight audit fixes (no file overlap, so it merges cleanly with them), and the affected plugin test suites pass with 0 new type errors vs main. Aside from the merge-order note below, it can be reviewed, merged, and reverted independently.

Dependency

Stacked on #2826 (shared primitives). This branch includes #2826's files so it builds and tests standalone in CI; merge #2826 first, then a rebase drops those files from this diff.

Draft — see the merge-order note above.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 91851ca)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Dead hashchange dispatch after cross-app navigation In navigateToSource, when sourceLink.appId is set (SLO cross-app pivot), the code calls navigateToApp and then dispatches a synthetic hashchange on the current window before the target app has mounted. The listener that would react lives in the destination app, which hasn't loaded yet, so the event is a no-op and the comment justifying it is misleading. If the target SLO app's HashRouter actually needs the event to route to #/slos/<id>, this pattern will race and may fail to land on the correct route on first navigation. if (sourceLink.appId) { // Cross-app navigation: the SLO detail page lives in a different OSD // application than the alerting app, so go through // `application.navigateToApp` to switch apps (workspace-aware), then // fire a synthetic hashchange for the target app's HashRouter to pick // up the route \u2014 navigateToApp uses pushState, which does not emit // hashchange on its own. coreRefs?.application?.navigateToApp(sourceLink.appId, { path: sourceLink.path }); window.dispatchEvent(new HashChangeEvent('hashchange')); return; } Source display name preference order sourceDisplayName prefers monitor_name before slo_name/sloId. For an SLO burn-rate alert produced via the OpenSearch backend that happens to also carry a monitor_name label (SLO burn-rate rules are backed by monitors), the row title will say "Source SLO" but the displayed name will be the underlying monitor's name rather than the SLO name. Consider gating on sloId first: if sloId is present, prefer slo_name ?? sloId; otherwise fall back to monitor/alertname/id. const labelRecord = allLabels as Record<string, string>; const sourceDisplayName = labelRecord?.monitor_name ?? labelRecord?.slo_name ?? sloId ?? labelRecord?.alertname ?? labelRecord?.monitor_id;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 91851ca

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fire hashchange after cross-app navigation navigateToApp is asynchronous and mounts the target app before the HashRouter is ready; dispatching hashchange synchronously against window in the current app almost never reaches the SLO app's router. Await/schedule the event after navigation resolves, or set location.hash as part of the path so the target app's initial route picks it up naturally. public/components/alerting/alert_detail_flyout.tsx [243-245] const navigateToSource = () => { if (!sourceLink) return; onClose(); if (sourceLink.appId) { - coreRefs?.application?.navigateToApp(sourceLink.appId, { path: sourceLink.path }); - window.dispatchEvent(new HashChangeEvent('hashchange')); + Promise.resolve( + coreRefs?.application?.navigateToApp(sourceLink.appId, { path: sourceLink.path }) + ).then(() => { + window.dispatchEvent(new HashChangeEvent('hashchange')); + }); return; } Suggestion importance[1-10]: 6 __ Why: Reasonable concern: navigateToApp is async, so dispatching hashchange synchronously may not reach the target app's router. Awaiting the promise before dispatching is more robust.	Low
General	Render trimmed value in text fallback The final fallback renders the raw value (which may be null/undefined), while the empty-check uses trimmed. If value is null or a non-string, React will still receive a non-string here in unusual paths, and whitespace-only strings render as blanks rather than the fallback. Render trimmed (or fallback) to keep behavior consistent with the empty branch. public/components/alerting/linkify_annotation.tsx [67] if (!trimmed) { return <span data-test-subj={dataTestSubj}>{fallback}</span>; } if (isSafeHttpUrl(trimmed)) { - // External links open in a new tab; `rel="noreferrer"` also implies - // `noopener`, so the opened runbook can't reach back through `window.opener`. return ( <EuiLink href={trimmed} target="_blank" rel="noreferrer" external data-test-subj={dataTestSubj} > {trimmed} </EuiLink> ); } -return <span data-test-subj={dataTestSubj}>{value}</span>; +return <span data-test-subj={dataTestSubj}>{trimmed}</span>; Suggestion importance[1-10]: 3 __ Why: Minor consistency improvement; since the empty case is already handled via trimmed, value at this point is a non-empty string, so the impact is small.	Low
	Make timezone offset fallback deterministic The test expects a numeric offset like +0545 for zones without an abbreviation (e.g. Asia/Kathmandu), but moment's z token returns the zone abbreviation which for Asia/Kathmandu is +0545 only in some moment-timezone versions — older versions return +0545 while others return the zone name. To make the numeric-offset fallback deterministic, explicitly detect the abbreviation-vs-offset case and use ZZ for offsets. public/components/alerting/time_format.ts [38-43] export function getTimezoneLabel( value: string | number | Date = Date.now(), tz: string = resolveDisplayTz() ): string { - return moment.tz(value, tz).format('z'); + const m = moment.tz(value, tz); + const abbr = m.format('z'); + // moment returns a numeric offset (e.g. "+0545") for zones without a + // named abbreviation; normalise those through `ZZ` for consistency. + return /^[+-]\d{2}:?\d{2}$/.test(abbr) ? m.format('ZZ') : abbr; } Suggestion importance[1-10]: 3 __ Why: The suggested improved_code produces +05:45 (with colon) via ZZ... actually ZZ returns +0545 which matches the test, so the change is largely equivalent and offers marginal robustness improvement.	Low

---

Previous suggestions

Suggestions up to commit 189fc2e

Category	Suggestion	Impact
General	Align source display name with row title For an SLO burn-rate alert that also carries monitor_name (possible when both label conventions coexist), sourceDisplayName will resolve to monitor_name even though the row title says "Source SLO", producing a mislabeled link. Prefer slo_name/slo_id first when sloId is present to keep the display consistent with sourceRowTitle. public/components/alerting/alert_detail_flyout.tsx [219-224] -const sourceDisplayName = - labelRecord?.monitor_name ?? - labelRecord?.slo_name ?? - sloId ?? - labelRecord?.alertname ?? - labelRecord?.monitor_id; +const sourceDisplayName = sloId + ? (labelRecord?.slo_name ?? sloId) + : (labelRecord?.monitor_name ?? + labelRecord?.alertname ?? + labelRecord?.monitor_id); Suggestion importance[1-10]: 6 __ Why: Correct correctness concern: when sloId is present but monitor_name also exists, the row title says "Source SLO" while the displayed name would be the monitor's name, creating a mislabel. Reordering preference when sloId is set aligns display with the title.	Low
Possible issue	Avoid rendering raw nullable value as text The final fallback renders the raw value (which may be null/undefined) instead of the safe trimmed string, so a null annotation value could render as literal "null" or trigger React warnings. Render trimmed (or the original string when non-empty) consistently, matching what the test expects when passing a non-URL string. public/components/alerting/linkify_annotation.tsx [67] -if (!trimmed) { - return <span data-test-subj={dataTestSubj}>{fallback}</span>; -} -if (isSafeHttpUrl(trimmed)) { - // External links open in a new tab; `rel="noreferrer"` also implies - // `noopener`, so the opened runbook can't reach back through `window.opener`. - return ( - <EuiLink - href={trimmed} - target="_blank" - rel="noreferrer" - external - data-test-subj={dataTestSubj} - > - {trimmed} - </EuiLink> - ); -} -return <span data-test-subj={dataTestSubj}>{value}</span>; +return <span data-test-subj={dataTestSubj}>{trimmed}</span>; Suggestion importance[1-10]: 5 __ Why: Valid observation: the final fallback branch renders value instead of trimmed, which could render unexpected content for non-string inputs. The impact is minor since the unsafe scheme case still displays a string, but consistency with trimmed is a reasonable improvement.	Low

Suggestions up to commit 27c97c6

Category	Suggestion	Impact
Possible issue	Avoid racing hashchange before target app mounts Firing a synthetic hashchange immediately after navigateToApp runs before the target app has mounted its HashRouter, so the event has no listener yet and the route is lost. Rely on navigateToApp's path (which sets the hash before mount) or defer the dispatch until after the target app is loaded; otherwise the SLO deep-link may land on the SLO app root instead of #/slos/. public/components/alerting/alert_detail_flyout.tsx [243-246] if (sourceLink.appId) { coreRefs?.application?.navigateToApp(sourceLink.appId, { path: sourceLink.path }); - window.dispatchEvent(new HashChangeEvent('hashchange')); return; } Suggestion importance[1-10]: 5 __ Why: The concern about a race between navigateToApp and the synthetic hashchange is plausible, but the PR author explicitly comments that navigateToApp uses pushState and doesn't emit hashchange, so dispatching it is intentional. The suggestion is speculative without concrete evidence of the bug.	Low
Security	Harden URL validation against whitespace/control chars The URL constructor accepts inputs with embedded whitespace/newlines and some browsers tolerate leading control characters, which can allow strings like "javascript:alert(1)" prefixed with whitespace to parse unexpectedly in edge engines. Reject values containing whitespace or control characters before parsing to make the safe-URL check robust across runtimes. public/components/alerting/linkify_annotation.tsx [26-33] export function isSafeHttpUrl(value: string): boolean { + if (/[\s\u0000-\u001F]/.test(value)) return false; try { const { protocol } = new URL(value); return protocol === 'http:' || protocol === 'https:'; } catch { return false; } } Suggestion importance[1-10]: 4 __ Why: The LinkifyAnnotation component already trims the value before calling isSafeHttpUrl, mitigating leading/trailing whitespace. Embedded control characters are an edge case with limited practical impact, though the added defense-in-depth is minor.	Low

Suggestions up to commit dc844aa

Category	Suggestion	Impact
General	Handle missing application service gracefully When coreRefs.application is unavailable, this silently no-ops after onClose() has already fired, leaving the user with a closed flyout and no navigation. Guard the call and either skip closing or fall back to a hash update so the user isn't stranded. public/components/alerting/alert_detail_flyout.tsx [243-246] if (sourceLink.appId) { - // Cross-app navigation: the SLO detail page lives in a different OSD - // application than the alerting app, so go through - // `application.navigateToApp` to switch apps (workspace-aware), then - // fire a synthetic hashchange for the target app's HashRouter to pick - // up the route — navigateToApp uses pushState, which does not emit - // hashchange on its own. - coreRefs?.application?.navigateToApp(sourceLink.appId, { path: sourceLink.path }); + const app = coreRefs?.application; + if (!app?.navigateToApp) { + window.location.hash = sourceLink.path; + window.dispatchEvent(new HashChangeEvent('hashchange')); + return; + } + app.navigateToApp(sourceLink.appId, { path: sourceLink.path }); window.dispatchEvent(new HashChangeEvent('hashchange')); return; } Suggestion importance[1-10]: 5 __ Why: Valid defensive improvement: if coreRefs.application is unavailable, the current code silently fails after closing the flyout. The fallback to hash update is reasonable, though this edge case is unlikely in practice.	Low
Security	Add explicit noopener for external links rel="noreferrer" implies noopener in modern browsers, but for defense-in-depth and older browser support, include noopener explicitly. This prevents tabnabbing attacks in browsers that do not implement the implicit behavior. public/components/alerting/linkify_annotation.tsx [59] if (isSafeHttpUrl(trimmed)) { - // External links open in a new tab; `rel="noreferrer"` also implies - // `noopener`, so the opened runbook can't reach back through `window.opener`. return ( <EuiLink href={trimmed} target="_blank" - rel="noreferrer" + rel="noopener noreferrer" external data-test-subj={dataTestSubj} > {trimmed} </EuiLink> ); } Suggestion importance[1-10]: 3 __ Why: Minor defense-in-depth improvement. rel="noreferrer" already implies noopener in all modern browsers, and the code comment acknowledges this, so the practical security benefit is minimal.	Low

[github-actions]

Persistent review updated to latest commit 27c97c6

[github-actions]

Persistent review updated to latest commit 189fc2e

[github-actions]

Persistent review updated to latest commit 91851ca