Skip to content

[AI Business Solutions] Enforce allow-listed Redirect URIs, token audiences and authorities for OAuth2 integrations - #11016

Open
dcenic wants to merge 22 commits into
mainfrom
features/648738InterCompanyTenantPinAuthCheckURL
Open

[AI Business Solutions] Enforce allow-listed Redirect URIs, token audiences and authorities for OAuth2 integrations#11016
dcenic wants to merge 22 commits into
mainfrom
features/648738InterCompanyTenantPinAuthCheckURL

Conversation

@dcenic

@dcenic dcenic commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What & why

InterCompany :
We should construct authority from tenant GUID, use Https and login.microsoftonline.com (and PPE endpoint for PPE). Currently it is taken from the setup record and we don’t have to do this, not even for Embed ISVs;
Harden validation of destination host.
SMTP custom OAuth:
No action: authority and audience are fixed already.
Outlook REST customer app :
Check the redirect URI for its authorization-code flow. We can do this by replacing the current “valid URI” check with an exact comparison against OAuth2.GetDefaultRedirectUrl()
SharePoint Connector:
Advise replacing broad Sites.ReadWrite.All with Sites.Selected, followed up by per-site granting of write permissions via Graph API, Graph Powershell, or Microsoft 365 CLI. This is a documentation and tooltip change: Change the guidance in it README.md: line 3.Change the tooltip in ExtSharePointAccount.Table.al: line 83.
SII Setup
Emit security audit and ordinary telemetry if target URL is invalid.

## Linked work

Fixes AB#648738

How I validated this

  • I read the full diff and it contains only changes I intended.
  • I built the affected app(s) locally with no new analyzer warnings.
  • I ran the change in Business Central and confirmed it behaves as expected.
  • I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Risk & compatibility

Low

…iences and authorities for OAuth2 integrations
@dcenic
dcenic requested a review from a team September 3, 2026 14:07
@dcenic
dcenic requested review from a team as code owners September 3, 2026 14:07
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Ownership: Needs Review Ownership is Other, low confidence, or needs manual correction Team: Other GitHub request for other area than SCM, Finance or Integration labels Sep 3, 2026
@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 3, 2026
Comment thread src/Apps/W1/Email - Outlook REST API/app/src/EmailOAuthClient.Codeunit.al Outdated
Comment thread src/Apps/W1/Email - Outlook REST API/app/src/EmailOAuthClient.Codeunit.al Outdated
…iences and authorities for OAuth2 integrations
…iences and authorities for OAuth2 integrations
…iences and authorities for OAuth2 integrations
…iences and authorities for OAuth2 integrations
Comment thread src/Apps/W1/Email - Outlook REST API/app/src/EmailOAuthClient.Codeunit.al Outdated
Comment thread src/Apps/W1/Email - Outlook REST API/app/src/EmailOAuthClient.Codeunit.al Outdated
Comment thread src/Layers/W1/Tests/ERM/ERMIntercompany.Codeunit.al Outdated
Comment thread src/Layers/ES/BaseApp/Local/SII/EServices/EDocument/SIISetup.Table.al Outdated
@github-actions

This comment was marked as resolved.

…iences and authorities for OAuth2 integrations
@dcenic

This comment was marked as resolved.

…iences and authorities for OAuth2 integrations
Comment thread src/Apps/W1/External File Storage - SharePoint Connector/App/README.md Outdated
Comment thread src/Layers/W1/Tests/ERM/ERMIntercompany.Codeunit.al
Comment thread src/Layers/W1/Tests/ERM/ERMIntercompany.Codeunit.al
…iences and authorities for OAuth2 integrations
…iences and authorities for OAuth2 integrations
Error(RedirectUrlErrorInfo);
end;

internal procedure RestoreDefaultRedirectUrl(RedirectUrlErrorInfo: ErrorInfo)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

This PR adds the stateful RestoreDefaultRedirectUrl recovery branch and routes page validation through ValidateRedirectUrl, but the added Outlook tests only cover the allow/block decision on ValidateRedirectUrl. Add a test that invokes RestoreDefaultRedirectUrl (or the page flow that surfaces it) and verifies the setup record is created or updated to OAuth2.GetDefaultRedirectUrl(), so the new recovery behavior cannot regress silently.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

ValidateEndpointUrl now extracts the rejected host and falls back to '(unparsable host)' for malformed input, but the updated tests assert only the generic allow-list fragment. Add checks for the host-specific text on an HTTP URL and for the unparsable-host fallback on malformed input, otherwise regressions in GetHostFromUrl() can still leave these negative tests green.

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6


if UrlHelper.IsPPE() then
UnexpectedDomain := StrPos(LowerCase(Url), '.dynamics-tie.com') = 0;
local procedure NotifyIfUntrustedConnectionUrl()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

CrossIntercomp. Partner Setup now sends and recalls a local notification for untrusted URLs, and this PR also hides the Redirect URL field on the intercompany setup pages, but the accompanying ERMIntercompany additions only unit-test connector helper methods. Add a TestPage scenario with notification handlers plus field-visibility assertions so the new page behavior is covered by tests instead of relying on manual verification.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6

…iences and authorities for OAuth2 integrations
exit(BearerAccessToken);
end;

internal procedure ValidateDestinationUrl(DestinationUrl: Text): Boolean

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

In codeunit 560, ValidateDestinationUrl and GetValidatedTokenEndpoint are declared internal, but in the current worktree their only production callers are inside the same codeunit. Narrowing these wrappers to local would keep the internal surface aligned with actual consumers and avoid exposing helper entry points more broadly than necessary.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Testing}$

SIISetup.ValidateEndpointUrl now raises 'The endpoint host %1 is not on the allow-list for this feature.' with only the parsed host as %1, but the existing negative tests in src/Layers/ES/Tests/Local/SIIEndpointUrlValidation.Codeunit.al (ValidateEndpointUrlRejectsHttpUrl and ValidateEndpointUrlRejectsMalformedUrl) still assert the old message via Assert.ExpectedError(StrSubstNo(InvalidEndpointUrlErr, NonHttpsUrl/MalformedUrl)) using the old label text and the full URL as the substitution value. I verified this directly in the worktree: the old label 'The endpoint URL %1 is not on the allow-list...' is still declared and used only in the test file, while production now emits 'The endpoint host %1 ...' with a bare host (or the '(unparsable host)' token for malformed input). Since Assert.ExpectedError does a substring match, the expected substrings no longer appear in the actual error text, so both tests will fail after this change ships. This is a confirmed CI-breaking regression, not just missing coverage.

Posting this finding as an issue comment because inline comment placement failed.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6

exit(true);
end;

internal procedure IsDestinationUrlTrusted(DestinationUrl: Text): Boolean

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

The new ERM coverage stops at IsDestinationUrlAllowed(..., suffix) and GetValidatedTokenEndpointForAuthority(..., authority). The production entry points added in this PR are IsDestinationUrlTrusted and GetValidatedTokenEndpoint, which add the PROD/PPE branching through UrlHelper.IsPPE()/IsPROD(). A wrong environment mapping or wrong trusted suffix/authority would leave the added tests green while the live intercompany connection flow rejects valid endpoints or accepts invalid ones. Add direct tests that exercise the public wrapper methods in both PROD and PPE contexts.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6

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

Labels

AL: Apps (W1) Add-on apps for W1 Ownership: Needs Review Ownership is Other, low confidence, or needs manual correction Team: Other GitHub request for other area than SCM, Finance or Integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants