[VIES Integration] Per-environment daily request rate-limit - #11734
Conversation
Good Sense Reviewer - Round 1Recommendation: AcceptWhat this PR doesThis change blocks direct execution of the VIES lookup codeunit from SaaS API, SOAP, OData, and background sessions, while leaving interactive validation and on-prem use unchanged. The guard runs before the service call and before any VAT registration log setup, so automated callers cannot reach the external VIES endpoint through codeunit 248. The placement looks correct for the reported problem. The existing codeunit 249 path already exits for API and non-interactive sessions before it runs codeunit 248, and this change closes the direct-codeunit path that could bypass that protection. Problem-solution fitFit: Strong The bug describes repeated non-interactive VIES calls from SaaS causing shared outbound access to be blocked. The change targets that exact path by rejecting non-interactive SaaS execution at the start of codeunit 248, without changing interactive validation. SuggestionsNone. Risk assessment and necessityRisk: The main regression surface is extensions or jobs that directly run codeunit 248 in SaaS; that is the intended behavior for this fix. Interactive UI validation remains allowed, the existing codeunit 249 caller path is already non-interactive-safe, and on-prem environments are not restricted. Necessity: The change is needed because blocking only the higher-level caller still leaves a direct automated path to the shared VIES service. The scope is narrow and matches the incident pattern.
|
ed031ec
dcenic
changed the title
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThe new round replaces the automated-session block with a SaaS-only daily quota for VIES lookups. The quota is stored in module-scoped isolated storage, emits telemetry in log-only mode, and has tests for blocking, log-only behavior, day rollover, and on-prem skip behavior. The quota design fits the shared-egress problem, but the implementation now runs before the normal validation and web-service path. It writes the quota and commits immediately, so later errors can leave unrelated caller work committed even though the validation failed. Status of previous suggestionsNone - the previous round had no suggestions. New observations (commits since round 1)S1 (🔴 High): Avoid committing caller work before VIES fails S2 (🔴 High): Make the quota increment atomic S3 (🟠 Moderate): Use a stable telemetry event id Risk assessment and necessityRisk: High until the transaction issue is fixed. Codeunit 248 can be called from validation, APIs, background sessions, and extensions, and the new Commit() can make caller data durable before a later validation or service error. The non-atomic counter also weakens the protection when calls arrive concurrently. Necessity: The shared VIES outbound address needs protection from high-volume tenant traffic, so the quota is a valid direction. The implementation should be adjusted before merge because the current transaction and concurrency behavior can create new reliability and data-integrity risks.
|
This comment was marked as duplicate.
This comment was marked as duplicate.
Good Sense Reviewer - Round 3Recommendation: Request ChangesWhat this PR doesThis round assigns a stable telemetry id to the daily VIES quota signal. The daily counter, SaaS-only behavior, log-only rollout, and tests are otherwise unchanged. The transaction concern from the previous round is resolved: documented Status of previous suggestions
New observations (commits since round 2)None - the new commit only addresses S3. Risk assessment and necessityRisk: S2 remains a concurrency blocker. With several parallel sessions, each loop can read the same count and overwrite the same next value, so actual VIES traffic can approach the deny-list volume while the stored counter is still below 2000. The current tests cover sequential calls only, and the full pull-request build is still running. Necessity: Protecting the shared VIES endpoint is important, and a tenant-wide quota is a reasonable direction. The counter must reliably limit parallel callers before enforcement is enabled; the telemetry-id correction is complete.
|
Thanks for the round-3 review. Responses to S1–S3: S1 — Avoid committing caller work before VIES fails. After checking the transaction semantics, I'd keep the commit rather than restructure — it's placed at the very top of OnRun, before any other work, and that is safe here: Codeunit.Run is an isolated, implicitly-committed transaction scope. Per the Codeunit.Run docs: "If you're already in a transaction you must commit first before calling Codeunit.Run." So codeunit 248 begins at a clean commit boundary — there is no uncommitted caller work in scope that an early commit could inadvertently persist. LockTable() → get-or-insert the single row → reset on a new UTC day → check the cap → increment → Modify() → Commit(), all before the SOAP call. The row lock serializes concurrent sessions, so parallel API/job-queue callers can no longer overwrite each other's increment — the count is exact, not best-effort. S3 — Stable telemetry event id. Done — the quota-reached signal uses the registered id 0000VL7, consistent with the nearby VIES telemetry. |
dcenic
changed the title
Good Sense Reviewer - Round 4Recommendation: Accept with SuggestionsWhat this PR doesThis round replaces the previous quota storage with a SaaS-only, table-backed daily quota that is enforced before the VIES call. The single non-company row is locked for the read-modify-write and committed before the outbound request, so the earlier steady-state concurrency concern is addressed. The change fits the shared-egress problem and now blocks excess calls instead of only logging them. Two non-blocking issues remain: locally invalid blank requests can consume quota, and the audit entry describes the last allowed lookup as blocked. Status of previous suggestions
New observations (commits since round 3)S4 (🟠 Moderate): Do not count locally invalid requests S5 (🟠 Moderate): Do not audit an allowed lookup as blocked Risk assessment and necessityRisk: the main regression surface is SaaS VAT validation across all companies in one environment. The locked table removes the previous lost-update risk, but invalid local calls can still burn the shared quota and the audit trail can describe an allowed lookup as a blocked one. Necessity: the change is necessary because uncontrolled VIES traffic from one environment can break validation for co-located environments on the same outbound address. A per-environment SaaS quota is the right scope, and the remaining findings are fixable without changing that design.
|
…ndard request path Address review feedback on the per-environment daily VIES quota: - S2: charge the quota only on the standard VIES request path, after the blank-number check and the OnRun IsHandled event, so handled/replaced or invalid lookups that never contact VIES no longer consume quota. - S1: extract the quota read-modify-write and its Commit into a dedicated codeunit 247 "VAT Lookup Quota Mgt." invoked via a codeunit run, so the counter commit is its own unit of work placed immediately before the outbound request. Note: codeunit 248 already commits around the outbound call in its normal flow, so this does not introduce a new ambient-commit point; it makes the quota increment durable and confines it to real VIES sends. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
alexei-dobriansky Pushed a change addressing the review. S2 - done. The quota is now charged only on the standard VIES request path: S1 - done (dedicated codeunit run). The quota read-modify-write and its One honest note on transaction semantics: a same-session codeunit run + |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…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>
|
Thanks for the AL review agent findings - addressed three, respectfully declining the interface one. Commit() commits caller's ambient transaction (Medium) - comments fixed. You're right that a same-session codeunit run does not create an isolated transaction. This was a deliberate design choice (dedicated run + Commit) but the comments overclaimed isolation. Reworded the doc/inline comments in codeunit 247 and codeunit 248 to state plainly that the Commit also commits the caller's ambient transaction - the same boundary codeunit 248 already commits at around the outbound call (existing Access = Internal on codeunit 247 (Medium) - done. Set Test TransactionModel (High) - done. The three quota tests now declare Extract an interface for the quota (High) - declining. For an internal, single-purpose quota counter in the Base Application, an interface + dependency injection is more indirection than this warrants. The logic is already unit-tested directly via internal test seams on codeunit 247 ( Also, for completeness: the earlier S4 (charge only requests that reach VIES) was already handled when the quota moved onto the standard request path after the blank-number check (commit ade6298), and S5 (audit wording) was reworded to say the daily limit was reached rather than "blocked" (commit 4349fed). |
Good Sense Reviewer - Round 5Recommendation: Accept with SuggestionsWhat this PR doesThis round moves quota enforcement to the standard VIES request path, after local validation and before the outbound call. It also extracts the locked counter into an internal codeunit and updates the audit wording and tests. Blank and subscriber-handled requests no longer consume quota, so the main correctness issue from the previous round is fixed. The audit text is clearer, but the last allowed lookup is still recorded with a failure result. Status of previous suggestions
New observations (commits since round 4)None. Risk assessment and necessityRisk: the quota now covers only requests that can reach VIES, and the locked environment-wide counter handles concurrent sessions. The remaining issue affects audit accuracy rather than quota enforcement. Necessity: uncontrolled traffic can deny-list a shared outbound address and disrupt other environments. The SaaS-only environment quota remains a targeted and necessary safeguard.
|
…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>
For S5, we do't care if the actual VIES call is successful or not. We want to log an audit failure if an environment reaches over 2000 calls per day. Whether they exceed it with 2001 calls or just make it with 2000 calls, it makes no big difference one call more or less. They have hit the ceiling and we want to raise a security audit failure. |
Good Sense Reviewer - Round 6Recommendation: Accept with SuggestionsWhat this PR doesThis round renames the quota telemetry label variable to follow the locked-text convention and resets committed quota test state in The net change still enforces a SaaS-only daily VIES lookup quota with a locked shared counter before the outbound call. The latest commit does not change runtime behavior, and the remaining audit issue is still limited to audit accuracy. Status of previous suggestions
New observations (commits since round 5)None - the latest changes only rename a label variable and reset test state before each test. S5 (🟠 Moderate): Do not audit an allowed lookup as blocked Risk assessment and necessityRisk: the latest runtime change is only a label variable rename, so quota behavior is unchanged. The locked environment-wide counter still handles concurrent sessions; the remaining risk is that audit data can show an allowed lookup as an authorization failure. Necessity: uncontrolled VIES traffic can deny-list a shared outbound address and break validation for other environments. The SaaS-only quota remains a targeted safeguard, and the added test cleanup is needed to keep committed quota state from leaking between tests.
|
7 participants
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
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.OnRun, it covers all VIES code paths (interactive, background, API, directCODEUNIT.Run(248), and codeunit 249 field validation, which funnels into 248).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#651007
How I validated this
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