From 06f86f88a2c2f89f2a7072626915f8949778195d Mon Sep 17 00:00:00 2001 From: chocolate-pie <106949016+chocolate-pie@users.noreply.github.com> Date: Sun, 13 Apr 2025 15:04:24 +0900 Subject: [PATCH] script: Clean up CSP management code (#36493) Current implementation takes arguments for specifying values of violation report, but is difficult to understand which value should be passed. These changes create new builder for violation report to address the issue. Testing: These changes do not require tests because they just refactor current code Signed-off-by: Chocolate Pie <106949016+chocolate-pie@users.noreply.github.com> --- components/script/dom/globalscope.rs | 24 ++-- components/script/script_runtime.rs | 27 ++-- components/script/security_manager.rs | 176 ++++++++++++++++---------- 3 files changed, 136 insertions(+), 91 deletions(-) diff --git a/components/script/dom/globalscope.rs b/components/script/dom/globalscope.rs index a474cd30f95..e23a35b349d 100644 --- a/components/script/dom/globalscope.rs +++ b/components/script/dom/globalscope.rs @@ -33,8 +33,7 @@ use ipc_channel::router::ROUTER; use js::glue::{IsWrapper, UnwrapObjectDynamic}; use js::jsapi::{ Compile1, CurrentGlobalOrNull, GetNonCCWObjectGlobal, HandleObject, Heap, - InstantiateGlobalStencil, InstantiateOptions, JSContext, JSObject, JSScript, RuntimeCode, - SetScriptPrivate, + InstantiateGlobalStencil, InstantiateOptions, JSContext, JSObject, JSScript, SetScriptPrivate, }; use js::jsval::{PrivateValue, UndefinedValue}; use js::panic::maybe_resume_unwind; @@ -139,7 +138,7 @@ use crate::realms::{AlreadyInRealm, InRealm, enter_realm}; use crate::script_module::{DynamicModuleList, ModuleScript, ModuleTree, ScriptFetchOptions}; use crate::script_runtime::{CanGc, JSContext as SafeJSContext, ThreadSafeJSContext}; use crate::script_thread::{ScriptThread, with_script_thread}; -use crate::security_manager::CSPViolationReporter; +use crate::security_manager::{CSPViolationReportBuilder, CSPViolationReportTask}; use crate::task_manager::TaskManager; use crate::task_source::SendableTaskSource; use crate::timers::{ @@ -2676,15 +2675,16 @@ impl GlobalScope { // FIXME: Don't fire event if `script-src` and `default-src` // were not passed. for policy in csp_list.0 { - let task = CSPViolationReporter::new( - self, - None, - policy.disposition == PolicyDisposition::Report, - RuntimeCode::JS, - scripted_caller.filename.clone(), - scripted_caller.line, - scripted_caller.col, - ); + let report = CSPViolationReportBuilder::default() + .resource("eval".to_owned()) + .effective_directive("script-src".to_owned()) + .report_only(policy.disposition == PolicyDisposition::Report) + .source_file(scripted_caller.filename.clone()) + .line_number(scripted_caller.line) + .column_number(scripted_caller.col) + .build(self); + let task = CSPViolationReportTask::new(self, report); + self.task_manager() .dom_manipulation_task_source() .queue(task); diff --git a/components/script/script_runtime.rs b/components/script/script_runtime.rs index 6ae5add2b96..b23be19720c 100644 --- a/components/script/script_runtime.rs +++ b/components/script/script_runtime.rs @@ -83,7 +83,7 @@ use crate::microtask::{EnqueuedPromiseCallback, Microtask, MicrotaskQueue}; use crate::realms::{AlreadyInRealm, InRealm}; use crate::script_module::EnsureModuleHooksInitialized; use crate::script_thread::trace_thread; -use crate::security_manager::CSPViolationReporter; +use crate::security_manager::{CSPViolationReportBuilder, CSPViolationReportTask}; use crate::task_source::SendableTaskSource; static JOB_QUEUE_TRAPS: JobQueueTraps = JobQueueTraps { @@ -390,6 +390,11 @@ unsafe extern "C" fn content_security_policy_allows( csp_list.is_wasm_evaluation_allowed() == CheckResult::Allowed; let scripted_caller = describe_scripted_caller(*cx).unwrap_or_default(); + let resource = match runtime_code { + RuntimeCode::JS => "eval".to_owned(), + RuntimeCode::WASM => "wasm-eval".to_owned(), + }; + allowed = match runtime_code { RuntimeCode::JS if is_js_evaluation_allowed => true, RuntimeCode::WASM if is_wasm_evaluation_allowed => true, @@ -400,15 +405,17 @@ unsafe extern "C" fn content_security_policy_allows( // FIXME: Don't fire event if `script-src` and `default-src` // were not passed. for policy in csp_list.0 { - let task = CSPViolationReporter::new( - &global, - sample.clone(), - policy.disposition == PolicyDisposition::Report, - runtime_code, - scripted_caller.filename.clone(), - scripted_caller.line, - scripted_caller.col, - ); + let report = CSPViolationReportBuilder::default() + .resource(resource.clone()) + .sample(sample.clone()) + .report_only(policy.disposition == PolicyDisposition::Report) + .source_file(scripted_caller.filename.clone()) + .line_number(scripted_caller.line) + .column_number(scripted_caller.col) + .effective_directive("script-src".to_owned()) + .build(&global); + let task = CSPViolationReportTask::new(&global, report); + global .task_manager() .dom_manipulation_task_source() diff --git a/components/script/security_manager.rs b/components/script/security_manager.rs index 223fd5e005a..60cf2267a2c 100644 --- a/components/script/security_manager.rs +++ b/components/script/security_manager.rs @@ -2,7 +2,6 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use js::jsapi::RuntimeCode; use net_traits::request::Referrer; use serde::Serialize; use servo_url::ServoUrl; @@ -23,14 +22,9 @@ use crate::dom::types::GlobalScope; use crate::script_runtime::CanGc; use crate::task::TaskOnce; -pub(crate) struct CSPViolationReporter { - sample: Option, - filename: String, - report_only: bool, - runtime_code: RuntimeCode, - line_number: u32, - column_number: u32, - target: Trusted, +pub(crate) struct CSPViolationReportTask { + event_target: Trusted, + violation_report: SecurityPolicyViolationReport, } #[derive(Debug, Serialize)] @@ -53,71 +47,63 @@ pub(crate) struct SecurityPolicyViolationReport { disposition: SecurityPolicyViolationEventDisposition, } -impl CSPViolationReporter { - pub(crate) fn new( - global: &GlobalScope, - sample: Option, - report_only: bool, - runtime_code: RuntimeCode, - filename: String, - line_number: u32, - column_number: u32, - ) -> CSPViolationReporter { - CSPViolationReporter { - sample, - filename, - report_only, - runtime_code, - line_number, - column_number, - target: Trusted::new(global.upcast::()), - } +#[derive(Default)] +pub(crate) struct CSPViolationReportBuilder { + pub report_only: bool, + /// + pub sample: Option, + /// + pub resource: String, + /// + pub line_number: u32, + /// + pub column_number: u32, + /// + pub source_file: String, + /// + pub effective_directive: String, +} + +impl CSPViolationReportBuilder { + pub fn report_only(mut self, report_only: bool) -> CSPViolationReportBuilder { + self.report_only = report_only; + self } - fn get_report(&self, global: &GlobalScope) -> SecurityPolicyViolationReport { - SecurityPolicyViolationReport { - sample: self.sample.clone(), - disposition: match self.report_only { - true => SecurityPolicyViolationEventDisposition::Report, - false => SecurityPolicyViolationEventDisposition::Enforce, - }, - // https://w3c.github.io/webappsec-csp/#violation-resource - blocked_url: match self.runtime_code { - RuntimeCode::JS => "eval".to_owned(), - RuntimeCode::WASM => "wasm-eval".to_owned(), - }, - // https://w3c.github.io/webappsec-csp/#violation-referrer - referrer: match global.get_referrer() { - Referrer::Client(url) => self.strip_url_for_reports(url), - Referrer::ReferrerUrl(url) => self.strip_url_for_reports(url), - _ => "".to_owned(), - }, - status_code: global.status_code().unwrap_or(200), - document_url: self.strip_url_for_reports(global.get_url()), - source_file: self.filename.clone(), - violated_directive: "script-src".to_owned(), - effective_directive: "script-src".to_owned(), - line_number: self.line_number, - column_number: self.column_number, - original_policy: String::default(), - } + /// + pub fn sample(mut self, sample: Option) -> CSPViolationReportBuilder { + self.sample = sample; + self } - fn fire_violation_event(&self, can_gc: CanGc) { - let target = self.target.root(); - let global = &target.global(); - let report = self.get_report(global); + /// + pub fn resource(mut self, resource: String) -> CSPViolationReportBuilder { + self.resource = resource; + self + } - let event = SecurityPolicyViolationEvent::new( - global, - Atom::from("securitypolicyviolation"), - EventBubbles::Bubbles, - EventCancelable::Cancelable, - &report.convert(), - can_gc, - ); + /// + pub fn line_number(mut self, line_number: u32) -> CSPViolationReportBuilder { + self.line_number = line_number; + self + } - event.upcast::().fire(&target, can_gc); + /// + pub fn column_number(mut self, column_number: u32) -> CSPViolationReportBuilder { + self.column_number = column_number; + self + } + + /// + pub fn source_file(mut self, source_file: String) -> CSPViolationReportBuilder { + self.source_file = source_file; + self + } + + /// + pub fn effective_directive(mut self, effective_directive: String) -> CSPViolationReportBuilder { + self.effective_directive = effective_directive; + self } /// @@ -136,12 +122,64 @@ impl CSPViolationReporter { // > Step 5: Return the result of executing the URL serializer on url. url.into_string() } + + pub fn build(self, global: &GlobalScope) -> SecurityPolicyViolationReport { + SecurityPolicyViolationReport { + violated_directive: self.effective_directive.clone(), + effective_directive: self.effective_directive.clone(), + document_url: self.strip_url_for_reports(global.get_url()), + disposition: match self.report_only { + true => SecurityPolicyViolationEventDisposition::Report, + false => SecurityPolicyViolationEventDisposition::Enforce, + }, + // https://w3c.github.io/webappsec-csp/#violation-referrer + referrer: match global.get_referrer() { + Referrer::Client(url) => self.strip_url_for_reports(url), + Referrer::ReferrerUrl(url) => self.strip_url_for_reports(url), + _ => "".to_owned(), + }, + sample: self.sample, + blocked_url: self.resource, + source_file: self.source_file, + original_policy: "".to_owned(), + line_number: self.line_number, + column_number: self.column_number, + status_code: global.status_code().unwrap_or(0), + } + } +} + +impl CSPViolationReportTask { + pub fn new( + global: &GlobalScope, + report: SecurityPolicyViolationReport, + ) -> CSPViolationReportTask { + CSPViolationReportTask { + violation_report: report, + event_target: Trusted::new(global.upcast::()), + } + } + + fn fire_violation_event(self, can_gc: CanGc) { + let target = self.event_target.root(); + let global = &target.global(); + let event = SecurityPolicyViolationEvent::new( + global, + Atom::from("securitypolicyviolation"), + EventBubbles::Bubbles, + EventCancelable::Cancelable, + &self.violation_report.convert(), + can_gc, + ); + + event.upcast::().fire(&target, can_gc); + } } /// Corresponds to the operation in 5.5 Report Violation /// /// > Queue a task to run the following steps: -impl TaskOnce for CSPViolationReporter { +impl TaskOnce for CSPViolationReportTask { fn run_once(self) { // > If target implements EventTarget, fire an event named securitypolicyviolation // > that uses the SecurityPolicyViolationEvent interface