-
Notifications
You must be signed in to change notification settings - Fork 0
Backend stabilization: subscription enforcement and idempotency security fix #145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: ludovit-scholtz <[email protected]>
…ement Co-authored-by: ludovit-scholtz <[email protected]>
…t parameter validation Co-authored-by: ludovit-scholtz <[email protected]>
Co-authored-by: ludovit-scholtz <[email protected]>
|
✅ CI checks passed! 📄 OpenAPI specification generated and available as workflow artifact: |
1 similar comment
|
✅ CI checks passed! 📄 OpenAPI specification generated and available as workflow artifact: |
|
✅ CI checks passed! 📄 OpenAPI specification generated and available as workflow artifact: |
2 similar comments
|
✅ CI checks passed! 📄 OpenAPI specification generated and available as workflow artifact: |
|
✅ CI checks passed! 📄 OpenAPI specification generated and available as workflow artifact: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds subscription-based enforcement for token deployments and fixes a critical security vulnerability in the idempotency implementation. The changes enable the MVP subscription business model by preventing free users from deploying unlimited tokens and fixing a bypass vulnerability where attackers could reuse idempotency keys with different parameters to circumvent business logic.
Changes:
- Added subscription tier enforcement to 11 token deployment endpoints (ERC20, ASA, ARC3, ARC200, ARC1400) with tiered limits: Free (3), Basic (10), Premium (50), Enterprise (unlimited)
- Fixed idempotency security vulnerability by hashing and validating request parameters, preventing idempotency key reuse with different request data
- Added 18 comprehensive unit tests covering subscription enforcement and idempotency security scenarios
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| BiatecTokensApi/Filters/TokenDeploymentSubscriptionAttribute.cs | New action filter implementing subscription tier validation and deployment counting for token creation endpoints |
| BiatecTokensApi/Filters/IdempotencyAttribute.cs | Enhanced with SHA256 request parameter hashing and mismatch detection to prevent security bypass |
| BiatecTokensApi/Services/SubscriptionTierService.cs | Extended with token deployment tracking methods (CanDeployTokenAsync, RecordTokenDeploymentAsync, GetTokenDeploymentCountAsync) |
| BiatecTokensApi/Services/Interface/ISubscriptionTierService.cs | Added interface methods for token deployment enforcement |
| BiatecTokensApi/Models/Subscription/SubscriptionTier.cs | Added MaxTokenDeployments and TokenDeploymentEnabled properties to tier configuration |
| BiatecTokensApi/Controllers/TokenController.cs | Applied TokenDeploymentSubscription attribute to all 11 token deployment endpoints |
| BiatecTokensTests/TokenDeploymentSubscriptionTests.cs | Comprehensive unit tests for subscription enforcement covering all tiers, authentication, error responses, and recording behavior |
| BiatecTokensTests/IdempotencySecurityTests.cs | Unit tests validating idempotency security including parameter mismatch detection, cache behavior, and header validation |
| BiatecTokensApi/doc/documentation.xml | Updated XML documentation for new filters, methods, and properties |
| public async Task<bool> CanDeployTokenAsync(string userAddress) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(userAddress)) | ||
| { | ||
| _logger.LogWarning("CanDeployTokenAsync called with null or empty userAddress"); | ||
| return false; | ||
| } | ||
|
|
||
| var tier = await GetUserTierAsync(userAddress); | ||
| var limits = SubscriptionTierConfiguration.GetTierLimits(tier); | ||
|
|
||
| if (!limits.TokenDeploymentEnabled) | ||
| { | ||
| _logger.LogWarning( | ||
| "Token deployment not enabled for user {UserAddress} with tier {Tier}", | ||
| userAddress, tier); | ||
| return false; | ||
| } | ||
|
|
||
| // Unlimited deployments (-1) always allowed | ||
| if (limits.MaxTokenDeployments == -1) | ||
| { | ||
| _logger.LogDebug( | ||
| "Token deployment allowed for user {UserAddress} (unlimited tier {Tier})", | ||
| userAddress, tier); | ||
| return true; | ||
| } | ||
|
|
||
| var currentCount = await GetTokenDeploymentCountAsync(userAddress); | ||
| var canDeploy = currentCount < limits.MaxTokenDeployments; | ||
|
|
||
| _logger.LogDebug( | ||
| "Token deployment check for user {UserAddress}: {CanDeploy} (Tier: {Tier}, Current: {Current}, Max: {Max})", | ||
| userAddress, canDeploy, tier, currentCount, limits.MaxTokenDeployments); | ||
|
|
||
| return canDeploy; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public Task<bool> RecordTokenDeploymentAsync(string userAddress) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(userAddress)) | ||
| { | ||
| _logger.LogWarning("RecordTokenDeploymentAsync called with null or empty userAddress"); | ||
| return Task.FromResult(false); | ||
| } | ||
|
|
||
| var normalizedAddress = userAddress.ToUpperInvariant(); | ||
| _tokenDeploymentCounts.AddOrUpdate(normalizedAddress, 1, (key, oldValue) => oldValue + 1); | ||
|
|
||
| var newCount = _tokenDeploymentCounts[normalizedAddress]; | ||
| _logger.LogInformation( | ||
| "Recorded token deployment for user {UserAddress}. New count: {Count}", | ||
| userAddress, newCount); | ||
|
|
||
| return Task.FromResult(true); | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a potential race condition between CanDeployTokenAsync (line 200-201) and RecordTokenDeploymentAsync (line 220). If two concurrent requests check CanDeployTokenAsync when count is at limit-1, both will pass validation, then both will increment the counter, allowing one request to exceed the limit. For example, with a limit of 3: Request A checks (count=2, passes), Request B checks (count=2, passes), Request A increments (count=3), Request B increments (count=4, exceeds limit). While this is documented as acceptable for MVP with in-memory storage, consider adding a comment explaining this known limitation or using a compare-and-swap pattern with AddOrUpdate to prevent exceeding limits.
| // Key: user address, Value: subscription tier | ||
| private readonly ConcurrentDictionary<string, SubscriptionTier> _userTiers; | ||
|
|
||
| // In-memory storage for token deployment counts |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions user tier assignments but the deployment counts dictionary below it is not mentioned in inline comments. While the class-level documentation exists outside the diff, it would be helpful to add a comment here noting that deployment counts are also in-memory and will be lost on restart, which is a known MVP limitation mentioned in the PR description.
| // In-memory storage for token deployment counts | |
| // In-memory storage for token deployment counts. | |
| // Note: counts are not persisted and will be reset on service restart (MVP limitation). |
| if (tierService == null || logger == null) | ||
| { | ||
| logger?.LogWarning("TokenDeploymentSubscriptionAttribute: Required services not available"); | ||
| await next(); | ||
| return; | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When required services (tierService or logger) are not available, the filter proceeds with the request without validation. This follows the same pattern as SubscriptionTierValidationAttribute (line 40-45 in SubscriptionTierValidationAttribute.cs), but represents a fail-open security posture that could allow unauthorized deployments if dependency injection is misconfigured. Consider changing this to fail-closed by returning an error response (500 Internal Server Error) instead of proceeding, which would prevent deployments when the subscription service is unavailable and make misconfigurations more obvious.
| context.Result = new BadRequestObjectResult(new | ||
| { | ||
| success = false, | ||
| errorCode = "IDEMPOTENCY_KEY_MISMATCH", | ||
| errorMessage = "The provided idempotency key has been used with different request parameters. Please use a unique key for this request or reuse the same parameters.", | ||
| correlationId = context.HttpContext.TraceIdentifier | ||
| }); |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error response uses an anonymous object instead of the ApiErrorResponse class, which is inconsistent with the rest of the codebase. The TokenDeploymentSubscriptionAttribute (in the same PR) and other filters like SubscriptionTierValidationAttribute use ApiErrorResponse for structured error responses. This inconsistency makes it harder to handle errors uniformly on the client side and breaks the established pattern for error handling. Consider replacing the anonymous object with an ApiErrorResponse instance that includes all standard fields like Success, ErrorCode, ErrorMessage, RemediationHint, Timestamp, CorrelationId, and Path.
| context.Result = new BadRequestObjectResult(new | ||
| { | ||
| success = false, | ||
| errorCode = "IDEMPOTENCY_KEY_MISMATCH", |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error code "IDEMPOTENCY_KEY_MISMATCH" is hardcoded as a string literal instead of being defined as a constant in the ErrorCodes class. All other error codes in the codebase are defined in BiatecTokensApi/Models/ErrorCodes.cs (e.g., ErrorCodes.SUBSCRIPTION_LIMIT_REACHED, ErrorCodes.UNAUTHORIZED). This hardcoded string breaks the established pattern and makes the code harder to maintain and test. Add IDEMPOTENCY_KEY_MISMATCH as a constant in the ErrorCodes class and reference it using ErrorCodes.IDEMPOTENCY_KEY_MISMATCH.
| [SetUp] | ||
| public void Setup() | ||
| { | ||
| _mockLogger = new Mock<ILogger<IdempotencyKeyAttribute>>(); | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static _cache field in IdempotencyKeyAttribute can cause test isolation issues because the cache is shared across all test instances. Tests like Idempotency_SameKeyAndParameters_ReturnsCachedResponse add entries to the cache that persist for subsequent tests. While CleanupExpiredEntries() removes expired entries, it only runs probabilistically (1% of the time) and may not clean up between tests. This can lead to non-deterministic test behavior where test execution order affects results. Consider adding a method to clear the cache for testing purposes or making the tests use unique idempotency keys that won't collide.
| // Note: In C#, anonymous objects with different property order create different types | ||
| // So we need to test with a dictionary instead |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "So we need to test with a dictionary instead", but the test continues using anonymous objects with different property orders. While the test is functionally correct (different property orders will serialize to different JSON and produce different hashes), the comment is misleading. Either update the comment to explain that JSON serialization preserves property order, making anonymous objects suitable for this test, or actually use a Dictionary to make the test match the comment's intention.
| // Note: In C#, anonymous objects with different property order create different types | |
| // So we need to test with a dictionary instead | |
| // Note: In C#, anonymous objects with different property orders are different types, | |
| // and JSON serialization preserves property order. Using anonymous objects with | |
| // reordered properties is therefore sufficient to verify that different parameter | |
| // orders produce different idempotency hashes. |
Problem
Token deployment endpoints lacked subscription tier enforcement, preventing revenue generation. Idempotency implementation allowed bypassing business logic by reusing keys with different request parameters.
Changes
Subscription enforcement on token deployments
SubscriptionTierLimitswithMaxTokenDeploymentsand tracking methodsTokenDeploymentSubscriptionAttributefilter validating tier limits before deploymentReturns
402 Payment Requiredwhen limit reached with structured error response containing current tier, usage, and upgrade path.Idempotency security fix
400 Bad RequestwithIDEMPOTENCY_KEY_MISMATCHwhen key reused with different paramsTest coverage
Added 18 unit tests covering subscription enforcement (10) and idempotency security (8). CodeQL scan clean.
Known limitations
Deployment counts stored in-memory (acceptable for MVP, documented for future persistence).
Original prompt
This section details on the original issue you should resolve
<issue_title>Backend MVP stabilization: Arc76 auth, subscription enforcement, and network metadata reliability</issue_title>
<issue_description>## Summary
Stabilize the backend API to support the MVP subscription business model and SaaS-style authentication by ensuring reliable API connectivity, subscription enforcement, and consistent network metadata delivery. This issue focuses on making the backend dependable for production workloads and for the frontend SaaS experience: Arc76 email/password authentication workflows must be supported, Arc14 secure backend communication must be enforced, subscription status must be checked on relevant API endpoints, and a unified network metadata response should be delivered for production networks and testnets. The goal is to remove backend instability as an MVP blocker and create a trustworthy foundation for the paid platform.
Business Value
Revenue cannot be generated without a functioning backend that enforces subscription plans and guarantees consistent API behavior. The roadmap makes it clear that the subscription system is not operational, which means the business cannot sell its core product. Reliable subscription enforcement and authentication are the gatekeepers for all premium functionality. If the backend returns inconsistent results or fails to validate subscription status, customers will lose trust, and the enterprise market will view the platform as non-compliant.
In regulated asset issuance, trust and auditability are critical. Customers will not deploy regulated tokens if the platform cannot reliably confirm who is authenticated, whether the user has access, and which network is being used for deployment. A stable backend provides the data integrity necessary for compliance. It also reduces support costs and engineering time spent on firefighting outages or inconsistent behavior, freeing the team to move toward enterprise compliance milestones.
Additionally, when network metadata is consistent and documented, the frontend can confidently prioritize mainnet networks without guesswork. This reduces the time to market for production deployments and helps the product leadership communicate a clear “production ready” message to customers. Backend stability therefore directly improves conversion, retention, and the ability to deliver on the revenue targets.
Product overview
See the product vision and roadmap for context: https://raw.githubusercontent.com/scholtz/biatec-tokens/refs/heads/main/business-owner-roadmap.md
Scope
In scope
Authentication and Arc76/Arc14 support
Subscription enforcement and authorization
API reliability and health monitoring
Network metadata consistency
Integration readiness
Out of scope
Technical approach and dependencies
Acceptance Criteria
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.