Skip to content

feat: enable coded action apps for SF [ACTN-11690] - #704

Open
Sandeepan-Ghosh-0312 wants to merge 5 commits into
mainfrom
feat/codedActionAppsSF
Open

feat: enable coded action apps for SF [ACTN-11690]#704
Sandeepan-Ghosh-0312 wants to merge 5 commits into
mainfrom
feat/codedActionAppsSF

Conversation

@Sandeepan-Ghosh-0312

@Sandeepan-Ghosh-0312 Sandeepan-Ghosh-0312 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Removing basedomain check to include Dedicated and SF environments

@Sandeepan-Ghosh-0312
Sandeepan-Ghosh-0312 requested a review from a team August 31, 2026 18:46
Comment thread src/core/auth/host-token-request.ts Outdated
Comment thread packages/coded-action-app/src/coded-action-app-service.ts Outdated
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review summary

One security issue found in both changed files.

endsWith on the raw origin string trusts unintended domains.
origin.endsWith('uipath.com') returns true for https://eviluipath.com because the string literal ends with those characters — any registrable domain whose name happens to terminate with uipath.com passes the check.

The fix is to parse the URL and check the hostname with a leading dot so only genuine subdomains match. Inline suggestions posted on both files:

  • src/core/auth/host-token-request.ts line 13
  • packages/coded-action-app/src/coded-action-app-service.ts line 188

@Sandeepan-Ghosh-0312 Sandeepan-Ghosh-0312 changed the title feat: enable coded action apps for SF feat: enable coded action apps for SF [ACTN-11690] Aug 31, 2026
Comment thread tests/unit/core/auth/host-token-request.test.ts Outdated
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review summary

New finding posted this run:

  • tests/unit/core/auth/host-token-request.test.ts line 57 — the 'returns false for an untrusted domain' test only exercises https://evil.example.com, which fails the endsWith check for an unrelated reason. The actual bypass vector (https://eviluipath.com) is not tested, so the suite gives a false green even with the vulnerable production code in place. Suggested adding both bypass cases as regression assertions.

The two existing open threads on the production-code security bug (host-token-request.ts:13 and coded-action-app-service.ts:188) remain open and unchanged; not re-raised here.

Comment thread tests/unit/core/auth/token-manager-embedded.test.ts Outdated
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding posted this run:

  • tests/unit/core/auth/token-manager-embedded.test.ts lines 40–41 — the updated mock for isValidHostOrigin uses origin.endsWith(suffix) on the raw origin string, which is the same vulnerable pattern this PR fixed in production code. 'https://eviluipath.com'.endsWith('uipath.com')true. The current test cases happen to work correctly (https://cloud.uipath.com and https://evil.example.com), but the mock doesn't accurately represent the real function and would mislead any future test that uses a look-alike domain. Suggested replacing with hostname-based checking to match production.

The three existing open threads on production-code and test security (host-token-request.ts, coded-action-app-service.ts, host-token-request.test.ts) remain open; not re-raised here. The production fixes in both service files are correct.

@sonarqubecloud

Copy link
Copy Markdown

… manager mock

The mock used origin.endsWith(suffix) on the raw origin string, the same
pattern this PR replaced in production code — it would trust a look-alike
registrable domain such as https://evil-uipath.com. Parse the URL and
compare the hostname so the mock matches isValidHostOrigin.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread packages/coded-action-app/src/coded-action-app-service.ts Outdated
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding posted this run:

  • packages/coded-action-app/src/coded-action-app-service.ts lines 193–194 — the catch block in isValidOrigin silently returns false without a console.warn, violating CLAUDE.md's convention that all swallowed catches must emit a warning. The companion isValidHostOrigin in host-token-request.ts correctly logs on catch; since the PR describes these two as mirroring each other, the inconsistency should be fixed.

All four previously-resolved threads were confirmed addressed in code — no threads unresolved.

The catch in isValidOrigin swallowed the URL parse failure silently, so a
malformed basedomain left no trace in the runtime console. Emit a
console.warn, matching the companion isValidHostOrigin.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/core/auth/host-token-request.ts Outdated
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding posted this run:

  • src/core/auth/host-token-request.ts lines 18–19 — the catch block in isValidHostOrigin uses the bare catch {} form, so the URL parse error is never bound and cannot be passed to console.warn. The companion isValidOrigin was just fixed in this PR to use catch (error) { console.warn(..., error) } — the same fix should be applied here for consistency and CLAUDE.md compliance (silent catches must emit console.warn(error)).

The catch discarded the error, so the warning didn't say why the origin URL
failed to parse. Bind it and pass it to console.warn, matching isValidOrigin;
update the assertion for the third argument.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

Host origins are customer-configurable in some environments (e.g. Service
Fabric) and follow no generic pattern, so a domain allowlist cannot express
which origins are legitimate. Remove the checks entirely:

- delete isValidHostOrigin and TRUSTED_HOST_DOMAINS from host-token-request
- rename trustedEmbeddingOrigin to hostEmbeddingOrigin, now derived from
  isHostEmbedded and embeddingOrigin alone
- drop the pre-listener trust guard and the sendMessageToParent check in
  ActionCenterTokenManager, keeping only the basedomain presence check
- delete isValidOrigin from CodedActionAppService; completeTask, getTask and
  sendMessageToParent now require only that basedomain is present

Origin pinning is unchanged: every request is sent to basedomain and only
messages whose event.origin matches it are accepted. Tests updated to assert
a non-uipath.com customer origin is used as given, replacing the untrusted
origin rejection cases.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant