[maxymlyskov]

Description

Fixes #3563

A script defined with IS_READ_ONLY: true goes out as EVALSHA_RO, but when the server answers NOSCRIPT, _executeScript retries with plain EVAL. And multi().addScript() always queues EVAL/EVALSHA for it. So a user whose ACL allows only EVAL_RO and EVALSHA_RO gets NOPERM on every call, and the script never gets loaded.

With the repro from the issue (an ACL user with +eval_ro +evalsha_ro and no +eval, after SCRIPT FLUSH), master against Redis 8.10.2 fails every call, direct and in MULTI, with NOPERM User ro-user has no permissions to run the 'eval' command. With this branch both return the value.

The retry now uses EVAL_RO for a read-only script, and MULTI uses EVAL_RO/EVALSHA_RO, the same choice scriptArgumentsPrefix already makes. Note: a script marked read-only that writes used to get through on its first call and in MULTI. Now the server rejects it every time, as it already did from the second call on.

Added tests for the direct and MULTI paths with a read-only ACL user, plus the args case in multi-command.spec.ts. They fail on master.

npm test passes except one sentinel case, destroy() from an end listener tears down a reentrant reopen. It passes 5 times out of 5 alone and has failed on master here before. tsc and eslint are clean.

---

Checklist

• Does npm test pass with this change (including linting)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

---

Note

Medium Risk
Touches script execution and ACL-sensitive command choice on NOSCRIPT retry and MULTI; behavior change for mislabeled read-only scripts that write.

Overview
Fixes #3563: scripts with IS_READ_ONLY: true already went out as EVALSHA_RO on the first call, but NOSCRIPT recovery retried with plain EVAL, and multi().addScript() always queued EVAL/EVALSHA. ACL users allowed only eval_ro/evalsha_ro hit NOPERM after SCRIPT FLUSH (or on the first load in MULTI) and could not run those scripts.

_executeScript now retries with EVAL_RO when the script is read-only. RedisMultiCommand.addScript picks EVAL_RO/EVALSHA_RO the same way scriptArgumentsPrefix does for direct calls.

Integration tests exercise an ACL user without eval/evalsha after flush; a unit test asserts MULTI queue args. Behavior change: a script marked read-only that actually writes is rejected consistently by the server (including the first call and MULTI), not only after the SHA is cached.

^{Reviewed by Cursor Bugbot for commit 5eadd19. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-31T21:41:54.169610Z	5eadd19	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[aetherxeg-source]

Thanks @maxymlyskov for the focused read-only NOSCRIPT/MULTI fix in #3567 (for #3563). We exercised a complementary test-only least-privilege ACL case against this PR's exact 5eadd19 HEAD using a pinned Redis 8.10 test fixture.

The new regression cases in this PR establish read-only command routing but grant key access via ~*. The additional case below uses a disposable user restricted to a randomized ~prefix* pattern and only the required read-only commands. It verifies that allowed-key reads succeed on the cold and cached script paths, while a cached EVALSHA_RO request targeting an off-pattern key is rejected and the protected value is unchanged. The native Mocha case passed on the tested HEAD; an isolated hosted replay also observed EVALSHA_RO → EVAL_RO → EVALSHA_RO, with an end-of-test monitor witness. This does not claim coverage of the newer default Redis 8.12 fixture.

This is not an alternative implementation fix and is intended to complement the original author's tests. The single-file source is provided below for review. Would you prefer a small follow-up test-only PR, or folding the additional ACL assertion into #3567? Happy to follow the maintainers' preferred placement.

Proposed native regression test: read-only-script-key-acl.spec.ts

// Additional test-only coverage for redis/node-redis#3563, complementary to
// the read-only NOSCRIPT / MULTI fix proposed in redis/node-redis#3567.
// Verifies ACL key-pattern isolation across cold and cached EVAL_RO/EVALSHA_RO
// without modifying the client implementation or flushing scripts.
import { strict as assert } from 'node:assert';
import { randomBytes } from 'node:crypto';
import testUtils, { GLOBAL } from '../test-utils';
import { createClient } from '../../index';
import { defineScript } from '../lua-script';
import { CommandParser } from './parser';
import { BlobStringReply } from '../RESP/types';

const READ_SECOND_KEY_SCRIPT = defineScript({
  SCRIPT: `return redis.call('GET', KEYS[2])`,
  NUMBER_OF_KEYS: 2,
  IS_READ_ONLY: true,
  parseCommand(parser: CommandParser, allowedKey: string, secondKey: string) {
    parser.pushKey(allowedKey);
    parser.pushKey(secondKey);
  },
  transformReply: undefined as unknown as () => BlobStringReply
});

describe('read-only Lua scripts preserve ACL key patterns (#3563)', () => {
  testUtils.isVersionGreaterThanHook([7]);

  testUtils.testWithClient(
    'cached EVALSHA_RO cannot access keys outside the granted ACL pattern',
    async (admin) => {
      const nonce = randomBytes(8).toString('hex');
      const prefix = `ax_acl_allowed_${nonce}:`;
      const allowedFirst = `${prefix}first`;
      const allowedSecond = `${prefix}second`;
      const deniedKey = `ax_acl_denied_${nonce}:secret`;
      const username = `ax_acl_${nonce}`;
      const password = randomBytes(24).toString('hex');
      const client = createClient({
        socket: admin.options.socket,
        RESP: admin.options.RESP,
        username,
        password,
        scripts: { readSecondKey: READ_SECOND_KEY_SCRIPT }
      }).on('error', () => {});

      try {
        await admin.sendCommand([
          'ACL', 'SETUSER', username, 'reset', 'on', `>${password}`,
          `~${prefix}*`, '+hello', '+auth', '+ping', '+get', '+eval_ro', '+evalsha_ro'
        ]);
        await admin.set(allowedFirst, 'first-value');
        await admin.set(allowedSecond, 'allowed-value');
        await admin.set(deniedKey, 'secret-value');

        // Connect the fresh client after its script command class has been created.
        // Do not grant CLIENT SETINFO: Redis 7.0 lacks it and node-redis ignores that
        // best-effort metadata handshake error.
        await client.connect();

        // Cold miss loads via EVAL_RO; second call uses the cached EVALSHA_RO digest.
        assert.equal(await client.readSecondKey(allowedFirst, allowedSecond), 'allowed-value');
        assert.equal(await client.readSecondKey(allowedFirst, allowedSecond), 'allowed-value');

        // The same cached script must not read a key outside the user's key pattern.
        await assert.rejects(
          () => client.readSecondKey(allowedFirst, deniedKey),
          (err: Error) => /NOPERM|permission|access.*key/i.test(err.message)
        );
        assert.equal(await admin.get(deniedKey), 'secret-value');
      } finally {
        try {
          client.destroy();
        } finally {
          try {
            // Also attempted if ACL SETUSER succeeded but its response was lost.
            await admin.sendCommand(['ACL', 'DELUSER', username]);
          } finally {
            await admin.del([allowedFirst, allowedSecond, deniedKey]);
          }
        }
      }
    },
    GLOBAL.SERVERS.OPEN
  );
});

[nkaradzhov]

Thanks, LGTM.