Skip to content

fix(recording): Limit config key length for request-upload configs#18449

Merged
nickvergessen merged 1 commit into
mainfrom
bugfix/noid/limit-request-upload-key
Jun 26, 2026
Merged

fix(recording): Limit config key length for request-upload configs#18449
nickvergessen merged 1 commit into
mainfrom
bugfix/noid/limit-request-upload-key

Conversation

@nickvergessen

@nickvergessen nickvergessen commented Jun 26, 2026

Copy link
Copy Markdown
Member

☑️ Resolves

Upload with a long filename otherwise causes an error:

<?xml version="1.0"?>
<ocs>
<meta>
<status>failure</status>
<statuscode>400</statuscode>
<message></message>
</meta>
<data>
<error>Value (recording-upload/iip3ca8c/dbe5c5e0e279a4f73442062c41f3fc7f_1782454462.mp4) for key is too long (64)</error>
</data>
</ocs>

🛠️ API Checklist

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not possible
  • 📘 API documentation in docs/ has been updated or is not required
  • 🔖 Capability is added or not needed

@nickvergessen nickvergessen added this to the ⛅ Next Major (35) milestone Jun 26, 2026
@nickvergessen nickvergessen self-assigned this Jun 26, 2026
@nickvergessen nickvergessen added bug feature: api 🛠️ OCS API for conversations, chats and participants feature: recordings ⏺️ Including the recording server labels Jun 26, 2026
@nickvergessen

nickvergessen commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

/backport to stable34

@nickvergessen

nickvergessen commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

/backport to stable33

@nickvergessen

nickvergessen commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

/backport to stable32

danxuliu
danxuliu reviewed Jun 26, 2026
View reviewed changes

@danxuliu danxuliu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than trimming, what about hashing the original key? Due to how the key is used I guess that even despite the trimming there will be no collisions/duplicates, but the hash should reduce the possibility even more.

Antreesy
Antreesy reviewed Jun 26, 2026
View reviewed changes
Comment thread lib/Service/RecordingService.php Outdated
private function getUploadShareConfigKey(Room $room, string $fileName): string {
return self::APPCONFIG_UPLOAD_PREFIX . $room->getToken() . '/' . basename($fileName);
$originalKey = self::APPCONFIG_UPLOAD_PREFIX . $room->getToken() . '/' . basename($fileName);
$key = substr($originalKey, 0, 64);

@Antreesy Antreesy Jun 26, 2026

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.

Maybe we should extract 64 chars from the right side? If path to recording directory is long, there might be duplicate keys?

@nickvergessen nickvergessen force-pushed the bugfix/noid/limit-request-upload-key branch from 53d8754 to 943b7de Compare June 26, 2026 11:45
Antreesy
Antreesy approved these changes Jun 26, 2026
View reviewed changes
Comment thread lib/Service/RecordingService.php

private function getUploadShareConfigKey(Room $room, string $fileName): string {
return self::APPCONFIG_UPLOAD_PREFIX . $room->getToken() . '/' . basename($fileName);
return self::APPCONFIG_UPLOAD_PREFIX . $room->getToken() . '/' . sha1(basename($fileName));

@Antreesy Antreesy Jun 26, 2026

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.

so it's 10 (prefix) + 10 (token) + '/' + 40 (hash) now?

@nickvergessen nickvergessen Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, which is <= 64 and therefor okay

@danxuliu danxuliu Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But tokens can be up to 30 characters long, no? Or am I misunderstanding the entropy parameter? 🤔

@nickvergessen
fix(recording): Reduce config key length for request-upload configs
a2fb60c 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/limit-request-upload-key branch from 943b7de to a2fb60c Compare June 26, 2026 11:53
@nickvergessen nickvergessen enabled auto-merge June 26, 2026 12:05
danxuliu
danxuliu previously requested changes Jun 26, 2026
View reviewed changes

@danxuliu danxuliu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Temporary blocking the auto-merge until the comment about the token length is clarified.

@nickvergessen nickvergessen mentioned this pull request Jun 26, 2026
Open
@danxuliu danxuliu dismissed their stale review June 26, 2026 12:22

It will be addressed in a follow-up

@nickvergessen nickvergessen merged commit 4048252 into main Jun 26, 2026
79 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/limit-request-upload-key branch June 26, 2026 12:22
This was referenced Jun 26, 2026
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Antreesy Antreesy Antreesy approved these changes
@danxuliu danxuliu danxuliu left review comments
@SystemKeeper SystemKeeper Awaiting requested review from SystemKeeper

Assignees

@nickvergessen nickvergessen

Labels

bug feature: api 🛠️ OCS API for conversations, chats and participants feature: recordings ⏺️ Including the recording server

Projects

None yet

Milestone

⛅ Next Major (35)

Development

Successfully merging this pull request may close these issues.

3 participants

@nickvergessen @danxuliu @Antreesy