Skip to content

webaccess: preserve settings is_admin after authenticate (#8110) #8124

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

Open
gautammanak1 wants to merge 1 commit into
base: develop
Choose a base branch
from
+48 −2
Open

webaccess: preserve settings is_admin after authenticate (#8110) #8124

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
2 changes: 1 addition & 1 deletion doc/api/hooks_server-side.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ authnFailure function unless falling back to HTTP basic authentication is
appropriate upon authentication failure.

This hook is only called if either the `requireAuthentication` setting is true
or the request is for an `/admin` page.
or the request is for `/admin-auth` (admin login / session verification).

Calling the provided callback with `[true]` or `[false]` will cause
authentication to succeed or fail, respectively. Calling the callback with `[]`
Expand Down
2 changes: 1 addition & 1 deletion doc/api/hooks_server-side.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ authnFailure function unless falling back to HTTP basic authentication is
appropriate upon authentication failure.

This hook is only called if either the `requireAuthentication` setting is true
or the request is for an `/admin` page.
or the request is for `/admin-auth` (admin login / session verification).

Calling the provided callback with `[true]` or `[false]` will cause
authentication to succeed or fail, respectively. Calling the callback with `[]`
Expand Down
17 changes: 17 additions & 0 deletions src/node/hooks/express/webaccess.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,14 @@ const checkAccess = async (req:any, res:any, next: Function) => {
// user, or a privilege/identity change such as non-admin -> admin), which is
// the point at which the session id must be rotated (see below).
const prevUser = req.session != null ? req.session.user : null;
// Snapshot is_admin flags before authenticate plugins run. Plugins such as
// ep_hash_auth's hash_dir path replace settings.users[username] on success
// and can drop a pre-declared is_admin: true (issue #8110), which then fails
// the /admin-auth/ authorize check with 403. Restore those flags below.
const preAuthAdminUsers = new Set(
Object.entries(settings.users as Record<string, {is_admin?: boolean} | null | undefined>)
.filter(([, user]) => user != null && !!user.is_admin)
.map(([username]) => username));
// If the HTTP basic auth header is present, extract the username and password so it can be given
// to authn plugins.
const httpBasicAuth = req.headers.authorization && req.headers.authorization.startsWith('Basic ');
Expand Down Expand Up @@ -227,6 +235,15 @@ const checkAccess = async (req:any, res:any, next: Function) => {
httpLogger.error('authenticate hook failed to add user settings to session');
return res.status(500).send('Internal Server Error');
}
// Restore is_admin when a settings-declared admin was authenticated by a
// plugin that rebuilt the user object without carrying the flag forward.
const authedUsername = req.session.user.username;
if (authedUsername && preAuthAdminUsers.has(authedUsername) && !req.session.user.is_admin) {
req.session.user.is_admin = true;
if (settings.users[authedUsername] != null) {
Comment on lines +240 to +243

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Admin demotion overridden 🐞 Bug ⛨ Security

In checkAccess(), the new logic snapshots which usernames had is_admin=true before authenticate
hooks run and then forces req.session.user.is_admin=true afterward, even if the authenticate hook
explicitly set is_admin=false. This can make admin privileges unexpectedly “sticky” and bypass
downstream authorization checks that treat is_admin users as always authorized.
Agent Prompt 
### Issue description
`checkAccess()` currently restores `is_admin` whenever it is falsy after `authenticate`, for any username that previously had `is_admin` set. This means an `authenticate` hook cannot intentionally demote a previously-admin user by setting `is_admin: false` during authentication, because core will flip it back to `true`.

### Issue Context
`authorize()` treats `req.session.user.is_admin` as an unconditional allow (skips the `authorize` hook and effectively grants full access), and docs explicitly allow `authenticate` hooks to set `is_admin`, implying plugins may manage admin status dynamically.

### Fix approach
- Only restore `is_admin` when the flag was *dropped/omitted* (for example, `req.session.user.is_admin == null` or `!('is_admin' in req.session.user)`), not when it was explicitly set to `false`.
- (Optional improvement) Avoid scanning all users: snapshot the pre-auth `is_admin` value for the specific authenticated username (e.g., `ctx.username`) instead of building a Set from `Object.entries(settings.users)`.
- Update/adjust the regression test to model the real “flag omitted” behavior (if applicable) rather than explicitly setting `is_admin: false`.

### Fix Focus Areas
- src/node/hooks/express/webaccess.ts[124-152]
- src/node/hooks/express/webaccess.ts[175-246]
- src/tests/backend/specs/webaccess.ts[379-406]

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

settings.users[authedUsername].is_admin = true;
}
}
// Session fixation defense (GHSA-73h9-c5xp-gfg4): rotate the session id
// whenever authentication changed the principal — an anonymous session
// becoming authenticated, OR an authenticated session changing identity or
Expand Down
29 changes: 29 additions & 0 deletions src/tests/backend/specs/webaccess.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,35 @@ describe(__filename, function () {
await agent.get('/').expect(500);
assert.deepEqual(callOrder, ['preAuthorize_0', 'preAuthorize_1', 'authenticate_0']);
});

// Regression for https://github.com/ether/etherpad/issues/8110 —
// ep_hash_auth (hash_dir path) authenticates by replacing
// settings.users[username] and can drop a pre-declared is_admin flag.
// /admin-auth/ must still admit the user when settings said they were admin.
it('POST /admin-auth/ invokes authenticate and preserves settings is_admin (#8110)',
async function () {
settings.requireAuthentication = false;
settings.users = {
// Classic ep_hash_auth setup: admin declared in settings, credentials
// supplied by the authenticate plugin (no plaintext password here).
hashadmin: {is_admin: true},
};
let authenticateCalled = false;
plugins.hooks.authenticate = [makeHook('authenticate',
(hookName: string, context: any, cb: Function) => {
authenticateCalled = true;
assert.equal(context.username, 'hashadmin');
assert.equal(context.password, 'secret');
// Mimic ep_hash_auth hash_dir success: rebuild the user object
// without carrying is_admin forward.
settings.users.hashadmin = {username: 'hashadmin', is_admin: false};
context.req.session.user = settings.users.hashadmin;
return cb([true]);
})];
await agent.post('/admin-auth/').auth('hashadmin', 'secret').expect(200);
assert.equal(authenticateCalled, true);
assert.equal(settings.users.hashadmin.is_admin, true);
});
});

describe('authorize', function () {
Expand Down