fix(core): reject a config carrying both auth methods at construction [APPS-37257] - #722
fix(core): reject a config carrying both auth methods at construction [APPS-37257]#722amrit-agarwal-1 wants to merge 2 commits into
Conversation
|
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
57e5de0 to
91dc8b8
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const named = (['clientId', 'redirectUri', 'scope'] as const).filter((field) => found[field]); | ||
| gaps.push(named.length > 0 | ||
| ? `the OAuth configuration is incomplete — ${named.join(', ')} set, ` + | ||
| `${(['clientId', 'redirectUri', 'scope'] as const).filter((f) => !found[f]).join(', ')} missing` |
There was a problem hiding this comment.
['clientId', 'redirectUri', 'scope'] as const is constructed twice in four lines, and AUTH_FIELDS already lives at module level. Per the conventions, static arrays that don't change between calls belong at module scope — repeated inline literals hide structure and get rebuilt on every invocation.
Extract a module-level constant alongside AUTH_FIELDS:
| const named = (['clientId', 'redirectUri', 'scope'] as const).filter((field) => found[field]); | |
| gaps.push(named.length > 0 | |
| ? `the OAuth configuration is incomplete — ${named.join(', ')} set, ` + | |
| `${(['clientId', 'redirectUri', 'scope'] as const).filter((f) => !found[f]).join(', ')} missing` | |
| const OAUTH_AUTH_FIELDS = ['clientId', 'redirectUri', 'scope'] as const; | |
| type OAuthAuthField = typeof OAUTH_AUTH_FIELDS[number]; |
Then describeGaps becomes:
const named = OAUTH_AUTH_FIELDS.filter((field) => found[field]);
gaps.push(named.length > 0
? `the OAuth configuration is incomplete — ${named.join(', ')} set, ` +
`${OAUTH_AUTH_FIELDS.filter((f) => !found[f]).join(', ')} missing`
: 'no authentication method set (needs `secret`, or clientId, redirectUri and scope)');(Line 51's ['baseUrl', 'orgName', 'tenantName'] as const has the same issue and can be extracted similarly as BASE_CONFIG_FIELDS.)
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
Raina451
left a comment
Raina451
left a comment
There was a problem hiding this comment.
this fix catches the conflict at runtime right? basically in browser, when user do npm run dev, it wont fail at that time , this error will be visible in browser console at runtime.
can we fix this at build time itself? something with type check or so?
This ticket specifically can't be caught at build time because the reporter's argument new UiPath({ baseUrl, orgName, tenantName, secret }) is valid on its own, and is the documented way to use secret auth; no type could reject it without rejecting correct code. The conflict only came into existence once that was merged with the OAuth tags on the page, and those are written into index.html at deploy time, rewritten later if the app's client id changes, with no rebuild - so the same bundle is valid in one tenant and invalid in another. Tightening the parameter type also overshoots the runtime rule: it rejects {secret, scope} and {secret, clientId}, which work today because hasOAuthConfig requires all three fields. |
… [APPS-37257]
`#mergeConfigSources` elects an auth method only when the caller names exactly
one of them. When the caller names a secret *and* at least one OAuth field, the
election XOR is false, nothing is dropped, and the merged config carries both
methods — so `isCompleteConfig`'s XOR fails and the constructor silently stores
`#partialConfig` without registering internals. The caller first hears about it
from an unrelated service constructor ("Invalid SDK instance"), or from
`initialize()` reporting a configuration that was never "not found".
The method's own comment claimed the case was "left intact for validateConfig()
to reject". It was not: `validateConfig`'s only call site sits inside
`#initializeWithConfig`, which runs only once `isCompleteConfig` has returned
true — precisely what a both-methods config makes false. The function was
unreachable dead code and is removed.
- Guard the merge, after the existing drop block, so PR #675's cross-source
resolution is preserved: a pure secret over injected OAuth meta tags still
works. Only a caller contradicting themselves throws.
- `conflictingAuthMessage` names every auth field and the layer that supplied it
(constructor argument / `<meta name="uipath:...">` / env var), and states which
fields to remove. It never interpolates a field value — `secret` is a bearer
token and this string reaches browser consoles.
- `missingConfigMessage` now describes a config that was found but is short,
instead of claiming none was found.
- Export `PartialUiPathConfig`, `BaseConfig` and `OAuthFields` from `/core`. The
constructor's declared parameter type was unreachable from that entry point,
which is what pushed callers to `as unknown as ConstructorParameters<...>[0]`.
- Drop `scope` from the secret-auth JSDoc example: pasted into a coded app it
inherits the other two OAuth fields from the meta tags and hits the new guard.
Every input that now throws already failed — later, and somewhere else.
Verified: typecheck, oxlint, 2760/2760 unit tests, build; and end to end against
the built dist with the dev plugin's meta tags faked — the reporter's scenario,
their explicit-undefined workaround, and `new UiPath()` with no config all still
construct and register.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
91dc8b8 to
d258ff2
Compare
|
One new finding posted this run: misleading test description in |
Review feedback on #722: - The precedence admonition in docs/authentication.md said the same thing three ways over thirteen lines. Four lines, same facts. - `falls back to generic guidance when no layer named an OAuth field` was wrong: the test's own `metaConfig` names all three OAuth fields. The condition is that the *constructor argument* named none — `namedOAuth` filters `sources.config` only — so a failure would have read as if no layer had OAuth at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
2 participants
Fixes APPS-37257.
Re-scoped: the ticket's literal case already ships
The ticket reports a pure secret config silently deferring in a coded app whose dev plugin injected
OAuth meta tags. That case was fixed by #675 —
#mergeConfigSourceselects the auth method anddrops the loser, so a caller naming only
secretworks. The reporter is on 1.6.1, which predates it;the 1.6.x remedy is to upgrade to >= 1.7.0, and this PR does not touch that path.
What is still broken on
mainWhen the caller names a secret and at least one OAuth field, the election XOR
(
namesSecret !== namesOAuth) is false, so nothing is dropped. The merged config then carries bothmethods,
isCompleteConfig's XOR fails, and the constructor silently stores#partialConfigwithoutcalling
SDKInternalsRegistry.set. The caller first learns about it from an unrelated serviceconstructor (
Invalid SDK instance...), or frominitialize()reporting a configuration that wasnever "not found".
The method's own comment claimed this case was "left intact for
validateConfig()to reject". It wasnot.
validateConfig's only call site is inside#initializeWithConfig, which runs only onceisCompleteConfighas returned true — precisely what a both-methods config makes false. Thefunction was unreachable dead code and is removed rather than resurrected.
Changes
#mergeConfigSources, placed after feat(core): zero-config authentication from the execution context #675's drop block so cross-source resolution ispreserved. Only a caller contradicting themselves throws. Inside the merge rather than the
constructor body, so
#loadConfiggets the same accurate error and the throw lands beforetelemetryClient.initializeandtrackEvent('Sdk.Auth').conflictingAuthMessagenames every auth field and the layer that supplied it — the constructorargument,
<meta name="uipath:...">, or the env var — and says which fields to remove. It neverinterpolates a field value:
secretis a bearer token and this string reaches browser consoles andlog pipelines. Pinned by a test.
missingConfigMessage(found?)now describes a config that was found but is short, instead ofclaiming none was found. The zero-argument text is byte-identical, so existing assertions hold.
PartialUiPathConfig,BaseConfig,OAuthFieldsfrom/core. The constructor's declaredparameter type was unreachable from that entry point, which is what pushed the reporter to
as unknown as ConstructorParameters<typeof UiPath>[0]. Same four names the root barrel exports — noentry-point mixing, which would duplicate
SDKInternalsRegistry.scopefrom the secret-auth JSDoc example (src/core/index.ts). Pasted into a coded appit inherits the other two OAuth fields from the injected meta tags and lands on the new guard. It was
the only artifact in the repo the guard breaks, it never typechecked against
UiPathSDKConfig, andscopewas never load-bearing there sincehasOAuthConfigrequires all three fields.docs/authentication.mdand the redirect-URI tip indocs/coded-apps/getting-started.mdnow say that precedence is per field, so omitted fields areinherited. Note the ticket has this backwards — constructor config merges over meta tags, not under.
.claude/skills/onboard-api/references/e2e-testing.mdquoted the old error string verbatim for thescopes-plural mistake; updated so it stays accurate.Verification
npm run typecheckclean ·npm run lint0 warnings 0 errors · 2760/2760 unit tests ·npm run buildclean, with all four config type names present indist/core/index.d.ts.End to end against the built
dist, with the dev plugin's meta tags faked:{...base, secret}— the ticket / #675isInitialized=true,new Cases(sdk)OK{...base, secret, scope}— the residue{...base, secret, clientId: undefined, ...}new UiPath()— documented coded-app defaultnew Cases(sdk)OK15 new/rewritten tests. Reviewers mutation-tested them: 11 fail on a full revert of the source change,
and the 4 that pass either way are the deliberate guardrails — each fails under the specific prohibited
mutation it exists to catch (predicate widened to any-OAuth-field, key-presence instead of truthiness,
guard moved before the drop block).
Deliberately out of scope
Filed separately rather than smuggled in:
improves the diagnosis for those; the deferral itself is load-bearing for the documented
no-argument coded-app pattern.
<meta name="uipath:client-id" content="">, soeven
new UiPath()never registers. That is a backend-side defect this change only re-labels.Sdk.Authevent to measure how often the cross-sourceresolution fires. Explicitly not a
console.warn— that would fire on every page load of everydeployed secret-mode coded app, a pattern
docs/authentication.mdsanctions.UiPathSDKConfig's?: neverto?: undefined. The reporter's cast is caused byexactOptionalPropertyTypes, which no tsconfig here sets; thatneveris the compile-time half ofthis guard. Wants their
compilerOptionsfirst.Related: STUD-81240 and
AGVSOL-3239 are two more reports of the same
Invalid SDK instancestring from different causes — module-scope service construction and a deployedblank page. #675 already improved that message; neither is touched here.
🤖 Generated with Claude Code