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

No more 403 and 404 redirecting #392

Merged
merged 1 commit into from
Mar 22, 2017
Merged

No more 403 and 404 redirecting #392

merged 1 commit into from
Mar 22, 2017

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Mar 20, 2017

Seems to be causing many issues lately:
Fix nextcloud/server#3847

There is no way we can notify people about such changes, right?

@nickvergessen nickvergessen added this to the Nextcloud 12.0 milestone Mar 20, 2017
@MorrisJobke
Copy link
Member

cc @josh4trunks but for me this looks okay, because we render a proper HTML page in such cases anyway.

@josh4trunks
Copy link
Contributor

This should also be removed in the "Nextcloud in a subdir of nginx" example.

The error pages broke OCS endpoints which return 403 and 404
nextcloud/server#3847

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

I didn't notice it's twice in the docs. fixed now

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

@MorrisJobke MorrisJobke merged commit c7a2700 into master Mar 22, 2017
@MorrisJobke MorrisJobke deleted the no-more-error-pages branch March 22, 2017 03:24
@MorrisJobke
Copy link
Member

stable11 6533a9c

@josh4trunks
Copy link
Contributor

I had a thought. For the deny all; location, those will still be handled by Nginx and never actually be passed to the now internal error page right?

Should we instead forward these all to index.php?

@MorrisJobke
Copy link
Member

Should we instead forward these all to index.php?

I would say so.

@josh4trunks
Copy link
Contributor

josh4trunks commented May 15, 2017

I am not sure if .htaccess currently rewrites these? It does [R=404,L]

Also, I'm wondering why I originally included the following files to be executed directly. I know there was a reason for it, but I don't see reference to them in .htaccess anymore.

cron.php
core/ajax/update.php
ocs/v1.php
ocs/v2.php
updater/*.php
ocs-provider/*.php

@MorrisJobke
Copy link
Member

Also, I'm wondering why I originally included the following files to be executed directly. I know there was a reason for it, but I don't see reference to them in .htaccess anymore.

They should be executed directly. The rewrite rules are extended in https://github.com/nextcloud/server/blob/ccd0ded812c2039c14293b78310f144f1b5d7538/lib/private/Setup.php#L459-L485

@josh4trunks
Copy link
Contributor

ahh great, I'll review that a bit

@josh4trunks
Copy link
Contributor

That is where the static files were referenced from!
(css|js|svg|gif|png|html|ttf|woff|ico|jpg|jpeg)

But... I do not think Nextcloud serves anything but svg, woff, and occasionally a png. I did see a ttf file in Nextcloud's source but I'm not sure if it's ever served.

@MorrisJobke
Copy link
Member

That is where the static files were referenced from!
(css|js|svg|gif|png|html|ttf|woff|ico|jpg|jpeg)

A right ... they were still in the cache. Let me revert the PR.

@MorrisJobke
Copy link
Member

A right ... they were still in the cache. Let me revert the PR.

And then you can create a new one with all the stuff (including the TTL for the cache)

@josh4trunks
Copy link
Contributor

I don't fully understand. Is Nextcloud setting a TTL / max-age for these somewhere?

@MorrisJobke
Copy link
Member

I don't fully understand. Is Nextcloud setting a TTL / max-age for these somewhere?

I meant: https://github.com/nextcloud/server/blob/bff6c8aafc49a3260294c3244571e3a31fd09cca/.htaccess#L24-L24

@MorrisJobke
Copy link
Member

because I reverted the other PR and there was also the update for the max-age included.

@josh4trunks
Copy link
Contributor

Ok, makes sense.
I will do that and also make those regexs not case insensitive. No need to check for uppercase if apache is not.

@josh4trunks
Copy link
Contributor

See PR here #468

MorrisJobke added a commit to nextcloud/server that referenced this pull request Jul 26, 2017
* Nextcloud is not properly loaded in the standalone version (especially the theming)
* it is already not listed anymore in the Nginx config (see nextcloud/documentation#392)
* the index.php-free version doesn't support this

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
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.

3 participants