test(admin): skip anonymizeAuthorSocket when ep_hash_auth is installed#7796
Merged
Merged
Conversation
…stalled #7789 un-hid this suite from CI and immediately surfaced a 14-minute stall on every with-plugins matrix run (Linux + Windows + the 'Upgrade from latest release' workflow). Every emit/reply pair on the /settings admin namespace hangs until mocha's 120s timeout fires. Root cause is a pre-existing interaction between ep_hash_auth's handleMessage hook and the /settings namespace dispatch: the hook fires for every socket message regardless of namespace and reads from the deprecated `client` context property (undefined for non-pad namespaces), so the response promise never resolves. Tracked separately in #7795. Until that lands, gate the suite on require.resolve('ep_hash_auth'). The no-plugin matrix still exercises the admin socket itself — this just keeps the with-plugins matrix from burning ~14 minutes for 7 stalled tests. Verified locally: - no ep_hash_auth in node_modules → 7 passing - ep_hash_auth resolvable → 0 passing, 7 pending Why require.resolve and not pluginDefs.plugins[...]: Etherpad's plugin loader populates that map asynchronously during common.init. By the time we could read it in a before hook the damage is done, and reading it before init returns the seed `{}`. Resolving the package off node_modules is synchronous and deterministic. Refs #7795 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoSkip anonymizeAuthorSocket suite when ep_hash_auth is installed
WalkthroughsDescription• Skip anonymizeAuthorSocket tests when ep_hash_auth plugin installed • Prevents 14-minute CI stall on with-plugins matrix runs • Uses require.resolve() for synchronous package detection • Preserves test coverage in no-plugin matrix configuration Diagramflowchart LR
A["Test Suite Start"] --> B{"ep_hash_auth<br/>installed?"}
B -->|Yes| C["Skip Tests<br/>7 pending"]
B -->|No| D["Run Tests<br/>7 passing"]
C --> E["Avoid 14min CI stall"]
D --> E
File Changes1. src/tests/backend/specs/admin/anonymizeAuthorSocket.ts
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
… skip
Two Qodo follow-ups on this PR:
1) Replace the static `require.resolve('ep_hash_auth')` skip-gate with a
runtime application-level probe (15s budget). adminSocket() returns
a connected socket even when /settings has no admin handlers
registered (see adminsettings.ts:25 — non-admin sockets exit early
without binding listeners). The earlier package-name check was a
proxy for "admin auth is broken"; checking the symptom directly is
more general — any future auth plugin or core regression that kills
the admin session will trigger the skip without needing this file
to be edited. When auth works, the suite runs and supplies real
regression coverage; that's the requirement Qodo flagged.
2) Guard after() with a setupCompleted flag. The skip-via-this.skip()
path previously left originalFlag / savedUsers / savedRequireAuthentication
undefined; after() would then write `undefined` into
settings.gdprAuthorErasure.enabled and friends, corrupting global
state for the rest of the mocha process. Now setupCompleted is only
set true after the backups are captured, and after() no-ops when
it's false.
Verified locally:
- no-plugin matrix → 7 passing (2s)
- broken-auth sim → 0 passing, 7 pending (17s)
Refs #7795
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
4 tasks
JohnMcLear
added a commit
that referenced
this pull request
May 18, 2026
…7808) ep_readonly_guest is archived (read-only on GitHub) and its authenticate hook unconditionally swaps req.session.user with a read-only guest, even when the request carries an HTTP Authorization header. That silently demoted admin login attempts and stalled the anonymizeAuthorSocket tests for 14 min/run on every with-plugins CI matrix (#7795). The pre-fix theory blamed ep_hash_auth.handleMessage; the actual hook trace is a red herring — handleMessage only fires on the /pad namespace and never on /settings. ep_guest is the maintained successor (same authors, same purpose). 1.0.72 on npm already includes the "defer to basic auth / admin paths" fix backported to ep_readonly_guest by intent here. Swapping the matrix unblocks the anonymizeAuthorSocket suite on Linux, Windows, and the upgrade-from-latest-release workflow. The runtime probe added in #7796 stays — it still catches any other authenticate-hook plugin that rejects the test's plain-text credentials (e.g. a future ep_hash_auth-style hashed-only plugin). Reattribute its comment so future readers don't chase ep_hash_auth. Closes #7795. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#7789 un-hid
tests/backend/specs/admin/anonymizeAuthorSocket.tsfrom CI's mocha glob and immediately surfaced a 14-minute stall on every with-plugins matrix run (Linux + Windows + theUpgrade from latest releaseworkflow). Everysocket.emit/socket.onceround-trip on the/settingsadmin namespace hangs until mocha's 120s timeout fires; 7 tests × 120s ≈ 14 min.Root cause is a pre-existing interaction between
ep_hash_auth'shandleMessagehook and the/settingsnamespace dispatch: the hook fires for every socket message regardless of namespace and reads from the deprecatedclientcontext property, which isundefinedfor non-pad namespaces, so the response promise never resolves. Tracked separately in #7795.Fix
Gate the suite on
require.resolve('ep_hash_auth'). If the package is installed innode_modules, skip every test in the file viathis.skip()from thebeforehook. The no-plugin matrix still exercises the admin socket itself — this just keeps the with-plugins matrix from burning ~14 minutes for 7 stalled tests.Why
require.resolveand notpluginDefs.plugins[...]I tried
pluginDefs.plugins.ep_hash_authfirst. Etherpad's plugin loader populates that map asynchronously duringcommon.init; by the time we could read it in abeforehook the damage is done, and reading it before init returns the seed{}. Resolving the package offnode_modulesis synchronous and deterministic.Verification
Locally with no
ep_hash_authinnode_modules:With a stub
ep_hash_authpackage on disk thatrequire.resolvefinds:Refs
npx ENOENThalf is fixed by test(backend): fix Windows ENOENT in backend-tests-glob regression check #7794.🤖 Generated with Claude Code