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

fix: add properties files to htaccess rewrite condition #48692

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

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Oct 14, 2024

Fix #48691

This is needed for the PDF viewer localisation since it requests a locale.properties file.

@szaimen szaimen added bug 3. to review Waiting for reviews labels Oct 14, 2024
@szaimen szaimen added this to the Nextcloud 31 milestone Oct 14, 2024
@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

/backport to stable30

@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

/backport to stable29

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Could you add a little mention in the commit message that this is needed for PDF viewer localisation? Thanks!

This is needed for the PDF viewer localisation since it requests a `locale.properties` file.

Signed-off-by: Simon L. <[email protected]>
@szaimen szaimen force-pushed the enh/noid/fix-properties-files branch from b4b56ce to d4b0309 Compare October 14, 2024 13:12
@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

Could you add a little mention in the commit message that this is needed for PDF viewer localisation? Thanks!

done

@joshtrichards
Copy link
Member

Haven't looked too closely yet, but off-hand this type of change suggests there was perhaps also a breaking change introduced at some point, no? Because we'll need this in our Admin Manual's nginx config too...

@nickvergessen
Copy link
Member

Haven't looked too closely yet, but off-hand this type of change suggests there was perhaps also a breaking change introduced at some point, no?

Yeah well mostlikely when the upstream library changed it's way of handling things

Because we'll need this in our Admin Manual's nginx config too...

Yes, please add similarly to nextcloud/documentation#12273

@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Oct 14, 2024
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

As agreed with @szaimen I will take over, as there are additional changes needed for the PDF viewer. Blocking to avoid merging until I can push them.

@susnux
Copy link
Contributor

susnux commented Oct 14, 2024

I wonder why this is working using just the default Apache htaccess? (currently it works for me without any changes).

But in general maybe this should be changed to the Apache equivalent of try_files for apps/xxx/js/...?

@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

I wonder why this is working using just the default Apache htaccess? (currently it works for me without any changes).

I suppose you did not enable pretty urls? (they are active for AIO by default)...

@@ -487,7 +487,7 @@ public static function updateHtaccess(): bool {
$content .= "\n Options -MultiViews";
$content .= "\n RewriteRule ^core/js/oc.js$ index.php [PT,E=PATH_INFO:$1]";
$content .= "\n RewriteRule ^core/preview.png$ index.php [PT,E=PATH_INFO:$1]";
$content .= "\n RewriteCond %{REQUEST_FILENAME} !\\.(css|js|mjs|svg|gif|ico|jpg|jpeg|png|webp|html|otf|ttf|woff2?|map|webm|mp4|mp3|ogg|wav|flac|wasm|tflite)$";
$content .= "\n RewriteCond %{REQUEST_FILENAME} !\\.(css|js|mjs|svg|gif|ico|jpg|jpeg|png|webp|html|otf|ttf|woff2?|map|webm|mp4|mp3|ogg|wav|flac|wasm|tflite|properties)$";
Copy link
Member

Choose a reason for hiding this comment

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

This feels dangerous to me. There could be other .properties files that should rather not get exposed by accident. Can you instead add a pattern that matches the exact file name only?

Copy link
Member

Choose a reason for hiding this comment

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

The properties files come from PDF.js locales; there is a main locale.properties and one XXX/viewer.properties for each locale, where XXX is the locale code.

Does something like this would be enough or you would prefer something even stricter?:

$content .= "\n  RewriteCond %{REQUEST_FILENAME} !pdfjs/.*/(locale|viewer)\\.properties$";

Copy link
Member

Choose a reason for hiding this comment

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

Looks good 👍

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress backport-request bug pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files_pdfviewer : localisation doesn't work with NC 29.0.7
7 participants