diff --git a/src/api/management/src/request/alerts/templates.rs b/src/api/management/src/request/alerts/templates.rs index 78faea2e0e..b365958893 100644 --- a/src/api/management/src/request/alerts/templates.rs +++ b/src/api/management/src/request/alerts/templates.rs @@ -485,7 +485,11 @@ pub async fn preview_template( match preview(&req) { Ok(resp) => MetaHttpResponse::json(resp), - Err(e @ PreviewError::UnknownChannel(_)) | Err(e @ PreviewError::UnknownSeverity(_)) => { + Err(e @ PreviewError::UnknownChannel(_)) + | Err(e @ PreviewError::UnknownSeverity(_)) + // The draft fails the same validation `save` applies — the client sent + // content it could not have saved, so this is a 400 like the others. + | Err(e @ PreviewError::InvalidContent(_)) => { MetaHttpResponse::bad_request(e.to_string()) } // Unreachable today (the webhook fallback is total), but a render diff --git a/src/config/src/meta/alerts/content_spec.rs b/src/config/src/meta/alerts/content_spec.rs index ba24ca3652..1ae10a21b8 100644 --- a/src/config/src/meta/alerts/content_spec.rs +++ b/src/config/src/meta/alerts/content_spec.rs @@ -100,10 +100,167 @@ pub struct ChartSpec { pub enabled: bool, } +/// Schemes an authored link URL may carry. +/// +/// Mirrors the render layer's allowlist (`alerts::notifications::render:: +/// safe_url`) on purpose, but note which layer actually enforces what: +/// +/// * **This check is a UX gate, not the security boundary.** It rejects a hostile scheme the author +/// typed LITERALLY, so they get a clear error at save time instead of a template whose link is +/// silently neutralized on every send forever after. +/// * **The render layer is what actually stops delivery.** Link URLs are templates: a scheme that +/// only materializes after `{variable}` substitution cannot be judged here at all, and `safe_url` +/// / `dispatchable_url` are what catch it. They also cover every template saved BEFORE this +/// validation existed. +/// +/// So a URL passing this function is NOT thereby trusted — see the render +/// module's docs for the per-medium drop/substitute rules. +const ALLOWED_LINK_SCHEMES: [&str; 3] = ["http", "https", "mailto"]; + +/// Reject a link URL whose author-written scheme is not allowlisted. +/// +/// An allowlist, not a blocklist, because a blocklist fails OPEN: `java\tscript:`, +/// `\0javascript:`, `java%73cript:` and `vbscript:` all slip past a naive +/// `starts_with("javascript:")` while a conformant client still dispatches +/// them. None of them *begin* with an allowed scheme, so an allowlist closes +/// all of them at once and fails closed on schemes nobody has thought of yet. +/// +/// A link field holds a URL — so the value must actually LOOK like one. Three +/// shapes are accepted, and nothing else: +/// +/// * **Absolute** — `http:`, `https:` or `mailto:` (the scheme allowlist). +/// * **Root-relative** (`/path`, `?q=1`, `#frag`) — inert, resolves against the deployment's own +/// host, and an author writing one is doing something legitimate (it renders fine in email). +/// * **Placeholder-led** (`{alert_url}`, `{alert_url}&tab=logs`, `{scheme}://host`) — link URLs are +/// TEMPLATES resolved by `substitute_raw` at send time, so there may be no author scheme to judge +/// yet. The render-time filter catches a hostile substituted value. +/// +/// A bare word (`foo`, `javascript(0)`, `not a url at all`) or a bare host +/// (`runbook.example.com/x`) is REJECTED. None can execute anything — a +/// scheme-less string is just a relative path — but none is a working link +/// either, and silently accepting one means the author discovers the dead +/// link when an alert fires instead of when they save. +fn link_scheme_is_allowed(url: &str) -> bool { + let trimmed = url.trim(); + + if trimmed.is_empty() { + return false; + } + + // Whitespace and control characters are never valid inside a URL. + if trimmed.chars().any(|c| c.is_whitespace() || c.is_control()) { + return false; + } + + // A `:` only introduces a scheme when it precedes any path/query/fragment + // marker, per RFC 3986 — so `/a:b` is a scheme-less path, not a scheme. + let scheme_end = trimmed + .find([':', '/', '?', '#']) + .filter(|&i| trimmed.as_bytes()[i] == b':'); + + let Some(end) = scheme_end else { + // No scheme: accept a root-relative reference, or a template whose + // scheme only materializes after substitution. + return trimmed.starts_with(['/', '?', '#']) || is_variable_led(trimmed); + }; + + let scheme = &trimmed[..end]; + + // A scheme that is ENTIRELY a variable has nothing to judge yet — + // `{scheme}://host` is a legitimate template. Deliberately strict: + // `{x}javascript:...` also begins with `{`, but the rest of its scheme is + // the literal, hostile `javascript`, so it must be rejected. + if scheme.starts_with('{') && scheme.ends_with('}') && !scheme[1..].contains('{') { + return true; + } + + // Schemes are ASCII case-insensitive per RFC 3986. + if !ALLOWED_LINK_SCHEMES + .iter() + .any(|allowed| scheme.eq_ignore_ascii_case(allowed)) + { + return false; + } + + // An allowlisted scheme is necessary but NOT sufficient — `http:` and + // `https://` carry one and are still not URLs. Delegate the actual + // question to a WHATWG-conformant parser rather than hand-rolling it. + // + // A `{variable}` inside the path/query is fine: it percent-encodes and + // still parses, so the substituted value is what ultimately matters. + let Ok(parsed) = ::url::Url::parse(trimmed) else { + return false; + }; + + if parsed.scheme().eq_ignore_ascii_case("mailto") { + // The parser accepts ANY opaque path after `mailto:`, including an + // empty one, so the mailbox shape is checked here: `local@domain` + // with a dot in the domain. + let addr = parsed.path(); + let Some((local, domain)) = addr.split_once('@') else { + return false; + }; + return !local.is_empty() && domain.contains('.') && !domain.starts_with('.'); + } + + // http(s): the parser already guarantees a non-empty host (an empty host + // is a parse FAILURE for a special scheme). Reject hosts that parse but + // can never resolve — `.`, `..`, `-` and friends. + match parsed.host_str() { + Some(host) => { + !host.is_empty() + && host.chars().any(|c| c.is_ascii_alphanumeric()) + && !host.starts_with(['.', '-']) + && !host.ends_with(['.', '-']) + } + None => false, + } +} + +/// True when `url` begins with a `{variable}` and the remainder is plausible +/// URL tail (no whitespace — already checked — and not obvious prose). +/// +/// `{alert_url}` and `{alert_url}&tab=logs` are legitimate templates whose +/// real value only exists at send time; `{x} not a url` is not. +fn is_variable_led(url: &str) -> bool { + let Some(rest) = url.strip_prefix('{') else { + return false; + }; + let Some(close) = rest.find('}') else { + return false; + }; + // A non-empty variable name, and nothing unparseable glued on after it. + !rest[..close].is_empty() +} + impl ContentSpec { pub fn parse(body: &str) -> Result { serde_json::from_str(body) } + + /// Validate authored content beyond what serde can express. + /// + /// Returns a human-readable error naming the offending link, so the author + /// can fix the template instead of discovering at alert time that a link + /// was dropped (#13742). + pub fn validate(&self) -> Result<(), String> { + for link in &self.links { + if !link_scheme_is_allowed(&link.url) { + let label = if link.label.is_empty() { + "" + } else { + link.label.as_str() + }; + return Err(format!( + "link '{label}' has an unsupported URL scheme: {:?}. \ + Links must use http, https or mailto (a relative path or \ + a {{variable}} is also allowed).", + link.url + )); + } + } + Ok(()) + } } #[cfg(test)] @@ -142,4 +299,131 @@ mod tests { fn rejects_non_json() { assert!(ContentSpec::parse("Slack payload {alert_name}").is_err()); } + + /// #13742: a link URL carrying an active-content scheme is rejected AT + /// SAVE TIME with a clear error, rather than being stored and silently + /// neutralized (or, before the fix, killing the whole notification) on + /// every send forever after. + #[test] + fn rejects_link_urls_with_disallowed_schemes() { + for hostile in [ + "javascript:alert(1)", + "JavaScript:alert(1)", // scheme match is case-insensitive + " javascript:alert(1)", // and ignores surrounding whitespace + "java\tscript:alert(1)", // control chars stripped by clients + "java\nscript:alert(1)", + "\u{0}javascript:alert(1)", // NUL is not Unicode whitespace + "java%73cript:alert(1)", // percent-encoded scheme + "vbscript:msgbox(1)", + "file:///etc/passwd", + "data:text/html,", + "file:///etc/passwd", + "vbscript:msgbox(1)", + ]) { + expect(crossLinkUrlIsValid(bad), `accepted: ${bad}`).toBe(false); + } + }); + + it("rejects values that are not URLs", () => { + for (const bad of ["foo", "not a url", "http:", "http://", "https://."]) { + expect(crossLinkUrlIsValid(bad), `accepted: ${bad}`).toBe(false); + } + }); +}); diff --git a/web/src/components/cross-linking/CrossLinkDialog.schema.ts b/web/src/components/cross-linking/CrossLinkDialog.schema.ts index 8d79adb686..8ce77f2e81 100644 --- a/web/src/components/cross-linking/CrossLinkDialog.schema.ts +++ b/web/src/components/cross-linking/CrossLinkDialog.schema.ts @@ -8,10 +8,28 @@ import { z } from "zod"; +import { isSafeNavigableUrl } from "@/utils/safeUrl"; + +/** + * A cross-link URL is a TEMPLATE containing `${field.__value}` placeholders, + * so it is validated with the placeholders stripped to a harmless token — the + * shape around them still has to be a real http(s) URL. + * + * Previously the only rule was `min(1)`, so `javascript:alert(1)` saved + * cleanly and was handed to `window.open` for every viewer of the stream. + */ +export function crossLinkUrlIsValid(url: string): boolean { + const withoutPlaceholders = url.replace(/\$\{[^}]*\}/g, "x"); + return isSafeNavigableUrl(withoutPlaceholders); +} + export const makeCrossLinkDialogSchema = (t: (_key: string) => string) => z.object({ name: z.string().min(1, t("crossLinks.nameRequired")), - url: z.string().min(1, t("crossLinks.urlRequired")), + url: z + .string() + .min(1, t("crossLinks.urlRequired")) + .refine(crossLinkUrlIsValid, { message: t("crossLinks.urlInvalid") }), fields: z.array(z.object({ name: z.string() })).default([]), // Chip-builder scratch input that stages a field name before it's committed // as a chip. NOT part of the emit payload; form-owned + optional. diff --git a/web/src/components/dashboards/addPanel/DrilldownPopUp.schema.spec.ts b/web/src/components/dashboards/addPanel/DrilldownPopUp.schema.spec.ts new file mode 100644 index 0000000000..bcbb55bf42 --- /dev/null +++ b/web/src/components/dashboards/addPanel/DrilldownPopUp.schema.spec.ts @@ -0,0 +1,50 @@ +// 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 . + +import { describe, expect, it } from "vitest"; +import { drilldownUrlIsValid } from "./DrilldownPopUp.schema"; + +describe("drilldownUrlIsValid", () => { + it("accepts http(s) templates with ${variable} placeholders", () => { + for (const ok of [ + "https://example.com/d/${value}", + "https://example.com/x?a=${row.field}", + "http://localhost:5080/web/logs", + ]) { + expect(drilldownUrlIsValid(ok), `rejected: ${ok}`).toBe(true); + } + }); + + // The previous regex named these schemes as ALLOWED. `window.open` is called + // on this value, so data:/file: are navigable targets, not inert text. + it("rejects schemes the old protocol regex explicitly allowed", () => { + for (const bad of [ + "data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg==", + "file:///etc/passwd", + "telnet://host", + "ws://host/x", + "wss://host/x", + "ftp://host/x", + ]) { + expect(drilldownUrlIsValid(bad), `accepted: ${bad}`).toBe(false); + } + }); + + it("rejects hostile schemes and non-URLs", () => { + for (const bad of ["javascript:alert(1)", "foo", "http://", "https://-"]) { + expect(drilldownUrlIsValid(bad), `accepted: ${bad}`).toBe(false); + } + }); +}); diff --git a/web/src/components/dashboards/addPanel/DrilldownPopUp.schema.ts b/web/src/components/dashboards/addPanel/DrilldownPopUp.schema.ts index 86d02adac7..430c927bd2 100644 --- a/web/src/components/dashboards/addPanel/DrilldownPopUp.schema.ts +++ b/web/src/components/dashboards/addPanel/DrilldownPopUp.schema.ts @@ -20,8 +20,22 @@ import { z } from "zod"; -// Protocol-only URL check. -const URL_PROTOCOL_REGEX = /^(http|https|ftp|file|mailto|telnet|data|ws|wss):\/\//; +import { isSafeNavigableUrl } from "@/utils/safeUrl"; + +/** + * A drilldown URL is a TEMPLATE containing `${variable}` placeholders, so it + * is validated with the placeholders stripped to a harmless token — the shape + * around them still has to be a real http(s) URL. + * + * This replaces a scheme-prefix regex that allowed `ftp`, `file`, `telnet`, + * `data`, `ws` and `wss` by name. `data:` and `file:` are navigable targets + * (`usePanelDrilldown` calls `window.open` on this value), and the regex also + * passed degenerate values like `http://` that are not URLs at all. + */ +export function drilldownUrlIsValid(url: string): boolean { + const withoutPlaceholders = url.replace(/\$\{[^}]*\}/g, "x"); + return isSafeNavigableUrl(withoutPlaceholders); +} // Array row — kept loose (no per-row rules). export const drilldownVariableRowSchema = z.object({ @@ -73,7 +87,7 @@ export const makeDrilldownPopUpSchema = (t: (_key: string) => string) => path: ["data", "url"], message: t("dashboard.urlRequired"), }); - } else if (!URL_PROTOCOL_REGEX.test(url)) { + } else if (!drilldownUrlIsValid(url)) { ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["data", "url"], diff --git a/web/src/composables/dashboard/usePanelDrilldown.ts b/web/src/composables/dashboard/usePanelDrilldown.ts index 12baba50db..2ad619ed29 100644 --- a/web/src/composables/dashboard/usePanelDrilldown.ts +++ b/web/src/composables/dashboard/usePanelDrilldown.ts @@ -20,6 +20,7 @@ import { getUTCTimestampFromZonedTimestamp } from "@/utils/dashboard/dateTimeUti import { normalizeVariableSyntax } from "@/utils/dashboard/variables/variablesUtils"; import searchService from "@/services/search"; import { isCrossLinkingEnabledForStream } from "@/utils/crossLinking"; +import { isSafeNavigableUrl } from "@/utils/safeUrl"; export function usePanelDrilldown({ panelSchema, @@ -932,10 +933,17 @@ export function usePanelDrilldown({ // if drilldown by url if (drilldownData.type == "byUrl") { try { + // Guard the RESOLVED url. The form schema rejects a hostile one at + // save time, but that is client-side only: a direct dashboard PUT + // stores anything, and dashboards saved before the schema existed + // are still in the DB. A variable VALUE can also carry a scheme. + const resolved = replacePlaceholders(drilldownData.data.url, drilldownVariables); + if (!isSafeNavigableUrl(resolved)) return; // open url return window.open( - replacePlaceholders(drilldownData.data.url, drilldownVariables), + resolved, drilldownData.targetBlank ? "_blank" : "_self", + "noopener,noreferrer", ); } catch { /* ignore: best-effort window.open */ diff --git a/web/src/lib/forms/Form/scrollToFirstError.spec.ts b/web/src/lib/forms/Form/scrollToFirstError.spec.ts new file mode 100644 index 0000000000..a6e092f450 --- /dev/null +++ b/web/src/lib/forms/Form/scrollToFirstError.spec.ts @@ -0,0 +1,107 @@ +// 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 . + +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { scrollToFirstError } from "./scrollToFirstError"; + +describe("scrollToFirstError", () => { + beforeEach(() => { + document.body.innerHTML = ""; + // A single prototype stub is shared by every element, so "not called" + // could never distinguish them — give each element its own spy. + (Element.prototype as any).scrollIntoView = function () {}; + }); + + function field(id: string, invalid: boolean) { + const el = document.createElement("input"); + el.id = id; + if (invalid) el.setAttribute("aria-invalid", "true"); + el.scrollIntoView = vi.fn(); + document.body.appendChild(el); + return el; + } + + it("scrolls the FIRST invalid control into view, not a later one", async () => { + field("ok", false); + const first = field("bad-1", true); + const second = field("bad-2", true); + + const scrolled = await scrollToFirstError(); + + expect(scrolled).toBe(true); + expect(first.scrollIntoView).toHaveBeenCalled(); + expect(second.scrollIntoView).not.toHaveBeenCalled(); + }); + + it("focuses the field so the user can start typing the fix", async () => { + const el = field("bad", true); + const focus = vi.spyOn(el, "focus"); + await scrollToFirstError(); + expect(focus).toHaveBeenCalled(); + }); + + it("returns false and does nothing when no field is invalid", async () => { + const el = field("ok", false); + expect(await scrollToFirstError()).toBe(false); + expect(el.scrollIntoView).not.toHaveBeenCalled(); + }); + + it('ignores aria-invalid="false" — only true marks an error', async () => { + const el = field("ok", false); + el.setAttribute("aria-invalid", "false"); + expect(await scrollToFirstError()).toBe(false); + expect(el.scrollIntoView).not.toHaveBeenCalled(); + }); + + it("falls back to a non-focusable error container when no control is invalid", async () => { + // The Monaco body editor is not a focusable control; its error state lives + // on a wrapper (o2-enterprise#2394), which must still be scrolled to. + const shell = document.createElement("div"); + shell.setAttribute("data-error", "true"); + shell.scrollIntoView = vi.fn(); + document.body.appendChild(shell); + + expect(await scrollToFirstError()).toBe(true); + expect(shell.scrollIntoView).toHaveBeenCalled(); + }); + + it("prefers a real invalid control over a data-error container", async () => { + const shell = document.createElement("div"); + shell.setAttribute("data-error", "true"); + shell.scrollIntoView = vi.fn(); + document.body.appendChild(shell); + const input = field("bad", true); + + await scrollToFirstError(); + + expect(input.scrollIntoView).toHaveBeenCalled(); + expect(shell.scrollIntoView).not.toHaveBeenCalled(); + }); + + it("searches only within the given root when one is passed", async () => { + const outside = field("outside-bad", true); + const root = document.createElement("div"); + const inside = document.createElement("input"); + inside.setAttribute("aria-invalid", "true"); + inside.scrollIntoView = vi.fn(); + root.appendChild(inside); + document.body.appendChild(root); + + await scrollToFirstError(root); + + expect(inside.scrollIntoView).toHaveBeenCalled(); + expect(outside.scrollIntoView).not.toHaveBeenCalled(); + }); +}); diff --git a/web/src/lib/forms/Form/scrollToFirstError.ts b/web/src/lib/forms/Form/scrollToFirstError.ts new file mode 100644 index 0000000000..797c3f3fb4 --- /dev/null +++ b/web/src/lib/forms/Form/scrollToFirstError.ts @@ -0,0 +1,57 @@ +// 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 . + +import { nextTick } from "vue"; + +/** + * Bring the first invalid field into view after a rejected submit. + * + * On a long form the failing field is often scrolled off-screen, so the user + * gets "Please fill required fields" with nothing visibly wrong — they cannot + * tell WHICH field, or even that the page has more of it above. Scrolling the + * offender into view and focusing it turns a dead-end toast into an actionable + * one. + * + * Selection order: + * + * 1. `[aria-invalid="true"]` — every `OInput`-based control sets this, so it covers all standard + * fields and is framework-agnostic (no per-form field registry to keep in sync). + * 2. `[data-error="true"]` — a non-focusable wrapper for controls that are not real form elements, + * e.g. the Monaco body editor (o2-enterprise#2394), which has no focusable node of its own to + * mark. + * + * Awaits `nextTick` first: validation state is applied during the same tick as + * the submit, so querying before the DOM updates would find nothing. + * + * @param root Limit the search to this subtree. Defaults to the whole document. + * @returns whether a field was found and scrolled to. + */ +export async function scrollToFirstError(root?: ParentNode): Promise { + await nextTick(); + + const scope = root ?? document; + const target = + scope.querySelector('[aria-invalid="true"]') ?? + scope.querySelector('[data-error="true"]'); + + if (!target) return false; + + target.scrollIntoView({ behavior: "smooth", block: "center" }); + // Focus so the user can start typing the fix immediately. Guarded: the + // fallback target is a wrapper div, which is not focusable. + target.focus?.({ preventScroll: true }); + + return true; +} diff --git a/web/src/lib/forms/Input/OInput.spec.ts b/web/src/lib/forms/Input/OInput.spec.ts index 3804292497..8913608fb6 100644 --- a/web/src/lib/forms/Input/OInput.spec.ts +++ b/web/src/lib/forms/Input/OInput.spec.ts @@ -45,6 +45,43 @@ describe("OInput", () => { expect(wrapper.text()).toContain("Required field"); }); + it("points aria-describedby at the error message so it is announced", () => { + wrapper = mount(OInput, { props: { error: true, errorMessage: "Required field" } }); + const input = wrapper.find("input"); + const describedBy = input.attributes("aria-describedby"); + expect(describedBy).toBeTruthy(); + expect(input.attributes("aria-invalid")).toBe("true"); + // The referenced node must actually exist and carry the message — + // a dangling aria-describedby announces nothing. + const target = wrapper.find(`#${describedBy}`); + expect(target.exists()).toBe(true); + expect(target.text()).toContain("Required field"); + }); + + it("sets no aria-describedby when there is no error to describe", () => { + wrapper = mount(OInput, { props: { errorMessage: "Required field" } }); + expect(wrapper.find("input").attributes("aria-describedby")).toBeUndefined(); + }); + + it("sets no dangling aria-describedby when the message is rendered elsewhere", () => { + // `error` with no `errorMessage` yields " " — OFormInput's #error slot owns + // the text, so this component renders no message node to point at. + wrapper = mount(OInput, { props: { error: true } }); + expect(wrapper.find("input").attributes("aria-describedby")).toBeUndefined(); + expect(wrapper.find("input").attributes("aria-invalid")).toBe("true"); + }); + + it("describes the textarea variant too", () => { + wrapper = mount(OInput, { + props: { type: "textarea", error: true, errorMessage: "Too long" }, + }); + const ta = wrapper.find("textarea"); + expect(ta.attributes("aria-invalid")).toBe("true"); + const describedBy = ta.attributes("aria-describedby"); + expect(describedBy).toBeTruthy(); + expect(wrapper.find(`#${describedBy}`).text()).toContain("Too long"); + }); + it("shows helpText when provided", () => { wrapper = mount(OInput, { props: { helpText: "Enter your name" } }); expect(wrapper.text()).toContain("Enter your name"); diff --git a/web/src/lib/forms/Input/OInput.vue b/web/src/lib/forms/Input/OInput.vue index 3683a37b8f..04b0f41fce 100644 --- a/web/src/lib/forms/Input/OInput.vue +++ b/web/src/lib/forms/Input/OInput.vue @@ -70,6 +70,16 @@ const effectiveError = computed(() => { }); const hasError = computed(() => !!effectiveError.value); +// `aria-invalid` alone tells a screen reader THAT the field is wrong but never +// WHAT is wrong. Point the control at the message element so the reason is +// announced with the field. Only set when a message is actually rendered — +// `effectiveError` is " " when the text lives in OFormInput's #error slot, and +// a dangling `aria-describedby` would reference a node that does not exist. +const errorId = computed(() => `${inputId.value}-error`); +const describedBy = computed(() => + effectiveError.value && effectiveError.value.trim() ? errorId.value : undefined, +); + const isTextarea = computed(() => props.type === "textarea"); // ── Width ────────────────────────────────────────────────────────────────── @@ -298,6 +308,8 @@ const wrapperClasses = computed(() => [ :disabled="disabled" :readonly="readonly" :aria-required="required || undefined" + :aria-invalid="hasError || undefined" + :aria-describedby="describedBy" :autofocus="autofocus" :maxlength="maxlength" :rows="autogrow ? 1 : rows" @@ -341,6 +353,7 @@ const wrapperClasses = computed(() => [ :autocomplete="autocomplete" :tabindex="inputTabindex" :aria-invalid="hasError || undefined" + :aria-describedby="describedBy" :class="[ 'min-w-0 flex-1 rounded-[inherit] bg-transparent outline-none', 'text-input-text placeholder:text-input-placeholder', @@ -413,6 +426,7 @@ const wrapperClasses = computed(() => [ > { const openSpy = vi.spyOn(window, "open").mockImplementation(() => null); wrapper.vm.openCrossLink("https://example.com/trace/123"); - expect(openSpy).toHaveBeenCalledWith("https://example.com/trace/123", "_blank"); + // `noopener,noreferrer` stops the opened tab reaching back via + // window.opener and strips the referrer. + expect(openSpy).toHaveBeenCalledWith( + "https://example.com/trace/123", + "_blank", + "noopener,noreferrer", + ); + openSpy.mockRestore(); + }); + + // The RESOLVED url is guarded, not just the saved template: links stored + // before save-time validation existed are still in the DB, and a field + // VALUE substituted into a template can carry a hostile scheme. + it("does not open a cross-link whose resolved url is unsafe", () => { + const openSpy = vi.spyOn(window, "open").mockImplementation(() => null); + + for (const hostile of [ + "javascript:alert(document.cookie)", + "data:text/html,", + "file:///etc/passwd", + "http://", + "not a url", + ]) { + wrapper.vm.openCrossLink(hostile); + } + + expect(openSpy).not.toHaveBeenCalled(); openSpy.mockRestore(); }); diff --git a/web/src/plugins/logs/DetailTable.vue b/web/src/plugins/logs/DetailTable.vue index b9440a7008..57468ba8d9 100644 --- a/web/src/plugins/logs/DetailTable.vue +++ b/web/src/plugins/logs/DetailTable.vue @@ -471,6 +471,7 @@ import OIcon from "@/lib/core/Icon/OIcon.vue"; import OTable from "@/lib/core/Table/OTable.vue"; import OSearchInput from "@/lib/forms/SearchInput/OSearchInput.vue"; import OSeparator from "@/lib/core/Separator/OSeparator.vue"; +import { isSafeNavigableUrl } from "@/utils/safeUrl"; const defaultValue: any = () => { return { data: {}, @@ -922,7 +923,9 @@ export default defineComponent({ }; const openCrossLink = (url: string) => { - window.open(url, "_blank"); + // Guard the RESOLVED url — see JsonPreview.vue's twin for the reasoning. + if (!isSafeNavigableUrl(url)) return; + window.open(url, "_blank", "noopener,noreferrer"); }; const viewTrace = () => { diff --git a/web/src/plugins/logs/JsonPreview.spec.ts b/web/src/plugins/logs/JsonPreview.spec.ts index 1696b42980..6775a16dab 100644 --- a/web/src/plugins/logs/JsonPreview.spec.ts +++ b/web/src/plugins/logs/JsonPreview.spec.ts @@ -1130,7 +1130,33 @@ describe("JsonPreview Component", () => { const openSpy = vi.spyOn(window, "open").mockImplementation(() => null); wrapper.vm.openCrossLink("https://example.com/trace/123"); - expect(openSpy).toHaveBeenCalledWith("https://example.com/trace/123", "_blank"); + // `noopener,noreferrer` stops the opened tab reaching back via + // window.opener and strips the referrer. + expect(openSpy).toHaveBeenCalledWith( + "https://example.com/trace/123", + "_blank", + "noopener,noreferrer", + ); + openSpy.mockRestore(); + }); + + // See the DetailTable twin: the RESOLVED url is guarded, because links + // saved before validation existed are still in the DB and a substituted + // field VALUE can carry a hostile scheme. + it("does not open a cross-link whose resolved url is unsafe", () => { + const openSpy = vi.spyOn(window, "open").mockImplementation(() => null); + + for (const hostile of [ + "javascript:alert(document.cookie)", + "data:text/html,", + "file:///etc/passwd", + "http://", + "not a url", + ]) { + wrapper.vm.openCrossLink(hostile); + } + + expect(openSpy).not.toHaveBeenCalled(); openSpy.mockRestore(); }); diff --git a/web/src/plugins/logs/JsonPreview.vue b/web/src/plugins/logs/JsonPreview.vue index d4bd31b9c0..0601fdaeae 100644 --- a/web/src/plugins/logs/JsonPreview.vue +++ b/web/src/plugins/logs/JsonPreview.vue @@ -280,6 +280,7 @@ import OSelect from "@/lib/forms/Select/OSelect.vue"; import { copyToClipboard } from "@/utils/clipboard"; import { timestampToTimezoneDate } from "@/utils/timezone"; import { toast } from "@/lib/feedback/Toast/useToast"; +import { isSafeNavigableUrl } from "@/utils/safeUrl"; export default { name: "JsonPreview", @@ -485,7 +486,14 @@ export default { const crossLinkDropdownVisible = ref(false); const openCrossLink = (url: string) => { - window.open(url, "_blank"); + // Guard the RESOLVED url, not just the saved template: links stored + // before save-time validation existed are still in the DB, and a field + // VALUE substituted into the template can carry a hostile scheme too. + if (!isSafeNavigableUrl(url)) { + crossLinkDropdownVisible.value = false; + return; + } + window.open(url, "_blank", "noopener,noreferrer"); crossLinkDropdownVisible.value = false; }; let multiStreamFields: any = ref([]); diff --git a/web/src/utils/common.spec.ts b/web/src/utils/common.spec.ts index 838a9c7a92..6ea0c766f0 100644 --- a/web/src/utils/common.spec.ts +++ b/web/src/utils/common.spec.ts @@ -38,6 +38,9 @@ describe("Common Utils", () => { Object.defineProperty(window, "location", { value: { href: "", + // `origin` is what the same-origin redirect check compares against; + // a real window.location always has it. + origin: "http://localhost:3000", replace: vi.fn(), }, writable: true, @@ -175,12 +178,26 @@ describe("Common Utils", () => { mockZincutils.getPath = vi.fn().mockReturnValue("/default-path"); }); - it("should redirect to external URL when redirectURI contains http", () => { - const externalUrl = "https://external-site.com/redirect"; + // SECURITY: `redirectURI` comes from the attacker-controllable + // `?short_url=` query param. It previously only had to CONTAIN "http", + // so an off-origin URL sent the user to a phishing site right after they + // authenticated. Off-origin must now fall back to the home path. + it("refuses an off-origin redirect and goes to the default path instead", () => { + const replaceSpy = vi.spyOn(window.location, "replace"); - redirectUser(externalUrl); + redirectUser("https://external-site.com/redirect"); - expect(window.location.href).toBe(externalUrl); + expect(replaceSpy).toHaveBeenCalledWith("/default-path"); + expect(window.location.href).not.toContain("external-site.com"); + }); + + it("refuses protocol-relative and backslash off-origin tricks", () => { + for (const hostile of ["//evil.com/", "/\\evil.com", "https://localhost.evil.com/"]) { + const replaceSpy = vi.spyOn(window.location, "replace"); + redirectUser(hostile); + expect(replaceSpy, `accepted: ${hostile}`).toHaveBeenCalledWith("/default-path"); + replaceSpy.mockRestore(); + } }); it("should use replace for internal redirects", () => { @@ -209,20 +226,12 @@ describe("Common Utils", () => { expect(replaceSpy).toHaveBeenCalledWith("/default-path"); }); - it("should handle https URLs", () => { - const httpsUrl = "https://secure.example.com/auth"; + it("still allows an absolute URL on the SAME origin", () => { + const sameOrigin = "http://localhost:3000/auth/callback"; - redirectUser(httpsUrl); + redirectUser(sameOrigin); - expect(window.location.href).toBe(httpsUrl); - }); - - it("should handle http URLs", () => { - const httpUrl = "http://example.com/redirect"; - - redirectUser(httpUrl); - - expect(window.location.href).toBe(httpUrl); + expect(window.location.href).toBe(sameOrigin); }); it("should handle relative paths", () => { diff --git a/web/src/utils/common.ts b/web/src/utils/common.ts index 2bff4e517a..de05aaeb07 100644 --- a/web/src/utils/common.ts +++ b/web/src/utils/common.ts @@ -16,6 +16,7 @@ import { ref } from "vue"; import organizationsService from "../services/organizations"; import { useLocalOrganization, getPath } from "./zincutils"; +import { isSameOriginRedirect } from "@/utils/safeUrl"; const selectedOrg = ref(""); const orgOptions = ref([{ label: Number, value: String }]); @@ -71,11 +72,21 @@ export const getDefaultOrganization = async (userInfo: any, org_identifier: any) export const redirectUser = (redirectURI: string | null) => { const path = getPath(); + // `redirectURI` comes from the `?short_url=` query param (router/index.ts), + // so it is attacker-controllable. The previous check was + // `redirectURI.includes("http")` — a substring test that happily matched + // `https://evil.com/` and sent the user off-site immediately AFTER they + // authenticated. Anything not on this origin falls back to the home path. if (redirectURI != null && redirectURI != "") { - if (redirectURI.includes("http")) { - window.location.href = redirectURI; - } else { + if (!isSameOriginRedirect(redirectURI)) { + // Off-origin: refuse it and go home instead. + window.location.replace(path); + } else if (redirectURI.startsWith("/")) { + // Relative path — keep using `replace` so the login page does not stay + // in history (unchanged from the original behaviour). window.location.replace(redirectURI); + } else { + window.location.href = redirectURI; } } else { // $router.push({ path: "/" }); diff --git a/web/src/utils/safeUrl.spec.ts b/web/src/utils/safeUrl.spec.ts new file mode 100644 index 0000000000..18e8f35780 --- /dev/null +++ b/web/src/utils/safeUrl.spec.ts @@ -0,0 +1,125 @@ +// 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 . + +import { describe, expect, it } from "vitest"; +import { isSafeNavigableUrl, isSameOriginRedirect } from "./safeUrl"; + +describe("isSafeNavigableUrl", () => { + it("accepts absolute http(s) URLs", () => { + for (const ok of [ + "https://example.com", + "http://example.com/path?q=1#f", + "https://example.com:8443/x", + "https://user:pass@example.com/x", + "https://192.168.1.1/x", + "https://[::1]:8080/x", + "HTTPS://EXAMPLE.COM/X", + ]) { + expect(isSafeNavigableUrl(ok), `rejected: ${ok}`).toBe(true); + } + }); + + it("rejects active-content schemes, including blocklist bypasses", () => { + for (const bad of [ + "javascript:alert(1)", + "JavaScript:alert(1)", + " javascript:alert(1)", + "java\tscript:alert(1)", + "java\nscript:alert(1)", + "\u0000javascript:alert(1)", + "vbscript:msgbox(1)", + // Explicitly allowed by the old drilldown regex — both are unsafe. + "data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg==", + "file:///etc/passwd", + "telnet://host", + "ws://host", + ]) { + expect(isSafeNavigableUrl(bad), `accepted: ${bad}`).toBe(false); + } + }); + + it("rejects an allowlisted scheme with no host (a WHATWG parse failure)", () => { + for (const bad of ["http:", "http://", "https://", "http:///", "http://?q=1", "http://#f"]) { + expect(isSafeNavigableUrl(bad), `accepted: ${bad}`).toBe(false); + } + }); + + it("rejects hosts that parse but cannot resolve", () => { + for (const bad of ["https://.", "http://..", "https://-", "http://.:80"]) { + expect(isSafeNavigableUrl(bad), `accepted: ${bad}`).toBe(false); + } + }); + + it("rejects values that are not URLs at all", () => { + for (const bad of ["", " ", "foo", "javascript(0)", "not a url at all"]) { + expect(isSafeNavigableUrl(bad), `accepted: ${bad}`).toBe(false); + } + }); + + it("optionally allows mailto: for link targets", () => { + expect(isSafeNavigableUrl("mailto:on@call.com")).toBe(false); + expect(isSafeNavigableUrl("mailto:on@call.com", { allowMailto: true })).toBe(true); + // Still needs a real mailbox. + expect(isSafeNavigableUrl("mailto:", { allowMailto: true })).toBe(false); + expect(isSafeNavigableUrl("mailto:foo", { allowMailto: true })).toBe(false); + }); + + it("optionally allows a {variable} template that resolves at render time", () => { + expect(isSafeNavigableUrl("{alert_url}")).toBe(false); + expect(isSafeNavigableUrl("{alert_url}", { allowTemplateVars: true })).toBe(true); + expect(isSafeNavigableUrl("https://x.example/{field}", { allowTemplateVars: true })).toBe(true); + // A variable elsewhere does not excuse a literal hostile scheme. + expect(isSafeNavigableUrl("{x}javascript:alert(1)", { allowTemplateVars: true })).toBe(false); + }); +}); + +describe("isSameOriginRedirect", () => { + const ORIGIN = "https://app.example.com"; + + it("accepts a relative path", () => { + for (const ok of ["/web/logs", "/", "/a?b=1#c"]) { + expect(isSameOriginRedirect(ok, ORIGIN), `rejected: ${ok}`).toBe(true); + } + }); + + it("accepts an absolute URL on the SAME origin", () => { + expect(isSameOriginRedirect(`${ORIGIN}/web/logs`, ORIGIN)).toBe(true); + }); + + it("rejects an absolute URL on a DIFFERENT origin", () => { + // The old check was `redirectURI.includes("http")`, which let every one + // of these through and redirected the user off-site after login. + for (const bad of [ + "https://evil.com/", + "http://evil.com/", + "https://app.example.com.evil.com/", // lookalike host + "//evil.com/", // protocol-relative + "https://app.example.com:8443/x", // different port = different origin + ]) { + expect(isSameOriginRedirect(bad, ORIGIN), `accepted: ${bad}`).toBe(false); + } + }); + + it("rejects hostile schemes and backslash tricks", () => { + for (const bad of ["javascript:alert(1)", "data:text/html,x", "\\\\evil.com", "/\\evil.com"]) { + expect(isSameOriginRedirect(bad, ORIGIN), `accepted: ${bad}`).toBe(false); + } + }); + + it("rejects empty/nullish input", () => { + expect(isSameOriginRedirect("", ORIGIN)).toBe(false); + expect(isSameOriginRedirect(null, ORIGIN)).toBe(false); + }); +}); diff --git a/web/src/utils/safeUrl.ts b/web/src/utils/safeUrl.ts new file mode 100644 index 0000000000..7c2bb2da39 --- /dev/null +++ b/web/src/utils/safeUrl.ts @@ -0,0 +1,133 @@ +// 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 . +// +// 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; + } +} diff --git a/web/src/views/Login.spec.ts b/web/src/views/Login.spec.ts index 872df87554..4cf45442db 100644 --- a/web/src/views/Login.spec.ts +++ b/web/src/views/Login.spec.ts @@ -435,20 +435,24 @@ describe("Login.vue", () => { expect(pushSpy).toHaveBeenCalledWith({ path: "/dashboard" }); }); - it("should redirect to external URL when redirectURI contains http", async () => { - // Mock azure_marketplace_token as null so it checks redirectURI + // SECURITY: `redirectURI` originates from the attacker-controllable + // `?short_url=` query param, so an OFF-ORIGIN value here is a post-auth + // open redirect (phishing). It must be refused and routed home instead. + it("refuses an off-origin redirect and routes to the home path", async () => { mockSessionStorage.getItem.mockImplementation((key: string) => { if (key === "azure_marketplace_token") return null; if (key === "redirectURI") return "https://external.com"; return null; }); + const pushSpy = vi.spyOn(router, "push"); + wrapper.vm.redirectUser(); expect(mockSessionStorage.getItem).toHaveBeenCalledWith("azure_marketplace_token"); - expect(mockSessionStorage.getItem).toHaveBeenCalledWith("redirectURI"); expect(mockSessionStorage.removeItem).toHaveBeenCalledWith("redirectURI"); - expect(window.location.href).toBe("https://external.com"); + expect(window.location.href).not.toBe("https://external.com"); + expect(pushSpy).toHaveBeenCalledWith({ path: "/" }); }); it("should redirect to home when no redirectURI", async () => { diff --git a/web/src/views/Login.vue b/web/src/views/Login.vue index d2d753a2e0..c7f2edda2f 100644 --- a/web/src/views/Login.vue +++ b/web/src/views/Login.vue @@ -58,6 +58,7 @@ import { import { useReo } from "@/services/reodotdev_analytics"; import { toast } from "@/lib/feedback/Toast/useToast"; import { useI18nTyped } from "@/types/i18n"; +import { isSameOriginRedirect } from "@/utils/safeUrl"; export default defineComponent({ name: "LoginPage", @@ -233,11 +234,14 @@ export default defineComponent({ const redirectURI = window.sessionStorage.getItem("redirectURI"); window.sessionStorage.removeItem("redirectURI"); - if (redirectURI != null && redirectURI != "") { - if (redirectURI.includes("http")) { - window.location.href = redirectURI; - } else { + // Same-origin only: `redirectURI` originates from the `?short_url=` + // query param, so an off-origin value here is a post-auth open redirect + // (see utils/common.ts redirectUser for the twin of this check). + if (redirectURI != null && redirectURI != "" && isSameOriginRedirect(redirectURI)) { + if (redirectURI.startsWith("/")) { router.push({ path: redirectURI }); + } else { + window.location.href = redirectURI; } } else { router.push({ path: "/" });