Skip to content

[29.0][VIES Integration] Per-environmentdaily request rate-limit - #11737

Merged
dcenic merged 7 commits into
releases/29.0from
bugs/651007VIESCodeunit248BGAPINo-on-releases-29.0
Sep 24, 2026
Merged

dcenic merged 7 commits into
releases/29.0from
bugs/651007VIESCodeunit248BGAPINo-on-releases-29.0

Conversation

@dcenic

@dcenic dcenic commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

What & why

Codeunit 248 ("VAT Lookup Ext. Data Hndl") calls the EU VIES service over SOAP to validate VAT registration numbers. VIES is unauthenticated and rate-limits by source IP. In Business Central online, many tenants share the same outbound egress IP per app service, so when a single noisy tenant floods VIES, VIES deny-lists the shared IP and every co-located tenant starts seeing "VIES service unavailable" errors.

We have a concrete example: one tenant scheduled a job that called codeunit 248 ~1.66 million times in 8 days, getting the whole app service deny-listed by VIES.

This PR adds a per-tenant (per-environment) daily request rate-limit to codeunit 248 so no single tenant can flood VIES and deny-list the shared outbound IP.

How it works

  • At the start of OnRun (before the SOAP request), the codeunit reads a per-day counter, resets it when the UTC day changes, increments it, then persists and commits it before the outbound call.
  • The counter lives in a Base Application table (243 "VAT Reg. No. Lookup Quota", DataPerCompany = false) holding a single environment-wide row. Because the read/modify/write executes in Base App code under a table lock, every caller shares one counter — including per-tenant extensions that call codeunit 248 (e.g. a custom "Verify All" action) and job-queue / API callers. Extensions cannot read or reset it.
  • The cap is 2000 lookups/day — roughly 10x the 99th-percentile legitimate daily usage, and ~10x below the volume at which VIES deny-lists a shared IP.
  • Because the counter is gated in OnRun, it covers all VIES code paths (interactive, background, API, direct CODEUNIT.Run(248), and codeunit 249 field validation, which funnels into 248).
  • Online (SaaS) only. On-premises tenants own their own outbound IP and only affect themselves, so the limit does not apply there.

Enforcement

When a tenant reaches the daily cap, further lookups are blocked (Error(DailyQuotaExceededErr)) for the rest of the UTC day; the call that hits the limit also emits a security-audit entry and telemetry, and blocked lookups are not counted. The counter is stored in a dedicated table (243 "VAT Reg. No. Lookup Quota", Access = Internal), so the cap value or enforcement behavior can be adjusted later as a pure code change that can be serviced into release branches.

Why this replaces the earlier approach

This PR previously blocked codeunit 248 in background/API sessions. That was incomplete: a foreground "Verify All" over a large customer list (or a PTE action) still reaches VIES, and it would break legitimate low-volume automated callers. A per-environment daily quota is benign to legitimate users (well under the cap) while still stopping the bulk-flooding pattern from every session type.

Linked work

Fixes AB#651032

How I validated this

  • I read the full diff and it contains only changes I intended.
  • I added or updated tests for the new behavior.

What I tested and the outcome

New unit tests in ERM VAT VIES Lookup UT (codeunit 134193) exercise the quota decision logic directly (the check runs before the SOAP call, so no service/mock is needed). They use the Environment Info test library to simulate SaaS and internal test-only seams on codeunit 248 to drive the counter:

  • DailyVIESCallQuotaBlocksWhenLimitReached — lookups beyond the daily limit are blocked and blocked lookups are not counted.
  • DailyVIESCallQuotaResetsOnNewDay — after the day rolls over the counter resets, the customer gets a fresh full daily allowance, and the cap re-applies within the same day.
  • DailyVIESCallQuotaSkippedOnPrem — the quota does not apply on-premises (nothing counted or blocked).

Risk & compatibility

  • Online only — on-premises behavior is unchanged.
  • The pre-existing background/API guard in codeunit 249 field validation is intentionally left in place — it is a complementary reliability control, not a duplicate of this quota.

