[Bug] Fix False Positive Custom Auth Tests #881

Ansonhkg · 2025-09-05T12:39:33Z

WHAT

Custom auth E2E tests were producing false-positive results. It was working because it was using the cached Alice's delegation auth sig instead of using a fresh PKP.

CHANGES

Fixed the custom auth test to use Eve's PKP instead of Alice.
Updated the custom auth validation ipfs cid QmTdTemgWBYS76ACdZPttsve6edukyXjCdsNYNK1QDeXKY
Disabled some custom auth tests for the time being, will need to have follow up PRs for that.

TEST

This PR depends on the hardcoded keysets fixes here

bun test e2e/src/tickets/custom-auth.spec.ts --timeout 500000

- add missing `maxPrice` in the custom auth payload - e2e: fix custom auths

…alice-auth-manager-data`

…h tests

Ansonhkg · 2025-09-05T12:59:02Z

packages/networks/src/networks/vNaga/shared/factories/BaseModuleFactory.ts

@@ -871,6 +872,9 @@ export function createBaseModule<T, M>(config: BaseModuleConfig<T, M>) {
              litActionCode: requestBody.litActionCode,
              litActionIpfsId: requestBody.litActionIpfsId,
              jsParams: requestBody.jsParams,
+              maxPrice: getUserMaxPrice({


This was missing in the payload

Ansonhkg · 2025-09-05T13:01:25Z

e2e/src/helper/tests/execute-js.ts

@@ -3,7 +3,8 @@ import { assert } from '../assertions';

 export const createExecuteJsTest = (
  ctx: Awaited<ReturnType<typeof init>>,
-  getAuthContext: () => any
+  getAuthContext: () => any,
+  pubkey?: string


optional param for custom auth to pass in pub key instead of using alice's

Why not pass the litClient, chosen authContext and the publicKey?

the litClient doesn't have an authContext. The authContext is generated separately by an authManager that could generates:

eoa auth context

pkp auth context

custom pkp auth context

the getAuthContext function is either one of the above. If custom auth context is passed in by the call site, then we will need the pkp public key.

Ansonhkg · 2025-09-05T13:01:44Z

e2e/src/helper/tests/encrypt-decrypt-flow.ts

@@ -3,7 +3,8 @@ import { assert } from '../assertions';

 export const createEncryptDecryptFlowTest = (
  ctx: Awaited<ReturnType<typeof init>>,
-  getAuthContext: () => any
+  getAuthContext: () => any,
+  address?: string


optional param for custom auth to pass in address instead of using alice's

I think this function params (and similar ones) are quite confusing and easy to break: Caller has to pass a (practially untyped) function to pick an auth context from the same ctx that it passes, but that auth context is only used to select which of alice's address to use, but on only one case of those, I can override it with address

And to get into that, I had to read the whole function. Without doing that, it is completely impossible to understand what address does and when to use it for example or what getAuthContext has to be and do

This would improve if init returned something like { litClient, alice: { /* alice's stuff */ }, bob: { /* bob's stuff */ }, ... }} so we could pick bob's context with all its things and alice's address. Her context and the function to get it only increase this function complexity. And this function can properly express what it needs to receive and what its params are for without being restricted to what init returns

Things inside are also confusing, Why do we even need alice?

re: (practially untyped)
the type and untype concern will be addressed in a follow up PR after pnpm migration.

re: that auth context is only used to select which of alice's address to use

The address? field is added purely so custom auth (Eve’s) can override it.

re: ... { litClient, alice: { /* alice's stuff / }, bob: { / bob's stuff */ }

Some tests in this PR were commented out due to messiness caused by these implicit parameters. These were added as a temporary measure to prove that custom auth is working.

A follow-up PR is required to clean this up. [JSS-102]

FedericoAmura · 2025-09-10T16:54:23Z

e2e/src/helper/tests/view-pkps-by-address.ts

@@ -3,7 +3,8 @@ import { assert } from '../assertions';

 export const createViewPKPsByAddressTest = (
  ctx: Awaited<ReturnType<typeof init>>,
-  getAuthContext: () => any
+  getAuthContext: () => any,
+  pubkey?: string


Why adding the pubkey param here? And why we have getAuthContext if it is not used?

This slightly-different-but-shared interface between some of the functions in this directory is confusing. It is not similar for all files, and doesn't seem to be a reason for that. And is is not even similar on the ones that follow it, some have pubkey, others address

FedericoAmura · 2025-09-10T17:07:29Z

e2e/src/helper/tests/execute-js.ts

@@ -3,7 +3,8 @@ import { assert } from '../assertions';

 export const createExecuteJsTest = (
  ctx: Awaited<ReturnType<typeof init>>,
-  getAuthContext: () => any
+  getAuthContext: () => any,
+  pubkey?: string


Why not pass the litClient, chosen authContext and the publicKey?

FedericoAmura · 2025-09-10T17:12:58Z

e2e/src/helper/tests/encrypt-decrypt-flow.ts

@@ -3,7 +3,8 @@ import { assert } from '../assertions';

 export const createEncryptDecryptFlowTest = (
  ctx: Awaited<ReturnType<typeof init>>,
-  getAuthContext: () => any
+  getAuthContext: () => any,
+  address?: string


I think this function params (and similar ones) are quite confusing and easy to break: Caller has to pass a (practially untyped) function to pick an auth context from the same ctx that it passes, but that auth context is only used to select which of alice's address to use, but on only one case of those, I can override it with address

And to get into that, I had to read the whole function. Without doing that, it is completely impossible to understand what address does and when to use it for example or what getAuthContext has to be and do

This would improve if init returned something like { litClient, alice: { /* alice's stuff */ }, bob: { /* bob's stuff */ }, ... }} so we could pick bob's context with all its things and alice's address. Her context and the function to get it only increase this function complexity. And this function can properly express what it needs to receive and what its params are for without being restricted to what init returns

Things inside are also confusing, Why do we even need alice?

FedericoAmura · 2025-09-10T17:25:57Z

e2e/src/tickets/custom-auth.spec.ts

+// CONFIGURATION
+const NETWORK = 'naga-dev';
+const SITE_OWNER_ACCOUNT = privateKeyToAccount(
+  process.env['LIVE_MASTER_ACCOUNT'] as `0x${string}`


Can we unify LIVE_MASTER_ACCOUNT and LOCAL_MASTER_ACCOUNT? User should always use an account that corresponds with the target network regardless of it being local or public

LOCAL_MASTER_ACCOUNT is usually the first anvil account default private key that is automatically funded. If you change that to your own private key, you will still need to local private key to fund your own private key in order to run the local tests. That's why they are separated.

FedericoAmura · 2025-09-10T17:29:43Z

e2e/src/tickets/custom-auth.spec.ts

+    // (async () => {
+    //   const dAppUniqueAuthMethodType = "0x20b2c2163698c4ba8166450ff2378d96c009016deba048b9b125a696c74ea4b5";
+    //   const { pkpPublicKey, username, password, authMethodId } = jsParams;
+
+    //   // Custom validation logic for amazing-app-x35ju8
+    //   const EXPECTED_USERNAME = 'alice';
+    //   const EXPECTED_PASSWORD = 'lit';
+    //   const userIsValid = username === EXPECTED_USERNAME && password === EXPECTED_PASSWORD;
+
+    //   // Check PKP permissions
+    //   const tokenId = await Lit.Actions.pubkeyToTokenId({ publicKey: pkpPublicKey });
+    //   const permittedAuthMethods = await Lit.Actions.getPermittedAuthMethods({ tokenId });
+
+    //   const isPermitted = permittedAuthMethods.some((permittedAuthMethod) => {
+    //     return permittedAuthMethod["auth_method_type"] === dAppUniqueAuthMethodType &&
+    //           permittedAuthMethod["id"] === authMethodId;
+    //   });
+
+    //   const isValid = isPermitted && userIsValid;
+    //   LitActions.setResponse({ response: isValid ? "true" : "false" });
+    // })();
+    // ============================================================
+    const validationIpfsCid = 'QmP3ZoTSGQ2P9cAZ4pBUjmPC34xbJZFRoWnCXT2SdHA2uD';


FedericoAmura · 2025-09-10T17:33:11Z

e2e/src/tickets/custom-auth.spec.ts

+    for (const userId of [
+      'alice',
+      // , 'bob'
+    ]) {


Unnecessary loop

The idea of this for loop is from a mock site owner's perspective, not just a test, but servers as an example which also published in step 3 in the e2e test dapp and step 3 in the doc here

FedericoAmura · 2025-09-10T18:43:41Z

packages/networks/src/networks/vNaga/shared/factories/BaseModuleFactory.ts

@@ -898,23 +901,18 @@ export function createBaseModule<T, M>(config: BaseModuleConfig<T, M>) {
        handleResponse: async (
          result: z.infer<typeof GenericEncryptedPayloadSchema>,
          pkpPublicKey: Hex | string,
-          jitContext: NagaJitContext
+          jitContext: NagaJitContext,
+          requestId?: string


The only call to this method has requestId right above and is not passing it here. Let's make it required

yes, the requestId is added here purely for observation as part of the custom auth test. a separate PR is required to include the request id in all handlers.

[JSS-101]

Ansonhkg added 3 commits September 5, 2025 13:38

fix(custom-auth):

e1fdd87

- add missing `maxPrice` in the custom auth payload - e2e: fix custom auths

chore(.gitignore): add missing entries for lit-auth-artillery and `…

