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

Add a resourcePrefix for Middleware.serveResources #2887

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

sullivan-
Copy link
Contributor

As it is, Middleware.serveResources(path) going to serve any file at all under src/main/resources, including e.g. somebody's reference.conf file. I added a resourcePrefix parameter for this, so that the resources served can be limited to, e.g., src/main/resources/public.

I also added delegates for serveResources and serveDirectory in Routes, as per this suggestion:

#2450 (comment)

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2024

CLA assistant check
All committers have signed the CLA.

@sullivan-
Copy link
Contributor Author

@TomTriple

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 64.34%. Comparing base (3860ebd) to head (9976516).

Files Patch % Lines
...tp/shared/src/main/scala/zio/http/Middleware.scala 0.00% 3 Missing ⚠️
...o-http/shared/src/main/scala/zio/http/Routes.scala 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2887      +/-   ##
==========================================
- Coverage   64.39%   64.34%   -0.05%     
==========================================
  Files         156      156              
  Lines        9148     9150       +2     
  Branches     1579     1593      +14     
==========================================
- Hits         5891     5888       -3     
- Misses       3257     3262       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sullivan- sullivan- force-pushed the resource-prefix-for-stream-resources branch from 9976516 to 3ac2749 Compare June 5, 2024 21:22
As it is, `Middleware.serveResources(path)` going to serve any file at all under src/main/resources, including e.g. somebody's reference.conf file.
I added a resourcePrefix parameter for this, so that the resources served can be limited to, e.g.,  src/main/resources/public.

I also added delegates for serveResources and serveDirectory in Routes, as per this suggestion:

zio#2450 (comment)
@sullivan- sullivan- force-pushed the resource-prefix-for-stream-resources branch from fe7b050 to 8342304 Compare June 6, 2024 14:32
@jdegoes jdegoes merged commit d0e5305 into zio:main Jun 6, 2024
33 checks passed
@TomTriple
Copy link
Contributor

TomTriple commented Jun 9, 2024

@sullivan- That addition is needed absolutely 👍 👍 👍 - but to be honest in that case I'd go for the exact same way you posted in discord regarding spring framework and publish under a hard-coded folder under src/main/resources e.g. src/main/resources/assets or src/main/resources/public.

As of now with the default to . I'm afraid it's too easy to just ignore/overlook that new parameter and continue publishing a resource.conf without anyone noticing - I think the same as @erikvanoosten that files directly under the directory src/main/resources/ should actually never be accessible (I think that is was spring is doing also).

Also I doubt a bit that we need to have the internal published directory name configurable from the beginning, probably everyone configures it the same and it sounds to me like a implementation detail that we should hide.

@sullivan-
Copy link
Contributor Author

I'm happy to change it if people want. Maybe it's better to just not supply "." as a default path. Hardcoding the path doesn't seem smart, as you can see from the Spring Boot example I posted in discord - nobody can agree on what the directory name should be, and they end up with three different directories under src/main/resources that get served up statically.

@erikvanoosten
Copy link
Contributor

erikvanoosten commented Jun 9, 2024

@sullivan- I think the principles for serving static assets should be:

  1. The root resource path MUST NOT be accessible. At all. Just no. Don't even think about it.
  2. Any other path can be made available at the user's choice. For example, web-jars mandates /META-INF/resources/webjars/. Who knows what other conventions exist...
  3. Zio-http can decide on a default path to shorten discussions when no existing convention applies.

Re. 1. I would even go so far as to prevent the user from making this available with zio-http library components. For example, by requiring that the resource path prefix must not be ..

Re. 2. It should be possible to serve from multiple resource path prefixes. For example, you may use a WebJar plus something similar that uses another convention.

Re. 3. I really wonder why spring-boot serves from all of these 4 resource paths (/META-INF/resources/, /resources/, /static/ and /public/) by default (okay, the first is for web-jars). It looks extreme to me. IMHO I would go with /public/ only, primarily because it is such a clear indicator that the files can be made public.

@erikvanoosten
Copy link
Contributor

BTW, once this it settled, it would be awesome if zio-http would be listed on https://www.webjars.org/documentation!

@sullivan-
Copy link
Contributor Author

Re. 3. I really wonder why spring-boot serves from all of these 4 resource paths (/META-INF/resources/, /resources/, /static/ and /public/) by default (okay, the first is for web-jars). It looks extreme to me. IMHO I would go with /public/ only, primarily because it is such a clear indicator that the files can be made public.

Multiple paying clients who each have their own ideas about where the assets belong?

@987Nabil
Copy link
Contributor

987Nabil commented Jun 9, 2024

I agree with @erikvanoosten about /public. It is the best in making the intention of the folder unambiguously clear to the user.

@sullivan-
Copy link
Contributor Author

sullivan- commented Jun 10, 2024

How about we take what is now merged and:

  1. Change the default path from "." to "public".
  2. Add a restriction that the path does not have "." as a prefix.

If this sounds agreeable I can make a PR.

One question. We have:

def serveDirectory(path: zio.http.Path, docRoot: java.io.File)

and:

def serveResources(path: zio.http.Path, resourcePrefix: String = ".")

Would it be better if the second method took File instead of String as argument?

@erikvanoosten
Copy link
Contributor

erikvanoosten commented Jun 10, 2024

Change the default path from "." to "public".

I am not sure about how it works exactly, but perhaps this needs to be "/public/"? Or any other way to make sure that it can't serve resources like /public.conf.

Would it be better if the second method took File instead of String as argument?

As far as I know a File can't represent a resource.

@sullivan-
Copy link
Contributor Author

Followup PR: #2928

sullivan- added a commit to sullivan-/zio-http that referenced this pull request Jun 23, 2024
1. Change the default path from "." to "public".
2. Add a restriction that the path does not have "." as a prefix.

as discussed in a prior PR, here:

zio#2887 (comment)
jdegoes pushed a commit that referenced this pull request Jun 23, 2024
* some cleanup on recent work serving files and resources

1. Change the default path from "." to "public".
2. Add a restriction that the path does not have "." as a prefix.

as discussed in a prior PR, here:

#2887 (comment)

* incorporate @erikvanoosten's suggestions

- replace `Handler.forbidden` with `require`
- firming up logic for resource prefix limitations
- some Scaladoc improvements

I also fixed a `resourcePrefix` default value from `"."` to `"public"` that I somehow missed before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants