mirror of
https://github.com/openobserve/openobserve.git
synced 2026-11-05 00:09:35 +00:00
Fixes #13742 Fixes openobserve/o2-enterprise#2394 ## The bug A content template link with a non-http scheme (`javascript:alert(1)`) made Slack reject the **entire** notification with `400 invalid_attachments`. The alert was lost outright and `triggers` recorded `notify_failed`. ## Root cause `safe_url` neutralises a hostile scheme by substituting `BLOCKED_URL` (`"#"`). That is **safe** but not **deliverable** — Slack, Teams, Discord and PagerDuty validate action URLs server-side and reject the whole payload when one is not an absolute `http(s)` URL. A security substitution was turning into total message loss. The two properties are different, and links need both. So the fix splits by medium: | renderer | link handling | |---|---| | Slack buttons, Teams actions, Discord `embed.url`, PagerDuty `href` | `dispatchable_url` → **drop** | | HTML email, email plaintext, SNS plaintext, Discord description | `safe_url` → substitute `#` | | Webhook envelope | unfiltered — versioned machine contract, documented | Dropping beats rewriting: the alert body is worth more than the button, and a button pointing somewhere misleading is worse than no button. `dispatchable_url` requires a **non-empty authority**, not just an allowlisted scheme — `http:javascript:alert(1)` and `https://` are valid *opaque* RFC 3986 http(s) URLs that Slack still rejects. It also rejects raw spaces and control characters, which variable substitution can introduce. ## Related issues fixed in the same area - **Empty `ZO_WEB_URL`** (not in the report): `alert_url` becomes a root-relative `/web/logs?…`, which `safe_url` passes through by design, and it lands in the always-appended "View in OpenObserve" button. Same `invalid_attachments`, on deployments that never authored a hostile template. - **PagerDuty** validates `links[].href` server-side too — same lost-alert class, converted. - **Save-time rejection** (`ContentSpec::validate`): a clear 400 naming the offending link, instead of a template whose link silently never works. Also wired into **preview** and **test-send**, which previously accepted a draft that couldn't be saved and echoed the raw hostile URL back to the UI. - **Slack image-fallback misdiagnosis**: removing the image proves the image was at fault, but only an image pointing at *our* `web_url` justifies tripping the process-wide chart suppression (1h, across every alert) and telling the operator to fix `ZO_WEB_URL`. A custom template's third-party image no longer does either. - **a11y**: `OInput` links its error message to the control via `aria-describedby`; the textarea branch also gained `aria-invalid`. ## o2-enterprise#2394 (fixed here, not in the enterprise repo) Clearing the Monaco body and saving toasted "Please fill required fields" with no indication of *where*. The schema already produced an error on the `body` path — a bare Monaco bridged in via `setFieldValue` just had no `OFormInput` error slot to render it. Now a red border + message driven by the same field state `OFormInput` reads. The aria wiring goes on the **shell**, not on `<QueryEditor>`: `CodeQueryEditor` sets `inheritAttrs: false` and binds `$attrs` to a non-focusable wrapper, so attributes passed to the component would be inert — false assurance for a screen-reader user. > The issue names the title field too. It's an `OFormInput` and already highlights correctly; I added a regression test pinning that rather than changing it. This fix is entirely in OSS (`web/`) since the template form isn't enterprise-gated, so there's no companion enterprise PR. ## Layering note Save-time validation is a **UX gate, not the security boundary**. Link URLs are templates — a scheme that only materialises after `{variable}` substitution can't be judged at save time. The render layer is what actually stops delivery, and it also covers every template saved before this validation existed. Both layers are needed; neither is sufficient. ## Testing - Every fix written test-first (verified RED) and **mutation-tested** — each new test fails when its fix is reverted in substance. - Golden files unchanged: legitimate URLs render byte-identically, so no over-blocking. - Rust: `alerts::notifications` 159 · `config` 3130 · `api-management` 424 — all passing. `cargo clippy --all-targets` and `cargo fmt --check` clean. - Frontend: full suite **41,597 passing** (run because `OInput` is shared library code). `vue-tsc`, `eslint`, and `npm run format:check` clean. - Two independent cold-start review passes; findings from both are folded in, including a P0 where the first version of `dispatchable_url` still let the original bug through. Pre-existing and unrelated: `alerts::destinations::tests::test_alert_destination_template_is_optional` fails locally on a DB connection reset — verified failing on unmodified `main` too.
134 lines
5.1 KiB
TypeScript
134 lines
5.1 KiB
TypeScript
// Copyright 2026 OpenObserve Inc.
|
|
//
|
|
// This program is free software: you can redistribute it and/or modify
|
|
// it under the terms of the GNU Affero General Public License as published by
|
|
// the Free Software Foundation, either version 3 of the License, or
|
|
// (at your option) any later version.
|
|
//
|
|
// This program is distributed in the hope that it will be useful
|
|
// but WITHOUT ANY WARRANTY; without even the implied warranty of
|
|
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
|
|
// GNU Affero General Public License for more details.
|
|
//
|
|
// You should have received a copy of the GNU Affero General Public License
|
|
// along with this program. If not, see <http://www.gnu.org/licenses/>.
|
|
//
|
|
// One place to answer "is this user-supplied string a URL we may navigate to?"
|
|
//
|
|
// This check had been written from scratch four times (`safeHttpUrl` in
|
|
// ingestion/setupCard/subs.ts, `getHttpUrl` in TableRenderer.vue, and twice in
|
|
// the synthetics schemas), and the two places that DIDN'T have it — dashboard
|
|
// drilldowns and stream cross-links — are exactly where unsafe values got in.
|
|
// Prefer this helper over a fifth hand-rolled regex.
|
|
|
|
/** Schemes that may ever be navigated to. NOT data:, file:, ws:, telnet:. */
|
|
const NAVIGABLE_SCHEMES = ["http:", "https:"];
|
|
|
|
export interface SafeUrlOptions {
|
|
/** Also accept `mailto:` with a real `local@domain` mailbox. */
|
|
allowMailto?: boolean;
|
|
/**
|
|
* Also accept a value whose scheme only materializes after `{variable}`
|
|
* substitution at render time (alert templates, drilldowns, cross-links).
|
|
* A literal hostile scheme is still rejected: `{x}javascript:` fails.
|
|
*/
|
|
allowTemplateVars?: boolean;
|
|
}
|
|
|
|
/**
|
|
* True when `value` is a URL that is safe to navigate to (`window.open`,
|
|
* `location.href`, an `:href` binding).
|
|
*
|
|
* Why a parser and not a regex: a scheme allowlist alone is not enough —
|
|
* `http:` and `https://` carry an allowlisted scheme and are still not URLs
|
|
* (per the WHATWG URL Standard an empty host is a *parse failure* for a
|
|
* special scheme). And a parser alone is not enough either — `javascript:`
|
|
* parses perfectly well. Both checks are required, in that order.
|
|
*/
|
|
export function isSafeNavigableUrl(value: unknown, opts: SafeUrlOptions = {}): boolean {
|
|
if (typeof value !== "string") return false;
|
|
const trimmed = value.trim();
|
|
if (trimmed === "") return false;
|
|
|
|
// Control characters and internal whitespace are never valid in a URL, and
|
|
// are how `java\tscript:` style bypasses are smuggled past naive checks.
|
|
for (const ch of trimmed) {
|
|
const c = ch.codePointAt(0)!;
|
|
if (c <= 0x20 || c === 0x7f) return false;
|
|
}
|
|
|
|
if (opts.allowTemplateVars) {
|
|
// A variable in SCHEME POSITION defers judgement to substitution time.
|
|
// Deliberately strict: `{x}javascript:alert(1)` also starts with `{`, but
|
|
// the rest of its scheme is the literal, hostile `javascript`, so the
|
|
// variable must be the WHOLE scheme (or the whole value).
|
|
const boundary = trimmed.search(/[:/?#]/);
|
|
const schemeIsVariable =
|
|
boundary === -1
|
|
? /^\{[^{}]+\}/.test(trimmed) // no scheme at all: `{alert_url}&tab=x`
|
|
: trimmed[boundary] === ":" && /^\{[^{}]+\}$/.test(trimmed.slice(0, boundary));
|
|
if (schemeIsVariable) return true;
|
|
}
|
|
|
|
let parsed: URL;
|
|
try {
|
|
parsed = new URL(trimmed);
|
|
} catch {
|
|
return false;
|
|
}
|
|
|
|
const scheme = parsed.protocol.toLowerCase();
|
|
|
|
if (scheme === "mailto:") {
|
|
if (!opts.allowMailto) return false;
|
|
const at = parsed.pathname.indexOf("@");
|
|
if (at <= 0) return false;
|
|
const domain = parsed.pathname.slice(at + 1);
|
|
return domain.includes(".") && !domain.startsWith(".");
|
|
}
|
|
|
|
if (!NAVIGABLE_SCHEMES.includes(scheme)) return false;
|
|
|
|
// `new URL()` guarantees a non-empty host for a special scheme, but a host
|
|
// may still be junk that can never resolve (".", "..", "-").
|
|
const host = parsed.hostname;
|
|
return host.length > 0 && /[a-z0-9]/i.test(host) && !/^[.-]/.test(host) && !/[.-]$/.test(host);
|
|
}
|
|
|
|
/**
|
|
* True when `value` is safe to redirect to after login — a relative path, or
|
|
* an absolute URL on `origin` itself.
|
|
*
|
|
* The previous check was `redirectURI.includes("http")`, a substring test that
|
|
* accepted `https://evil.com/` and sent the user off-site immediately after
|
|
* authenticating (the classic post-auth phishing setup).
|
|
*/
|
|
export function isSameOriginRedirect(
|
|
value: string | null | undefined,
|
|
origin: string = window.location.origin,
|
|
): boolean {
|
|
if (!value) return false;
|
|
const trimmed = value.trim();
|
|
if (trimmed === "") return false;
|
|
|
|
for (const ch of trimmed) {
|
|
const c = ch.codePointAt(0)!;
|
|
if (c <= 0x20 || c === 0x7f) return false;
|
|
}
|
|
|
|
// A backslash is normalized to `/` by browsers, so `/\evil.com` and
|
|
// `\\evil.com` are protocol-relative in disguise.
|
|
if (trimmed.includes("\\")) return false;
|
|
// Protocol-relative (`//evil.com`) points off-origin despite looking relative.
|
|
if (trimmed.startsWith("//")) return false;
|
|
|
|
// Plain relative path: same origin by construction.
|
|
if (trimmed.startsWith("/")) return true;
|
|
|
|
try {
|
|
return new URL(trimmed, origin).origin === new URL(origin).origin;
|
|
} catch {
|
|
return false;
|
|
}
|
|
}
|