jakartaee / faces

Jakarta Faces
Other
108 stars 55 forks source link

Provide Subresource Integrity for Web Resources #1983

Closed EugenFischer closed 6 days ago

EugenFischer commented 3 weeks ago

From my point of view, support for SRI for web resources would be necessary.

OmniFaces has a powerful way to handle web resources with CombinedResourceHandler. But you have to make do with many exceptions and restrictions.

From a security point of view, it would make sense to equip the web resources with SRI out of the box.

What do you think, would such an extension be useful and feasible?

Many greetings

melloware commented 3 weeks ago

@EugenFischer so you want the SRI on every JSF resource produced with h:output script? Correct?

EugenFischer commented 3 weeks ago

@melloware, yep, on script and style resources

BalusC commented 2 weeks ago

You mean, the framework must auto generate the integrity (and probably crossorigin) attribute like OmniFaces CombinedResourceHandler does? Point taken, but for sake of clarity, you could for the time being manually set it as a passthrough attribute:

<... xmlns:a="jakarta.faces.passthrough">
    ...
    <h:outputScript library="jakarta.faces" name="faces.js" a:integrity="sha384-xxxxxxxxxxxxxxx" />
BalusC commented 2 weeks ago

I approve this proposal btw.

@tandraschko, WDYT? Candidate for 5.0? The W3C spec is currently still in draft mode but it could be final before close of 5.0.

tandraschko commented 2 weeks ago

what should the JSF impl do? Just render the integrity attribute, if not defined by the user? ad adding crossorigin=anonymous?

I wonder whats the benefit as our resource urls are all relativ to the current host? the first liks mostly talk about CDN.

BalusC commented 2 weeks ago

Right. This makes only sense for CDN resources.

tandraschko commented 2 weeks ago

Will omnifaces load the remote resource now and store it here? https://github.com/omnifaces/omnifaces/blob/372a78d66ec8b460066ca7e6d56c96c1bccc6dd0/src/main/java/org/omnifaces/resourcehandler/ResourceIdentifier.java#L162C39-L162C53

BalusC commented 2 weeks ago

Nope, I just didn't think through it. It makes indeed no sense for local web resources. On the other hand, h:outputScript/h:outputStylesheet have no support for CDN resources so it wouldn't be possible to use them for CDN resources in first place.

@EugenFischer Did you have local or CDN resources in mind while opening this issue? Was you referring to OmniFaces CDNResource helper class in the statement "but you have to make do with many exceptions and restrictions"? Which problem exactly, ultimately, was you trying to solve which made you to come up with this issue?

EugenFischer commented 1 week ago

@BalusC
i wanted to use it for local web resources. i saw that you had also calculated an SRI for the collected javascrits / styles in the CombinedResourceHandler. i don't know if this is a smart idea, but i wanted to make sure that no outdated (cached by some proxy) or truncated resources arrive at the client. (because we are currently trying to solve just such a problem) CombinedResourceHandler is an awesome and powerful tool, but the combining makes it very difficult to check if all necessary resources arrive in the expected version.

BalusC commented 1 week ago

but i wanted to make sure that no outdated (cached by some proxy)

Just add VersionedResourceHandler.

or truncated resources arrive at the client

Entire combined resource returns 404 if that is the case so just check for response status.

BalusC commented 6 days ago

Closing off as this isn't well thought-out.

SRI has no use on local resources.

Standard h:outputStylesheet/h:outputScript do not support cross origin resources in first place. They only support local resources.

So in order to have SRI we should take a step back and specify/implement support for cross origin resources in some way. In OmniFaces this is supposed to be only the case when your custom ResourceHandler returns an instance of OmniFaces CDNResource. The CombinedResourceHandler was supposed to set the integrity only for that but by mistake it also sets integrity on local resources. This was unintentional.