Tapestry 5
  1. Tapestry 5
  2. TAP5-815

Asset dispatcher allows any file inside the webapp visible and downloadable

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Invalid
    • Affects Version/s: 5.1.0.6
    • Fix Version/s: 5.2.0, 5.0.19
    • Component/s: None
    • Labels:
      None

      Description

      Take any asset and you have an URL like domain.com/assets/ctx/f10407a6c1753e39/css/main.css. If you request domain.com/assets/ctx/f10407a6c1753e39/, a list containing all the files inside the webapp root is shown. It gives you the hint at downloading any file you want, including anyting inside WEB-INF and assets that should be protected by ResourceDigestGenerator.

        Activity

        Hide
        Thiago H. de Paula Figueiredo added a comment -

        The problem seems to be in this snippet at AssetResourceLocatorImpl:

        if (path.startsWith(applicationAssetPrefix))
        return findContextResource(path.substring(applicationAssetPrefix.length()));

        String resourcePath = aliasManager.toResourcePath(path);

        Resource resource = new ClasspathResource(resourcePath);

        if (!resourceCache.requiresDigest(resource)) return resource;

        The digest is never taken when the resource path starts with the application asset prefix, which is exatcly the path prefix used in this bug.

        Show
        Thiago H. de Paula Figueiredo added a comment - The problem seems to be in this snippet at AssetResourceLocatorImpl: if (path.startsWith(applicationAssetPrefix)) return findContextResource(path.substring(applicationAssetPrefix.length())); String resourcePath = aliasManager.toResourcePath(path); Resource resource = new ClasspathResource(resourcePath); if (!resourceCache.requiresDigest(resource)) return resource; The digest is never taken when the resource path starts with the application asset prefix, which is exatcly the path prefix used in this bug.
        Hide
        Ulrich Stärk added a comment -

        In 5.2-SNAPSHOT you can still access files located on the classpath or in the webapp context, except for .class und .tml files in the classpath (due to ResourceDigestGenerator). .tml files in the context are still accessible. There is no directory listing though. So this also partly applies to the current development tree.

        The problem here is that Tapestry is using a blacklisting approach: It allows all access unless otherwise specified, for example by contributing to the ResourceDigestGenerator. This principle is unsecure by design. Instead Tapestry should do whitelisting, i.e. only allow access to explicitly allowed resources. Since Tapestry already knows about all the Assets required by a page or component (by looking at the @Path, @IncludeJavaScriptLibrary and @IncludeStylesheet annotations and the context: and asset: binding prefixes), such a whitelisting approach could be realized: Just allow access to Assets really required by pages or components.

        Show
        Ulrich Stärk added a comment - In 5.2-SNAPSHOT you can still access files located on the classpath or in the webapp context, except for .class und .tml files in the classpath (due to ResourceDigestGenerator). .tml files in the context are still accessible. There is no directory listing though. So this also partly applies to the current development tree. The problem here is that Tapestry is using a blacklisting approach: It allows all access unless otherwise specified, for example by contributing to the ResourceDigestGenerator. This principle is unsecure by design. Instead Tapestry should do whitelisting, i.e. only allow access to explicitly allowed resources. Since Tapestry already knows about all the Assets required by a page or component (by looking at the @Path, @IncludeJavaScriptLibrary and @IncludeStylesheet annotations and the context: and asset: binding prefixes), such a whitelisting approach could be realized: Just allow access to Assets really required by pages or components.
        Hide
        Thiago H. de Paula Figueiredo added a comment -

        I agree with Ulrich that a whitelist approach is probably the best one, but allowing onle access to assets used in pages is too restrictive IMHO. It would make working with anything that isn't a Tapestry page a hassle. I would suggest to have a chain of command, each object in it receiving the requested URL and responding true (ok), false (file is forbidden) or null (this object doesn't handle this URL, ask the same thing to the next object. This chain of command terminator would be a very restrictive one.

        Show
        Thiago H. de Paula Figueiredo added a comment - I agree with Ulrich that a whitelist approach is probably the best one, but allowing onle access to assets used in pages is too restrictive IMHO. It would make working with anything that isn't a Tapestry page a hassle. I would suggest to have a chain of command, each object in it receiving the requested URL and responding true (ok), false (file is forbidden) or null (this object doesn't handle this URL, ask the same thing to the next object. This chain of command terminator would be a very restrictive one.
        Hide
        Ulrich Stärk added a comment -

        Accessing a Tapestry-managed asset from a non-Tapestry source like a static html file should be avoided IMO. Such assets should be stored in the webapp context and can than be handled by the container and not by Tapestry.

        Show
        Ulrich Stärk added a comment - Accessing a Tapestry-managed asset from a non-Tapestry source like a static html file should be avoided IMO. Such assets should be stored in the webapp context and can than be handled by the container and not by Tapestry.
        Hide
        Thiago H. de Paula Figueiredo added a comment -

        I agree with Ulrich. The Tapestry asset handling should only be used by Tapestry components, pages and mixins.

        Show
        Thiago H. de Paula Figueiredo added a comment - I agree with Ulrich. The Tapestry asset handling should only be used by Tapestry components, pages and mixins.
        Hide
        Robert Zeigler added a comment -

        Ulrich, "Just allow access to Assets really required by pages or components" is easier said than done. The assets required by a page are not known until the first time a page is requested and the correspond page model is built. Which means that it's difficult, at best, for an IOC module to access this information at service instantiation time; it will be instantiated when the first request comes in, /before/ the corresponding page is even loaded (due to dispatcher ordering), and that's on the first request, for a single page, before any other pages are loaded. Any sort of asset authorization service that wanted to auto-enable required assets would need to have some sort of "addVisibleResource" method that is called whenever an asset is encountered/created during render. I would advocate instead a whitelist approach where allowed files/file patterns are contributed via ioc contributions. This would simplify things significantly.

        As for assets used only by components, pages, and mixins, that's also a bit tricky, since it's possible for someone to write an alternative asset source that's used, eg, for file downloads (ie, not necessarily directly referenced by a page/component/mixin).

        Incidentally, a long while ago, I implemented and made available for public use an "AssetProtectionDispatcher" that is configured essentially via chain of command as specified by Thiago above, with slight variation (a bit more flexible; individual contributions specify whether they explicitly allow or deny access). The module further provides two "AssetPathAuthorizer" implementations: one for explicit whitelisting by resource name, and the other for whitelisting by url pattern, with the whitelist being the last in the chain of command. The module contributes a default set of values to the whitelist (everything used by tapestry's core components), but you'll need to add explicit access to other resources (eg: contributing a .*\.jpg to the RegexAuthorizer).

        Maven repo:
        http://maven.saiwai-solutions.com
        groupid: com.saiwaisolutions
        artifactid: AssetProtectionDispatcher
        version: 1.0.0

        Alternatively, an older version is available on Tassel:
        http://saiwai-solutions.com/Tassel/app?service=external/ViewComponent&sp=SAssetProtectionDispatcher

        Version 1.0.0 also adds some default configurations to handle chenillekit-based assets.
        Cheers!

        Show
        Robert Zeigler added a comment - Ulrich, "Just allow access to Assets really required by pages or components" is easier said than done. The assets required by a page are not known until the first time a page is requested and the correspond page model is built. Which means that it's difficult, at best, for an IOC module to access this information at service instantiation time; it will be instantiated when the first request comes in, /before/ the corresponding page is even loaded (due to dispatcher ordering), and that's on the first request, for a single page, before any other pages are loaded. Any sort of asset authorization service that wanted to auto-enable required assets would need to have some sort of "addVisibleResource" method that is called whenever an asset is encountered/created during render. I would advocate instead a whitelist approach where allowed files/file patterns are contributed via ioc contributions. This would simplify things significantly. As for assets used only by components, pages, and mixins, that's also a bit tricky, since it's possible for someone to write an alternative asset source that's used, eg, for file downloads (ie, not necessarily directly referenced by a page/component/mixin). Incidentally, a long while ago, I implemented and made available for public use an "AssetProtectionDispatcher" that is configured essentially via chain of command as specified by Thiago above, with slight variation (a bit more flexible; individual contributions specify whether they explicitly allow or deny access). The module further provides two "AssetPathAuthorizer" implementations: one for explicit whitelisting by resource name, and the other for whitelisting by url pattern, with the whitelist being the last in the chain of command. The module contributes a default set of values to the whitelist (everything used by tapestry's core components), but you'll need to add explicit access to other resources (eg: contributing a .*\.jpg to the RegexAuthorizer). Maven repo: http://maven.saiwai-solutions.com groupid: com.saiwaisolutions artifactid: AssetProtectionDispatcher version: 1.0.0 Alternatively, an older version is available on Tassel: http://saiwai-solutions.com/Tassel/app?service=external/ViewComponent&sp=SAssetProtectionDispatcher Version 1.0.0 also adds some default configurations to handle chenillekit-based assets. Cheers!
        Hide
        Ulrich Stärk added a comment -

        I had some singleton service holding a collection of allowed assets in mind. This would be injected into AssetSource and queried whether access should be allowed. Allowde Assets get added from AssetObjectProvider, AssetInjectionProvider, IncludeJavaScriptLibraryWorker, IncludeStylesheetWorker, ContextBindingFactory and AssetBindingFactory.
        If people choose to override the default AssetSource they have to live with being responsible for taking care of security. We could btw. also do the checks in the corresponding AssetFactories.

        Uli

        Show
        Ulrich Stärk added a comment - I had some singleton service holding a collection of allowed assets in mind. This would be injected into AssetSource and queried whether access should be allowed. Allowde Assets get added from AssetObjectProvider, AssetInjectionProvider, IncludeJavaScriptLibraryWorker, IncludeStylesheetWorker, ContextBindingFactory and AssetBindingFactory. If people choose to override the default AssetSource they have to live with being responsible for taking care of security. We could btw. also do the checks in the corresponding AssetFactories. Uli
        Hide
        Christian Köberl added a comment -

        At least the directory index should be fixed quickly. This is a massive security issue for all Tapestry applications.
        e.g. http://tapestry-test.appspot.com/assets/

        A simple check in AssetDispatcher like this would help:
        if(path.endsWith("/") || path.indexOf('.') < 0)
        return false;

        Show
        Christian Köberl added a comment - At least the directory index should be fixed quickly. This is a massive security issue for all Tapestry applications. e.g. http://tapestry-test.appspot.com/assets/ A simple check in AssetDispatcher like this would help: if(path.endsWith("/") || path.indexOf('.') < 0) return false;
        Hide
        David Rees added a comment -

        This similarly 5.0.18 similarly as well - and the issue has been open for quite some time. Is 5.0 maintained at all for security related issues? What about 5.1?

        Show
        David Rees added a comment - This similarly 5.0.18 similarly as well - and the issue has been open for quite some time. Is 5.0 maintained at all for security related issues? What about 5.1?
        Hide
        Alex Kotchnev added a comment -

        I'm totally blown away by the lack of interest this issue has received. In my opinion, this is the type of issue that FORCES a point release, it is that severe and important. There are several existing solutions that can easily be plugged into the framework, yet no action.

        To my dismay, this has been open since Aug, and the issue has been known for 5.0 for a lot longer than that.

        Show
        Alex Kotchnev added a comment - I'm totally blown away by the lack of interest this issue has received. In my opinion, this is the type of issue that FORCES a point release, it is that severe and important. There are several existing solutions that can easily be plugged into the framework, yet no action. To my dismay, this has been open since Aug, and the issue has been known for 5.0 for a lot longer than that.
        Hide
        Christian Riedel added a comment -

        well it's just half as popular as TAP-138
        i think if anyone contributes a framework-ready solution, some committer will do a security release.

        Show
        Christian Riedel added a comment - well it's just half as popular as TAP-138 i think if anyone contributes a framework-ready solution, some committer will do a security release.
        Hide
        Igor Drobiazko added a comment -

        Can we live with Robert's solution? Please comment.

        Show
        Igor Drobiazko added a comment - Can we live with Robert's solution? Please comment.
        Hide
        Ulrich Stärk added a comment -

        I guess we could, but having another mechanism for securing assets (in addition to ResourceDigestGenerator) adds another layer of complexity. So if we integrated that into Tapestry - and we definitely have to integrate something as important as this - we should get rid of ResourceDigestGenerator.
        Alternatively we could use ResourceDigestGenerator to also secure context assets and integrate Christians code snippet to prevent direcotry listings, but I prefer Roberts solution since it is much more flexible.
        His code is missing any licensing terms though, so I don't know whether we can just integrate it.

        Uli

        Show
        Ulrich Stärk added a comment - I guess we could, but having another mechanism for securing assets (in addition to ResourceDigestGenerator) adds another layer of complexity. So if we integrated that into Tapestry - and we definitely have to integrate something as important as this - we should get rid of ResourceDigestGenerator. Alternatively we could use ResourceDigestGenerator to also secure context assets and integrate Christians code snippet to prevent direcotry listings, but I prefer Roberts solution since it is much more flexible. His code is missing any licensing terms though, so I don't know whether we can just integrate it. Uli
        Hide
        Thiago H. de Paula Figueiredo added a comment -

        I don't think Robert's solution is enough, as attackers can still guess some files location (hibernate.cfg.xml and web.xml, for example) even without the directory listing. I think a viable solution would be the one I proposed in the first commet.

        Show
        Thiago H. de Paula Figueiredo added a comment - I don't think Robert's solution is enough, as attackers can still guess some files location (hibernate.cfg.xml and web.xml, for example) even without the directory listing. I think a viable solution would be the one I proposed in the first commet.
        Hide
        Igor Drobiazko added a comment -

        So you prefer the ResourceDigestGenerator solution described by Ulrich? What about your concerns in TAP5-896? No more concerns?

        As Ulrich suggested creating a digest for context asstes seems to be ok.

        Show
        Igor Drobiazko added a comment - So you prefer the ResourceDigestGenerator solution described by Ulrich? What about your concerns in TAP5-896 ? No more concerns? As Ulrich suggested creating a digest for context asstes seems to be ok.
        Hide
        Christian Riedel added a comment - - edited

        I don't think Robert's solution would make anything available because it's whitelist-based. Or did I miss something?

        Show
        Christian Riedel added a comment - - edited I don't think Robert's solution would make anything available because it's whitelist-based. Or did I miss something?
        Hide
        Ulrich Stärk added a comment -

        No you didn't. Robert's solution includes a whitelist approach. It should protect anything that's not explicitly allowed, including xml files such as those mentioned by Thiago.

        Show
        Ulrich Stärk added a comment - No you didn't. Robert's solution includes a whitelist approach. It should protect anything that's not explicitly allowed, including xml files such as those mentioned by Thiago.
        Hide
        Thiago H. de Paula Figueiredo added a comment -

        I'm sorry. I was talking about Christian Köberl, not Robert's one. In fact, I completely agree with Robert's approach. That's exactly what I would do. It's the approach I use in my Tapestry Access Logger package to define what URL's are logged are what aren't.

        Show
        Thiago H. de Paula Figueiredo added a comment - I'm sorry. I was talking about Christian Köberl, not Robert's one. In fact, I completely agree with Robert's approach. That's exactly what I would do. It's the approach I use in my Tapestry Access Logger package to define what URL's are logged are what aren't.
        Hide
        Alex Kotchnev added a comment -

        It seems to me that integrating Robert's AssetProtectionDispatcher as the default in the framework, together w/ an entry in the main docs explaining the types of assets that are "open" by default (e.g. maybe opening up *.css, *.js, *.jpg) would do it for most uses. There certainly seem to be more "advanced" solutions with the framework automatically whitelisting anything referenced as an Asset, but it seems the complexity would slow down releasing something ASAP.

        Show
        Alex Kotchnev added a comment - It seems to me that integrating Robert's AssetProtectionDispatcher as the default in the framework, together w/ an entry in the main docs explaining the types of assets that are "open" by default (e.g. maybe opening up *.css, *.js, *.jpg) would do it for most uses. There certainly seem to be more "advanced" solutions with the framework automatically whitelisting anything referenced as an Asset, but it seems the complexity would slow down releasing something ASAP.
        Hide
        Robert Zeigler added a comment - - edited

        If the consensus is to incorporate the AssetProtectionDispatcher + whitelist contributions from the module I wrote, then I'm happy to donate that code to the tapestry project. If I don't hear any nays before then, I'll integrate it this weekend.

        Show
        Robert Zeigler added a comment - - edited If the consensus is to incorporate the AssetProtectionDispatcher + whitelist contributions from the module I wrote, then I'm happy to donate that code to the tapestry project. If I don't hear any nays before then, I'll integrate it this weekend.
        Hide
        David Rees added a comment -

        FWIW, Robert's plugin appears to be working for us so far in our T5.1.0.5 application, and agree with it's approach.

        But what about legacy T5.0.18 apps? We don't always have the ability of performing major updates to those apps (to upgrade to T5.1) in a timely manner...

        Show
        David Rees added a comment - FWIW, Robert's plugin appears to be working for us so far in our T5.1.0.5 application, and agree with it's approach. But what about legacy T5.0.18 apps? We don't always have the ability of performing major updates to those apps (to upgrade to T5.1) in a timely manner...
        Hide
        Robert Zeigler added a comment -

        My thought was to backport the change into the 5.0.x branch and add a security-update release.

        Show
        Robert Zeigler added a comment - My thought was to backport the change into the 5.0.x branch and add a security-update release.
        Hide
        Alex Kotchnev added a comment -

        If I recall correctly, the AssetDispatcher worked fine in 5.0.x (excluding a blackbird issue that I think was fixed in Asset Dispatcher 1.0).

        Show
        Alex Kotchnev added a comment - If I recall correctly, the AssetDispatcher worked fine in 5.0.x (excluding a blackbird issue that I think was fixed in Asset Dispatcher 1.0).
        Hide
        David Rees added a comment -

        @Robert - Thanks that sounds great!

        @Alex - Not sure what you mean - I just tested one of my 5.0.18 apps yesterday and I was able to download assets without issue.

        Show
        David Rees added a comment - @Robert - Thanks that sounds great! @Alex - Not sure what you mean - I just tested one of my 5.0.18 apps yesterday and I was able to download assets without issue.
        Hide
        Christian Riedel added a comment -

        @Alex: the issue definitely affects 5.0.18, too! I pointed this out in a discussion on the mailing list. Just have a look into the second post in the thread:

        http://old.nabble.com/Running-Tapestry-5.0.18-on-Google-App-Engine-ts25133064s302.html

        Show
        Christian Riedel added a comment - @Alex: the issue definitely affects 5.0.18, too! I pointed this out in a discussion on the mailing list. Just have a look into the second post in the thread: http://old.nabble.com/Running-Tapestry-5.0.18-on-Google-App-Engine-ts25133064s302.html
        Hide
        Robert Zeigler added a comment -

        Pretty sure Alex was referring to the functionality of my AssetProtectionDispatcher withi 5.0.18.

        Show
        Robert Zeigler added a comment - Pretty sure Alex was referring to the functionality of my AssetProtectionDispatcher withi 5.0.18.
        Hide
        Christian Riedel added a comment -

        oh, right. makes more sense...

        Show
        Christian Riedel added a comment - oh, right. makes more sense...
        Hide
        Christian Köberl added a comment -

        I just "hacked" our application and found even more concerning thing - if you add a "/" to your URL you can even look at assets extensions contributed to ResourceDigestGenerator (default: tml, class).

        We use this mechanism to hide assets like xml, properties, ...

        This requires a digest:
        http://tapestry-test.appspot.com/assets/at/priv/koeberl/tapestrymail/pages/BadTemplate.tml

        This works:
        http://tapestry-test.appspot.com/assets/at/priv/koeberl/tapestrymail/pages/BadTemplate.tml/
        http://tapestry-test.appspot.com/assets/at/priv/koeberl/tapestrymail/pages/BadTemplate.class/

        Could any committer please add this to AssetDispatcher line 65 (in 5.1.0.5):
        if(path.endsWith("/") || path.indexOf('.') < 0)
        return false;

        And then create 5.0.19 and 5.1.0.6.

        I know this will not fix everything but at least not all Tapestry apps out there are open as a book.

        Thanks,
        Chris

        Show
        Christian Köberl added a comment - I just "hacked" our application and found even more concerning thing - if you add a "/" to your URL you can even look at assets extensions contributed to ResourceDigestGenerator (default: tml, class). We use this mechanism to hide assets like xml, properties, ... This requires a digest: http://tapestry-test.appspot.com/assets/at/priv/koeberl/tapestrymail/pages/BadTemplate.tml This works: http://tapestry-test.appspot.com/assets/at/priv/koeberl/tapestrymail/pages/BadTemplate.tml/ http://tapestry-test.appspot.com/assets/at/priv/koeberl/tapestrymail/pages/BadTemplate.class/ Could any committer please add this to AssetDispatcher line 65 (in 5.1.0.5): if(path.endsWith("/") || path.indexOf('.') < 0) return false; And then create 5.0.19 and 5.1.0.6. I know this will not fix everything but at least not all Tapestry apps out there are open as a book. Thanks, Chris
        Hide
        Robert Zeigler added a comment -

        Hey Chris,

        I just committed the AssetProtectionDispatcher stuff (to 5.0 and 5.1 branches and to trunk). That should solve your issue, but if you want to double check that, it would be great.
        Leaving this issue open for the time being to give people a chance to review. I'll close it tonight or tomorrow if I don't hear anything more.

        Show
        Robert Zeigler added a comment - Hey Chris, I just committed the AssetProtectionDispatcher stuff (to 5.0 and 5.1 branches and to trunk). That should solve your issue, but if you want to double check that, it would be great. Leaving this issue open for the time being to give people a chance to review. I'll close it tonight or tomorrow if I don't hear anything more.
        Hide
        Jan le Roux added a comment -

        I can confirm that it works with 5.0.19-SNAPSHOT (svn version 834439, https://svn.apache.org/repos/asf/tapestry/tapestry5/branches/5.0).

        Thanks!

        Show
        Jan le Roux added a comment - I can confirm that it works with 5.0.19-SNAPSHOT (svn version 834439, https://svn.apache.org/repos/asf/tapestry/tapestry5/branches/5.0 ). Thanks!
        Hide
        Ulrich Stärk added a comment -

        Don't close it yet. I just created a project from the archetype and somehow the css file containing the layout can't be accessed although css is added to the whitelist by default. I'm investigating.

        Show
        Ulrich Stärk added a comment - Don't close it yet. I just created a project from the archetype and somehow the css file containing the layout can't be accessed although css is added to the whitelist by default. I'm investigating.
        Hide
        Ulrich Stärk added a comment - - edited

        There should be an additional contribution to the RegexAuthorizer service:

        regex.add(RequestConstants.CONTEXT_FOLDER + appVersion + "/" + pathPattern);

        with @Symbol(SymbolConstants.APPLICATION_VERSION) String appVersion

        Otherwise access to css,js,jpg and all the other stuff coming from the individual application is denied by default. I know the docs say exactly so but I think allowing some standard stuff from out of the box is OK. People might just spend too much time figuring out why some standard things like css files, javascripts and pictures are blocked.

        Uli

        Show
        Ulrich Stärk added a comment - - edited There should be an additional contribution to the RegexAuthorizer service: regex.add(RequestConstants.CONTEXT_FOLDER + appVersion + "/" + pathPattern); with @Symbol(SymbolConstants.APPLICATION_VERSION) String appVersion Otherwise access to css,js,jpg and all the other stuff coming from the individual application is denied by default. I know the docs say exactly so but I think allowing some standard stuff from out of the box is OK. People might just spend too much time figuring out why some standard things like css files, javascripts and pictures are blocked. Uli
        Hide
        Geoff Callender added a comment -

        Hey Robert,

        I haven't had a chance to review the AssetProtectionDispatcher, but can you confirm its default setup matches the following bits of the servlet spec? I think the servlet spec describes the behaviour that developers would reasonably expect, regardless of the fact that T5 doesn't use servlets.

        1. ALWAYS deny clients access to WEB-INF:

        "any requests from the client to access the resources in WEB-INF/ directory must be returned with a SC_NOT_FOUND(404) response." (Servlet Spec 2.4 section 9.5)

        2. ALWAYS deny clients access to META-INF:

        "any requests to access the resources in META-INF directory must be returned with a SC_NOT_FOUND(404) response." (Servlet spec 2.4 section 9.6)

        3. By default, allow access to static resources:

        "Web containers are required to support access to web resources by clients that have not authenticated themselves to the container. This is the common mode of access to web resources on the Internet." (Servlet Spec 2.4 section 12.7)

        If resources such as .tml files need to be hidden then either move them into WEB-INF/classes (which I'd argue is where they belong anyway as they are a non-configurable part of the app) or blacklist them.

        As for displaying index pages as the client traverses the resources, I think we're all agreed it's wrong.

        Geoff

        Show
        Geoff Callender added a comment - Hey Robert, I haven't had a chance to review the AssetProtectionDispatcher, but can you confirm its default setup matches the following bits of the servlet spec? I think the servlet spec describes the behaviour that developers would reasonably expect, regardless of the fact that T5 doesn't use servlets. 1. ALWAYS deny clients access to WEB-INF: "any requests from the client to access the resources in WEB-INF/ directory must be returned with a SC_NOT_FOUND(404) response." (Servlet Spec 2.4 section 9.5) 2. ALWAYS deny clients access to META-INF: "any requests to access the resources in META-INF directory must be returned with a SC_NOT_FOUND(404) response." (Servlet spec 2.4 section 9.6) 3. By default, allow access to static resources: "Web containers are required to support access to web resources by clients that have not authenticated themselves to the container. This is the common mode of access to web resources on the Internet." (Servlet Spec 2.4 section 12.7) If resources such as .tml files need to be hidden then either move them into WEB-INF/classes (which I'd argue is where they belong anyway as they are a non-configurable part of the app) or blacklist them. As for displaying index pages as the client traverses the resources, I think we're all agreed it's wrong. Geoff
        Hide
        Robert Zeigler added a comment -

        Tricky devil.

        There are still some issues in the 5.1 branch and trunk related to context asset handling and protecting vs. opening the correct set of assets. Reopening for now.

        Show
        Robert Zeigler added a comment - Tricky devil. There are still some issues in the 5.1 branch and trunk related to context asset handling and protecting vs. opening the correct set of assets. Reopening for now.
        Hide
        Robert Zeigler added a comment -

        Geoff:

        1) Yes.
        2) yes.
        3) yes.

        Show
        Robert Zeigler added a comment - Geoff: 1) Yes. 2) yes. 3) yes.
        Hide
        Robert Zeigler added a comment -

        :Fixed for real this time. META-INF and WEB-INF protected, .tml files protected, other context assets available, non-whitelisted classpath assets protected,

        Show
        Robert Zeigler added a comment - :Fixed for real this time. META-INF and WEB-INF protected, .tml files protected, other context assets available, non-whitelisted classpath assets protected,
        Hide
        Geoff Callender added a comment -

        Good stuff, Robert.

        Show
        Geoff Callender added a comment - Good stuff, Robert.
        Hide
        Ulrich Stärk added a comment -

        It seems we still don't got it right 100%. In order for common context assets like images, css and js to be available, the user has to set SymbolConstants.CONTEXT_ASSETS_AVAILABLE to true OR contribute to the regex authorizer. Since everyone will want common assets to be available and set it to true (because it's the easiest thing to do), this is useless and just represents an additional burden to the user.

        Show
        Ulrich Stärk added a comment - It seems we still don't got it right 100%. In order for common context assets like images, css and js to be available, the user has to set SymbolConstants.CONTEXT_ASSETS_AVAILABLE to true OR contribute to the regex authorizer. Since everyone will want common assets to be available and set it to true (because it's the easiest thing to do), this is useless and just represents an additional burden to the user.
        Hide
        Robert Zeigler added a comment -

        Not quite true, on three accounts,
        1) Users don't have to make any contributions... at least, they shouldn't. See TapestryModule line 2398 in trunk (2091 in 5.1 branch). We contribute the symbol as "true" to factory defaults.
        2) It's not true that "everyone" will want common assets to be available.
        3) It's no longer just "common" context assets that are available by default. In the /context/, the availability should be following the servlet spec, as per Geoff Callender's 03/Dec/09 comment.

        Cheers.

        Show
        Robert Zeigler added a comment - Not quite true, on three accounts, 1) Users don't have to make any contributions... at least, they shouldn't. See TapestryModule line 2398 in trunk (2091 in 5.1 branch). We contribute the symbol as "true" to factory defaults. 2) It's not true that "everyone" will want common assets to be available. 3) It's no longer just "common" context assets that are available by default. In the /context/, the availability should be following the servlet spec, as per Geoff Callender's 03/Dec/09 comment. Cheers.
        Hide
        Ulrich Stärk added a comment -

        To 1): You are right. That stupid m2eclipse was using a snapshot from december instead of updating it to the most recent and I had workspace resolution turned off.
        To 2): But most will.
        To 3): Acknowledged. That's a good thing.

        Show
        Ulrich Stärk added a comment - To 1): You are right. That stupid m2eclipse was using a snapshot from december instead of updating it to the most recent and I had workspace resolution turned off. To 2): But most will. To 3): Acknowledged. That's a good thing.
        Hide
        Jochen Kemnade added a comment -

        In the current solution (as of 5.1.0.7) there is still a problem with two of the datefield component's image assets, whose basenames contain periods. I created a new issue for that (TAP5-989).

        Show
        Jochen Kemnade added a comment - In the current solution (as of 5.1.0.7) there is still a problem with two of the datefield component's image assets, whose basenames contain periods. I created a new issue for that ( TAP5-989 ).
        Hide
        Ulrich Stärk added a comment -

        This is still open in 5.1.0.6 which we haven't released. If nobody objects, I will backport the 5.2 solution (see SVN rev 935563) to 5.1.0.6 and try a new release 5.1.0.7.

        Uli

        Show
        Ulrich Stärk added a comment - This is still open in 5.1.0.6 which we haven't released. If nobody objects, I will backport the 5.2 solution (see SVN rev 935563) to 5.1.0.6 and try a new release 5.1.0.7. Uli
        Hide
        David Rees added a comment -

        No objections here, but shouldn't it be a 5.1.0.8 or 5.1.0.9 since the current latest snapshot is 5.1.0.8-SNAPSHOT?

        Show
        David Rees added a comment - No objections here, but shouldn't it be a 5.1.0.8 or 5.1.0.9 since the current latest snapshot is 5.1.0.8-SNAPSHOT?
        Hide
        Massimo Lusetti added a comment -

        The current 5.3 impl of this is correct about resource protection

        If the resource is a dir from the context is served if it's not under WEB-INF, it it's from classpath a NPE is thrown.

        Maybe a NPE is not that nice but now it serve the purpose.

        Show
        Massimo Lusetti added a comment - The current 5.3 impl of this is correct about resource protection If the resource is a dir from the context is served if it's not under WEB-INF, it it's from classpath a NPE is thrown. Maybe a NPE is not that nice but now it serve the purpose.

          People

          • Assignee:
            Massimo Lusetti
            Reporter:
            Thiago H. de Paula Figueiredo
          • Votes:
            31 Vote for this issue
            Watchers:
            24 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development