11178a5

…alice-auth-manager-data`

refactor(custom-auth): clean up comments and formatting in custom aut…

a19777b

…h tests

Ansonhkg added 🐞 Bug Fix Something isn't working v8 | Naga labels Sep 5, 2025

Ansonhkg had a problem deploying to Health Check September 5, 2025 12:39 — with GitHub Actions Failure

Ansonhkg commented Sep 5, 2025

View reviewed changes

Ansonhkg self-assigned this Sep 5, 2025

Ansonhkg changed the title ~~[Bug] Fix False Positive Custom Auth Tests~~ 3 - [Bug] Fix False Positive Custom Auth Tests Sep 5, 2025

Ansonhkg changed the title ~~3 - [Bug] Fix False Positive Custom Auth Tests~~ [Bug] Fix False Positive Custom Auth Tests Sep 5, 2025

FedericoAmura reviewed Sep 10, 2025

View reviewed changes

Ansonhkg added 2 commits September 12, 2025 23:29

fix: #881 (comment)

279a481

https://github.com/LIT-Protocol/js-sdk/pull/881#discussion_r2337344946

2bf0d96

Ansonhkg had a problem deploying to Health Check September 12, 2025 22:34 — with GitHub Actions Failure

Ansonhkg requested a review from FedericoAmura September 12, 2025 22:42

[Bug] Fix False Positive Custom Auth Tests #881

Are you sure you want to change the base?

[Bug] Fix False Positive Custom Auth Tests #881

Conversation

Ansonhkg commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHAT

CHANGES

TEST

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ansonhkg commented Sep 5, 2025 •

edited

Loading