Skip to content

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

💥 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

@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

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

@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

@C0rby is a changelog needed for this?

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.

@IljaN
Copy link
Contributor

IljaN commented Mar 24, 2023

Potential fix here: #40697

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.

8 participants