@dcenic
dcenic requested a review from a team September 22, 2026 13:20
@dcenic
dcenic requested a review from a team as a code owner September 22, 2026 13:20
@dcenic
dcenic enabled auto-merge (squash) September 22, 2026 13:21
@dcenic dcenic changed the title [29.x][VIES Integration] Disallow running codeunit 248 in background and API sessions [29.0][VIES Integration] Disallow running codeunit 248 in background and API sessions Sep 22, 2026
@github-actions github-actions Bot added the Team: Finance GitHub request for Finance area label Sep 22, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Sep 22, 2026
@dcenic dcenic closed this Sep 22, 2026
auto-merge was automatically disabled September 22, 2026 15:52

Pull request was closed

Replace the background/API session block for codeunit 248 with a per-environment
daily VIES lookup quota (table 243 "VAT Reg. No. Lookup Quota"), enforced on SaaS only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dcenic dcenic reopened this Sep 23, 2026
@dcenic dcenic changed the title [29.0][VIES Integration] Disallow running codeunit 248 in background and API sessions [29.0][VIES Integration] Per-tenant daily request rate-limit Sep 23, 2026
@dcenic dcenic changed the title [29.0][VIES Integration] Per-tenant daily request rate-limit [29.0][VIES Integration] Per-environmentdaily request rate-limit Sep 23, 2026
Add Tests-VAT to BaseApp internalsVisibleTo so codeunit 134193 can reach the
internal VIES quota test helpers on codeunit 248 (fixes AL0161 in the backport build).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dcenic
dcenic requested a review from a team as a code owner September 23, 2026 12:46
@dcenic
dcenic enabled auto-merge (squash) September 23, 2026 12:48
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This change adds a SaaS-only daily quota for EU VIES VAT registration number lookups. The counter is shared across companies in the environment, guarded by a table lock, and tested for blocking, daily reset, and on-premises bypass.

The shared counter and lock shape fit the reported flooding problem, but the current placement changes the transaction boundary of every lookup. The new quota commit happens before the service call and before several existing errors, so a failed validation can now persist unrelated caller work.

Problem-solution fit

Fit: Partial

The change targets the right failure mode: one environment can flood the unauthenticated VIES service and affect others sharing the outbound address. The implementation needs adjustment because it can commit caller work on a later failure and can count calls that do not actually send the standard VIES request.

Suggestions

S1 (🔴 High): Commit happens before lookup errors
The quota commit runs before the VIES request and before later errors can be raised. If the caller has pending changes, those changes can be committed even though validation fails. Persist the quota without committing the caller transaction before the lookup finishes.

S2 (🟠 Moderate): Quota is charged before handled event
The quota check runs before the event can decide that BaseApp will not send the standard VIES request. Subscribers that handle or replace the lookup can consume quota or be blocked even when no standard request is sent. Move the quota check into the standard VIES path after that decision.

Risk assessment and necessity

Risk: The new table and lock are narrow, but codeunit 248 is a shared validation path. The early commit can persist unrelated caller changes on failed validation, and the pre-event quota check can affect extensions that replace the standard lookup.

Necessity: A SaaS-only environment quota is justified because uncontrolled VIES traffic can deny-list a shared outbound address. The scope is appropriate, but the quota must not change existing transaction semantics or charge non-standard requests.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11737 round=1 by=alexei-dobriansky at=2026-09-23T13:05:24Z lastSha=7f5521451411f95a60b87ea09565f607c68589ee reviewKey=433926bae0888c295638c08223aae0d0aab0e9a0ee6adda3de94466b055ef34c suggestions=S1@297b8ed0,S2@2307d670

@dcenic

dcenic commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks alexei-dobriansky — investigated. Note this branch is a line-for-line backport of #11734, so the substantive design points here really apply to the main PR; the fixes will land on #11734 and be mirrored to the backports.

S1 (Commit before the lookup can fail) — agree, will address on #11734. The Commit() in RegisterAndCheckVIESCallQuota is intentional: it releases the environment-wide quota row lock before the multi-second SOAP call (otherwise every VIES lookup in the environment serializes on that single row) and keeps the counter durable across a caller rollback. You're right, though, that committing the ambient transaction also makes a caller's pending writes durable and prevents rollback on a later validation failure. Fix: update the counter in an isolated write so only the quota row is committed, instead of committing the caller's transaction.

S2 (charge only requests that can reach VIES) — agree. Confirmed the quota is registered in OnRun before the OnRunOnBeforeLookupVatRegistrationFromWebService (IsHandled) event and before the empty-VAT-number check raised in LookupVatRegistrationFromWebService. Fix: move the registration into the standard VIES path, after the IsHandled decision and the blank-number check, so handled/replaced lookups and blank-number errors don't consume daily quota.

So: S1 and S2 are fair and will be fixed on the main PR (#11734) and ported here; treating them as design refinements rather than backport-specific defects.

…ndard request path

Backport of the review fixes: charge the per-environment daily VIES quota only on the
standard request path (after the blank-number check and OnRun IsHandled event), and move
the quota read-modify-write and its Commit into dedicated codeunit 247 "VAT Lookup Quota Mgt.".

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dcenic

dcenic commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

alexei-dobriansky Pushed a change addressing the review.

S2 - done. The quota is now charged only on the standard VIES request path: RegisterAndCheckVIESCallQuota was removed from OnRun, and the quota is invoked inside SendRequestToVatRegistrationService, after the blank-number check and only on the not-IsHandled path. Handled/replaced lookups and blank-number errors no longer consume daily quota.

S1 - done (dedicated codeunit run). The quota read-modify-write and its Commit moved into a new dedicated codeunit 247 "VAT Lookup Quota Mgt.", invoked via a codeunit run immediately before the outbound request, so the counter commit is its own unit of work and the increment stays durable.

One honest note on transaction semantics: a same-session codeunit run + Commit still commits the ambient transaction (AL has no partial commit; only a separate session would fully isolate, which is incompatible with synchronous blocking). In practice codeunit 248 already commits around the outbound call in its normal flow (the existing Commit in LookupVatRegistrationFromWebService), so this does not add a new ambient-commit point - it confines the quota commit to real VIES sends and makes it durable.

S3 - the quota test seams moved off the shared codeunit 248 onto the dedicated codeunit 247 and remain internal (reachable only by Microsoft test apps via internalsVisibleTo).

dcenic and others added 2 commits September 23, 2026 20:51
…eached, not blocked

The audit entry fires on the last allowed lookup (the one that reaches the daily limit),
so the message should describe the limit being reached rather than the lookup being blocked.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…st transaction model

Address AL review agent feedback:
- Mark codeunit 247 "VAT Lookup Quota Mgt." as Access = Internal (implementation detail).
- Correct the doc/inline comments so they no longer imply the dedicated codeunit run isolates
  the transaction: the Commit also commits the caller's ambient transaction (the same boundary
  codeunit 248 already commits at around the outbound call).
- Mark the quota tests with TransactionModel::AutoCommit since they commit the counter.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dcenic

dcenic commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on the review state: the Good Sense Reviewer - Round 1 (Request Changes) above was raised against the first version of this work (blocking codeunit 248 in background/API sessions), before the per-environment daily VIES rate-limit was ported here. It no longer reflects the current code on this branch.

The current implementation matches main #11734: a SaaS-only, table-backed daily quota (table 243 "VAT Reg. No. Lookup Quota") charged on the standard VIES request path via a dedicated codeunit 247 "VAT Lookup Quota Mgt.". The design review (S1-S5) and the AL review agent findings were addressed on #11734 and mirrored here:

  • Quota charged only on the standard path, after the blank-number check and the OnRun IsHandled event.
  • Quota counter read-modify-write + commit moved into dedicated codeunit 247.
  • Audit message reworded to say the daily limit was reached (not "blocked").
  • Codeunit 247 set to Access = Internal; quota tests marked TransactionModel::AutoCommit.

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Request Changes

