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

don't redirect (with `/` added) in core; do it in the `file.dir` handler #820

Merged
merged 7 commits into from Mar 2, 2016

Conversation

Projects
None yet
3 participants
@kazuho
Member

kazuho commented Feb 29, 2016

Still WIP.

When H2O receives a request against a path configured with paths directive (or by calling h2o_config_register_path) without a trailing slash, the server sends a 301 that redirects the client to a path with / added.

We have seen a few people argue against this behavior (see #711).

The behavior is also a headache; with the current design it is impossible to create a directive for per-file mapping (instead of the existing per-directory mapping) of statically-served files, which is useful for setting specific paths to certain URI paths (e.g. robots.txt, favicon.ico).

paths:
  /favicon.ico:
    file.file: /path/to/favicon.ico

Considering the facts, this PR tries to move the 301 redirect from core to the handler of the file.dir configuration directive, thereby resolving the described two problems.

The downside of the PR is that this would be a breaking change for the standalone server - we would need to update the version number to 2.0 as well as checking that all the built-in handlers correctly handle the requests without / at the end.
For libh2o users, we could simply add an extra argument to h2o_config_register_path that lets users specify which behavior is desirable.

@kazuho kazuho changed the title from do add trailing slash and redirect in core; do it in the `file.dir` handler to don't add trailing slash and redirect in core; do it in the `file.dir` handler Mar 1, 2016

@kazuho kazuho changed the title from don't add trailing slash and redirect in core; do it in the `file.dir` handler to don't redirect (with `/` added) in core; do it in the `file.dir` handler Mar 1, 2016

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Mar 1, 2016

Member

The PR changes the behavior of redirect handler (and reverse.proxy.url) as follows:

~1.7.x:

  /abc:
    redirect: http://example.com/def # /abc/xyz => /defxyz
  /abc:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz
  /abc/:
    redirect: http://example.com/def # /abc/xyz => /defxyz
  /abc/:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz

this PR:

paths:
  /abc:
    redirect: http://example.com/def # /abc/xyz => /def/xyz
  /abc:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz
  /abc/:
    redirect: http://example.com/def # /abc/xyz => /def/xyz
  /abc/:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz

The change makes sense to me. The differences between the two are the first and the third example in the current implementation, in which /abc/xyz are converted to /defxyz - looks pretty odd.

If we consider this behavior as a bug, then we would not need to consider this as a breaking change for the standalone server.

edit: fix errors pointed out by #820 (comment)

Member

kazuho commented Mar 1, 2016

The PR changes the behavior of redirect handler (and reverse.proxy.url) as follows:

~1.7.x:

  /abc:
    redirect: http://example.com/def # /abc/xyz => /defxyz
  /abc:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz
  /abc/:
    redirect: http://example.com/def # /abc/xyz => /defxyz
  /abc/:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz

this PR:

paths:
  /abc:
    redirect: http://example.com/def # /abc/xyz => /def/xyz
  /abc:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz
  /abc/:
    redirect: http://example.com/def # /abc/xyz => /def/xyz
  /abc/:
    redirect: http://example.com/def/ # /abc/xyz => /def/xyz

The change makes sense to me. The differences between the two are the first and the third example in the current implementation, in which /abc/xyz are converted to /defxyz - looks pretty odd.

If we consider this behavior as a bug, then we would not need to consider this as a breaking change for the standalone server.

edit: fix errors pointed out by #820 (comment)

@utrenkner

This comment has been minimized.

Show comment
Hide comment
@utrenkner

utrenkner Mar 1, 2016

@kazuho

In the only difference between the two which is the third example in the current implementation,

From your post it seems like the result of the first example would also change defxyz to /def/xyz. Is this correct or not?

utrenkner commented Mar 1, 2016

@kazuho

In the only difference between the two which is the third example in the current implementation,

From your post it seems like the result of the first example would also change defxyz to /def/xyz. Is this correct or not?

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Mar 1, 2016

Member

@utrenkner Thank you for pointing that out. Updated my previous comment.

Member

kazuho commented Mar 1, 2016

@utrenkner Thank you for pointing that out. Updated my previous comment.

kazuho added a commit that referenced this pull request Mar 2, 2016

Merge pull request #820 from h2o/kazuho/append-slash-in-file-handler
don't redirect (with `/` added) in core; do it in the `file.dir` handler

@kazuho kazuho merged commit 36362fe into master Mar 2, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@kazuho kazuho added this to the v2.0 milestone Mar 2, 2016

@omarkilani

This comment has been minimized.

Show comment
Hide comment
@omarkilani

omarkilani Mar 3, 2016

This is a really great change. :) Will solve a number of issues I'm having rewriting a large Apache config.

omarkilani commented Mar 3, 2016

This is a really great change. :) Will solve a number of issues I'm having rewriting a large Apache config.

mingodad added a commit to mingodad/h2o that referenced this pull request Mar 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment