jakartaee / faces

Jakarta Faces
Other
108 stars 55 forks source link

Address ambiguity of <url-pattern> in faces-config.xml #1973

Open BalusC opened 2 months ago

BalusC commented 2 months ago

https://github.com/eclipse-ee4j/mojarra/issues/5503

The <url-pattern> element is used in faces-config.xml in 2 places:

  1. Resource contracts:
    <contract-mapping>
        <url-pattern>/foo.xhtml</url-pattern>
        <url-pattern>/bar.xhtml</url-pattern>
    </contract-mapping>
  2. Protected views:
    <protected-views>
        <url-pattern>/foo.xhtml</url-pattern>
        <url-pattern>/bar.xhtml</url-pattern>
    </protected-views>

Both have got nothing to do with the <url-pattern> as used in <servlet-mapping> of web.xml as per Servlet spec 12.1. Yet the literal XML element name <url-pattern> is implying that and it's confusing a lot of developers who don't carefully consult the spec/documentation. In reality, they're compared against the Faces view ID (which is always the same irrespective of Faces Servlet mapping used) and they're thus not compared against the Servlet request URI as intuitively expected by those familiar with the Servlet spec.

There's one more difference: the <url-pattern> for resource contracts support wildcards as in * and /subfolder/* (but not *.ext). However the one for protected views doesn't at all support wildcards. This adds yet more confusion. Do note that the * wildcard is even not supported by Servlet spec.

We should probably rename <url-pattern> for clarity even though both features are used very rarely.

Proposal: <view-id-pattern> for those supporting wildcards and <view-id> for those not supporting wildcards.

tandraschko commented 2 months ago

Why not just allow a real URL pattern? Wildcard matching would be great for directories AFAIR its even implemented like this in MF

BalusC commented 2 months ago

Allowing real URL pattern would require more work for implementors and induce yet more ambiguity/confusion for endusers. E.g. when using minimal config, FacesServlet by default already listens on /faces/, .faces, .jsf and .xhtml. If developers need to register protected views on an actual URL pattern rather than view ID, then that would end up to be cumbersome by being forced to register 4 URL patterns for each single view in order to make sure it's really protected upon every possible request URL referencing the specific view.

In other words, if Servlet 12.1 spec were actually mandated by Faces protected views spec, then in order to completely secure /foo.xhtml with default faces servlet config, you would need:

<protected-views>
    <url-pattern>/foo.xhtml</url-pattern>
    <url-pattern>/foo.jsf</url-pattern>
    <url-pattern>/foo.faces</url-pattern>
    <url-pattern>/faces/foo.xhtml</url-pattern>
</protected-views>

while you should be able to get away with

<protected-views>
    <view-id>/foo.xhtml</view-id>
</protected-views>

I however do agree that it could be beneficial if protected views also support wildcards. But only in flavor of "view ID pattern" not "URL pattern".

<protected-views>
    <view-id-pattern>/protected-views/*</view-id-pattern>
</protected-views>

And that would be a new feature request. The real issue here is only that the term "URL pattern" is technically incorect when looking at the spec and therefore needs to be renamed. For protected views the current Faces spec basically says these should be <view-id> and for resource contracts the current Faces spec basically says these should be <view-id-pattern>.

BalusC commented 1 week ago

The alternative would be to adjust the spec on this but this ends up ugly as it would still potentially confuse developers who don't carefully read the spec.