Skip to content

Commit e99ff5e

Browse files
committed
fix: fall back instead of stranding empty .pad files
initializeMissingFrontmatter hard-required protected pads, so a `.pad` that arrived outside the UI -- over WebDAV, from another integration, or from before the setting changed -- failed every open attempt with 403 when protected pads were off, even though public pads were available. There was no way out for the user: recovery needs frontmatter the file doesn't have. Use the same fallback the template path already uses. It still closes the `/initialize` bypass (nothing enabled means an exception), but produces an openable pad whenever some type is allowed. Rename resolveForTemplate() to resolveCreatableMode(), since it now serves both callers, and note in its docblock that the fallback can widen access: a protected template becomes a public pad when protected pads are off. README, api-reference and templates docs say so too.
1 parent 708c721 commit e99ff5e

8 files changed

Lines changed: 106 additions & 36 deletions

File tree

README.md

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,15 @@ Two settings control which pad types the app offers, both enabled by default:
108108
Nextcloud account.
109109

110110
Switching a type off hides its `+ New` entries and refuses new pads of that
111-
type on the regular create paths — the API create endpoints, the template
112-
flows and the first-open initialisation of a `.pad` that has no pad yet. Pads
113-
that already exist keep working either way.
111+
type on the API create endpoints. Pads that already exist keep working either
112+
way.
113+
114+
Two paths fall back rather than refuse, so nothing gets stranded: a template
115+
carrying the disabled mode, and the first-open initialisation of a `.pad`
116+
that arrived outside the app (over WebDAV, say). Both create the pad in the
117+
enabled mode; only with both types off is creation refused outright. Note
118+
that this can widen access — a protected template becomes a public pad when
119+
protected pads are off.
114120

115121
The setting decides what the app offers; it is not a hard boundary against a
116122
user who crafts `.pad` files by hand. Recovering an orphaned `.pad` from its

docs/api-reference.md

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,15 @@ Base: `/apps/etherpad_nextcloud`
7171
- read-only protected share: `is_readonly_snapshot=true`, empty `url`, `snapshot_text`, and sanitized `snapshot_html`; no Etherpad session cookie
7272
- public/external pad share: regular public Etherpad URL
7373

74-
Admins can switch either pad type off (see the admin settings). Creating a pad
75-
of a disabled type is refused with `403` by `POST /pads`,
76-
`POST /pads/create-by-parent`, `POST /pads/from-template` and the
77-
`initialize` endpoints; pads that already exist are unaffected. Templates
78-
carrying a disabled mode are created in the enabled mode instead of failing.
74+
Admins can switch either pad type off (see the admin settings). `POST /pads`
75+
and `POST /pads/create-by-parent` refuse a disabled `accessMode` with `403`.
76+
Pads that already exist are unaffected.
77+
78+
`POST /pads/from-template` and the `initialize` endpoints do not refuse a
79+
disabled mode — they create the pad in the enabled mode instead, so a
80+
template or a `.pad` that arrived outside the UI stays usable. They only
81+
return `403` when no pad type is enabled at all. Note this can widen access:
82+
a protected template becomes a public pad when protected pads are off.
7983

8084
The refusal carries `code: pad_type_disabled`, plus `access_mode` naming the
8185
disabled type — that field is absent when no pad type is enabled at all.

docs/templates.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,4 @@ Error responses:
9191
- **Failed template materialisation falls back to a blank pad.** If anything in the listener throws (binding race, Etherpad unreachable, malformed template), the byte-copy NC made is wiped and the new file behaves like a normal empty `.pad` — the regular missing-frontmatter init kicks in on first open.
9292
- **Placeholder substitution applies to both the plain-text and the HTML snapshot in the body**. If a placeholder ends up inside an HTML attribute (`<a href="{{date}}">`), it gets resolved too — keep placeholders in human-readable locations to avoid surprises.
9393
- **No template registry** — every `.pad` in the user's *Templates* folder is a candidate. There's no separate "is a template" flag.
94-
- **A template keeps its own access mode**, unless the admin switched that pad type off. In that case the pad is created in the enabled mode instead of failing, so the template's content still lands. With no pad type enabled at all, template creation is refused.
94+
- **A template keeps its own access mode**, unless the admin switched that pad type off. In that case the pad is created in the enabled mode instead of failing, so the template's content still lands. With no pad type enabled at all, template creation is refused. Be aware this can widen access: a protected template creates a public pad when protected pads are off — on such an instance that is the only option, but the resulting pad is open to anyone with the link.

lib/Service/PadBootstrapService.php

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,13 @@ public function initializeMissingFrontmatter(string $uid, File $file, string $ex
101101
// re-initialising an existing one — the policy applies. Files that
102102
// already have a binding fall into the branch above and keep
103103
// working whatever the admin configured.
104-
$this->padTypePolicy->requireEnabled(BindingService::ACCESS_PROTECTED);
105-
$padId = $this->provisionPadId(BindingService::ACCESS_PROTECTED);
106-
$accessMode = BindingService::ACCESS_PROTECTED;
104+
//
105+
// Fall back rather than refuse: an empty `.pad` can arrive outside
106+
// the UI (WebDAV, another integration, or from before the setting
107+
// changed), and a hard requirement would leave it permanently
108+
// unopenable even when the other pad type is available.
109+
$accessMode = $this->padTypePolicy->resolveCreatableMode(BindingService::ACCESS_PROTECTED);
110+
$padId = $this->provisionPadId($accessMode);
107111
$this->bindingService->createBinding($fileId, $padId, $accessMode);
108112
$createdNewBinding = true;
109113
$createdNewPad = true;

lib/Service/PadCreationService.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ public function materializeTemplateInto(File $target, File $template, ?IUser $us
343343
// A template keeps the access mode of the pad it was made from. If the
344344
// admin switched that type off, create the pad in the other enabled
345345
// mode rather than refusing — the template's content is the point.
346-
$accessMode = $this->padTypePolicy->resolveForTemplate($accessMode);
346+
$accessMode = $this->padTypePolicy->resolveCreatableMode($accessMode);
347347

348348
$snapshot = $this->padFileService->getSnapshotPartsFromBody($pad->body);
349349
$resolvedText = $this->placeholderResolver->applyForContent($snapshot['text'], $user);

lib/Service/PadTypePolicy.php

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,21 @@ public function requireEnabled(string $accessMode): void {
5353
}
5454

5555
/**
56-
* A template carries the access mode of the pad it was made from. When
57-
* that mode is switched off, create the pad in the other enabled mode
58-
* rather than refusing: the template's content is what the user is after,
59-
* and the policy still holds for the resulting pad.
56+
* Pick a mode that may actually be created, preferring the requested one.
57+
*
58+
* Used where refusing would strand the user rather than protect anything:
59+
* a template carries the mode of the pad it was made from, and a `.pad`
60+
* file that arrived outside the UI (WebDAV, another integration) has to
61+
* become *some* pad on first open. Falling back keeps the content
62+
* reachable while the policy still holds for the resulting pad.
63+
*
64+
* Note this can widen access — a protected template becomes a public pad
65+
* when protected pads are off. That is the instance's only option at that
66+
* point, but it is a downgrade in the security-relevant direction.
6067
*
6168
* @throws PadTypeDisabledException when no pad type is enabled at all
6269
*/
63-
public function resolveForTemplate(string $requested): string {
70+
public function resolveCreatableMode(string $requested): string {
6471
if ($this->isEnabled($requested)) {
6572
return $requested;
6673
}

tests/phpunit/unit/PadBootstrapServiceTest.php

Lines changed: 63 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -157,10 +157,10 @@ public function testInitializeMissingFrontmatterCleansUpWhenWriteFails(): void {
157157
$service->initializeMissingFrontmatter('alice', $file, '');
158158
}
159159

160-
public function testBlankInitialisationIsRefusedWhenProtectedPadsAreDisabled(): void {
161-
// Without a binding this provisions a brand-new pad, so the admin's
162-
// setting applies — otherwise an empty .pad plus /initialize would be
163-
// a way around it.
160+
public function testBlankInitialisationIsRefusedWhenNoPadTypeIsEnabled(): void {
161+
// Without a binding this provisions a brand-new pad, so the policy
162+
// applies — otherwise an empty .pad plus /initialize would be a way
163+
// around it.
164164
$bindingService = $this->createMock(BindingService::class);
165165
$bindingService->method('findByFileId')->willReturn(null);
166166
$bindingService->expects($this->never())->method('createBinding');
@@ -171,26 +171,75 @@ public function testBlankInitialisationIsRefusedWhenProtectedPadsAreDisabled():
171171
$etherpadClient = $this->createMock(EtherpadClient::class);
172172
$etherpadClient->expects($this->never())->method('createGroup');
173173
$etherpadClient->expects($this->never())->method('createGroupPad');
174+
$etherpadClient->expects($this->never())->method('createPad');
174175

175176
$file = $this->createMock(File::class);
176177
$file->method('getId')->willReturn(4321);
177178
$file->expects($this->never())->method('putContent');
178179

180+
$service = $this->buildService($bindingService, $padFileService, $etherpadClient, $this->buildPadTypePolicy(false, false));
181+
182+
$this->expectException(\OCA\EtherpadNextcloud\Exception\PadTypeDisabledException::class);
183+
184+
$service->initializeMissingFrontmatter('alice', $file, '');
185+
}
186+
187+
public function testBlankInitialisationFallsBackToPublicWhenProtectedPadsAreDisabled(): void {
188+
// A .pad can arrive outside the UI (WebDAV, another integration). If
189+
// protected pads are off but public ones are allowed, the file must
190+
// still become openable instead of 403-ing forever.
191+
$bindingService = $this->createMock(BindingService::class);
192+
$bindingService->method('findByFileId')->willReturn(null);
193+
$bindingService->expects($this->once())
194+
->method('createBinding')
195+
->with(4321, 'nc-abcdefghijklmnopqrstuvwx', BindingService::ACCESS_PUBLIC);
196+
197+
$padFileService = $this->createMock(PadFileService::class);
198+
$padFileService->method('parseLegacyOwnpadShortcut')->willReturn(null);
199+
$padFileService->method('buildInitialDocument')->willReturn('doc-content');
200+
201+
$etherpadClient = $this->createMock(EtherpadClient::class);
202+
$etherpadClient->expects($this->never())->method('createGroupPad');
203+
$etherpadClient->expects($this->once())->method('createPad')->with('nc-abcdefghijklmnopqrstuvwx');
204+
$etherpadClient->method('buildPadUrl')->willReturn('https://pad.example.test/p/nc-abcdefghijklmnopqrstuvwx');
205+
206+
$secureRandom = $this->createMock(ISecureRandom::class);
207+
$secureRandom->method('generate')->willReturn('abcdefghijklmnopqrstuvwx');
208+
209+
$file = $this->createMock(File::class);
210+
$file->method('getId')->willReturn(4321);
211+
$file->expects($this->once())->method('putContent')->with('doc-content');
212+
179213
$service = new PadBootstrapService(
180214
$bindingService,
181215
$padFileService,
182216
$etherpadClient,
183-
$this->createMock(ISecureRandom::class),
217+
$secureRandom,
184218
$this->createMock(LoggerInterface::class),
185219
$this->createMock(\OCA\EtherpadNextcloud\Service\PadLegacyMigrationService::class),
186-
$this->buildPadTypePolicy(false),
220+
$this->buildPadTypePolicy(false, true),
187221
);
188222

189-
$this->expectException(\OCA\EtherpadNextcloud\Exception\PadTypeDisabledException::class);
190-
191223
$service->initializeMissingFrontmatter('alice', $file, '');
192224
}
193225

226+
private function buildService(
227+
BindingService $bindingService,
228+
PadFileService $padFileService,
229+
EtherpadClient $etherpadClient,
230+
\OCA\EtherpadNextcloud\Service\PadTypePolicy $policy,
231+
): PadBootstrapService {
232+
return new PadBootstrapService(
233+
$bindingService,
234+
$padFileService,
235+
$etherpadClient,
236+
$this->createMock(ISecureRandom::class),
237+
$this->createMock(LoggerInterface::class),
238+
$this->createMock(\OCA\EtherpadNextcloud\Service\PadLegacyMigrationService::class),
239+
$policy,
240+
);
241+
}
242+
194243
public function testExistingBindingStillInitialisesWhenProtectedPadsAreDisabled(): void {
195244
// The pad already exists; the setting governs creation only, so this
196245
// file must keep opening.
@@ -259,14 +308,14 @@ public function testLegacyMigrationStillRunsWhenProtectedPadsAreDisabled(): void
259308
));
260309
}
261310

262-
private function buildPadTypePolicy(bool $protectedEnabled): \OCA\EtherpadNextcloud\Service\PadTypePolicy {
311+
private function buildPadTypePolicy(bool $protectedEnabled, bool $publicEnabled = true): \OCA\EtherpadNextcloud\Service\PadTypePolicy {
263312
$config = $this->createMock(\OCP\IConfig::class);
264313
$config->method('getAppValue')->willReturnCallback(
265-
static fn (string $app, string $key, string $default = ''): string => (
266-
$key === \OCA\EtherpadNextcloud\Service\PadTypePolicy::SETTING_PROTECTED
267-
? ($protectedEnabled ? 'yes' : 'no')
268-
: $default
269-
)
314+
static fn (string $app, string $key, string $default = ''): string => match ($key) {
315+
\OCA\EtherpadNextcloud\Service\PadTypePolicy::SETTING_PROTECTED => $protectedEnabled ? 'yes' : 'no',
316+
\OCA\EtherpadNextcloud\Service\PadTypePolicy::SETTING_PUBLIC => $publicEnabled ? 'yes' : 'no',
317+
default => $default,
318+
}
270319
);
271320
return new \OCA\EtherpadNextcloud\Service\PadTypePolicy($config);
272321
}

tests/phpunit/unit/PadTypePolicyTest.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public function testTemplateKeepsItsOwnModeWhenThatTypeIsEnabled(): void {
6767

6868
self::assertSame(
6969
BindingService::ACCESS_PUBLIC,
70-
$policy->resolveForTemplate(BindingService::ACCESS_PUBLIC)
70+
$policy->resolveCreatableMode(BindingService::ACCESS_PUBLIC)
7171
);
7272
}
7373

@@ -78,7 +78,7 @@ public function testTemplateFallsBackToTheEnabledTypeInsteadOfFailing(): void {
7878

7979
self::assertSame(
8080
BindingService::ACCESS_PROTECTED,
81-
$policy->resolveForTemplate(BindingService::ACCESS_PUBLIC)
81+
$policy->resolveCreatableMode(BindingService::ACCESS_PUBLIC)
8282
);
8383
}
8484

@@ -87,7 +87,7 @@ public function testTemplateFallsBackToPublicWhenProtectedIsDisabled(): void {
8787

8888
self::assertSame(
8989
BindingService::ACCESS_PUBLIC,
90-
$policy->resolveForTemplate(BindingService::ACCESS_PROTECTED)
90+
$policy->resolveCreatableMode(BindingService::ACCESS_PROTECTED)
9191
);
9292
}
9393

@@ -98,7 +98,7 @@ public function testTemplateFailsWhenNoPadTypeIsEnabledAtAll(): void {
9898
]);
9999

100100
try {
101-
$policy->resolveForTemplate(BindingService::ACCESS_PROTECTED);
101+
$policy->resolveCreatableMode(BindingService::ACCESS_PROTECTED);
102102
self::fail('Expected PadTypeDisabledException');
103103
} catch (PadTypeDisabledException $e) {
104104
// No single mode to blame when nothing is enabled.

0 commit comments

Comments
 (0)