Repository: servo/servo
File: components/script/dom/security/csp.rs (Rust)

Code before:
        for violation in violations {
            let resource = &violation.resource;
            let (sample, resource_str) = match resource {
                ViolationResource::Inline { sample } => (sample.clone(), "inline".to_owned()),
                ViolationResource::Url(url) => (Some(String::new()), url.clone().into()),
                ViolationResource::TrustedTypePolicy { sample } => {
                    (Some(sample.clone()), "trusted-types-policy".to_owned())
                },
                ViolationResource::TrustedTypeSink { sample } => {
                    (Some(sample.clone()), "trusted-types-sink".to_owned())
                },
                ViolationResource::Eval { sample } => (sample.clone(), "eval".to_owned()),
                ViolationResource::WasmEval => (None, "wasm-eval".to_owned()),
            };

            let (source_file, line_number, column_number) = if source_position_was_provided ||
                // if it is not a URL resource, use the computed fallback source position
                !matches!(violation.resource, ViolationResource::Url(_))
            {
                (
                    source_position.source_file.clone(),
                    source_position.line_number,
                    source_position.column_number,
                )
            } else {
                (self.get_url().to_string(), 0, 0)
            };
            let report = CSPViolationReportBuilder::default()
                .resource(resource_str)
                .sample(sample)
                .effective_directive(violation.directive.name)
                .original_policy(violation.policy.to_string())
                .report_only(violation.policy.disposition == PolicyDisposition::Report)
                .source_file(source_file)
                .line_number(line_number)
                .column_number(column_number)
                .build(self);
            // Step 1: Let global be violation’s global object.
            // We use `self` as `global`;
            // Step 2: Let target be violation’s element.
            let target = element.and_then(|event_target| {
                // Step 3.1: If target is not null, and global is a Window,
                // and target’s shadow-including root is not global’s associated Document, set target to null.
                if let Some(window) = self.downcast::<Window>() {
                    // If a node is connected, its owner document is always the shadow-including root.
                    // If it isn't connected, then it also doesn't have a corresponding document, hence
                    // it can't be this document.
                    if event_target.upcast::<Node>().owner_document() != window.Document() {
                        return None;
                    }
                }

Review comment (bug):
I feel like we are over-complicating things here and we would be better off fixing the callers of this method. Moreover, since `self.get_url()` is the URL for the globalscope, not the resource. Therefore, I think in almost all cases this URL is wrong since we expect the URL of the blocked resource, not the global it is part of?

Instead, can we update all caller-sides to provide the relevant information. If I understand you correctly, in the following cases we should set the filename, but not the line numbers:

- Link elements
- Image elements

And for script elements, it depends whether it is inline or external.

If that's the case, can we revert the changes made in this function and update the elements to pass in the right source position based on these expectations?

Additionally, it might be good to split up the PR as it fixes two distinct issues. One is related to nonce values, the other one is related to the source position. Therefore, I suggest to split it up in those two, to ease reviewing.

Lastly, it would help if WPT tests start passing, yet none are with this PR thus far. I understand that's blocked on the other mentioned issue in the HTML parser, so that's a bit unfortunate. It could be good to write a WPT test that only tests for one thing (e.g. the URL without line and column number) and then we defer passing the other tests until the HTML parser is updated.

Does that help you further?

Code after:
}

#[expect(unsafe_code)]
fn compute_scripted_caller_source_position() -> SourcePosition {
    match unsafe { describe_scripted_caller(*GlobalScope::get_cx()) } {
        Ok(scripted_caller) => SourcePosition {
            source_file: scripted_caller.filename,
            line_number: scripted_caller.line,
            column_number: scripted_caller.col + 1,
        },
        Err(()) => SourcePosition {
            source_file: String::new(),
            line_number: 0,
            column_number: 0,
        },
    }
}

impl GlobalCspReporting for GlobalScope {
    /// <https://www.w3.org/TR/CSP/#report-violation>
    fn report_csp_violations(
        &self,
        violations: Vec<Violation>,
        element: Option<&Element>,
        source_position: Option<SourcePosition>,
    ) {
        if violations.is_empty() {
            return;
        }
        warn!("Reporting CSP violations: {:?}", violations);
        let source_position =
            source_position.unwrap_or_else(compute_scripted_caller_source_position);
        for violation in violations {
            let (sample, resource) = match violation.resource {
                ViolationResource::Inline { sample } => (sample, "inline".to_owned()),
                ViolationResource::Url(url) => (Some(String::new()), url.into()),
                ViolationResource::TrustedTypePolicy { sample } => {
                    (Some(sample), "trusted-types-policy".to_owned())
                },
                ViolationResource::TrustedTypeSink { sample } => {
                    (Some(sample), "trusted-types-sink".to_owned())
                },
                ViolationResource::Eval { sample } => (sample, "eval".to_owned()),
                ViolationResource::WasmEval => (None, "wasm-eval".to_owned()),
            };
            let report = CSPViolationReportBuilder::default()
                .resource(resource)
                .sample(sample)
                .effective_directive(violation.directive.name)
                .original_policy(violation.policy.to_string())
                .report_only(violation.policy.disposition == PolicyDisposition::Report)