Skip to content

feat(http): centralize Content-Disposition header generation#59152

Draft
joshtrichards wants to merge 16 commits intomasterfrom
jtr/feat-http-Content-Disposition-util-class
Draft

feat(http): centralize Content-Disposition header generation#59152
joshtrichards wants to merge 16 commits intomasterfrom
jtr/feat-http-Content-Disposition-util-class

Conversation

@joshtrichards
Copy link
Member

  • Resolves: #

Summary

Consolidates all Content-Disposition header generation into a single utility class (OC\Http\ContentDisposition). This class is a wrapper for Symfony's HeaderUtils::makeDisposition() with Nextcloud specific multibyte-safe ASCII fallback generator (inspired by Symfony's BinaryFileResponse).

This brings consistency across the codebase for Content-Disposition handling and fixes some problems:

  • DownloadResponse used manual strtr() quoting -- lacked filename* for non-ASCII names
  • FileDisplayResponse used rawurldecode() (which made no sense at all but happened to work for most filenames, but certainly not all)
  • FilesPlugin and files_versions/Plugin had UserAgent sniffing and special handling for IE/Freebox -- unnecessary complexity, legacy, and not done anywhere else
  • TrashbinPlugin lacked filename* (non-ASCII) handling
  • ImageExportPlugin, MultiGetExportPlugin, TaskProcessingApiController, LogSettingsController used unescaped filenames (not mattering in some cases, but less-than-robust in others)
  • Each site had different (inconsistent) quoting/encoding

Changes:

  • Adds lib/private/Http/ContentDisposition.php, a thin wrapper around Symfony\Component\HttpFoundation\HeaderUtils::makeDisposition()
  • Migrates all call sites to use ContentDisposition::make()
  • Removes UA-sniffing constants and IRequest dependency from files_versions/Plugin
  • Removes IRequest UA-sniffing from FilesPlugin::httpGet()
  • Updates all existing tests and adds ContentDispositionTest
  • Updates integration test expected header values

Public API note: DownloadResponse, FileDisplayResponse, and DataDownloadResponse retain their existing constructor signatures. Apps using these classes require no changes -- they simply get correct RFC 6266 headers automatically.

TODO

  • ...

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Switch to more robust centralized ContentDisposition

Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
…dling

Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
…plementation

Signed-off-by: Josh <josh.t.richards@gmail.com>
Format is now consistent
No UA exceptions

Signed-off-by: Josh <josh.t.richards@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug enhancement feature: files ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant