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.

Sign up for GitHub

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

Jump to bottom

Add rewrite base to generated .htaccess rules #40697

Merged
merged 5 commits into from
Mar 28, 2023
Merged

Add rewrite base to generated .htaccess rules #40697

merged 5 commits into from
Mar 28, 2023

Conversation

IljaN
Copy link
Contributor

@IljaN IljaN commented Mar 24, 2023

Description

If htaccess.RewriteBase is set we need to prepend it in the generated rules.

Related Issue

Motivation and Context

The hardened .htaccess fix caused a regression for installations which have owncloud in a subfolder.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@IljaN
Add rewrite base to generated .htaccess rules
2f9d842 
If htaccess.RewriteBase is set we need to prepend it to the generated rules.
@update-docs
Copy link

update-docs bot commented Mar 24, 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.

@IljaN IljaN mentioned this pull request Mar 24, 2023
Merged
@ownclouders
Copy link
Contributor

ownclouders commented Mar 24, 2023 •
edited
Loading

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

https://drone.owncloud.com/owncloud/core/38141/170

@ownclouders
Copy link
Contributor

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

https://drone.owncloud.com/owncloud/core/38112/54

@ownclouders
Copy link
Contributor

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

https://drone.owncloud.com/owncloud/core/38112/51

@IljaN IljaN force-pushed the add-rewrite-base branch from 015c420 to 14d7f49 Compare March 24, 2023 09:07
jnweiger
jnweiger suggested changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

A literal unescaped $rewriteBase can destroy the regexp.
We need to apply proper regexp escaping, this is a security relevant.

@IljaN
Copy link
Contributor Author

IljaN commented Mar 24, 2023

We could do this in a addtional PR by sanitizing $rewriteBase with preg_quote. But IMO offtopic for this PR. A path shouldn't contain any Regex characters and is coming from the trusted config anyways, no?

@IljaN
Copy link
Contributor Author

IljaN commented Mar 24, 2023

Fail: https://drone.owncloud.com/owncloud/core/38114/172/15

@jnweiger
Copy link
Contributor

jnweiger commented Mar 24, 2023 •
edited
Loading

We could do this in a addtional PR by sanitizing $rewriteBase with preg_quote. But IMO offtopic for this PR. A path shouldn't contain any Regex characters and is coming from the trusted config anyways, no?

No, I don't think so. This path is perfectly legal, imho.
occ config:system:set htaccess.RewriteBase --value '/funny$nameing-scheme-*-but+we+like+it'

Pfft. On another thought, we don't quote strings in .htaccess, that means it breaks apart, when RewriteBase contains whitespace...

jnweiger
jnweiger reviewed Mar 24, 2023
View reviewed changes
lib/private/Setup.php Outdated Show resolved Hide resolved
@IljaN
Copy link
Contributor Author

IljaN commented Mar 24, 2023

Hmhh this rewrite rule seems to break tests which run on apache:

./tests/acceptance/run.sh --type api
Script path: /drone/src/tests/acceptance
Not using php inbuilt server for running scenario ...
Updating .htaccess for proper rewrites
-:2: parser error : Start tag expected, '<' not found

jnweiger
jnweiger approved these changes Mar 27, 2023
View reviewed changes
lib/private/Setup.php
$content .= "\n RewriteCond %{REQUEST_URI} !^$rewriteBaseRe/updater/";
$content .= "\n RewriteCond %{REQUEST_URI} !^$rewriteBaseRe/ocs-provider/";
$content .= "\n RewriteCond %{REQUEST_URI} !^$rewriteBaseRe/ocm-provider/";
$content .= "\n RewriteCond %{REQUEST_URI} !^$rewriteBaseRe/\\.well-known/(acme-challenge|pki-validation)/.*";
Copy link
Contributor

Choose a reason for hiding this comment

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

.well-known usually lives at the root.
But I don't think it hurts to also enable it also at the ownCloud subfolder level.

Our .htaccess does not limit .well-known at the root, when it is in a subfolder. Afaik, that is the way how .htaccess works.

@jnweiger @IljaN
@IljaN we have a double // in the way, the test suite generates the r…
bb3cd9d 
…ewriteBase.

I am adding a trim, to get rid of double slashes. That should make the CI pass.
@IljaN IljaN force-pushed the add-rewrite-base branch from fd19c0d to bb3cd9d Compare March 28, 2023 12:47
@sonarqubecloud
Copy link

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

@IljaN IljaN merged commit d51cb0a into master Mar 28, 2023
@IljaN IljaN deleted the add-rewrite-base branch March 28, 2023 13:32
IljaN added a commit that referenced this pull request Mar 29, 2023
@IljaN
changelog for #40697
8f3cd4e
@IljaN IljaN mentioned this pull request Mar 29, 2023
Merged
pako81 pushed a commit that referenced this pull request Mar 29, 2023
@IljaN
[skip-ci] changelog for #40697 (#40711)
124fe43 
* changelog for #40697

* update changelog

---------

Co-authored-by: Pasquale Tripodi <ptripodi@owncloud.com>
@phil-davis phil-davis mentioned this pull request Apr 2, 2023
Closed
@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

@jnweiger jnweiger jnweiger approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@pako81 pako81 Awaiting requested review from pako81

@jvillafanez jvillafanez Awaiting requested review from jvillafanez

Assignees
No one assigned
Labels
2 - Developing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[QA] hardened .htaccess breaks index.php-less setup when ownCloud URL has a subfolder
3 participants
@IljaN @ownclouders @jnweiger