Skip to content

fix: encode search highlights returned as a DAV property - #41760

Merged
DeepDiver1975 merged 1 commit into
masterfrom
fix/oc10-124-dav-highlights-escaping
Jul 29, 2026
Merged

fix: encode search highlights returned as a DAV property#41760
DeepDiver1975 merged 1 commit into
masterfrom
fix/oc10-124-dav-highlights-escaping

Conversation

@DeepDiver1975

Copy link
Copy Markdown
Contributor

Description

The search-highlights property of the files search REPORT returned the elasticsearch highlight fragment unencoded. That fragment is built from file content, so a user who shared a crafted file could place arbitrary markup into the property value. The property deliberately carries HTML — line breaks are substituted with <br/> — so a client rendering it as HTML would execute the injected markup.

The fragment is now encoded before the line break substitution. The order matters: encoding afterwards would also escape the <br/> injected here on purpose.

$highlight = \htmlspecialchars($paths[$path]->highlights[0], ENT_QUOTES, 'UTF-8');
$propFind->set(self::REPORT_HIGHLIGHTS, \str_replace(["\r\n", "\r", "\n"], '<br/>', $highlight));

Related Issue

Motivation and Context

This is the second egress channel for the same fragment. owncloud/search_elastic#348 sets 'encoder' => 'html' on the highlight query, so with that PR in place the value already arrives encoded — but this change keeps the property safe independently of which search backend produced it, and independently of that app's version.

Scope, stated precisely rather than overclaimed: I verified with a live XMLWriter run that Sabre already XML-escapes property text, so this was not an exploitable XML-transport hole. The value semantically carries HTML, so this is hardening of an HTML-bearing property, not a second active XSS.

How Has This Been Tested?

  • test environment: PHP 8.3.32, PHPUnit 9.6.35.
  • new test: testOnReportEscapesHighlights, a 4-case data provider covering <script>, <img onerror>, a payload combined with a newline (asserting the intended <br/> still comes through as real markup), and an already-encoded fragment (asserting single encoding only). Written test-first — all 4 cases failed before the fix.
  • regression: apps/dav/tests/unit/Connector/ → OK, 466 tests / 1720 assertions. FilesSearchReportPluginTest alone → OK, 22 tests / 208 assertions; the pre-existing highlight assertion still passes because its fixture has nothing to escape.
  • style: php-cs-fixer on both changed files → 0 of 2 files to fix.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added — covered in the companion search_elastic PR, which owns the rendering path
  • Changelog item (changelog/unreleased/41760)

Note: the changelog file is named for the anticipated PR number — rename if this lands as a different number.

🤖 Generated with Claude Code

The search-highlights property of the files search REPORT returned the
elasticsearch highlight fragment unencoded. That fragment is built from file
content, so a user who shared a crafted file could place arbitrary markup in
the property value. The property deliberately carries HTML - line breaks are
substituted with <br/> - so a client rendering it as HTML would execute the
injected markup.

The fragment is now encoded before the line break substitution. The order
matters: encoding afterwards would also escape the <br/> injected here on
purpose.

search_elastic additionally sets 'encoder' => 'html' on the highlight query,
so the value arrives encoded already. This change keeps the property safe
independently of which search backend produced it.

Fixes OC10-124

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@DeepDiver1975
DeepDiver1975 merged commit b1d65fd into master Jul 29, 2026
29 checks passed
@DeepDiver1975
DeepDiver1975 deleted the fix/oc10-124-dav-highlights-escaping branch July 29, 2026 10:36
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.

2 participants