From 534698a16beddefafe8eede04bfd36b510dcba8f Mon Sep 17 00:00:00 2001 From: Liu Liu Date: Tue, 30 Jun 2026 09:25:53 -0700 Subject: [PATCH] sanitize href --- .changeset/sanitize-href-uri-scheme.md | 5 ++ app/components/primer/base_component.rb | 1 + app/components/primer/component.rb | 16 +++++ app/lib/primer/safe_href_helper.rb | 36 +++++++++++ test/components/base_component_test.rb | 40 ++++++++++++ test/components/beta/label_test.rb | 15 +++++ test/components/primer/beta/button_test.rb | 15 +++++ test/lib/safe_href_helper_test.rb | 72 ++++++++++++++++++++++ 8 files changed, 200 insertions(+) create mode 100644 .changeset/sanitize-href-uri-scheme.md create mode 100644 app/lib/primer/safe_href_helper.rb create mode 100644 test/lib/safe_href_helper_test.rb diff --git a/.changeset/sanitize-href-uri-scheme.md b/.changeset/sanitize-href-uri-scheme.md new file mode 100644 index 0000000000..b27b7ad385 --- /dev/null +++ b/.changeset/sanitize-href-uri-scheme.md @@ -0,0 +1,5 @@ +--- +"@primer/view-components": patch +--- + +Reject `javascript:` and `vbscript:` URI schemes in `href` (defense in depth). When a component (e.g. `Primer::Beta::Label`, `Primer::Beta::Button`, `Primer::Beta::Link`) is rendered as an anchor with an unsafe `href`, the value is now rejected — raising in non-production environments and silently dropped (rendered as an anchor with no `href`) in production. diff --git a/app/components/primer/base_component.rb b/app/components/primer/base_component.rb index 2e90df8e8c..c65a65aa50 100644 --- a/app/components/primer/base_component.rb +++ b/app/components/primer/base_component.rb @@ -156,6 +156,7 @@ def initialize(tag:, classes: nil, **system_arguments) @tag = tag @system_arguments = validate_arguments(tag: tag, **system_arguments) + sanitize_href!(@system_arguments) @result = Primer::Classify.call(**@system_arguments.merge(classes: classes)) diff --git a/app/components/primer/component.rb b/app/components/primer/component.rb index 84b5e8c620..89b02c068f 100644 --- a/app/components/primer/component.rb +++ b/app/components/primer/component.rb @@ -22,6 +22,7 @@ class Component < ViewComponent::Base include AttributesHelper include ClassNameHelper include FetchOrFallbackHelper + include SafeHrefHelper include TestSelectorHelper include JoinStyleArgumentsHelper include ViewHelper @@ -137,6 +138,21 @@ def deny_tag_argument(**arguments) deny_single_argument(:tag, "This component has a fixed tag.", **arguments) end + # Removes `href` values that point at disallowed URI schemes (`javascript:`, + # `vbscript:`). Raises in non-production environments so the offending call + # site gets fixed; in production we silently drop the attribute so the link + # is rendered inert rather than crashing the page. + def sanitize_href!(arguments) + return unless arguments.key?(:href) + return unless Primer::SafeHrefHelper.unsafe_href?(arguments[:href]) + + if should_raise_error? + raise ArgumentError, "Rejected dangerous URI scheme in `href`: #{arguments[:href].inspect}" + end + + arguments[:href] = nil + end + def should_raise_error? !Rails.env.production? && raise_on_invalid_options? && !ENV["PRIMER_WARNINGS_DISABLED"] end diff --git a/app/lib/primer/safe_href_helper.rb b/app/lib/primer/safe_href_helper.rb new file mode 100644 index 0000000000..3f7b69d382 --- /dev/null +++ b/app/lib/primer/safe_href_helper.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +# Primer::SafeHrefHelper +# +# Detects unsafe URI schemes (e.g. `javascript:`) in `href` values so they can +# be neutralized before being rendered into HTML attributes. Without this check, +# a caller that forwards untrusted input into a component's `href:` argument can +# trigger XSS when the user clicks the rendered anchor. +module Primer + # :nodoc: + module SafeHrefHelper + # URI schemes that can execute script in the browser and are never valid + # destinations for a Primer-rendered link. + DISALLOWED_HREF_SCHEMES = %w[javascript vbscript].freeze + + # Returns true when `href` starts with a disallowed URI scheme. + # + # Mirrors browser URL parsing by stripping ASCII whitespace and control + # characters (including tab/CR/LF) before extracting the scheme. This + # prevents bypasses such as `j\tavascript:...`, ` JaVaScRiPt:...`, or a + # leading null byte, all of which browsers happily execute. + def self.unsafe_href?(href) + return false if href.nil? + + normalized = href.to_s.gsub(/[\u0000-\u0020]/, "") + scheme = normalized[/\A([a-z][a-z0-9+\-.]*):/i, 1] + return false unless scheme + + DISALLOWED_HREF_SCHEMES.include?(scheme.downcase) + end + + def unsafe_href?(href) + SafeHrefHelper.unsafe_href?(href) + end + end +end diff --git a/test/components/base_component_test.rb b/test/components/base_component_test.rb index 8097176a82..d232f8d26b 100644 --- a/test/components/base_component_test.rb +++ b/test/components/base_component_test.rb @@ -92,6 +92,46 @@ def test_renders_as_a_link assert_selector("a[href='http://google.com']") end + def test_rejects_javascript_href_in_non_production + err = assert_raises(ArgumentError) do + render_inline(Primer::BaseComponent.new(tag: :a, href: "javascript:alert(1)")) + end + assert_match(/Rejected dangerous URI scheme/, err.message) + end + + def test_neutralizes_javascript_href_in_production + with_raise_on_invalid_options(false) do + render_inline(Primer::BaseComponent.new(tag: :a, href: "javascript:alert(1)")) { "x" } + + assert_selector("a") + refute_selector("a[href]") + end + end + + def test_neutralizes_vbscript_href_in_production + with_raise_on_invalid_options(false) do + render_inline(Primer::BaseComponent.new(tag: :a, href: "vbscript:msgbox(1)")) { "x" } + + refute_selector("a[href]") + end + end + + def test_neutralizes_javascript_href_with_whitespace_bypass_attempt + with_raise_on_invalid_options(false) do + render_inline(Primer::BaseComponent.new(tag: :a, href: "\tJaVaScRiPt:alert(1)")) { "x" } + + refute_selector("a[href]") + end + end + + def test_allows_safe_hrefs + render_inline(Primer::BaseComponent.new(tag: :a, href: "/foo/bar")) + assert_selector("a[href='/foo/bar']") + + render_inline(Primer::BaseComponent.new(tag: :a, href: "mailto:hello@example.com")) + assert_selector("a[href='mailto:hello@example.com']") + end + # We were calling tag.send(as), passing in :p ended up calling `p`, aka `puts` # Due to how Rails uses method_missing in TagHelper. See Slack convo: # https://github.slack.com/archives/C0HV3F37A/p1556216733019500 diff --git a/test/components/beta/label_test.rb b/test/components/beta/label_test.rb index 77a203a866..6e1c0a288f 100644 --- a/test/components/beta/label_test.rb +++ b/test/components/beta/label_test.rb @@ -95,4 +95,19 @@ def test_falls_back_when_variant_isn_t_valid def test_status assert_component_state(Primer::Beta::Label, :beta) end + + def test_rejects_javascript_href_when_tag_is_anchor + assert_raises(ArgumentError) do + render_inline(Primer::Beta::Label.new(tag: :a, href: "javascript:alert(document.domain)")) { "x" } + end + end + + def test_neutralizes_javascript_href_when_tag_is_anchor_in_production + with_raise_on_invalid_options(false) do + render_inline(Primer::Beta::Label.new(tag: :a, href: "javascript:alert(document.domain)")) { "x" } + + assert_selector("a.Label") + refute_selector("a.Label[href]") + end + end end diff --git a/test/components/primer/beta/button_test.rb b/test/components/primer/beta/button_test.rb index 6fb090419c..e99c532a95 100644 --- a/test/components/primer/beta/button_test.rb +++ b/test/components/primer/beta/button_test.rb @@ -87,4 +87,19 @@ def test_warns_on_uses_of_dropdown assert_equal "The `dropdown:` argument is no longer supported on Primer::Beta::Button. Use the `trailing_action` slot instead.", err.message end + + def test_rejects_javascript_href_when_rendered_as_anchor + assert_raises(ArgumentError) do + render_inline(Primer::Beta::Button.new(tag: :a, href: "javascript:alert(document.domain)")) { "Button" } + end + end + + def test_neutralizes_javascript_href_when_rendered_as_anchor_in_production + with_raise_on_invalid_options(false) do + render_inline(Primer::Beta::Button.new(tag: :a, href: "javascript:alert(document.domain)")) { "Button" } + + assert_selector("a.Button", text: "Button") + refute_selector("a.Button[href]") + end + end end diff --git a/test/lib/safe_href_helper_test.rb b/test/lib/safe_href_helper_test.rb new file mode 100644 index 0000000000..c15f986855 --- /dev/null +++ b/test/lib/safe_href_helper_test.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require "lib/test_helper" + +class Primer::SafeHrefHelperTest < Minitest::Test + include Primer::SafeHrefHelper + + def test_returns_false_for_nil + refute Primer::SafeHrefHelper.unsafe_href?(nil) + end + + def test_returns_false_for_relative_paths + refute Primer::SafeHrefHelper.unsafe_href?("/foo/bar") + refute Primer::SafeHrefHelper.unsafe_href?("foo/bar") + refute Primer::SafeHrefHelper.unsafe_href?("#anchor") + refute Primer::SafeHrefHelper.unsafe_href?("?query=1") + refute Primer::SafeHrefHelper.unsafe_href?("") + end + + def test_returns_false_for_safe_schemes + refute Primer::SafeHrefHelper.unsafe_href?("https://example.com") + refute Primer::SafeHrefHelper.unsafe_href?("http://example.com") + refute Primer::SafeHrefHelper.unsafe_href?("mailto:foo@example.com") + refute Primer::SafeHrefHelper.unsafe_href?("ftp://example.com") + refute Primer::SafeHrefHelper.unsafe_href?("tel:+15551234567") + refute Primer::SafeHrefHelper.unsafe_href?("data:image/png;base64,abc") + end + + def test_detects_javascript_uri + assert Primer::SafeHrefHelper.unsafe_href?("javascript:alert(1)") + end + + def test_detects_vbscript_uri + assert Primer::SafeHrefHelper.unsafe_href?("vbscript:msgbox(1)") + end + + def test_detects_mixed_case_schemes + assert Primer::SafeHrefHelper.unsafe_href?("JaVaScRiPt:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("JAVASCRIPT:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("VbScript:alert(1)") + end + + def test_detects_leading_whitespace_bypasses + assert Primer::SafeHrefHelper.unsafe_href?(" javascript:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("\tjavascript:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("\njavascript:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("\rjavascript:alert(1)") + end + + def test_detects_embedded_whitespace_and_control_chars_in_scheme + # Browsers strip tab/CR/LF inside URLs per the WHATWG URL spec, so these execute. + assert Primer::SafeHrefHelper.unsafe_href?("java\tscript:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("java\nscript:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("java\rscript:alert(1)") + assert Primer::SafeHrefHelper.unsafe_href?("j\u0000avascript:alert(1)") + end + + def test_does_not_misclassify_schemes_that_merely_contain_javascript + refute Primer::SafeHrefHelper.unsafe_href?("https://example.com/javascript:foo") + refute Primer::SafeHrefHelper.unsafe_href?("/javascript:foo") + end + + def test_handles_non_string_values + refute Primer::SafeHrefHelper.unsafe_href?(123) + refute Primer::SafeHrefHelper.unsafe_href?(:foo) + end + + def test_instance_method_delegates_to_module_method + assert unsafe_href?("javascript:alert(1)") + refute unsafe_href?("https://example.com") + end +end