X Tutup
Skip to content

tweak rewrite conditions in htaccess#40584

Merged
C0rby merged 1 commit intomasterfrom
tweak-htaccess
Feb 1, 2023
Merged

tweak rewrite conditions in htaccess#40584
C0rby merged 1 commit intomasterfrom
tweak-htaccess

Conversation

@C0rby
Copy link
Contributor

@C0rby C0rby commented Jan 17, 2023

This change hardens the rewrite rules to match the exact paths we want and not any subpaths e.g. /somefolder/status.php.

Thanks to Terry Franklin, Matt Harris, Hayden Barker and Colin Smith (aka yoloClin) from Radiant Security (https://radiant.security) for reporting this.

@C0rby C0rby requested review from IljaN and xoxys January 17, 2023 13:52
@C0rby C0rby self-assigned this Jan 17, 2023
@update-docs
Copy link

update-docs bot commented Jan 17, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

ownclouders commented Jan 17, 2023 •
edited
Loading

💥 Acceptance tests pipeline cliCreateLocalStorage-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37812/105

@C0rby
Copy link
Contributor Author

C0rby commented Jan 17, 2023

@IljaN, could you maybe tell me why the test is failing?
I'm not sure if this is because of my change.

@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Jan 17, 2023

@IljaN, could you maybe tell me why the test is failing?
I'm not sure if this is because of my change.

yes - it is related.

There is some code somewhere which is doing magic on htaccess.
Due to these changes this is falling apart ....

@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Jan 17, 2023

core/tests/acceptance/run.sh

Line 764 in 32b6984

remote_occ ${ADMIN_AUTH} ${OCC_URL} "config:system:set htaccess.RewriteBase --value /${WEBSERVER_PATH}/"

DeepDiver1975
DeepDiver1975 previously requested changes Jan 17, 2023
View reviewed changes
@mdusher
Copy link
Contributor

mdusher commented Jan 25, 2023

It may be worth looking at swapping %{REQUEST_FILENAME} out for %{REQUEST_URI} as according to the mod_rewrite documentation (https://httpd.apache.org/docs/2.4/mod/mod_rewrite.html) there is some cases where %{REQUEST_FILENAME} can resolve to the full local path of the script/file (eg. /var/www/owncloud/status.php instead /status.php) which would break those RewriteCond rules.

@C0rby C0rby force-pushed the tweak-htaccess branch 2 times, most recently from a6dcaf9 to 215a779 Compare January 31, 2023 17:00
@C0rby
Copy link
Contributor Author

C0rby commented Jan 31, 2023

It may be worth looking at swapping %{REQUEST_FILENAME} out for %{REQUEST_URI} as according to the mod_rewrite documentation (https://httpd.apache.org/docs/2.4/mod/mod_rewrite.html) there is some cases where %{REQUEST_FILENAME} can resolve to the full local path of the script/file (eg. /var/www/owncloud/status.php instead /status.php) which would break those RewriteCond rules.

That is actually why the CI failed.... Thanks for the hint. 👍

@C0rby C0rby requested review from a user and DeepDiver1975 and removed request for a user January 31, 2023 19:59
@phil-davis
Copy link
Contributor

phil-davis commented Feb 1, 2023

@C0rby is a changelog needed for this?

tweak rewrite conditions in htaccess
35c7d41 
This change hardens the rewrite rules to match the exact paths we want
and not any subpaths e.g. `/somefolder/status.php`.
Thanks to Terry Franklin, Matt Harris, Hayden Barker and Colin
Smith (aka yoloClin) from Radiant Security (https://radiant.security)
for reporting this.
@C0rby
Copy link
Contributor Author

C0rby commented Feb 1, 2023

@C0rby is a changelog needed for this?

I added one. 👍

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@C0rby C0rby dismissed DeepDiver1975’s stale review February 1, 2023 23:11

Comment is resolved now.

@C0rby C0rby merged commit 5f062ba into master Feb 1, 2023
@delete-merged-branch delete-merged-branch bot deleted the tweak-htaccess branch February 1, 2023 23:11
@C0rby
Copy link
Contributor Author

C0rby commented Mar 16, 2023

Problem could be cause by new adding ^ not match by e.g. /owncloud/remote.php if use sub-dir install?

I did test sub-dir installations and that worked at that time but maybe I've missed something.

@pako81 pako81 mentioned this pull request Mar 24, 2023
Closed
@IljaN IljaN mentioned this pull request Mar 24, 2023
Merged
11 tasks
@IljaN
Copy link
Contributor

IljaN commented Mar 24, 2023

Potential fix here: #40697

@cfi-gb cfi-gb mentioned this pull request Dec 3, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

2 more reviewers

@IljaN IljaN IljaN approved these changes

@xoxys xoxys xoxys approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@C0rby C0rby

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@C0rby @ownclouders @DeepDiver1975 @mdusher @phil-davis @IljaN @xoxys
X Tutup