Remove legacy code#507
Open
brionmario wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR consolidates the authentication client architecture across the Asgardeo JavaScript SDK by removing the legacy ChangesCore Client Refactoring and Unified Architecture
Browser Package Client and Middleware Refactoring
Node and Express Package Client Implementations
Framework-Specific Client Updates (Next.js, Nuxt, React, Vue)
Sequence Diagram(s)None generated—this PR is primarily an internal architectural refactoring that consolidates client implementations without introducing new end-user-facing features or significantly altering authentication control flows. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/node/src/LegacyAsgardeoNodeClient.ts (1)
317-322:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
reInitializeJSDoc return type to match implementation.Line 321 still documents
Promise<void>, but Line 335 returnsPromise<boolean>. This will publish inaccurate API docs.📝 Suggested doc fix
- * `@return` {Promise<void>} -A Promise that resolves with a void. + * `@return` {Promise<boolean>} - A Promise that resolves with whether re-initialization succeeded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/node/src/LegacyAsgardeoNodeClient.ts` around lines 317 - 322, The JSDoc for reInitialize in LegacyAsgardeoNodeClient is incorrect: it declares a return type of Promise<void> but the implementation returns Promise<boolean>; update the JSDoc return annotation to `@return` {Promise<boolean>} (or adjust the implementation to return void if intended) so the documented type matches the actual return value of the reInitialize method.packages/express/src/LegacyAsgardeoExpressClient.ts (1)
179-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
reInitializereturn contract with the delegated node client.Line 179 is typed as
Promise<void>, but delegates to a method that returnsPromise<boolean>. This drops the success signal and creates API inconsistency across packages.✅ Suggested fix
- public async reInitialize(config: Partial<AuthClientConfig>): Promise<void> { + public async reInitialize(config: Partial<AuthClientConfig>): Promise<boolean> { return this._authClient.reInitialize(config); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/express/src/LegacyAsgardeoExpressClient.ts` around lines 179 - 181, The reInitialize method in LegacyAsgardeoExpressClient currently returns Promise<void> but delegates to _authClient.reInitialize which returns Promise<boolean>, losing the success signal; change the method signature of reInitialize to return Promise<boolean> and simply return the result of this._authClient.reInitialize(config) so the boolean success value is preserved (adjust any callers or types if needed) — verify the class LegacyAsgardeoExpressClient and the delegated _authClient.reInitialize usage are updated accordingly.
🟡 Minor comments (6)
packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts-53-71 (1)
53-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
elsebranch lacks the sameaccess_token/id_tokenpresence checks as theifbranch.When
sourceInstanceIdis null,sessionDatais taken from default storage without any validation. If a caller invokes this before sign-in or afterclearSession, the chained.replace(...)will passundefinedintogetAuthenticatedUserInfo/string operations and surface as a confusingTypeErrorinstead of anAsgardeoAuthException. Mirror the source-instance guard for the default path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts` around lines 53 - 71, The else branch that reads sessionData from storageManager.getSessionData(userId) must perform the same presence/validation checks for sessionData.access_token and sessionData.id_token as the source-instance path; update the code in replaceCustomGrantTemplateTags.ts to validate sessionData.access_token and sessionData.id_token after await storageManager.getSessionData(...) and before calling getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token) or using sessionData.access_token, and if either is missing throw the same AsgardeoAuthException (or the same error type/handler used in the if branch) so subsequent .replace(...) calls do not receive undefined values.packages/javascript/src/api/loadOpenIDProviderConfiguration.ts-53-66 (1)
53-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOriginal fetch error is discarded; the
AsgardeoAuthExceptioncarries no actionable context.
throw new Error()on line 58 immediately falls through to the catch which builds an exception without referencing the original error, response status, status text, or response body. Surface that information so the exception is debuggable.🔧 Suggested fix
- try { - response = await fetch(resolvedWellKnownEndpoint); - if (response.status !== 200 || !response.ok) { - throw new Error(); - } - } catch { + let originalError: unknown; + try { + response = await fetch(resolvedWellKnownEndpoint); + if (!response.ok) { + originalError = `HTTP ${response.status} ${response.statusText}`; + throw new Error(originalError as string); + } + } catch (error) { + originalError = originalError ?? error; throw new AsgardeoAuthException( 'JS-AUTH_CORE-GOPMD-HE01', 'Invalid well-known response', - 'The well known endpoint response has been failed with an error.', + `The well-known endpoint request failed: ${String(originalError)}`, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/api/loadOpenIDProviderConfiguration.ts` around lines 53 - 66, The fetch error and response details are being discarded; modify the loadOpenIDProviderConfiguration code so that you capture the original fetch error and include useful context when throwing AsgardeoAuthException: when fetch throws, catch the original error (e.g., err) and include err.message; when response exists but response.status !== 200 || !response.ok, read response.status, response.statusText and await response.text() (or response.json() guardedly) and include those values in the AsgardeoAuthException so the exception payload contains the endpoint (resolvedWellKnownEndpoint), HTTP status, statusText and any response body or original error message for debugging.packages/javascript/src/api/requestAccessToken.ts-86-93 (1)
86-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPKCE
code_verifieris interpolated via a template literal and silently becomes"undefined"when missing.If
getTemporaryDataParameter(...)resolves toundefined/null(e.g. session storage was cleared between authorize and token exchange, or PKCE key derivation drifted), this code happily sendscode_verifier=undefinedto the token endpoint, producing an opaque OAuthinvalid_grantinstead of a clear SDK error.Read it into a variable, validate, and only then attach it.
🔧 Suggested fix
if (configData.enablePKCE) { - body.set( - 'code_verifier', - `${await storageManager.getTemporaryDataParameter(extractPkceStorageKeyFromState(state), userId)}`, - ); - - await storageManager.removeTemporaryDataParameter(extractPkceStorageKeyFromState(state), userId); + const pkceKey: string = extractPkceStorageKeyFromState(state); + const codeVerifier = await storageManager.getTemporaryDataParameter(pkceKey, userId); + + if (!codeVerifier) { + throw new AsgardeoAuthException( + 'JS-AUTH_CORE-RAT1-NF04', + 'PKCE code verifier not found.', + 'The PKCE code verifier could not be retrieved from storage for the given state.', + ); + } + + body.set('code_verifier', String(codeVerifier)); + await storageManager.removeTemporaryDataParameter(pkceKey, userId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/api/requestAccessToken.ts` around lines 86 - 93, In requestAccessToken when configData.enablePKCE is true, don't inline the await into the template literal; instead call storageManager.getTemporaryDataParameter(extractPkceStorageKeyFromState(state), userId) into a variable, validate that the returned codeVerifier is neither null nor undefined and throw or return a clear SDK error if missing, and only then call body.set('code_verifier', codeVerifier) and storageManager.removeTemporaryDataParameter(...); this ensures you don't send "undefined" to the token endpoint and provides a helpful error when the PKCE verifier is absent.packages/javascript/src/AsgardeoJavaScriptClient.ts-443-453 (1)
443-453:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInstance
clearSessionmethod fires-and-forgets the async helper without awaiting.The instance method (lines 443–448) invokes
clearSession()withoutawait, causing the method to return immediately while the async operation runs in the background. Storage write failures are silently swallowed, and callers cannot reliably sequence anything that depends on the session being cleared (e.g., redirect to sign-in). The static variant properly awaits the helper and relies on the_storageManagersingleton.Make the instance method
asyncandawaittheclearSession()call:Suggested fix
- public clearSession(sessionId?: string): void { + public async clearSession(sessionId?: string): Promise<void> { if (this.storageManager) { - clearSession(this.storageManager, sessionId); + await clearSession(this.storageManager, sessionId); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/AsgardeoJavaScriptClient.ts` around lines 443 - 453, The instance method clearSession currently calls the async helper without awaiting; change the instance method declaration to async (public async clearSession(sessionId?: string): Promise<void>) and await the helper call (await clearSession(this.storageManager, sessionId)); ensure the method returns a Promise<void> like the static variant and keep the null-check on this.storageManager before awaiting so errors from the storage write surface to callers (same helper clearSession and static clearSession references).packages/browser/src/AsgardeoSPAClient.ts-1279-1279 (1)
1279-1279:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect JSDoc
@memberofannotation.The
@memberoftag incorrectly referencesAsgardeoJavaScriptClient, but this method belongs to theAsgardeoSPAClientclass. This will break documentation generation and mislead developers.📝 Proposed fix
- * `@memberof` AsgardeoJavaScriptClient + * `@memberof` AsgardeoSPAClient🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/browser/src/AsgardeoSPAClient.ts` at line 1279, The JSDoc for the method incorrectly uses `@memberof` AsgardeoJavaScriptClient instead of the actual class name; update the JSDoc on the method in the AsgardeoSPAClient class so the `@memberof` annotation reads AsgardeoSPAClient (locate the incorrect tag near the AsgardeoSPAClient method declaration and replace the member reference accordingly) to restore correct documentation generation.packages/browser/src/AsgardeoSPAClient.ts-917-917 (1)
917-917:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect JSDoc
@memberofannotation.The
@memberoftag incorrectly referencesAsgardeoJavaScriptClient, but this method belongs to theAsgardeoSPAClientclass. This will break documentation generation and mislead developers.📝 Proposed fix
- * `@memberof` AsgardeoJavaScriptClient + * `@memberof` AsgardeoSPAClient🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/browser/src/AsgardeoSPAClient.ts` at line 917, The JSDoc block incorrectly tags the method as belonging to AsgardeoJavaScriptClient; update the JSDoc `@memberof` annotation to `AsgardeoSPAClient` so the documentation correctly associates the method with the AsgardeoSPAClient class (search for the JSDoc above the method inside the AsgardeoSPAClient class in AsgardeoSPAClient.ts and replace `@memberof AsgardeoJavaScriptClient` with `@memberof AsgardeoSPAClient`, and check for any other identical mis-tagged JSDoc blocks to correct).
🧹 Nitpick comments (8)
packages/javascript/src/AsgardeoJavaScriptClient.ts (2)
202-209: 💤 Low valuePKCE getter from
cryptoHelperis called with optional chaining — masks an early hard failure.
this.cryptoHelperis initialized unconditionally in the constructor (line 99). The?.on lines 202-203 looks defensive but is misleading: if it were truly undefined here,getCodeVerifier()returningundefinedwould silently propagate into storage and produce a downstream "invalid PKCE" failure at the token endpoint instead of an actionable error. Drop the?.so a real bug here fails loudly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/AsgardeoJavaScriptClient.ts` around lines 202 - 209, The PKCE getters are being called with optional chaining which masks a missing crypto helper and can lead to silent downstream failures; remove the optional chaining on this.cryptoHelper when calling getCodeVerifier() and getCodeChallenge(codeVerifier) so that if cryptoHelper is unexpectedly undefined the code throws immediately, and ensure the returned codeVerifier is validated (non-null) before calling storageManager.setTemporaryDataParameter(pkceKey, codeVerifier, userId) to avoid storing an invalid PKCE value.
460-460: 💤 Low valueType mismatch: passing
Record<string, string>whereExtendedAuthorizeRequestUrlParamsexpects values of typestring | boolean.Both
getAgentToken(line 456) andgetOBOSignInURL(line 506) declarecustomParamasRecord<string, string>, butgetSignInUrl()expectsExtendedAuthorizeRequestUrlParams, which is defined asKnownExtendedAuthorizeRequestUrlParams & Record<string, string | boolean>. The value type mismatch (stringvsstring | boolean) allows the code to compile due to structural typing, but this masks potential typos in property names likeresponse_modevsresponseMode. Explicitly typingcustomParamasExtendedAuthorizeRequestUrlParamswould improve type safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/AsgardeoJavaScriptClient.ts` at line 460, The local variable/parameter customParam used in getAgentToken and getOBOSignInURL is typed as Record<string,string> but passed into getSignInUrl which expects ExtendedAuthorizeRequestUrlParams (values string | boolean); change the declarations/signatures of customParam in both getAgentToken and getOBOSignInURL to ExtendedAuthorizeRequestUrlParams so type-checking catches incorrect keys/boolean values, update any callers to pass that type, and run TypeScript checks to fix any resulting type errors; references: getAgentToken, getOBOSignInURL, getSignInUrl, ExtendedAuthorizeRequestUrlParams, customParam.packages/javascript/src/api/exchangeToken.ts (2)
63-69: ⚡ Quick winBody construction relies on un-encoded substitutions; consider building with
URLSearchParams.Each
valueis passed throughreplaceCustomGrantTemplateTags, which returns raw strings (see root-cause comment on that utility), and then joined with&to form the body. Even after fixing encoding inside the helper, building the body viaURLSearchParams.append(key, newValue)would be more robust to non-stringkey/valuepairs inconfig.dataand stay consistent withrequestAccessToken.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/api/exchangeToken.ts` around lines 63 - 69, The body is built by joining raw substituted strings which risks improper URL-encoding and non-string values; update the construction inside exchangeToken.ts to use URLSearchParams: iterate Object.entries(config.data), call replaceCustomGrantTemplateTags(storageManager, cryptoHelper, value as string, userId) for each value, and use params.append(String(key), String(newValue)) (or handle null/undefined appropriately) instead of returning `${key}=${newValue}`; this will mirror the safer approach used in requestAccessToken.ts and ensure proper encoding for all keys/values.
102-108: 💤 Low valueRedundant
response.status !== 200check and mistyped JSON payload.
!response.okalready covers non-2xx; thestatus !== 200half rejects valid 2xx responses such as 201/204.(await response.json()) as stringmistypes the OAuth error body, which is a JSON object — stringify it (or type it accurately) before passing as the exception description.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/api/exchangeToken.ts` around lines 102 - 108, Remove the redundant explicit status check and rely on response.ok (so drop `response.status !== 200`) and fix the mistyped JSON payload passed to AsgardeoAuthException by passing a stringified or correctly typed JSON body instead of `(await response.json()) as string` (e.g., `JSON.stringify(await response.json())` or cast to an appropriate error object) when constructing the AsgardeoAuthException for the `response` handling in exchangeToken.ts.packages/javascript/src/api/refreshAccessToken.ts (1)
84-90: ⚡ Quick win
tokenResponse.json() as stringis mistyped; OAuth error responses are JSON objects.Per RFC 6749 §5.2 the error body is
{ "error": ..., "error_description": ..., ... }. Casting tostringhides this from callers and ends up serialized as[object Object]if anyone string-concatenates it. Either type it as the OAuth error object, orJSON.stringifyit for the exception description.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/api/refreshAccessToken.ts` around lines 84 - 90, The code throws AsgardeoAuthException using (await tokenResponse.json()) cast to string; instead parse the JSON into the proper OAuth error object and pass either the structured object or a stringified version to the exception. Update the tokenResponse handling in refreshAccessToken: await tokenResponse.json() into a const (e.g., oauthError), then call new AsgardeoAuthException('JS-AUTH_CORE-RAT2-HE04', `Refreshing access token failed: ${JSON.stringify(oauthError)}`, oauthError) (or type oauthError to an interface matching RFC6749 fields) so callers get the actual JSON error rather than “[object Object]”.packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts (1)
34-61: 💤 Low valueMove the
typeof textguard above the storage/scope work.Both
getConfigData()andgetSessionData()are awaited before the early return on line 60. If the function is called with a non-stringtext, that I/O is wasted (and may even throw before we get the chance to short-circuit). Hoist the type check to the top of the function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts` around lines 34 - 61, The typeof text guard should be moved to the very start of replaceCustomGrantTemplateTags (before any awaits) so non-string inputs short-circuit without calling storageManager.getConfigData(), storageManager.getSessionData(), or processOpenIDScopes; reorder the function to check "if (typeof text !== 'string') return text;" first, then proceed to call getConfigData(), derive sourceInstanceId/instanceKey, await getSessionData(userId[, instanceKey]) and finally call processOpenIDScopes(configData.scopes).packages/javascript/src/api/loadOpenIDProviderConfiguration.ts (2)
57-57: 💤 Low value
response.status !== 200is redundant with!response.okand is overly strict.
response.okalready covers the 2xx range. The extrastatus !== 200clause unnecessarily rejects valid 2xx responses (e.g. 204, though uncommon for discovery). Drop the status comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/api/loadOpenIDProviderConfiguration.ts` at line 57, The conditional in loadOpenIDProviderConfiguration that checks "response.status !== 200 || !response.ok" is too strict and redundant; replace it with a single "!response.ok" check so all successful 2xx responses are accepted. Locate the fetch/response handling in loadOpenIDProviderConfiguration (the block using response.status/response.ok) and remove the "response.status !== 200" part, updating any error message to reference response.status or response.statusText when logging the failure.
44-99: 💤 Low value
configData as anyand triplicated "persist metadata + set initiated flag" block.Two refinements:
- Line 44 and line 77 cast to
anybecausewellKnownEndpoint,discovery,baseUrl,endpoints, andplatformaren't part ofStrictAuthClientConfig. Extend the type (or define a discovery-specific shape) so this loader is type-safe.- The pattern
setOIDCProviderMetaData(...) → setTemporaryDataParameter(OPENID_PROVIDER_CONFIG_INITIATED, true) → returnis repeated three times. Extracting a helper would make the control flow clearer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/src/api/loadOpenIDProviderConfiguration.ts` around lines 44 - 99, Update loadOpenIDProviderConfiguration to avoid casting configData to any and to remove duplicated persistence logic: define/extend a typed interface (e.g., OIDCDiscoveryConfig or extend StrictAuthClientConfig) that includes wellKnownEndpoint, platform, discovery, baseUrl, and endpoints and use it instead of casting; keep the existing resolution branches that call resolveEndpoints, resolveEndpointsByBaseURL and resolveEndpointsExplicitly, but extract the repeated sequence storageManager.setOIDCProviderMetaData(...) followed by storageManager.setTemporaryDataParameter(OIDCDiscoveryConstants.Storage.StorageKeys.OPENID_PROVIDER_CONFIG_INITIATED, true) into a small helper function (e.g., persistProviderMetaData) and call it from each branch after awaiting the appropriate resolve* function; ensure exception handling around fetch/resolve remains unchanged and remove the cast-to-any usages (reference symbols: loadOpenIDProviderConfiguration, StrictAuthClientConfig, resolveEndpoints, resolveEndpointsByBaseURL, resolveEndpointsExplicitly, storageManager.setOIDCProviderMetaData, storageManager.setTemporaryDataParameter, OIDCDiscoveryConstants.Storage.StorageKeys.OPENID_PROVIDER_CONFIG_INITIATED).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/express/src/middleware/authentication.ts`:
- Around line 31-34: The code incorrectly uses Express APIs: replace the
constructor call new express.Router() with the factory function
express.Router(), and fix the type name express.nextFunction to
express.NextFunction wherever used (e.g., in the middleware signature passed to
router.use and any other middleware definitions in this file such as the
handlers that reference next). Update these references for the router variable
and all middleware function parameter typings (lines noted in review) so the
code compiles with Express TypeScript definitions.
In `@packages/javascript/src/api/handleTokenResponse.ts`:
- Around line 56-58: The current call to validateIdToken runs whenever
shouldValidateIdToken is true even if parsedResponse.id_token is undefined,
causing failures for flows that don't return an id_token; update the block
around validateIdToken(storageManager, cryptoHelper, parsedResponse.id_token) to
first check that parsedResponse.id_token is present (truthy) and only call
validateIdToken when it exists, otherwise skip validation (or handle the absence
explicitly if you prefer to throw a specific error); ensure you reference
shouldValidateIdToken, parsedResponse.id_token, and validateIdToken when making
the change.
In `@packages/javascript/src/api/refreshAccessToken.ts`:
- Line 32: refreshAccessToken currently reads token_endpoint from
storageManager.loadOpenIDProviderConfiguration() without ensuring discovery has
run, so cold refreshes can see an empty endpoint; update refreshAccessToken to
follow the same guard used in requestAccessToken.ts and exchangeToken.ts by
checking OPENID_PROVIDER_CONFIG_INITIATED and calling
loadOpenIDProviderConfiguration(storageManager, false) when not set before
reading token_endpoint, ensuring tokenEndpoint is populated and avoiding the
erroneous "No refresh token endpoint found" path.
- Around line 54-62: The refresh-token request currently builds the body via raw
template literals (body array) which fails to URL-encode opaque values like
sessionData.refresh_token and configData.clientSecret; replace the string[] body
construction with a URLSearchParams instance (e.g., create params = new
URLSearchParams()), use params.append('client_id', configData.clientId),
params.append('refresh_token', sessionData.refresh_token),
params.append('grant_type', 'refresh_token') and conditionally
params.append('client_secret', configData.clientSecret) so the refresh token and
client secret are properly percent-encoded before sending (use params.toString()
or pass params directly as the request body).
In `@packages/javascript/src/api/revokeAccessToken.ts`:
- Around line 42-50: The revocation request builds an
application/x-www-form-urlencoded body by concatenating raw values which can
break on reserved characters; update the body construction in revokeAccessToken
to URL-encode form values (use encodeURIComponent or equivalent) for client_id,
token (from await storageManager.getSessionData(userId).access_token), and
client_secret whenever added, and ensure you do the same change for the other
identical block referenced at lines 55-57 so all form parameters are encoded
before joining into the request body.
In `@packages/javascript/src/api/validateIdToken.ts`:
- Around line 57-62: In validateIdToken, do not assume the JWKS error body is
JSON; when response.status !== 200 (and !response.ok) wrap parsing in a
try/catch: first attempt await response.json(), but on failure fall back to
await response.text() (or response.statusText) and pass that safe string into
the AsgardeoAuthException constructor so you still throw a structured error;
update the error creation site that currently calls (await response.json()) to
use this try/catch-backed body variable.
In `@packages/javascript/src/AsgardeoJavaScriptClient.ts`:
- Around line 95-106: The constructor currently calls the async
_initStorage(...) without awaiting, causing storageManager to be unset for
synchronous calls; fix by creating and assigning a ready Promise in the
constructor (e.g. this.ready = this._initStorage(config, cacheStore) when config
is present) and update public methods that access storageManager (e.g.
getSignInUrl, any other methods reading storageManager) to await this.ready at
entry, or alternatively remove the constructor invocation and require callers to
call initialize() which already awaits _initStorage; reference symbols:
constructor, _initStorage, initialize, storageManager, getSignInUrl, ready.
- Around line 93-126: The class currently writes to a process-wide static
AsgardeoJavaScriptClient._storageManager in _initStorage which causes
cross-instance interference; change this by removing the static field and either
(A) make clearSession and all callers use the instance-level storageManager
(convert static clearSession into an instance method and update
SPAUtils/session-management-helper to call instance.clearSession(sessionId)), or
(B) introduce a registry (e.g., a Map keyed by instanceId or clientId) that
_initStorage registers this.storageManager into and have static clearSession
look up the correct manager by instanceId before operating; update all
references to use the chosen approach and ensure _storageManager static is no
longer globally overwritten.
- Around line 531-593: Several public AsgardeoClient methods in
AsgardeoJavaScriptClient (switchOrganization, getAllOrganizations,
getMyOrganizations, getCurrentOrganization, getUserProfile, isLoading,
updateUserProfile, getConfiguration, signInSilently, setSession, signIn,
signOut, signUp) are stubs that throw a generic Error and will crash at runtime;
replace those generic throws with throwing a typed AsgardeoAuthException
(include a stable error code like "NOT_IMPLEMENTED_IN_JS_CLIENT" and a clear
message) so callers can handle the missing implementation reliably, and ensure
the exception class used (AsgardeoAuthException) is imported/defined and
documented in the PR/changelog; alternatively prefer making these methods
optional on the AsgardeoClient interface if you want to avoid implementing them
here, but if you keep them, change each method body in AsgardeoJavaScriptClient
to throw the typed exception instead of new Error.
- Around line 339-343: The getDecodedIdToken method currently prefers the stored
token over the explicit idToken parameter, making callers' provided idToken
ignored; change the precedence so the explicit idToken parameter is used first
(i.e., use idToken ?? storedIdToken) or remove the idToken parameter if it isn't
meant to override storage; update the return to pass the resolved token into
this.cryptoHelper.decodeJwtToken<IdToken>, and ensure storedIdToken is still
fetched via this.storageManager.getSessionData(userId) for fallback and
null/undefined-safe handling.
- Around line 433-441: The reInitialize method is awaiting neither the
asynchronous storageManager.getConfigData() nor handling its Promise, so
deepMerge is merging a Promise into the config; change reInitialize to await
storageManager.getConfigData() to obtain the actual currentConfig before calling
deepMerge, then pass the merged object to storageManager.setConfigData and
continue to await loadOpenIDProviderConfiguration; reference the reInitialize
method, storageManager.getConfigData, deepMerge, setConfigData, and
loadOpenIDProviderConfiguration to locate and update the code.
In `@packages/javascript/src/utils/getAuthenticatedUserInfo.ts`:
- Around line 26-31: The username derivation in getAuthenticatedUserInfo.ts is
too strict: update how the username variable is computed so it falls back to
common OIDC claims; instead of only using payload['username'], derive username
from payload['username'] ?? payload['preferred_username'] ?? payload['sub'] ??
''. Keep existing fullName/displayName logic but ensure displayName still falls
back to fullName if preferred_username is missing.
In `@packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts`:
- Around line 63-71: The replacements in replaceCustomGrantTemplateTags are
inserted directly into an x-www-form-urlencoded body and must be URL-encoded;
update the function to encode each substituted value (access_token, username
from getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token), scope,
configData.clientId, and configData.clientSecret ?? '') using encodeURIComponent
before calling .replace for the corresponding
TokenExchangeConstants.Placeholders so the resulting payload is safe against
characters like =, &, +, and / (alternatively, switch callers such as
exchangeToken to build the body with URLSearchParams, but the quick fix is to
wrap each replacement value with encodeURIComponent inside
replaceCustomGrantTemplateTags).
In `@packages/javascript/src/utils/resolveEndpoints.ts`:
- Around line 31-35: The PascalCase-to-snake_case conversion in
resolveEndpoints.ts is adding a leading underscore when endpointName starts with
an uppercase letter (e.g., AuthorizationEndpoint -> _authorization_endpoint);
update the conversion logic for snakeCasedName (used when populating
oidcProviderMetaData from configData.endpoints) so it does not produce a leading
underscore — either by lowercasing the first character before inserting
underscores for internal capitals or by stripping a leading underscore after the
current replace; ensure you continue to use endpointName, snakeCasedName and
oidcProviderMetaData in the same assignment.
In `@packages/javascript/src/utils/resolveEndpointsExplicitly.ts`:
- Around line 42-52: The snake_case conversion currently prefixes an underscore
for initial capitals (e.g., AuthorizationEndpoint -> _authorization_endpoint)
because the replace callback doesn't check the character index; update the
conversion used when building snakeCasedName (in the isRequiredEndpointsContains
logic and the similar conversion around lines 67-70) to use the replace
callback's index parameter and only prepend '_' when index > 0, for example:
replace(/[A-Z]/g, (letter, idx) => (idx ? '_' : '') + letter.toLowerCase());
ensure both occurrences (the one inside the requiredEndpoints.every check and
the other occurrence noted) are updated so matching and output keys are correct.
---
Outside diff comments:
In `@packages/express/src/LegacyAsgardeoExpressClient.ts`:
- Around line 179-181: The reInitialize method in LegacyAsgardeoExpressClient
currently returns Promise<void> but delegates to _authClient.reInitialize which
returns Promise<boolean>, losing the success signal; change the method signature
of reInitialize to return Promise<boolean> and simply return the result of
this._authClient.reInitialize(config) so the boolean success value is preserved
(adjust any callers or types if needed) — verify the class
LegacyAsgardeoExpressClient and the delegated _authClient.reInitialize usage are
updated accordingly.
In `@packages/node/src/LegacyAsgardeoNodeClient.ts`:
- Around line 317-322: The JSDoc for reInitialize in LegacyAsgardeoNodeClient is
incorrect: it declares a return type of Promise<void> but the implementation
returns Promise<boolean>; update the JSDoc return annotation to `@return`
{Promise<boolean>} (or adjust the implementation to return void if intended) so
the documented type matches the actual return value of the reInitialize method.
---
Minor comments:
In `@packages/browser/src/AsgardeoSPAClient.ts`:
- Line 1279: The JSDoc for the method incorrectly uses `@memberof`
AsgardeoJavaScriptClient instead of the actual class name; update the JSDoc on
the method in the AsgardeoSPAClient class so the `@memberof` annotation reads
AsgardeoSPAClient (locate the incorrect tag near the AsgardeoSPAClient method
declaration and replace the member reference accordingly) to restore correct
documentation generation.
- Line 917: The JSDoc block incorrectly tags the method as belonging to
AsgardeoJavaScriptClient; update the JSDoc `@memberof` annotation to
`AsgardeoSPAClient` so the documentation correctly associates the method with
the AsgardeoSPAClient class (search for the JSDoc above the method inside the
AsgardeoSPAClient class in AsgardeoSPAClient.ts and replace `@memberof
AsgardeoJavaScriptClient` with `@memberof AsgardeoSPAClient`, and check for any
other identical mis-tagged JSDoc blocks to correct).
In `@packages/javascript/src/api/loadOpenIDProviderConfiguration.ts`:
- Around line 53-66: The fetch error and response details are being discarded;
modify the loadOpenIDProviderConfiguration code so that you capture the original
fetch error and include useful context when throwing AsgardeoAuthException: when
fetch throws, catch the original error (e.g., err) and include err.message; when
response exists but response.status !== 200 || !response.ok, read
response.status, response.statusText and await response.text() (or
response.json() guardedly) and include those values in the AsgardeoAuthException
so the exception payload contains the endpoint (resolvedWellKnownEndpoint), HTTP
status, statusText and any response body or original error message for
debugging.
In `@packages/javascript/src/api/requestAccessToken.ts`:
- Around line 86-93: In requestAccessToken when configData.enablePKCE is true,
don't inline the await into the template literal; instead call
storageManager.getTemporaryDataParameter(extractPkceStorageKeyFromState(state),
userId) into a variable, validate that the returned codeVerifier is neither null
nor undefined and throw or return a clear SDK error if missing, and only then
call body.set('code_verifier', codeVerifier) and
storageManager.removeTemporaryDataParameter(...); this ensures you don't send
"undefined" to the token endpoint and provides a helpful error when the PKCE
verifier is absent.
In `@packages/javascript/src/AsgardeoJavaScriptClient.ts`:
- Around line 443-453: The instance method clearSession currently calls the
async helper without awaiting; change the instance method declaration to async
(public async clearSession(sessionId?: string): Promise<void>) and await the
helper call (await clearSession(this.storageManager, sessionId)); ensure the
method returns a Promise<void> like the static variant and keep the null-check
on this.storageManager before awaiting so errors from the storage write surface
to callers (same helper clearSession and static clearSession references).
In `@packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts`:
- Around line 53-71: The else branch that reads sessionData from
storageManager.getSessionData(userId) must perform the same presence/validation
checks for sessionData.access_token and sessionData.id_token as the
source-instance path; update the code in replaceCustomGrantTemplateTags.ts to
validate sessionData.access_token and sessionData.id_token after await
storageManager.getSessionData(...) and before calling
getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token) or using
sessionData.access_token, and if either is missing throw the same
AsgardeoAuthException (or the same error type/handler used in the if branch) so
subsequent .replace(...) calls do not receive undefined values.
---
Nitpick comments:
In `@packages/javascript/src/api/exchangeToken.ts`:
- Around line 63-69: The body is built by joining raw substituted strings which
risks improper URL-encoding and non-string values; update the construction
inside exchangeToken.ts to use URLSearchParams: iterate
Object.entries(config.data), call replaceCustomGrantTemplateTags(storageManager,
cryptoHelper, value as string, userId) for each value, and use
params.append(String(key), String(newValue)) (or handle null/undefined
appropriately) instead of returning `${key}=${newValue}`; this will mirror the
safer approach used in requestAccessToken.ts and ensure proper encoding for all
keys/values.
- Around line 102-108: Remove the redundant explicit status check and rely on
response.ok (so drop `response.status !== 200`) and fix the mistyped JSON
payload passed to AsgardeoAuthException by passing a stringified or correctly
typed JSON body instead of `(await response.json()) as string` (e.g.,
`JSON.stringify(await response.json())` or cast to an appropriate error object)
when constructing the AsgardeoAuthException for the `response` handling in
exchangeToken.ts.
In `@packages/javascript/src/api/loadOpenIDProviderConfiguration.ts`:
- Line 57: The conditional in loadOpenIDProviderConfiguration that checks
"response.status !== 200 || !response.ok" is too strict and redundant; replace
it with a single "!response.ok" check so all successful 2xx responses are
accepted. Locate the fetch/response handling in loadOpenIDProviderConfiguration
(the block using response.status/response.ok) and remove the "response.status
!== 200" part, updating any error message to reference response.status or
response.statusText when logging the failure.
- Around line 44-99: Update loadOpenIDProviderConfiguration to avoid casting
configData to any and to remove duplicated persistence logic: define/extend a
typed interface (e.g., OIDCDiscoveryConfig or extend StrictAuthClientConfig)
that includes wellKnownEndpoint, platform, discovery, baseUrl, and endpoints and
use it instead of casting; keep the existing resolution branches that call
resolveEndpoints, resolveEndpointsByBaseURL and resolveEndpointsExplicitly, but
extract the repeated sequence storageManager.setOIDCProviderMetaData(...)
followed by
storageManager.setTemporaryDataParameter(OIDCDiscoveryConstants.Storage.StorageKeys.OPENID_PROVIDER_CONFIG_INITIATED,
true) into a small helper function (e.g., persistProviderMetaData) and call it
from each branch after awaiting the appropriate resolve* function; ensure
exception handling around fetch/resolve remains unchanged and remove the
cast-to-any usages (reference symbols: loadOpenIDProviderConfiguration,
StrictAuthClientConfig, resolveEndpoints, resolveEndpointsByBaseURL,
resolveEndpointsExplicitly, storageManager.setOIDCProviderMetaData,
storageManager.setTemporaryDataParameter,
OIDCDiscoveryConstants.Storage.StorageKeys.OPENID_PROVIDER_CONFIG_INITIATED).
In `@packages/javascript/src/api/refreshAccessToken.ts`:
- Around line 84-90: The code throws AsgardeoAuthException using (await
tokenResponse.json()) cast to string; instead parse the JSON into the proper
OAuth error object and pass either the structured object or a stringified
version to the exception. Update the tokenResponse handling in
refreshAccessToken: await tokenResponse.json() into a const (e.g., oauthError),
then call new AsgardeoAuthException('JS-AUTH_CORE-RAT2-HE04', `Refreshing access
token failed: ${JSON.stringify(oauthError)}`, oauthError) (or type oauthError to
an interface matching RFC6749 fields) so callers get the actual JSON error
rather than “[object Object]”.
In `@packages/javascript/src/AsgardeoJavaScriptClient.ts`:
- Around line 202-209: The PKCE getters are being called with optional chaining
which masks a missing crypto helper and can lead to silent downstream failures;
remove the optional chaining on this.cryptoHelper when calling getCodeVerifier()
and getCodeChallenge(codeVerifier) so that if cryptoHelper is unexpectedly
undefined the code throws immediately, and ensure the returned codeVerifier is
validated (non-null) before calling
storageManager.setTemporaryDataParameter(pkceKey, codeVerifier, userId) to avoid
storing an invalid PKCE value.
- Line 460: The local variable/parameter customParam used in getAgentToken and
getOBOSignInURL is typed as Record<string,string> but passed into getSignInUrl
which expects ExtendedAuthorizeRequestUrlParams (values string | boolean);
change the declarations/signatures of customParam in both getAgentToken and
getOBOSignInURL to ExtendedAuthorizeRequestUrlParams so type-checking catches
incorrect keys/boolean values, update any callers to pass that type, and run
TypeScript checks to fix any resulting type errors; references: getAgentToken,
getOBOSignInURL, getSignInUrl, ExtendedAuthorizeRequestUrlParams, customParam.
In `@packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts`:
- Around line 34-61: The typeof text guard should be moved to the very start of
replaceCustomGrantTemplateTags (before any awaits) so non-string inputs
short-circuit without calling storageManager.getConfigData(),
storageManager.getSessionData(), or processOpenIDScopes; reorder the function to
check "if (typeof text !== 'string') return text;" first, then proceed to call
getConfigData(), derive sourceInstanceId/instanceKey, await
getSessionData(userId[, instanceKey]) and finally call
processOpenIDScopes(configData.scopes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae226a6d-54d8-431a-bc80-a33c08341049
📒 Files selected for processing (99)
packages/browser/src/AsgardeoSPAClient.tspackages/browser/src/DefaultCrypto.tspackages/browser/src/__legacy__/clients/index.tspackages/browser/src/__legacy__/constants/index.tspackages/browser/src/__legacy__/helpers/index.tspackages/browser/src/__legacy__/models/index.tspackages/browser/src/__legacy__/stores/index.tspackages/browser/src/__legacy__/utils/index.tspackages/browser/src/__legacy__/worker/index.tspackages/browser/src/clients/index.tspackages/browser/src/clients/mainThreadClient.tspackages/browser/src/clients/webWorkerClient.tspackages/browser/src/constants/errors.tspackages/browser/src/constants/hooks.tspackages/browser/src/constants/index.tspackages/browser/src/constants/message-types.tspackages/browser/src/constants/parameters.tspackages/browser/src/constants/session-management.tspackages/browser/src/constants/storage.tspackages/browser/src/helpers/authentication-helper.tspackages/browser/src/helpers/index.tspackages/browser/src/helpers/session-management-helper.tspackages/browser/src/helpers/spa-helper.tspackages/browser/src/index.tspackages/browser/src/models/browser-storage.tspackages/browser/src/models/http-client.tspackages/browser/src/models/index.tspackages/browser/src/models/message.tspackages/browser/src/models/request-custom-grant.tspackages/browser/src/models/session-management-helper.tspackages/browser/src/models/sign-in.tspackages/browser/src/models/sign-out-error.tspackages/browser/src/models/spa-client-config.tspackages/browser/src/models/thread-client.tspackages/browser/src/models/web-worker.tspackages/browser/src/stores/LocalStore.tspackages/browser/src/stores/MemoryStore.tspackages/browser/src/stores/SessionStore.tspackages/browser/src/stores/index.tspackages/browser/src/utils/MessageUtils.tspackages/browser/src/utils/SPAUtils.tspackages/browser/src/utils/http.tspackages/browser/src/utils/index.tspackages/browser/src/web.worker.tspackages/browser/src/worker/index.tspackages/browser/src/worker/workerCore.tspackages/browser/src/worker/workerReceiver.tspackages/express/src/LegacyAsgardeoExpressClient.tspackages/express/src/__legacy__/constants/index.tspackages/express/src/__legacy__/middleware/authentication.tspackages/express/src/__legacy__/middleware/index.tspackages/express/src/__legacy__/utils/express-utils.tspackages/express/src/constants/default-options.tspackages/express/src/constants/index.tspackages/express/src/constants/logger-config.tspackages/express/src/index.tspackages/express/src/middleware/authentication.tspackages/express/src/middleware/index.tspackages/express/src/middleware/protect-route.tspackages/express/src/models/data.tspackages/express/src/models/express-client-config.tspackages/express/src/models/index.tspackages/express/src/models/protect-route.tspackages/express/src/utils/ExpressUtils.tspackages/javascript/src/AsgardeoJavaScriptClient.tspackages/javascript/src/StorageManager.tspackages/javascript/src/__legacy__/client.tspackages/javascript/src/__legacy__/helpers/authentication-helper.tspackages/javascript/src/__legacy__/models/index.tspackages/javascript/src/api/exchangeToken.tspackages/javascript/src/api/handleTokenResponse.tspackages/javascript/src/api/loadOpenIDProviderConfiguration.tspackages/javascript/src/api/refreshAccessToken.tspackages/javascript/src/api/requestAccessToken.tspackages/javascript/src/api/revokeAccessToken.tspackages/javascript/src/api/validateIdToken.tspackages/javascript/src/index.tspackages/javascript/src/models/auth-client-config.tspackages/javascript/src/utils/clearSession.tspackages/javascript/src/utils/getAuthenticatedUserInfo.tspackages/javascript/src/utils/replaceCustomGrantTemplateTags.tspackages/javascript/src/utils/resolveEndpoints.tspackages/javascript/src/utils/resolveEndpointsByBaseURL.tspackages/javascript/src/utils/resolveEndpointsExplicitly.tspackages/node/src/AsgardeoNodeCore.tspackages/node/src/DefaultCrypto.tspackages/node/src/LegacyAsgardeoNodeClient.tspackages/node/src/constants/index.tspackages/node/src/constants/logger-config.tspackages/node/src/constants/uuid-config.tspackages/node/src/index.tspackages/node/src/models/index.tspackages/node/src/models/session-data.tspackages/node/src/models/url-callback.tspackages/node/src/stores/MemoryCacheStore.tspackages/node/src/stores/index.tspackages/node/src/utils/NodeLogger.tspackages/node/src/utils/SessionUtils.tspackages/node/src/utils/index.ts
💤 Files with no reviewable changes (14)
- packages/browser/src/legacy/constants/index.ts
- packages/browser/src/legacy/worker/index.ts
- packages/javascript/src/legacy/models/index.ts
- packages/browser/src/legacy/stores/index.ts
- packages/express/src/legacy/middleware/index.ts
- packages/express/src/legacy/constants/index.ts
- packages/express/src/legacy/utils/express-utils.ts
- packages/browser/src/legacy/models/index.ts
- packages/javascript/src/legacy/helpers/authentication-helper.ts
- packages/browser/src/legacy/helpers/index.ts
- packages/browser/src/legacy/utils/index.ts
- packages/browser/src/legacy/clients/index.ts
- packages/express/src/legacy/middleware/authentication.ts
- packages/javascript/src/legacy/client.ts
Comment on lines
+31
to
+34
| ): any => { | ||
| const router = new express.Router(); | ||
|
|
||
| router.use(async (req: express.Request, res: express.Response, next: express.nextFunction): Promise<void> => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify problematic Express API usages in this PR branch.
rg -n --type ts 'new express\.Router|express\.nextFunction' packages/express/src/middleware/authentication.tsRepository: asgardeo/javascript
Length of output: 435
🌐 Web query:
Express.js TypeScript Router instantiation new Router vs Router() correct API
💡 Result:
In Express, the correct instantiation/API for the router is: const router = express.Router([options]) (i.e., Router is called like a factory function, not via new). [1][2] Using new Router is not the documented API and will fail unless you’re using a different custom Router class/library; Express’s docs and TypeScript examples use express.Router / Router call syntax. [1][2] Practical TypeScript patterns: 1) Router per module: - import express, { Router } from 'express' (or import { Router } from 'express') - const router = Router (or const router = express.Router) This is the common/official pattern for creating mountable route handlers. [1][3] 2) Mount routers in your app: - app.use('/prefix', router) Routers are designed to be mounted. [2][1] Why your example in the question can misbehave: - In content.ts you should not import the app/router from app.ts (that can create circular dependencies and can lead to trying to call Express Application methods as if they were routers). The Express routing guide shows the opposite architecture: each router module creates and exports its own router instance, and the main app mounts it. [2] So, for “Express.js TypeScript Router instantiation new Router vs Router correct API”: use express.Router / Router and do not use new Router. [1][2]
Citations:
- 1: https://expressjs.com/en/api
- 2: https://expressjs.com/en/guide/routing.html
- 3: https://expressjs.com/en/guide/using-middleware.html
🌐 Web query:
Express.js NextFunction vs nextFunction TypeScript correct typing
💡 Result:
In Express + TypeScript, the correct typing is to use Express’s exported NextFunction type for the third parameter of non-error middleware/handlers, and the same NextFunction type for the fourth parameter of error middleware/handlers. 1) Normal middleware / route handlers (3 args) Use: - (req: Request, res: Response, next: NextFunction) The @types/express definitions type RequestHandler as taking next: NextFunction as the third argument [1]. Example: import { Request, Response, NextFunction } from 'express'; export function myMiddleware(req: Request, res: Response, next: NextFunction) { next; } 2) Error-handling middleware (4 args) Use: - (err: any, req: Request, res: Response, next: NextFunction) The @types/express definitions type ErrorRequestHandler as having next: NextFunction as the fourth argument [1]. Example: import { Request, Response, NextFunction } from 'express'; export function myErrorMiddleware(err: any, req: Request, res: Response, next: NextFunction) { //... next(err); } 3) What “next” actually is (and why naming NextFunction is fine) Express middleware receives a callback called next (by convention) as the third argument [2]. The type for that callback is NextFunction in the Express typings (it’s an interface with call signatures) [1]. So: “NextFunction vs nextFunction” is just naming. In TypeScript you should import and use NextFunction (capital N) as the type. The parameter variable name can be anything (next, n, etc.), but the type should be NextFunction [2][1]. 4) Common pitfall Don’t type the parameter as something like nextFunction (lowercase) unless you actually declared that identifier yourself. The Express typings export NextFunction (capital N) [1]. Sources: [1][2]
Citations:
- 1: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/express-serve-static-core/index.d.ts
- 2: https://expressjs.com/en/guide/writing-middleware.html
Fix Express API typing/instantiation (Router and NextFunction).
new express.Router() must be express.Router() (factory function, not constructor), and express.nextFunction must be express.NextFunction (capitalized). These are incorrect Express TypeScript APIs and will break compilation.
Affected locations: Lines 32, 34, 42, 57 in packages/express/src/middleware/authentication.ts
Suggested fix
): any => {
- const router = new express.Router();
+): express.Router => {
+ const router = express.Router();
- router.use(async (req: express.Request, res: express.Response, next: express.nextFunction): Promise<void> => {
+ router.use((req: express.Request, res: express.Response, next: express.NextFunction): void => {
req.asgardeoAuth = asgardeoExpressClient;
res.asgardeoAuth = asgardeoExpressClient;
next();
});
@@
- async (req: express.Request, res: express.Response, next: express.nextFunction): Promise<void> => {
+ async (req: express.Request, res: express.Response, next: express.NextFunction): Promise<void> => {
@@
- async (req: express.Request, res: express.Response, next: express.nextFunction): Promise<void> => {
+ async (req: express.Request, res: express.Response, next: express.NextFunction): Promise<void> => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ): any => { | |
| const router = new express.Router(); | |
| router.use(async (req: express.Request, res: express.Response, next: express.nextFunction): Promise<void> => { | |
| ): express.Router => { | |
| const router = express.Router(); | |
| router.use((req: express.Request, res: express.Response, next: express.NextFunction): void => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/express/src/middleware/authentication.ts` around lines 31 - 34, The
code incorrectly uses Express APIs: replace the constructor call new
express.Router() with the factory function express.Router(), and fix the type
name express.nextFunction to express.NextFunction wherever used (e.g., in the
middleware signature passed to router.use and any other middleware definitions
in this file such as the handlers that reference next). Update these references
for the router variable and all middleware function parameter typings (lines
noted in review) so the code compiles with Express TypeScript definitions.
Comment on lines
+56
to
+58
| if (shouldValidateIdToken) { | ||
| await validateIdToken(storageManager, cryptoHelper, parsedResponse.id_token); | ||
| } |
There was a problem hiding this comment.
Guard ID token validation when id_token is absent.
When token validation is enabled, this path validates even if the response has no id_token (common in refresh/token-exchange flows), which can cause runtime failures.
Suggested fix
- if (shouldValidateIdToken) {
- await validateIdToken(storageManager, cryptoHelper, parsedResponse.id_token);
+ if (shouldValidateIdToken && parsedResponse.id_token) {
+ await validateIdToken(storageManager, cryptoHelper, parsedResponse.id_token);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/api/handleTokenResponse.ts` around lines 56 - 58, The
current call to validateIdToken runs whenever shouldValidateIdToken is true even
if parsedResponse.id_token is undefined, causing failures for flows that don't
return an id_token; update the block around validateIdToken(storageManager,
cryptoHelper, parsedResponse.id_token) to first check that
parsedResponse.id_token is present (truthy) and only call validateIdToken when
it exists, otherwise skip validation (or handle the absence explicitly if you
prefer to throw a specific error); ensure you reference shouldValidateIdToken,
parsedResponse.id_token, and validateIdToken when making the change.
| cryptoHelper: IsomorphicCrypto, | ||
| userId?: string, | ||
| ): Promise<TokenResponse> { | ||
| const tokenEndpoint: string | undefined = (await storageManager.loadOpenIDProviderConfiguration()).token_endpoint; |
There was a problem hiding this comment.
Discovery isn't initialized here — token endpoint can be empty if refreshAccessToken is called before any other token flow.
storageManager.loadOpenIDProviderConfiguration() returns the previously persisted metadata; it does not run the discovery flow. requestAccessToken.ts (lines 40-46) and exchangeToken.ts (lines 35-41) guard with loadOpenIDProviderConfiguration(storageManager, false) if OPENID_PROVIDER_CONFIG_INITIATED is not set. The same guard should be applied here, otherwise a cold refresh (e.g. after a page reload where memory storage is used) will fall into the "No refresh token endpoint found" path even though discovery would have resolved it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/api/refreshAccessToken.ts` at line 32,
refreshAccessToken currently reads token_endpoint from
storageManager.loadOpenIDProviderConfiguration() without ensuring discovery has
run, so cold refreshes can see an empty endpoint; update refreshAccessToken to
follow the same guard used in requestAccessToken.ts and exchangeToken.ts by
checking OPENID_PROVIDER_CONFIG_INITIATED and calling
loadOpenIDProviderConfiguration(storageManager, false) when not set before
reading token_endpoint, ensuring tokenEndpoint is populated and avoiding the
erroneous "No refresh token endpoint found" path.
Comment on lines
+54
to
+62
| const body: string[] = []; | ||
|
|
||
| body.push(`client_id=${configData.clientId}`); | ||
| body.push(`refresh_token=${sessionData.refresh_token}`); | ||
| body.push('grant_type=refresh_token'); | ||
|
|
||
| if (configData.clientSecret && configData.clientSecret.trim().length > 0) { | ||
| body.push(`client_secret=${configData.clientSecret}`); | ||
| } |
There was a problem hiding this comment.
Refresh-token request body is not URL-encoded — refresh tokens routinely contain =/+// and will corrupt the request.
refresh_token (and client_secret when present) are opaque strings that very commonly contain characters reserved in application/x-www-form-urlencoded (notably =, +, /, and base64 padding =). Raw template-literal concatenation passes those through unencoded, causing the server to receive a malformed body — e.g. refresh_token=abc=def is parsed as refresh_token=abc with a stray def.
requestAccessToken.ts (line 68 onward) already uses URLSearchParams, which encodes automatically. Use the same here for both correctness and consistency.
🔧 Suggested fix
- const body: string[] = [];
-
- body.push(`client_id=${configData.clientId}`);
- body.push(`refresh_token=${sessionData.refresh_token}`);
- body.push('grant_type=refresh_token');
-
- if (configData.clientSecret && configData.clientSecret.trim().length > 0) {
- body.push(`client_secret=${configData.clientSecret}`);
- }
+ const body: URLSearchParams = new URLSearchParams();
+
+ body.set('client_id', configData.clientId);
+ body.set('refresh_token', sessionData.refresh_token);
+ body.set('grant_type', 'refresh_token');
+
+ if (configData.clientSecret && configData.clientSecret.trim().length > 0) {
+ body.set('client_secret', configData.clientSecret);
+ }
@@
try {
tokenResponse = await fetch(tokenEndpoint, {
- body: body.join('&'),
+ body,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const body: string[] = []; | |
| body.push(`client_id=${configData.clientId}`); | |
| body.push(`refresh_token=${sessionData.refresh_token}`); | |
| body.push('grant_type=refresh_token'); | |
| if (configData.clientSecret && configData.clientSecret.trim().length > 0) { | |
| body.push(`client_secret=${configData.clientSecret}`); | |
| } | |
| const body: URLSearchParams = new URLSearchParams(); | |
| body.set('client_id', configData.clientId); | |
| body.set('refresh_token', sessionData.refresh_token); | |
| body.set('grant_type', 'refresh_token'); | |
| if (configData.clientSecret && configData.clientSecret.trim().length > 0) { | |
| body.set('client_secret', configData.clientSecret); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/api/refreshAccessToken.ts` around lines 54 - 62, The
refresh-token request currently builds the body via raw template literals (body
array) which fails to URL-encode opaque values like sessionData.refresh_token
and configData.clientSecret; replace the string[] body construction with a
URLSearchParams instance (e.g., create params = new URLSearchParams()), use
params.append('client_id', configData.clientId), params.append('refresh_token',
sessionData.refresh_token), params.append('grant_type', 'refresh_token') and
conditionally params.append('client_secret', configData.clientSecret) so the
refresh token and client secret are properly percent-encoded before sending (use
params.toString() or pass params directly as the request body).
Comment on lines
+42
to
+50
| const body: string[] = []; | ||
|
|
||
| body.push(`client_id=${configData.clientId}`); | ||
| body.push(`token=${(await storageManager.getSessionData(userId)).access_token}`); | ||
| body.push('token_type_hint=access_token'); | ||
|
|
||
| if (configData.clientSecret && configData.clientSecret.trim().length > 0) { | ||
| body.push(`client_secret=${configData.clientSecret}`); | ||
| } |
There was a problem hiding this comment.
URL-encode revocation form parameters instead of raw concatenation.
client_id, token, and client_secret are written unencoded into an application/x-www-form-urlencoded body, which can break requests for reserved characters.
Suggested fix
- const body: string[] = [];
-
- body.push(`client_id=${configData.clientId}`);
- body.push(`token=${(await storageManager.getSessionData(userId)).access_token}`);
- body.push('token_type_hint=access_token');
+ const body = new URLSearchParams();
+ body.set('client_id', configData.clientId);
+ body.set('token', (await storageManager.getSessionData(userId)).access_token);
+ body.set('token_type_hint', 'access_token');
if (configData.clientSecret && configData.clientSecret.trim().length > 0) {
- body.push(`client_secret=${configData.clientSecret}`);
+ body.set('client_secret', configData.clientSecret);
}
@@
- body: body.join('&'),
+ body: body.toString(),Also applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/api/revokeAccessToken.ts` around lines 42 - 50, The
revocation request builds an application/x-www-form-urlencoded body by
concatenating raw values which can break on reserved characters; update the body
construction in revokeAccessToken to URL-encode form values (use
encodeURIComponent or equivalent) for client_id, token (from await
storageManager.getSessionData(userId).access_token), and client_secret whenever
added, and ensure you do the same change for the other identical block
referenced at lines 55-57 so all form parameters are encoded before joining into
the request body.
Comment on lines
+531
to
+593
| /* eslint-disable class-methods-use-this, @typescript-eslint/no-unused-vars */ | ||
| switchOrganization(_organization: Organization, _sessionId?: string): Promise<TokenResponse | Response> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| getAllOrganizations(_options?: any, _sessionId?: string): Promise<AllOrganizationsApiResponse> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| getMyOrganizations(_options?: any, _sessionId?: string): Promise<Organization[]> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| getCurrentOrganization(_sessionId?: string): Promise<Organization | null> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| getUserProfile(_options?: any): Promise<UserProfile> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| isLoading(): boolean { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| updateUserProfile(_payload: any, _userId?: string): Promise<User> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| getConfiguration(): T { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| signInSilently(_options?: SignInOptions): Promise<User | boolean> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| setSession(_sessionData: Record<string, unknown>, _sessionId?: string): Promise<void> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| signIn(_options?: SignInOptions): Promise<User> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| signOut( | ||
| _options?: SignOutOptions, | ||
| _sessionIdOrAfterSignOut?: string | ((afterSignOutUrl: string) => void), | ||
| _afterSignOut?: (afterSignOutUrl: string) => void, | ||
| ): Promise<string> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
|
|
||
| signUp(options?: SignUpOptions): Promise<void>; | ||
|
|
||
| signUp(payload: EmbeddedFlowExecuteRequestPayload): Promise<EmbeddedFlowExecuteResponse>; | ||
|
|
||
| signUp( | ||
| _optionsOrPayload?: SignUpOptions | EmbeddedFlowExecuteRequestPayload, | ||
| ): Promise<void | EmbeddedFlowExecuteResponse> { | ||
| throw new Error('Method not implemented.'); | ||
| } | ||
| /* eslint-enable class-methods-use-this, @typescript-eslint/no-unused-vars */ |
There was a problem hiding this comment.
13 public AsgardeoClient methods throw "Method not implemented." — they will surface at runtime if reached via the typed contract.
switchOrganization, getAllOrganizations, getMyOrganizations, getCurrentOrganization, getUserProfile, isLoading, updateUserProfile, getConfiguration, signInSilently, setSession, signIn, signOut, and signUp are all stubs. Anything typed to AsgardeoClient<T> (browser/node wrappers, app code) will compile cleanly and blow up at runtime.
Options, in order of preference: (1) mark these as optional on the AsgardeoClient interface, (2) implement them as part of this refactor, (3) split the interface so AsgardeoJavaScriptClient only declares what it supports, or (4) at minimum throw a typed AsgardeoAuthException with a stable error code and document the gap in the PR description / changelog so downstream packages know what isn't backed yet.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/AsgardeoJavaScriptClient.ts` around lines 531 - 593,
Several public AsgardeoClient methods in AsgardeoJavaScriptClient
(switchOrganization, getAllOrganizations, getMyOrganizations,
getCurrentOrganization, getUserProfile, isLoading, updateUserProfile,
getConfiguration, signInSilently, setSession, signIn, signOut, signUp) are stubs
that throw a generic Error and will crash at runtime; replace those generic
throws with throwing a typed AsgardeoAuthException (include a stable error code
like "NOT_IMPLEMENTED_IN_JS_CLIENT" and a clear message) so callers can handle
the missing implementation reliably, and ensure the exception class used
(AsgardeoAuthException) is imported/defined and documented in the PR/changelog;
alternatively prefer making these methods optional on the AsgardeoClient
interface if you want to avoid implementing them here, but if you keep them,
change each method body in AsgardeoJavaScriptClient to throw the typed exception
instead of new Error.
Comment on lines
+26
to
+31
| const username: string = payload?.['username'] ?? ''; | ||
| const givenName: string = payload?.['given_name'] ?? ''; | ||
| const familyName: string = payload?.['family_name'] ?? ''; | ||
| const fullName: string = givenName && familyName ? `${givenName} ${familyName}` : givenName || familyName || ''; | ||
| const displayName: string = payload.preferred_username ?? fullName; | ||
|
|
There was a problem hiding this comment.
username derivation is too strict and can resolve to empty for valid OIDC tokens.
On Line 26, relying only on username misses common claims (preferred_username, sub). This can produce blank usernames in downstream flows.
Suggested fix
- const username: string = payload?.['username'] ?? '';
+ const username: string =
+ payload?.['username'] ??
+ payload?.preferred_username ??
+ payload?.sub ??
+ '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const username: string = payload?.['username'] ?? ''; | |
| const givenName: string = payload?.['given_name'] ?? ''; | |
| const familyName: string = payload?.['family_name'] ?? ''; | |
| const fullName: string = givenName && familyName ? `${givenName} ${familyName}` : givenName || familyName || ''; | |
| const displayName: string = payload.preferred_username ?? fullName; | |
| const username: string = | |
| payload?.['username'] ?? | |
| payload?.preferred_username ?? | |
| payload?.sub ?? | |
| ''; | |
| const givenName: string = payload?.['given_name'] ?? ''; | |
| const familyName: string = payload?.['family_name'] ?? ''; | |
| const fullName: string = givenName && familyName ? `${givenName} ${familyName}` : givenName || familyName || ''; | |
| const displayName: string = payload.preferred_username ?? fullName; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/utils/getAuthenticatedUserInfo.ts` around lines 26 -
31, The username derivation in getAuthenticatedUserInfo.ts is too strict: update
how the username variable is computed so it falls back to common OIDC claims;
instead of only using payload['username'], derive username from
payload['username'] ?? payload['preferred_username'] ?? payload['sub'] ?? ''.
Keep existing fullName/displayName logic but ensure displayName still falls back
to fullName if preferred_username is missing.
Comment on lines
+63
to
+71
| return text | ||
| .replace(TokenExchangeConstants.Placeholders.ACCESS_TOKEN, sessionData.access_token) | ||
| .replace( | ||
| TokenExchangeConstants.Placeholders.USERNAME, | ||
| getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token).username, | ||
| ) | ||
| .replace(TokenExchangeConstants.Placeholders.SCOPES, scope) | ||
| .replace(TokenExchangeConstants.Placeholders.CLIENT_ID, configData.clientId) | ||
| .replace(TokenExchangeConstants.Placeholders.CLIENT_SECRET, configData.clientSecret ?? ''); |
There was a problem hiding this comment.
Substituted values are not URL-encoded — callers join them into form bodies.
exchangeToken builds the request body as `${key}=${newValue}` joined by & (see exchangeToken.ts line 63-69). Values returned here can contain =, &, +, / (common in JWTs/base64 padding and in client_secret characters), which will produce malformed application/x-www-form-urlencoded payloads or be silently corrupted (e.g. + decoded as space at the server).
Encode each substituted value here (or have the caller build with URLSearchParams) to make this robust to any token/secret content.
🔧 Suggested fix
return text
- .replace(TokenExchangeConstants.Placeholders.ACCESS_TOKEN, sessionData.access_token)
+ .replace(TokenExchangeConstants.Placeholders.ACCESS_TOKEN, encodeURIComponent(sessionData.access_token))
.replace(
TokenExchangeConstants.Placeholders.USERNAME,
- getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token).username,
+ encodeURIComponent(getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token).username),
)
- .replace(TokenExchangeConstants.Placeholders.SCOPES, scope)
- .replace(TokenExchangeConstants.Placeholders.CLIENT_ID, configData.clientId)
- .replace(TokenExchangeConstants.Placeholders.CLIENT_SECRET, configData.clientSecret ?? '');
+ .replace(TokenExchangeConstants.Placeholders.SCOPES, encodeURIComponent(scope))
+ .replace(TokenExchangeConstants.Placeholders.CLIENT_ID, encodeURIComponent(configData.clientId))
+ .replace(TokenExchangeConstants.Placeholders.CLIENT_SECRET, encodeURIComponent(configData.clientSecret ?? ''));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return text | |
| .replace(TokenExchangeConstants.Placeholders.ACCESS_TOKEN, sessionData.access_token) | |
| .replace( | |
| TokenExchangeConstants.Placeholders.USERNAME, | |
| getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token).username, | |
| ) | |
| .replace(TokenExchangeConstants.Placeholders.SCOPES, scope) | |
| .replace(TokenExchangeConstants.Placeholders.CLIENT_ID, configData.clientId) | |
| .replace(TokenExchangeConstants.Placeholders.CLIENT_SECRET, configData.clientSecret ?? ''); | |
| return text | |
| .replace(TokenExchangeConstants.Placeholders.ACCESS_TOKEN, encodeURIComponent(sessionData.access_token)) | |
| .replace( | |
| TokenExchangeConstants.Placeholders.USERNAME, | |
| encodeURIComponent(getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token).username), | |
| ) | |
| .replace(TokenExchangeConstants.Placeholders.SCOPES, encodeURIComponent(scope)) | |
| .replace(TokenExchangeConstants.Placeholders.CLIENT_ID, encodeURIComponent(configData.clientId)) | |
| .replace(TokenExchangeConstants.Placeholders.CLIENT_SECRET, encodeURIComponent(configData.clientSecret ?? '')); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts` around lines
63 - 71, The replacements in replaceCustomGrantTemplateTags are inserted
directly into an x-www-form-urlencoded body and must be URL-encoded; update the
function to encode each substituted value (access_token, username from
getAuthenticatedUserInfo(cryptoHelper, sessionData.id_token), scope,
configData.clientId, and configData.clientSecret ?? '') using encodeURIComponent
before calling .replace for the corresponding
TokenExchangeConstants.Placeholders so the resulting payload is safe against
characters like =, &, +, and / (alternatively, switch callers such as
exchangeToken to build the body with URLSearchParams, but the quick fix is to
wrap each replacement value with encodeURIComponent inside
replaceCustomGrantTemplateTags).
Comment on lines
+31
to
+35
| Object.keys(configData.endpoints).forEach((endpointName: string) => { | ||
| const snakeCasedName: string = endpointName.replace(/[A-Z]/g, (letter: string) => `_${letter.toLowerCase()}`); | ||
|
|
||
| oidcProviderMetaData[snakeCasedName] = configData?.endpoints ? configData.endpoints[endpointName] : ''; | ||
| }); |
There was a problem hiding this comment.
Endpoint key normalization has a PascalCase conversion bug.
On Line 32, the conversion adds a leading underscore when the key starts uppercase (e.g., AuthorizationEndpoint), producing wrong metadata keys.
Suggested fix
+ const toSnakeCase = (name: string): string =>
+ name
+ .replace(/([a-z0-9])([A-Z])/g, '$1_$2')
+ .replace(/([A-Z])([A-Z][a-z])/g, '$1_$2')
+ .toLowerCase();
+
if (configData.endpoints) {
Object.keys(configData.endpoints).forEach((endpointName: string) => {
- const snakeCasedName: string = endpointName.replace(/[A-Z]/g, (letter: string) => `_${letter.toLowerCase()}`);
+ const snakeCasedName: string = toSnakeCase(endpointName);
oidcProviderMetaData[snakeCasedName] = configData?.endpoints ? configData.endpoints[endpointName] : '';
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/utils/resolveEndpoints.ts` around lines 31 - 35, The
PascalCase-to-snake_case conversion in resolveEndpoints.ts is adding a leading
underscore when endpointName starts with an uppercase letter (e.g.,
AuthorizationEndpoint -> _authorization_endpoint); update the conversion logic
for snakeCasedName (used when populating oidcProviderMetaData from
configData.endpoints) so it does not produce a leading underscore — either by
lowercasing the first character before inserting underscores for internal
capitals or by stripping a leading underscore after the current replace; ensure
you continue to use endpointName, snakeCasedName and oidcProviderMetaData in the
same assignment.
Comment on lines
+42
to
+52
| const isRequiredEndpointsContains: boolean = configData.endpoints | ||
| ? requiredEndpoints.every((reqEndpointName: string) => | ||
| configData.endpoints | ||
| ? Object.keys(configData.endpoints).some((endpointName: string) => { | ||
| const snakeCasedName: string = endpointName.replace( | ||
| /[A-Z]/g, | ||
| (letter: string) => `_${letter.toLowerCase()}`, | ||
| ); | ||
|
|
||
| return snakeCasedName === reqEndpointName; | ||
| }) |
There was a problem hiding this comment.
PascalCase endpoint names are converted incorrectly (leading underscore bug).
The conversion on Line 46 and Line 68 prefixes _ for initial capitals, so AuthorizationEndpoint becomes _authorization_endpoint. That breaks required-endpoint matching and output keys.
Suggested fix
+ const toSnakeCase = (name: string): string =>
+ name
+ .replace(/([a-z0-9])([A-Z])/g, '$1_$2')
+ .replace(/([A-Z])([A-Z][a-z])/g, '$1_$2')
+ .toLowerCase();
+
const isRequiredEndpointsContains: boolean = configData.endpoints
? requiredEndpoints.every((reqEndpointName: string) =>
configData.endpoints
? Object.keys(configData.endpoints).some((endpointName: string) => {
- const snakeCasedName: string = endpointName.replace(
- /[A-Z]/g,
- (letter: string) => `_${letter.toLowerCase()}`,
- );
+ const snakeCasedName: string = toSnakeCase(endpointName);
return snakeCasedName === reqEndpointName;
})
: false,
)
: false;
@@
if (configData.endpoints) {
Object.keys(configData.endpoints).forEach((endpointName: string) => {
- const snakeCasedName: string = endpointName.replace(/[A-Z]/g, (letter: string) => `_${letter.toLowerCase()}`);
+ const snakeCasedName: string = toSnakeCase(endpointName);
oidcProviderMetaData[snakeCasedName] = configData?.endpoints ? configData.endpoints[endpointName] : '';
});
}Also applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/src/utils/resolveEndpointsExplicitly.ts` around lines 42
- 52, The snake_case conversion currently prefixes an underscore for initial
capitals (e.g., AuthorizationEndpoint -> _authorization_endpoint) because the
replace callback doesn't check the character index; update the conversion used
when building snakeCasedName (in the isRequiredEndpointsContains logic and the
similar conversion around lines 67-70) to use the replace callback's index
parameter and only prepend '_' when index > 0, for example: replace(/[A-Z]/g,
(letter, idx) => (idx ? '_' : '') + letter.toLowerCase()); ensure both
occurrences (the one inside the requiredEndpoints.every check and the other
occurrence noted) are updated so matching and output keys are correct.
brionmario
force-pushed
the
refactor-legacy
branch
from
d2dd1a2 to
edd99f0
Compare
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/nextjs/src/AsgardeoNextClient.ts (1)
100-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet
isInitializedonly aftersuper.initialize()succeeds.Line 117 flips the singleton guard before the awaited initialization finishes. If
super.initialize(...)throws, the client stays permanently marked as initialized and every retry short-circuits.Proposed fix
override async initialize(config: T, storage?: Storage): Promise<boolean> { if (this.isInitialized) { return Promise.resolve(true); } @@ - this.isInitialized = true; - let resolvedOrganizationHandle: string | undefined = organizationHandle; @@ - return super.initialize( + const result: boolean = await super.initialize( { afterSignInUrl: afterSignInUrl ?? origin, afterSignOutUrl: afterSignOutUrl ?? origin, baseUrl, @@ } as unknown as T, storage, ); + + this.isInitialized = true; + + return result; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextjs/src/AsgardeoNextClient.ts` around lines 100 - 141, The guard this.isInitialized is being set before awaiting super.initialize, so if super.initialize throws the client will be permanently marked initialized; remove the early this.isInitialized = true and instead call await super.initialize(...) first, capture its boolean result, and only set this.isInitialized = true when that result is true (then return the result); reference the initialize override and the super.initialize(...) call and ensure the singleton flag is updated only after successful completion.packages/nuxt/src/runtime/server/AsgardeoNuxtClient.ts (1)
136-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't coerce expired tokens back to one hour.
Line 144 turns
0into3600via|| 3600, so already-expired sessions are rehydrated as valid for another hour. The fallback is already handled whenexpiresInSecondsis computed.Proposed fix
- expires_in: String(expiresInSeconds || 3600), + expires_in: String(expiresInSeconds),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuxt/src/runtime/server/AsgardeoNuxtClient.ts` around lines 136 - 145, The code in AsgardeoNuxtClient that prepares session data for storage is coercing an actually-zero expiresInSeconds to 3600 by using "|| 3600" when calling storageManager.setSessionData; update the storage call (in the block that builds access_token/created_at/expires_in/id_token) to use the computed expiresInSeconds directly (e.g., String(expiresInSeconds)) instead of String(expiresInSeconds || 3600) so expired sessions aren’t rehydrated as valid for another hour; keep iatSeconds and created_at logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/express/src/AsgardeoExpressClient.ts`:
- Around line 142-155: The authRedirectCallback currently calls next()
immediately after res.redirect(), which can cause "headers already sent" errors;
remove the conditional block that invokes next() after the res.redirect() call
inside authRedirectCallback so the middleware chain stops once the redirect is
issued (keep any other uses of next elsewhere but do not call next() after
res.redirect() in authRedirectCallback).
In `@packages/express/src/index.ts`:
- Around line 19-21: The PR removed the old public entrypoint symbols causing
breaking imports; restore a deprecated compatibility barrel that re-exports the
previous surface so existing consumers keep working during the release window.
Specifically, add back a compatibility export in this module that re-exports the
legacy symbols (e.g., the previously published models export and the old client
export names such as AsgardeoExpressClient and any legacy aliases) and mark them
deprecated in comments, or alternatively gate removal behind an explicit major/
breaking release; ensure the compatibility exports reference the existing
'./models' and './AsgardeoExpressClient' symbols so import paths and names
continue to resolve.
In `@packages/nextjs/src/AsgardeoNextClient.ts`:
- Around line 434-435: The method AsgardeoNextClient.override getConfiguration
currently declares a synchronous return type T but returns the async
getConfigData() result; change getConfiguration to be async and return
Promise<T> (i.e., declare "override async getConfiguration(): Promise<T>") and
return await this.getConfigData() cast to T so the runtime Promise matches the
type; ensure the override matches the async signature from AsgardeoNodeClient
and update any callers to await the returned Promise if necessary.
In `@packages/node/src/AsgardeoNodeClient.ts`:
- Around line 192-193: The code currently calls
storageManager.removeSessionData(userId) then
storageManager.getTemporaryData(userId), which only reads temporary PKCE/state
data and leaves it behind; replace the second call with await
storageManager.removeTemporaryData(userId) so the temporary data (PKCE/state
blob) is deleted after a failed token refresh. Update the call site where
getTemporaryData is used (same function containing removeSessionData and userId)
to invoke removeTemporaryData and await it to ensure cleanup.
In `@packages/node/src/index.ts`:
- Around line 29-30: Add back deprecated compatibility aliases in the package
entrypoint so existing consumers of the old `@asgardeo/node` symbols don't break:
in packages/node/src/index.ts keep the current exports and also re-export the
legacy public symbols (the original names previously exposed from this
entrypoint) as deprecated aliases that forward to the new implementations (e.g.,
re-export any legacy types/functions/classes that used to be provided from
'./models' and './utils/NodeLogger' under their old names). Mark those aliases
deprecated in comments and optionally gate them behind a clearly named
BREAKING_RELEASE flag if you want an explicit opt-in for a future major release.
---
Outside diff comments:
In `@packages/nextjs/src/AsgardeoNextClient.ts`:
- Around line 100-141: The guard this.isInitialized is being set before awaiting
super.initialize, so if super.initialize throws the client will be permanently
marked initialized; remove the early this.isInitialized = true and instead call
await super.initialize(...) first, capture its boolean result, and only set
this.isInitialized = true when that result is true (then return the result);
reference the initialize override and the super.initialize(...) call and ensure
the singleton flag is updated only after successful completion.
In `@packages/nuxt/src/runtime/server/AsgardeoNuxtClient.ts`:
- Around line 136-145: The code in AsgardeoNuxtClient that prepares session data
for storage is coercing an actually-zero expiresInSeconds to 3600 by using "||
3600" when calling storageManager.setSessionData; update the storage call (in
the block that builds access_token/created_at/expires_in/id_token) to use the
computed expiresInSeconds directly (e.g., String(expiresInSeconds)) instead of
String(expiresInSeconds || 3600) so expired sessions aren’t rehydrated as valid
for another hour; keep iatSeconds and created_at logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d623c0e-7ebe-4d9c-b1f9-b7cac7a8c5bb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
packages/browser/src/AsgardeoSPAClient.tspackages/browser/src/DefaultCrypto.tspackages/browser/src/__legacy__/clients/index.tspackages/browser/src/__legacy__/constants/index.tspackages/browser/src/__legacy__/helpers/index.tspackages/browser/src/__legacy__/models/index.tspackages/browser/src/__legacy__/stores/index.tspackages/browser/src/__legacy__/utils/index.tspackages/browser/src/__legacy__/worker/index.tspackages/browser/src/clients/index.tspackages/browser/src/clients/mainThreadClient.tspackages/browser/src/clients/webWorkerClient.tspackages/browser/src/constants/errors.tspackages/browser/src/constants/hooks.tspackages/browser/src/constants/index.tspackages/browser/src/constants/message-types.tspackages/browser/src/constants/parameters.tspackages/browser/src/constants/session-management.tspackages/browser/src/constants/storage.tspackages/browser/src/helpers/authentication-helper.tspackages/browser/src/helpers/index.tspackages/browser/src/helpers/session-management-helper.tspackages/browser/src/helpers/spa-helper.tspackages/browser/src/index.tspackages/browser/src/models/browser-storage.tspackages/browser/src/models/http-client.tspackages/browser/src/models/index.tspackages/browser/src/models/message.tspackages/browser/src/models/request-custom-grant.tspackages/browser/src/models/session-management-helper.tspackages/browser/src/models/sign-in.tspackages/browser/src/models/sign-out-error.tspackages/browser/src/models/spa-client-config.tspackages/browser/src/models/thread-client.tspackages/browser/src/models/web-worker.tspackages/browser/src/stores/LocalStore.tspackages/browser/src/stores/MemoryStore.tspackages/browser/src/stores/SessionStore.tspackages/browser/src/stores/index.tspackages/browser/src/utils/MessageUtils.tspackages/browser/src/utils/SPAUtils.tspackages/browser/src/utils/http.tspackages/browser/src/utils/index.tspackages/browser/src/web.worker.tspackages/browser/src/worker/index.tspackages/browser/src/worker/workerCore.tspackages/browser/src/worker/workerReceiver.tspackages/express/src/AsgardeoExpressClient.tspackages/express/src/__legacy__/client.tspackages/express/src/__legacy__/constants/index.tspackages/express/src/__legacy__/middleware/authentication.tspackages/express/src/__legacy__/middleware/index.tspackages/express/src/__legacy__/utils/express-utils.tspackages/express/src/constants/default-options.tspackages/express/src/constants/index.tspackages/express/src/constants/logger-config.tspackages/express/src/index.tspackages/express/src/middleware/authentication.tspackages/express/src/middleware/index.tspackages/express/src/middleware/protect-route.tspackages/express/src/models/data.tspackages/express/src/models/express-client-config.tspackages/express/src/models/index.tspackages/express/src/models/protect-route.tspackages/express/src/utils/ExpressUtils.tspackages/javascript/src/AsgardeoJavaScriptClient.tspackages/javascript/src/StorageManager.tspackages/javascript/src/__legacy__/client.tspackages/javascript/src/__legacy__/helpers/authentication-helper.tspackages/javascript/src/__legacy__/models/index.tspackages/javascript/src/api/exchangeToken.tspackages/javascript/src/api/handleTokenResponse.tspackages/javascript/src/api/loadOpenIDProviderConfiguration.tspackages/javascript/src/api/refreshAccessToken.tspackages/javascript/src/api/requestAccessToken.tspackages/javascript/src/api/revokeAccessToken.tspackages/javascript/src/api/validateIdToken.tspackages/javascript/src/index.tspackages/javascript/src/models/auth-client-config.tspackages/javascript/src/utils/clearSession.tspackages/javascript/src/utils/getAuthenticatedUserInfo.tspackages/javascript/src/utils/replaceCustomGrantTemplateTags.tspackages/javascript/src/utils/resolveEndpoints.tspackages/javascript/src/utils/resolveEndpointsByBaseURL.tspackages/javascript/src/utils/resolveEndpointsExplicitly.tspackages/nextjs/src/AsgardeoNextClient.tspackages/nextjs/src/server/index.tspackages/node/src/AsgardeoNodeClient.tspackages/node/src/DefaultCrypto.tspackages/node/src/__legacy__/client.tspackages/node/src/__legacy__/core/authentication.tspackages/node/src/constants/index.tspackages/node/src/constants/logger-config.tspackages/node/src/constants/uuid-config.tspackages/node/src/index.tspackages/node/src/models/index.tspackages/node/src/models/session-data.tspackages/node/src/models/url-callback.tspackages/node/src/stores/MemoryCacheStore.tspackages/node/src/stores/index.tspackages/node/src/utils/NodeLogger.tspackages/node/src/utils/SessionUtils.tspackages/node/src/utils/index.tspackages/nuxt/src/runtime/server/AsgardeoNuxtClient.tspackages/react/src/AsgardeoReactClient.tspackages/react/src/__temp__/api.tspackages/vue/src/AsgardeoVueClient.ts
💤 Files with no reviewable changes (17)
- packages/javascript/src/legacy/models/index.ts
- packages/express/src/legacy/constants/index.ts
- packages/node/src/legacy/client.ts
- packages/browser/src/legacy/utils/index.ts
- packages/express/src/legacy/client.ts
- packages/browser/src/legacy/stores/index.ts
- packages/express/src/legacy/utils/express-utils.ts
- packages/browser/src/legacy/worker/index.ts
- packages/browser/src/legacy/clients/index.ts
- packages/express/src/legacy/middleware/authentication.ts
- packages/browser/src/legacy/models/index.ts
- packages/javascript/src/legacy/helpers/authentication-helper.ts
- packages/express/src/legacy/middleware/index.ts
- packages/node/src/legacy/core/authentication.ts
- packages/javascript/src/legacy/client.ts
- packages/browser/src/legacy/constants/index.ts
- packages/browser/src/legacy/helpers/index.ts
✅ Files skipped from review due to trivial changes (18)
- packages/express/src/middleware/index.ts
- packages/browser/src/worker/index.ts
- packages/browser/src/clients/index.ts
- packages/node/src/stores/index.ts
- packages/express/src/models/data.ts
- packages/node/src/utils/index.ts
- packages/browser/src/utils/index.ts
- packages/browser/src/stores/index.ts
- packages/express/src/constants/logger-config.ts
- packages/express/src/models/protect-route.ts
- packages/javascript/src/StorageManager.ts
- packages/express/src/constants/default-options.ts
- packages/browser/src/models/spa-client-config.ts
- packages/javascript/src/models/auth-client-config.ts
- packages/vue/src/AsgardeoVueClient.ts
- packages/browser/src/models/index.ts
- packages/javascript/src/utils/getAuthenticatedUserInfo.ts
- packages/express/src/middleware/protect-route.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- packages/browser/src/helpers/index.ts
- packages/browser/src/utils/SPAUtils.ts
- packages/browser/src/helpers/session-management-helper.ts
- packages/javascript/src/api/refreshAccessToken.ts
- packages/javascript/src/utils/resolveEndpointsByBaseURL.ts
- packages/browser/src/clients/webWorkerClient.ts
- packages/javascript/src/api/requestAccessToken.ts
- packages/browser/src/index.ts
- packages/browser/src/helpers/authentication-helper.ts
- packages/javascript/src/api/loadOpenIDProviderConfiguration.ts
- packages/javascript/src/index.ts
- packages/express/src/models/index.ts
- packages/browser/src/helpers/spa-helper.ts
- packages/javascript/src/utils/clearSession.ts
- packages/javascript/src/utils/resolveEndpoints.ts
- packages/browser/src/worker/workerReceiver.ts
- packages/javascript/src/api/revokeAccessToken.ts
- packages/express/src/utils/ExpressUtils.ts
- packages/javascript/src/utils/resolveEndpointsExplicitly.ts
- packages/javascript/src/api/exchangeToken.ts
- packages/javascript/src/api/handleTokenResponse.ts
- packages/browser/src/clients/mainThreadClient.ts
- packages/express/src/middleware/authentication.ts
- packages/javascript/src/utils/replaceCustomGrantTemplateTags.ts
- packages/browser/src/AsgardeoSPAClient.ts
- packages/browser/src/web.worker.ts
- packages/javascript/src/api/validateIdToken.ts
- packages/browser/src/utils/http.ts
- packages/browser/src/worker/workerCore.ts
- packages/javascript/src/AsgardeoJavaScriptClient.ts
Comment on lines
+142
to
+155
| const authRedirectCallback: AuthURLCallback = (url: string) => { | ||
| if (url) { | ||
| Logger.debug('Redirecting to: ' + url); | ||
| res.cookie('ASGARDEO_SESSION_ID', userId, { | ||
| maxAge: AsgardeoExpressClient._clientConfig.cookieConfig?.maxAge ?? CookieConfig.defaultMaxAge, | ||
| httpOnly: AsgardeoExpressClient._clientConfig.cookieConfig?.httpOnly ?? CookieConfig.defaultHttpOnly, | ||
| sameSite: AsgardeoExpressClient._clientConfig.cookieConfig?.sameSite ?? CookieConfig.defaultSameSite, | ||
| secure: AsgardeoExpressClient._clientConfig.cookieConfig?.secure ?? CookieConfig.defaultSecure, | ||
| }); | ||
| res.redirect(url); | ||
|
|
||
| if (typeof next === 'function') { | ||
| next(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/express/src/AsgardeoExpressClient.ts | head -200Repository: asgardeo/javascript
Length of output: 8277
🏁 Script executed:
rg -A 5 "type AuthURLCallback" packages/Repository: asgardeo/javascript
Length of output: 45
🏁 Script executed:
rg -A 3 "type AuthURLCallback\|interface AuthURLCallback" --type typescriptRepository: asgardeo/javascript
Length of output: 98
🏁 Script executed:
rg "AuthURLCallback" --type-list | head -20Repository: asgardeo/javascript
Length of output: 453
🏁 Script executed:
rg "type AuthURLCallback\|interface AuthURLCallback" packages/Repository: asgardeo/javascript
Length of output: 45
🏁 Script executed:
rg "AuthURLCallback" . --max-count 10Repository: asgardeo/javascript
Length of output: 924
🏁 Script executed:
cat -n packages/node/src/models/url-callback.tsRepository: asgardeo/javascript
Length of output: 927
🏁 Script executed:
rg -B 5 -A 10 "authURLCallback\(" packages/node/src/AsgardeoNodeClient.ts | head -60Repository: asgardeo/javascript
Length of output: 422
🏁 Script executed:
# Verify the flow context - check what happens after authRedirectCallback returns in parent class
rg -B 3 -A 15 "authURLCallback\(authURL\)" packages/node/src/AsgardeoNodeClient.tsRepository: asgardeo/javascript
Length of output: 518
Remove the next() call after redirect.
In Express middleware, res.redirect() commits the response and closes the request lifecycle. Calling next() afterward attempts to continue the middleware chain on an already-committed response, which violates Express semantics and can cause "headers already sent" errors or unexpected middleware behavior.
Proposed fix
const authRedirectCallback: AuthURLCallback = (url: string) => {
if (url) {
Logger.debug('Redirecting to: ' + url);
res.cookie('ASGARDEO_SESSION_ID', userId, {
maxAge: AsgardeoExpressClient._clientConfig.cookieConfig?.maxAge ?? CookieConfig.defaultMaxAge,
httpOnly: AsgardeoExpressClient._clientConfig.cookieConfig?.httpOnly ?? CookieConfig.defaultHttpOnly,
sameSite: AsgardeoExpressClient._clientConfig.cookieConfig?.sameSite ?? CookieConfig.defaultSameSite,
secure: AsgardeoExpressClient._clientConfig.cookieConfig?.secure ?? CookieConfig.defaultSecure,
});
- res.redirect(url);
-
- if (typeof next === 'function') {
- next();
- }
+ res.redirect(url);
+ return;
}
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/express/src/AsgardeoExpressClient.ts` around lines 142 - 155, The
authRedirectCallback currently calls next() immediately after res.redirect(),
which can cause "headers already sent" errors; remove the conditional block that
invokes next() after the res.redirect() call inside authRedirectCallback so the
middleware chain stops once the redirect is issued (keep any other uses of next
elsewhere but do not call next() after res.redirect() in authRedirectCallback).
Comment on lines
+19
to
+21
| export * from './models'; | ||
|
|
||
| export {AsgardeoExpressClient} from './AsgardeoExpressClient'; |
There was a problem hiding this comment.
Keep a compatibility export for the removed legacy surface.
This removes published @asgardeo/express entrypoint symbols, so apps importing the old client/models will break on upgrade. Please leave a deprecated alias barrel here for a release window, or gate the removal behind an explicit breaking release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/express/src/index.ts` around lines 19 - 21, The PR removed the old
public entrypoint symbols causing breaking imports; restore a deprecated
compatibility barrel that re-exports the previous surface so existing consumers
keep working during the release window. Specifically, add back a compatibility
export in this module that re-exports the legacy symbols (e.g., the previously
published models export and the old client export names such as
AsgardeoExpressClient and any legacy aliases) and mark them deprecated in
comments, or alternatively gate removal behind an explicit major/ breaking
release; ensure the compatibility exports reference the existing './models' and
'./AsgardeoExpressClient' symbols so import paths and names continue to resolve.
Comment on lines
434
to
+435
| override getConfiguration(): T { | ||
| return this.asgardeo.getConfigData() as unknown as T; | ||
| return this.getConfigData() as unknown as T; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'getConfiguration\s*\(' packages/nextjs/src packages/node/src packages/javascript/src
rg -n -C2 'getConfigData\s*\(' packages/nextjs/src/AsgardeoNextClient.ts packages/node/src/AsgardeoNodeClient.ts packages/javascript/srcRepository: asgardeo/javascript
Length of output: 15839
🏁 Script executed:
# Check AsgardeoNextClient class definition and getConfigData
head -100 packages/nextjs/src/AsgardeoNextClient.ts | grep -E '(class AsgardeoNextClient|extends)'
# Look for getConfigData definition in AsgardeoNextClient
rg -n 'getConfigData' packages/nextjs/src/AsgardeoNextClient.ts | head -20
# Check parent class definition
rg -n 'class AsgardeoNodeClient' packages/node/src/AsgardeoNodeClient.ts -A 3Repository: asgardeo/javascript
Length of output: 1306
getConfiguration() returns a Promise at runtime while declaring synchronous return type.
The method signature declares override getConfiguration(): T but the implementation return this.getConfigData() as unknown as T returns a Promise. Since getConfigData() is async and inherited from AsgardeoNodeClient, callers receive a Promise object disguised as T at runtime. The type cast masks this from TypeScript. All current callers use await to work around this, but the type contract is broken—any code following the declared signature would fail at runtime.
Requires either making getConfiguration() async with return type Promise<T>, caching config for synchronous access, or a redesigned API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nextjs/src/AsgardeoNextClient.ts` around lines 434 - 435, The method
AsgardeoNextClient.override getConfiguration currently declares a synchronous
return type T but returns the async getConfigData() result; change
getConfiguration to be async and return Promise<T> (i.e., declare "override
async getConfiguration(): Promise<T>") and return await this.getConfigData()
cast to T so the runtime Promise matches the type; ensure the override matches
the async signature from AsgardeoNodeClient and update any callers to await the
returned Promise if necessary.
Comment on lines
+192
to
+193
| storageManager.removeSessionData(userId); | ||
| await storageManager.getTemporaryData(userId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'getTemporaryData|removeTemporaryData|clearTemporaryData|setTemporaryData' packagesRepository: asgardeo/javascript
Length of output: 19177
🏁 Script executed:
cd packages/node && sed -n '180,200p' src/AsgardeoNodeClient.tsRepository: asgardeo/javascript
Length of output: 594
Change getTemporaryData() to removeTemporaryData() on line 193.
After a failed token refresh, line 192 removes the session data but line 193 only reads the temporary data without deleting it. This leaves stale PKCE/state blob behind for the next sign-in attempt, contaminating the auth flow for the same user. Use await storageManager.removeTemporaryData(userId); instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/node/src/AsgardeoNodeClient.ts` around lines 192 - 193, The code
currently calls storageManager.removeSessionData(userId) then
storageManager.getTemporaryData(userId), which only reads temporary PKCE/state
data and leaves it behind; replace the second call with await
storageManager.removeTemporaryData(userId) so the temporary data (PKCE/state
blob) is deleted after a failed token refresh. Update the call site where
getTemporaryData is used (same function containing removeSessionData and userId)
to invoke removeTemporaryData and await it to ensure cleanup.
Comment on lines
+29
to
+30
| export * from './models'; | ||
| export * from './utils/NodeLogger'; |
There was a problem hiding this comment.
Keep a compatibility alias for the removed legacy exports.
This drops published @asgardeo/node entrypoint symbols, so existing consumers importing the legacy surface will hard-break on upgrade. Please leave deprecated aliases here for a release window, or gate this behind an explicit breaking release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/node/src/index.ts` around lines 29 - 30, Add back deprecated
compatibility aliases in the package entrypoint so existing consumers of the old
`@asgardeo/node` symbols don't break: in packages/node/src/index.ts keep the
current exports and also re-export the legacy public symbols (the original names
previously exposed from this entrypoint) as deprecated aliases that forward to
the new implementations (e.g., re-export any legacy types/functions/classes that
used to be provided from './models' and './utils/NodeLogger' under their old
names). Mark those aliases deprecated in comments and optionally gate them
behind a clearly named BREAKING_RELEASE flag if you want an explicit opt-in for
a future major release.
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
2 participants
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.
Purpose
Related Issues
Related PRs
Checklist
Security checks
Summary by CodeRabbit
Release Notes
Refactor
Chores