Skip to content

Add rewrite base to generated .htaccess rules#40697

Merged
IljaN merged 5 commits into
masterfrom
add-rewrite-base
Mar 28, 2023
Merged

Add rewrite base to generated .htaccess rules#40697
IljaN merged 5 commits into
masterfrom
add-rewrite-base

Conversation

@IljaN

@IljaN IljaN commented Mar 24, 2023

Copy link
Copy Markdown
Contributor

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

update-docs Bot commented Mar 24, 2023

Copy link
Copy Markdown

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
IljaN
IljaN commented Mar 24, 2023
View reviewed changes
Comment thread lib/private/Setup.php
@ownclouders

ownclouders commented Mar 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

💥 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

ownclouders commented Mar 24, 2023

Copy link
Copy Markdown
Contributor

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

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

@ownclouders

ownclouders commented Mar 24, 2023

Copy link
Copy Markdown
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

@jnweiger jnweiger left a comment

Copy link
Copy Markdown
Contributor

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

IljaN commented Mar 24, 2023

Copy link
Copy Markdown
Contributor Author

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

IljaN commented Mar 24, 2023

Copy link
Copy Markdown
Contributor Author

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

@jnweiger

jnweiger commented Mar 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread lib/private/Setup.php Outdated
@IljaN

IljaN commented Mar 24, 2023

Copy link
Copy Markdown
Contributor Author

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
Comment thread 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)/.*";

@jnweiger jnweiger Mar 27, 2023

Copy link
Copy Markdown
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

sonarqubecloud Bot commented Mar 28, 2023

Copy link
Copy Markdown

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

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@pako81 pako81 Awaiting requested review from pako81

@jvillafanez jvillafanez Awaiting requested review from jvillafanez

1 more reviewer

@jnweiger jnweiger jnweiger approved these changes

Reviewers whose approvals may not affect merge requirements

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