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 /index.html when uri ending with a word that doesn't contains a dot #226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

s-nt-s
Copy link

@s-nt-s s-nt-s commented Feb 21, 2023

Description of changes:

Whit this change, when RewritePathWithTrailingSlashToIndex=true , uris ending in a word that doesn't contains a dot (".") has the same behavior as a uri ending in a slash ("/).

In others words, http://example.com/path and http://example.com/path/ are rewrite as http://example.com/path/index.html

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

… ending with a word that doesn't contains a dot
@ottokruse
Copy link
Collaborator

PR very much appreciated!

Question, how "right" is this in general? It might be specific to your use case.

Perhaps we should put it under another parameter eg RewritePathWithoutExtensionToIndex

@s-nt-s
Copy link
Author

s-nt-s commented Feb 21, 2023

I think my approach is right because of, in my experience, is quite common get the same result for urls like http://example.com/path and http://example.com/path/ (for example, https://github.com/s-nt-s/ and https://github.com/s-nt-s give same result).

But maybe is better, as you said, use a new parameter like RewritePathWithoutExtensionToIndex. But I don't sure how implement it. Can src/lambda-edge/rewrite-trailing-slash/index.ts get RewritePathWithoutExtensionToIndex and RewritePathWithTrailingSlashToIndex value to choice which rewrite actions it have to be done?

Thanks.

@ottokruse
Copy link
Collaborator

... in my experience, is quite common get the same result ...

Let's get some data points. Just asked a colleague's opinion and he thinks otherwise.

How do eg other static site hosters do this (Apache, Next.js, etc) ?

@s-nt-s
Copy link
Author

s-nt-s commented Feb 21, 2023

How do eg other static site hosters do this (Apache, Next.js, etc) ?

I check apache and nginx using docker:

$ cat apache.dockerfile
FROM httpd:2.4
RUN mkdir -p /usr/local/apache2/htdocs/path/
RUN echo "hello" > /usr/local/apache2/htdocs/path/index.html

$ docker build -f apache.dockerfile --tag apache-example .
$ docker run --publish 10080:80 --detach --name apache-example-10080 apache-example
$ curl -L http://localhost:10080/path/
hello
$ curl -Ls -o /dev/null -w "%{url_effective}\n" http://localhost:10080/path
http://localhost:10080/path/

$ cat nginx.dockerfile 
FROM nginx:1.23.3
RUN mkdir -p /usr/share/nginx/html/path/
RUN echo "hello" > /usr/share/nginx/html/path/index.html

$ docker run --publish 10081:80 --detach --name nginx-example-10081 nginx-example
$ curl -L http://localhost:10081/path/
hello
$ curl -Ls -o /dev/null -w "%{url_effective}\n" http://localhost:10081/path
http://localhost/path/

So by default Apache and nginx redirect .../path to .../path/, in others words, you get same page for .../path and .../path/.

Other examples/server:

# GitHub.com redirect `.../path` to `.../path/`
$ curl -sI https://s-nt-s.github.io | grep -i server:
server: GitHub.com
$ curl -Ls -o /dev/null -w "%{url_effective}\n" https://s-nt-s.github.io/cloudfront_letsencrypt_freenom
https://s-nt-s.github.io/cloudfront_letsencrypt_freenom/

# Vercel redirect `.../path/` to `.../path`
$ curl -sI https://nextjs.org/docs/getting-started | grep -i server:
server: Vercel
$ curl -Ls -o /dev/null -w "%{url_effective}\n" https://nextjs.org/docs/
https://nextjs.org/docs

# Amazon Server redirect `.../path` to `.../path/`
$ curl -sI https://aws.amazon.com/es/contact-us/ | grep -i server:
server: Server
$ curl -Ls -o /dev/null -w "%{url_effective}\n" https://aws.amazon.com/es/contact-us
https://aws.amazon.com/es/contact-us/

So also serve same page for .../path and .../path/

@ottokruse
Copy link
Collaborator

Thanks for the research. That's pretty convincing.

Still a doubt, what if you want to host a file without an extension. Let's say, as example, some Dockerfile, such as this one: https://github.com/nginxinc/docker-nginx/blob/5ce65c3efd395ee2d82d32670f233140e92dba99/mainline/alpine/Dockerfile

Guess you could upload that to S3 as <prefix>/Dockerfile/index.html then and it would work (not very intuitive though is it?)

Can we invent a solution that works in all cases?

I'm playing with the thought of returning a 307 redirect to <requested key>/index.html in an origin response lambda, if S3 returns a 404 on an object without an extension.

@ottokruse
Copy link
Collaborator

Tweak to that thought: return a 307 redirect to <requested key>/ (i.e. append trailing slash) in an origin response lambda, if S3 returns a 404 on an object without an extension (and that does not end with a slash).

What do you think?

@ottokruse
Copy link
Collaborator

Hi @s-nt-s , share your thoughts?

@s-nt-s
Copy link
Author

s-nt-s commented Mar 15, 2023

Hi.

Maybe I am wrong and this is not a big deal, but If Lambda@Edge have to check if the object exists or no in order to decide if return 307 or 404, it need permissions to read in s3 and spend a lite more time for do its job. (I don't know if a Lambda@Edge inherits permissions from its cloudfront so its have read permission over s3 origin transparently or no)

I think that in most of case is not worthy. As you point in your first comment, probably is better add a RewritePathWithoutExtensionToIndex and let the developer configure as he wants.

@ottokruse
Copy link
Collaborator

The Lambda@Edge origin response would see the response code 404 come back. That is part of the event payload, i.e. that function doesn't actually need to go to S3. It can be a synchronous function even.

@vincentclee
Copy link

@s-nt-s Thank You for this. I needed something similar.

Here is my fork with your changes rebased
https://github.com/vincentclee/cloudfront-authorization-at-edge/pull/1

@jantmanAtCox
Copy link

@ottokruse This is useful for us as well. What can we do to help get this merged?

@ottokruse
Copy link
Collaborator

Sounds like the PR as is, is what you folks would all be happy with? When can go ahead with it.

Will merge and release the new version to SAR this week somewhere when I have some spare time.

@ottokruse
Copy link
Collaborator

Just to double check. Everyone here is fine with that blocking the possibility of hosting files without extension? I still feel uneasy about it

@jantmanAtCox
Copy link

Honestly I would prefer if this were hidden behind a boolean flag so that it could be enabled but would default to the current behavior. Unfortunately, (1) I have zero Node experience, and (2) I'm not even using 90% of this repo, I'm just using the Lambda code inside infrastructure managed via terraform.

@tripplilley
Copy link

Just to double check. Everyone here is fine with that blocking the possibility of hosting files without extension? I still feel uneasy about it

I've just stumbled on this PR while updating my fork. I have some work to do reviewing the PR and assembling references to the relevant standards, but I am generally against this being applied without deeper consideration. The index.html convention is exactly that -- a convention. I'll return with a more considered response when I've finished my homework, but I just wanted to lodge the objection in case I get sidetracked 🙂

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.

5 participants