What this PR does

This round moves the daily VIES quota onto the standard request path, after the handled-event decision and blank-number check. It also moves the locked counter into an internal codeunit, clarifies the audit message, and updates the tests for committed quota writes.

The placement change resolves the quota-charging concern. However, the dedicated codeunit does not create an isolated transaction: its explicit Commit() still commits ambient caller changes before request preparation, the SOAP call, and later errors.

Status of previous suggestions
ID Title Status Author response
S1 Commit happens before lookup errors Disputed The existing standard callers are expected to start at a clean boundary, and codeunit 248 already commits after a successful lookup. The new commit still changes failed-lookup behavior for other callers.
S2 Quota is charged before handled event Addressed The quota now runs after the handled-event decision and blank-number check.
New observations (commits since round 1)

None - the new commits address the previous placement concern and refine the quota implementation.

Risk assessment and necessity

Risk: The locked single-row table addresses concurrent lost updates, and the standard-path placement avoids charging handled or blank requests. The remaining data-integrity risk is that a service or request-preparation error can occur after the quota code commits unrelated ambient changes; codeunit 248 can also be called by extensions outside the known standard callers.

Necessity: Protecting the shared VIES endpoint from excessive traffic is necessary, and the per-environment SaaS quota fits that problem. The transaction boundary must still be safe for failed lookups before this can merge. The pull-request build is still running.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11737 round=2 by=alexei-dobriansky at=2026-09-23T19:21:10Z lastSha=e591bbc85876918d3ad6b765b1f3561c5e2d7df9 reviewKey=115385cc4161ea6b472020162aa1ae86872fe4a487b04a95211913ed53a013a0 suggestions=S1@297b8ed0:disputed,S2@2307d670:addressed parentRound=1

…a state per test

- Rename DailyQuotaReachedMsg -> DailyQuotaReachedTxt (telemetry/locked string convention).
- Reset the VIES quota row and SaaS testability flag in the test Initialize() so an AutoCommit
  quota test that fails mid-way cannot leak committed state into later tests under a non-isolated
  test runner.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 3

Recommendation: Request Changes

What this PR does

This round only renames the quota telemetry label from Msg to Txt and resets the committed quota test state at the start of each test. Those changes are fine and reduce test leakage risk.

The main quota design is still the same. The per-environment SaaS quota is well targeted, but the quota code still commits before request preparation, the outbound VIES call, and later error handling can finish.

Status of previous suggestions
ID Title Status Author response
S1 Commit happens before lookup errors Disputed The previous explanation says the known standard callers start at a clean boundary, but codeunit 248 is reusable and this commit still changes failed-lookup behavior for other callers.
S2 Quota is charged before handled event Addressed The quota now runs after the handled-event decision and blank-number check.
New observations (commits since round 2)

None - the new commit only renames a telemetry label and resets quota test state between tests.

Risk assessment and necessity

Risk: The single locked quota row prevents lost counter updates, and the new test reset avoids committed quota state leaking between tests. The remaining data-integrity risk is unchanged: a failure after the quota commit can leave unrelated caller changes saved even though the lookup failed.

Necessity: Limiting VIES traffic per online environment is necessary and the quota is the right shape for the reported bulk-flooding problem. The implementation still needs a transaction-safe way to make the quota durable without committing the caller's ambient work before failure-prone lookup code runs.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11737 round=3 by=alexei-dobriansky at=2026-09-24T01:05:06Z lastSha=e1c1a23c20ae10937417077d921ab3ad3582df06 reviewKey=c9b29715cbc197efa1b8fda3569a8cda0669e03b09d19fa683ef2f24d1a7026f suggestions=S1@297b8ed0:disputed,S2@2307d670:addressed parentRound=2

@dcenic
dcenic merged commit 57f9ef8 into releases/29.0 Sep 24, 2026
172 checks passed
@dcenic
dcenic deleted the bugs/651007VIESCodeunit248BGAPINo-on-releases-29.0 branch September 24, 2026 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants