Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RequireSettings.Attributes are lost for dependencies #17283

Open
rjpowers10 opened this issue Dec 27, 2024 · 1 comment
Open

RequireSettings.Attributes are lost for dependencies #17283

rjpowers10 opened this issue Dec 27, 2024 · 1 comment
Labels

Comments

@rjpowers10
Copy link
Contributor

rjpowers10 commented Dec 27, 2024

Describe the bug

I have two scripts, A and B. Script A depends on script B. I set an HTML attribute on the RequireSettings object for script B, which in 1.8.2 would eventually propagate to the script tag helper. In 2.1.3, that attribute is lost.

Orchard Core version

2.1.3

To Reproduce

So, this is kind of complicated so I'll just try to talk my way through it first.

Ultimately, I'm trying to set the nonce HTML attribute on all scripts so I can implement a content security policy. The way I did this in 1.8.2 is I wrote a wrapper around ResourceManager. Every call to RegisterResource would take the resulting RequireSettings object and call RequireSettings.SetAttribute("nonce", "my-nonce") before returning it. Essentially it looks something like this:

MyResourceManagerWrapper

public RequireSettings RegisterResource(string resourceType, string resourceName)
{
    RequireSettings requireSettings = _orchardCoreResourceManager.RegisterResource(resourceType, resourceName);
    AppendNonce(requireSettings, resourceType); // Sets the "nonce" HTML attribute
    return requireSettings;
}

I think this can change be traced to #15438.

// Settings is given so the location can cascade down into dependencies. For example, if Foo depends on Bar,
// and Foo's required location is Head, so too should Bar's location, similarly, if Foo is First positioned,
// Bar dependency should be too. This behavior only applies to the dependencies.

var dependencySettings = ((RequireSettings)allResources[resource])?.New() ?? new RequireSettings(_options, resource);
dependencySettings = isTopLevel ? dependencySettings.Combine(settings) : dependencySettings.AtLocation(settings.Location);
dependencySettings = dependencySettings.CombinePosition(settings);

At this point we're building up script A and expanding its dependencies. allResources[resource] returns the RequireSettings for script B, which has my nonce attribute on it. But then we call the New method, which doesn't copy the attributes.

I think the idea here is we're building up an ad-hoc RequireSettings for script B that is mostly based on script B, but the location and position are cascading down from script A. Okay, that makes sense. But I still want the HTML attributes I set on script B to stick around. Should the New method be copying those?

I think the intention of the PR was to stop script A's attributes from cascading to script B, but it is maybe an unintended side effect that script B's attributes are lost.

Before #15438 it would have called NewAndCombine which did copy the attributes.

@rjpowers10
Copy link
Contributor Author

I'm also open to other approaches. My ultimate goal is to set the nonce attribute for all scripts. If there's a better way I'm all for that. I know I can also set attributes on the ResourceDefinition, but I need the nonce to be per-request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant