Skip to content

ci: swap deprecated ep_readonly_guest for ep_guest in plugin matrix #7808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JohnMcLear merged 1 commit into from
May 18, 2026
Merged

ci: swap deprecated ep_readonly_guest for ep_guest in plugin matrix #7808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/backend-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ jobs:
ep_hash_auth
ep_headings2
ep_markdown
ep_readonly_guest
ep_guest
ep_set_title_on_pad
ep_spellcheck
ep_subscript_and_superscript
Expand Down Expand Up @@ -289,7 +289,7 @@ jobs:
ep_hash_auth
ep_headings2
ep_markdown
ep_readonly_guest
ep_guest
ep_set_title_on_pad
ep_spellcheck
ep_subscript_and_superscript
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/frontend-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ jobs:
ep_hash_auth
ep_headings2
ep_markdown
ep_readonly_guest
ep_guest
ep_set_title_on_pad
ep_spellcheck
ep_subscript_and_superscript
Expand Down Expand Up @@ -308,7 +308,7 @@ jobs:
ep_hash_auth
ep_headings2
ep_markdown
ep_readonly_guest
ep_guest
ep_set_title_on_pad
ep_spellcheck
ep_subscript_and_superscript
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/load-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ jobs:
ep_hash_auth
ep_headings2
ep_markdown
ep_readonly_guest
ep_guest
ep_set_title_on_pad
ep_spellcheck
ep_subscript_and_superscript
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/upgrade-from-latest-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ jobs:
ep_hash_auth
ep_headings2
ep_markdown
ep_readonly_guest
ep_guest
ep_set_title_on_pad
ep_spellcheck
ep_subscript_and_superscript
Expand Down
23 changes: 14 additions & 9 deletions src/tests/backend/specs/admin/anonymizeAuthorSocket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,21 @@ const ask = (socket: any, evt: string, payload: any, replyEvt: string) =>
});

// adminSocket() depends on Etherpad's default plain-text password check for
// settings.users[name].password. Plugins like ep_hash_auth replace the
// authenticate hook to expect hashed credentials, so the basic-auth probe
// returns no admin session, /settings's connection handler returns without
// settings.users[name].password. Any authenticate-hook plugin that claims
// the request before the built-in basic-auth fallback can block this:
// the historical offender was ep_readonly_guest, whose authenticate hook
// sorts itself first and silently swaps req.session.user with a guest
// (#7795); ep_hash_auth-style plugins that expect hashed credentials
// would do the same. When that happens the basic-auth probe returns no
// admin session, /settings's connection handler returns without
// registering listeners (see src/node/hooks/express/adminsettings.ts:25),
// and every socket.emit() afterwards waits forever for a reply that
// nothing will ever send. The socket itself still connects when admin
// session is missing, so the probe has to run at the application layer:
// emit a known `/settings` event (`load`) and wait for the matching reply
// (`settings`). If it doesn't arrive within the budget, skip — much
// cheaper than letting mocha's 120s per-test timeout absorb 7 stalled
// tests. Tracked in #7795.
// emit a known `/settings` event (`authorLoad`) and wait for the matching
// reply (`results:authorLoad`). If it doesn't arrive within the budget,
// skip — much cheaper than letting mocha's 120s per-test timeout absorb
// 7 stalled tests.
const PROBE_BUDGET_MS = 15000;
const adminSocketWithProbe = async (budgetMs: number): Promise<{
ok: true; socket: any;
Expand Down Expand Up @@ -135,8 +139,9 @@ describe(__filename, function () {
if (!probe.ok) {
console.warn(
`[anonymizeAuthorSocket] admin socket probe failed (${probe.reason}); ` +
'skipping suite — likely an authenticate-hook plugin (e.g. ep_hash_auth) ' +
'rejecting the test\'s plain-text admin credentials. Tracked in #7795.');
'skipping suite — an authenticate-hook plugin (e.g. ep_readonly_guest, ' +
'or an ep_hash_auth-style plugin requiring hashed credentials) is ' +
'rejecting the test\'s plain-text admin credentials.');
this.skip();
return;
Comment on lines 139 to 146
Copy link
Copy Markdown

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot May 18, 2026

Choose a reason for hiding this comment

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

Action required

1. Suite still skips on failure 📎 Requirement gap ☼ Reliability

The anonymizeAuthorSocket suite still calls this.skip() when the admin socket probe fails, which
can let plugin-enabled CI go green without exercising the 7 /settings socket tests. This also
means the PR does not provide a failing regression signal if the authenticate-hook interaction
regresses in the future.
Agent Prompt 
## Issue description
`src/tests/backend/specs/admin/anonymizeAuthorSocket.ts` currently skips the entire suite when the admin socket probe fails. Per the compliance checklist, this mitigation should be removed once the underlying issue is addressed, and bug fixes should be backed by a regression test that fails when the issue reappears.

## Issue Context
The probe is intended to prevent long CI hangs, but `this.skip()` allows CI to pass without validating `/settings` emit/reply behavior under plugins.

## Fix Focus Areas
- src/tests/backend/specs/admin/anonymizeAuthorSocket.ts[139-146]

## Implementation notes
- Replace `this.skip()` with a failing assertion (e.g., `assert.fail(...)`) or `throw new Error(...)` so CI fails fast (within the probe budget) instead of silently skipping.
- Ensure the failure message clearly identifies that an authenticate-hook plugin prevented admin session establishment and caused missing `/settings` handlers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}
Expand Down
Loading