Muhannad/api gateway - #124
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 20562020 | Triggered | Generic Password | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
| 23522284 | Triggered | DigitalOcean Spaces Keys | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
| 23846048 | Triggered | ODBC Connection String | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
| 9611220 | Triggered | AMQP Credentials | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
| 27115147 | Triggered | DigitalOcean Spaces Keys | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
| 27115146 | Triggered | DigitalOcean Spaces Keys | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
| 69690 | Triggered | DigitalOcean Spaces Keys | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
| 27115145 | Triggered | AMQP Credentials | 9b98028 | SW.Bitween.Web/Properties/launchSettings.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds API Gateway support: new domain entities and SDK models, DB migrations across providers, controller endpoints for sync/async gateway calls with partner authorization and Fibonacci backoff polling, global adapter values and template resolution, native-adapter discovery and runtime path (native vs serverless), plus a new NativeAdapters project (HTTP handler and helpers). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GatewayController
participant DB as BitweenDbContext
participant RequestCtx as RequestContext
participant Xchange as XchangeService
participant Cache as InfolinkCache
Client->>GatewayController: POST /api/gateway/{name}/sync or /async + body + partner key
GatewayController->>DB: Load ApiGateway by UrlName
GatewayController->>RequestCtx: Resolve partner key
RequestCtx->>DB: (via extensions) Find Partner
GatewayController->>DB: Load Subscription for partner
GatewayController->>Cache: ListGlobalAdapterValuesSetsAsync()
GatewayController->>Xchange: SubmitSubscriptionXchange(..., gatewayPartner, globalAdapterValuesSets)
alt resultSync = false
GatewayController->>Client: 202 Accepted (xchangeId)
else resultSync = true
GatewayController->>Xchange: Poll for XchangeResult (backoff loop)
Xchange-->>GatewayController: XchangeResult (Success/Failure/ResponseFileId)
alt Success with ResponseFileId
GatewayController->>Xchange: GetFile(responseFileId)
Xchange-->>GatewayController: File content
GatewayController->>Client: 200 OK (content) or 400 (if ResponseBad)
else Success without response
GatewayController->>Client: 200 OK (xchangeId)
else Failure
GatewayController->>Client: 400 Bad Request
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
SW.Bitween.Web/appsettings.json (1)
4-45:⚠️ Potential issue | 🔴 Critical🚨 Real secrets and credentials committed to source control.
This file contains plaintext credentials for multiple external cloud services (DigitalOcean DB, CloudAMQP RabbitMQ, DigitalOcean Spaces), a token signing key, and trivially weak admin credentials (
1:1). These are not localhost-only placeholders — they point to live cloud infrastructure and will be part of the repository's Git history permanently once merged.Immediate actions required:
- Rotate all exposed credentials now — they must be considered compromised once pushed to any remote.
- Move all secrets to a secure store (environment variables, .NET User Secrets for local dev, or a vault like Azure Key Vault / HashiCorp Vault for deployed environments).
- Replace values in
appsettings.jsonwith non-sensitive placeholders or remove the keys entirely.- Ensure
appsettings.Development.json(with real dev values) is in.gitignore.Static analysis also flagged this: Gitleaks detected generic API keys on Lines 7 and 30; Checkov detected basic auth credentials on Lines 8–9.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Web/appsettings.json` around lines 4 - 45, This file contains real secrets in keys like ConnectionStrings (InfolinkDb, RabbitMQ, BitweenDb_Postgresql), CloudFiles (AccessKeyId, SecretAccessKey, ServiceUrl), Token:Key, and Bitween:AdminCredentials; remove these plaintext secrets from appsettings.json and replace with non-sensitive placeholders (e.g., "<REDACTED>" or env var names), then wire the runtime to read the real values from a secure source (environment variables, .NET User Secrets for local dev, or a secrets vault) and ensure any development override file (e.g., appsettings.Development.json) that may contain secrets is added to .gitignore; finally, treat the exposed credentials as compromised and rotate them immediately.SW.Bitween.Api/Services/Caching/InMemoryInfolinkCache.cs (1)
169-175:⚠️ Potential issue | 🔴 Critical
Revoke()does not clear theGlobalAdapterValuesSetcache entry.
GlobalAdapterValuesSetis loaded and cached inLoad()(line 42/49), butRevoke()only removesSubscription,Notifier,Document, andWorkGroup. After a CRUD operation onGlobalAdapterValuesSet, callingRevoke()will leave stale data in the cache until the 10-minute TTL expires.Proposed fix
public void Revoke() { _cache.Remove(nameof(Subscription)); _cache.Remove(nameof(Notifier)); _cache.Remove(nameof(Document)); _cache.Remove(nameof(WorkGroup)); + _cache.Remove(nameof(GlobalAdapterValuesSet)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Services/Caching/InMemoryInfolinkCache.cs` around lines 169 - 175, Revoke() currently removes Subscription, Notifier, Document, and WorkGroup but misses the GlobalAdapterValuesSet entry cached by Load(); update the Revoke() method to also remove the GlobalAdapterValuesSet key (call _cache.Remove(nameof(GlobalAdapterValuesSet))) so the cache is invalidated after CRUD ops, and verify the key name matches how Load() stores it (nameof(GlobalAdapterValuesSet) or the exact string constant) to avoid stale entries.SW.Bitween.Api/Resources/Subscriptions/Update.cs (1)
103-195:⚠️ Potential issue | 🟠 MajorAvoid null reference when property collections are omitted.
MapperProperties,HandlerProperties, andReceiverPropertiescan be null in update payloads; the current.Where(...)will throw instead of producing validation errors.🛠️ Suggested fix
- var missing = mustProps.ToHashSet(StringComparer.OrdinalIgnoreCase) - .Except(i.Where(p => !string.IsNullOrEmpty(p.Value)).Select(p => p.Key)); + var provided = (i ?? Enumerable.Empty<KeyAndValue>()) + .Where(p => !string.IsNullOrEmpty(p.Value)) + .Select(p => p.Key); + var missing = mustProps.ToHashSet(StringComparer.OrdinalIgnoreCase) + .Except(provided);- var missing = mustProps.ToHashSet(StringComparer.OrdinalIgnoreCase) - .Except(i.Where(p => !string.IsNullOrEmpty(p.Value)).Select(p => p.Key)); + var provided = (i ?? Enumerable.Empty<KeyAndValue>()) + .Where(p => !string.IsNullOrEmpty(p.Value)) + .Select(p => p.Key); + var missing = mustProps.ToHashSet(StringComparer.OrdinalIgnoreCase) + .Except(provided);- var missing = mustProps.ToHashSet(StringComparer.OrdinalIgnoreCase) - .Except(model.ReceiverProperties.Where(p => !string.IsNullOrEmpty(p.Value)).Select(p => p.Key)); + var provided = (model.ReceiverProperties ?? Enumerable.Empty<KeyAndValue>()) + .Where(p => !string.IsNullOrEmpty(p.Value)) + .Select(p => p.Key); + var missing = mustProps.ToHashSet(StringComparer.OrdinalIgnoreCase) + .Except(provided);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/Subscriptions/Update.cs` around lines 103 - 195, The validation assumes MapperProperties, HandlerProperties and ReceiverProperties are non-null and calls .Where(...) which can throw; update the three CustomAsync blocks (the RuleFor(i => i.MapperProperties) handler, RuleFor(i => i.HandlerProperties) handler, and the ReceiverProperties check inside RuleFor(i => i).CustomAsync) to treat null collections as empty before filtering—e.g. replace uses like i.Where(p => !string.IsNullOrEmpty(p.Value)).Select(p => p.Key) with (i ?? Enumerable.Empty<KeyValuePair<string,string>>()).Where(...).Select(...) or the correct empty sequence type for the property, so missing/null property collections produce no exception and validation still reports missing required keys.
🟠 Major comments (16)
SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs-112-114 (1)
112-114:⚠️ Potential issue | 🟠 MajorNull values are silently dropped from arrays — data loss.
When deserializing
[1, null, 3], the null element is discarded, producing[1, 3]. This changes the semantics of the data (e.g., positional arrays, sparse data). The same issue exists inReadObject(line 140-141) where{"key": null}loses the entry entirely.🐛 Proposed fix for both ReadArray and ReadObject
In
ReadArray:default: var v = ReadValue(reader); - if (v != null) - list.Add(v); + list.Add(v); break;In
ReadObject:var v = ReadValue(reader); - if (v != null) - obj[propertyName] = v; + obj[propertyName] = v; break;Note:
objwould need to beDictionary<string, object?>(or the class would need to useIDictionary<string, object?>) to store nulls properly. Consider whether the dictionary value type should beobject?throughout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs` around lines 112 - 114, ReadArray and ReadObject currently drop nulls because they only add values when ReadValue(reader) != null; change both to always add the result even if null (i.e., call list.Add(v) unconditionally in ReadArray and assign obj[key] = v in ReadObject), and update the container types to allow nulls by using Dictionary<string, object?> (or IDictionary<string, object?>) for objects and List<object?> for arrays so null elements and null-valued keys are preserved throughout deserialization.SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs-66-80 (1)
66-80:⚠️ Potential issue | 🟠 MajorOAuth2 flow lacks error handling on the token response.
If the OAuth2 token endpoint returns an error (non-2xx),
resDeserialized?.access_tokenwill silently benull, and subsequent API calls will fail with an opaque auth error. Add status validation similar to the Login flow.Proposed fix
var oauthResponse = await client.SendAsync(oathRequest); + oauthResponse.EnsureSuccessStatusCode(); var res = await oauthResponse.Content.ReadAsStringAsync();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs` around lines 66 - 80, The OAuth2 branch in HttpHandler (when _options.AuthType == "OAuth2") doesn't validate the token endpoint response; update the block around oathRequest/oauthResponse/resDeserialized to verify oauthResponse.IsSuccessStatusCode (like the Login flow), and if not successful read the response, log or throw a descriptive exception including the status code and response body; also guard against null resDeserialized or null resDeserialized.access_token and handle that case by logging/throwing before assigning client.DefaultRequestHeaders.Authorization (reference OAuth2Response, oauthResponse, resDeserialized, and client.DefaultRequestHeaders.Authorization).SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs-36-36 (1)
36-36:⚠️ Potential issue | 🟠 Major
HttpClientis created per call and never disposed — risk of socket exhaustion.
HttpClientis designed to be long-lived and reused. Creating a new instance perHandlecall leaks sockets and can exhaust the connection pool under load. At minimum, wrap it in ausingstatement; ideally, injectIHttpClientFactoryor a sharedHttpClientinstance.Minimal fix: dispose after use
- HttpClient client = new HttpClient(); + using HttpClient client = new HttpClient();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs` at line 36, The HttpClient instance is created per call in HttpHandler (line with "HttpClient client = new HttpClient()") and never disposed, risking socket exhaustion; update the HttpHandler.Handle (or the class constructor) to reuse a single HttpClient or accept an injected IHttpClientFactory/IHttpClient to create clients, or at minimum wrap the per-call HttpClient in a using/Dispose; prefer constructor-injected IHttpClientFactory or a shared readonly HttpClient field on HttpHandler to ensure long-lived reuse and remove the per-call new HttpClient() allocation.SW.Bitween.Web/appsettings.Migration.json-1-8 (1)
1-8:⚠️ Potential issue | 🟠 MajorHardcoded credentials committed to source control.
This file contains a plaintext password (
root/password) for the migration database. Even if intended only for local development, committing credentials sets a bad precedent and risks accidental use in non-local environments. Consider adding this file to.gitignoreand using environment variables or user-secrets instead, or at minimum document clearly that this is a local-only template.#!/bin/bash # Check if appsettings.Migration.json is in .gitignore echo "=== .gitignore check ===" rg -n "appsettings.Migration" .gitignore 2>/dev/null || echo "Not found in .gitignore" # Check if there are other appsettings files with credentials echo "=== Other appsettings with passwords ===" rg -n "Password=" --glob "appsettings*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Web/appsettings.Migration.json` around lines 1 - 8, Remove the hardcoded DB credentials from appsettings.Migration.json by replacing the "ConnectionStrings:BitweenDb" value with a placeholder or reference to environment variables (e.g., use configuration keys rather than plaintext), add appsettings.Migration.json to .gitignore so it isn’t committed, create a checked-in template (e.g., appsettings.Migration.template.json) showing the required keys ("ConnectionStrings"/"BitweenDb" and "Bitween:DatabaseType") and document in README or comments that developers must set real credentials via environment variables or user-secrets for local development only.SW.Bitween.Api/Domain/GlobalAdapterValue/GlobalAdapterValuesSet.cs-7-11 (1)
7-11:⚠️ Potential issue | 🟠 Major
Valuescan be null at runtime, risking NullReferenceException.The
Valuesproperty has no default initializer. InStartupValuesFiller.cs(line ~57),globalSet.Values.FirstOrDefault(...)is called without a null check, which will throw ifValuesis null. With nullable enabled in the project, the compiler should also warn about uninitialized non-nullable properties.🛡️ Proposed fix — initialize properties
public class GlobalAdapterValuesSet:BaseEntity<string> { - public string Name { get; set; } - public Dictionary<string, string> Values { get; set; } + public string Name { get; set; } = string.Empty; + public Dictionary<string, string> Values { get; set; } = new(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Domain/GlobalAdapterValue/GlobalAdapterValuesSet.cs` around lines 7 - 11, The Values property on GlobalAdapterValuesSet can be null and is later accessed without a null check; update the GlobalAdapterValuesSet class to initialize Values with an empty Dictionary<string,string> (e.g., default = new Dictionary<string,string>()) so callers like the code that calls globalSet.Values.FirstOrDefault(...) cannot hit a NullReferenceException; alternatively, make Values nullable (Dictionary<string,string>?) and add null checks where used (e.g., in StartupValuesFiller usage) — prefer initializing the property to a non-null empty dictionary to preserve callers and satisfy nullable warnings.SW.Bitween.Api/Interfaces/IInfolinkCache.cs-23-24 (1)
23-24: 🛠️ Refactor suggestion | 🟠 MajorRename
GlobalAdapterValuesSetByIdtoGlobalAdapterValuesSetByIdAsyncto match the naming convention of other Task-returning methods in the interface.Every other method returning
Taskuses theAsyncsuffix (e.g.,DocumentByIdAsync,WorkGroupByIdAsync,ListGlobalAdapterValuesSetsAsync). This method should follow the same pattern.♻️ Changes required
- IInfolinkCache.cs line 23
- InMemoryInfolinkCache.cs line 147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Interfaces/IInfolinkCache.cs` around lines 23 - 24, Rename the method GlobalAdapterValuesSetById to GlobalAdapterValuesSetByIdAsync in the IInfolinkCache interface and update the implementation InMemoryInfolinkCache (and any other implementers/call sites) to match the new name and signature; ensure Task<GlobalAdapterValuesSet> GlobalAdapterValuesSetByIdAsync(string globalAdapterValuesSetId) replaces the old declaration, the InMemoryInfolinkCache method named GlobalAdapterValuesSetById is renamed to GlobalAdapterValuesSetByIdAsync, and update all references/usages to call the Async-named method so the interface and implementation remain consistent.SW.Bitween.Sdk/Model/GlobalAdapterValuesSet.cs-6-16 (1)
6-16:⚠️ Potential issue | 🟠 Major
Idproperty hiding:GlobalAdapterValuesSetRow.IdhidesGlobalAdapterValuesSetCreate.Id.
GlobalAdapterValuesSetRow(line 15) re-declaresIdwhich is already inherited fromGlobalAdapterValuesSetCreate(line 8) through the chainRow → Update → Create. This will produce a CS0108 compiler warning and can cause subtle bugs when the object is accessed through a base-type reference (the wrongIdproperty would be read).Additionally, the hierarchy is unusual —
CreateDTO typically shouldn't carryId, whileUpdate/Rowshould. Consider restructuring soIdlives only on the types that need it.Proposed restructuring
- public class GlobalAdapterValuesSetCreate : IName - { - public string Id { get; set; } - public string Name { get; set; } - public Dictionary<string, string> Values { get; set; } - } - - public class GlobalAdapterValuesSetRow : GlobalAdapterValuesSetUpdate - { - public string Id { get; set; } - } - - public class GlobalAdapterValuesSetUpdate : GlobalAdapterValuesSetCreate - { - } + public class GlobalAdapterValuesSetCreate : IName + { + public string Id { get; set; } + public string Name { get; set; } + public Dictionary<string, string> Values { get; set; } + } + + public class GlobalAdapterValuesSetUpdate : GlobalAdapterValuesSetCreate + { + } + + public class GlobalAdapterValuesSetRow : GlobalAdapterValuesSetUpdate + { + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Sdk/Model/GlobalAdapterValuesSet.cs` around lines 6 - 16, GlobalAdapterValuesSetRow.Id is hiding GlobalAdapterValuesSetCreate.Id; fix by removing Id from the DTO that should not carry it: delete the Id property from GlobalAdapterValuesSetCreate and GlobalAdapterValuesSetRow, and declare a single Id property on GlobalAdapterValuesSetUpdate so only update/row types have Id (adjust constructors/serializers if any rely on the removed Id).SW.Bitween.Api/Resources/Adapters/Search.cs-46-48 (1)
46-48:⚠️ Potential issue | 🟠 Major
ToDictionarywill throw on duplicate keys if native and external adapter names overlap.If any key from
nativeAdaptersalso appears incloudFilesList,ToDictionary(k => k, v => v)throwsArgumentException. Consider using a safe merge or deduplicating the combined list:Proposed fix
- var allAdapters = nativeAdapters.Concat(cloudFilesList); - - return allAdapters.ToDictionary(k => k, v => v); + var allAdapters = nativeAdapters.Concat(cloudFilesList).Distinct(); + + return allAdapters.ToDictionary(k => k, v => v);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/Adapters/Search.cs` around lines 46 - 48, The current merge using nativeAdapters.Concat(cloudFilesList) then ToDictionary(k => k, v => v) will throw if an adapter name appears in both lists; change the merge to deduplicate before building the dictionary, e.g. replace allAdapters = nativeAdapters.Concat(cloudFilesList) with a deduped sequence (use .Distinct() on the concatenated enumerable or GroupBy the keys and pick the first) and then call ToDictionary on that deduped sequence so duplicates in nativeAdapters or cloudFilesList (symbols: nativeAdapters, cloudFilesList, allAdapters, ToDictionary) no longer cause ArgumentException.SW.Bitween.Api/Resources/ApiGateways/RemovePartner.cs-32-38 (1)
32-38:⚠️ Potential issue | 🟠 MajorRemovePartner must accept SubscriptionId to match the composite primary key design.
The
ApiGatewayPartnerstable has a composite PK(ApiGatewayId, PartnerId, SubscriptionId). TheAddPartnerhandler explicitly validates uniqueness using bothPartnerIdandSubscriptionId(line 46 of AddPartner.cs), confirming that multiple subscriptions per partner per gateway are intentional. However,RemovePartnerfilters byPartnerIdalone and usesFirstOrDefault, which silently removes only the first match—creating an asymmetry between add and remove operations.Add
SubscriptionIdtoRemovePartnerRequestto enable precise removal of specific partner-subscription links:Proposed fix
public class RemovePartnerRequest { public int PartnerId { get; set; } + public int SubscriptionId { get; set; } }- var partnerLink = gateway.Partners? - .FirstOrDefault(p => p.PartnerId == request.PartnerId); + var partnerLink = gateway.Partners? + .FirstOrDefault(p => p.PartnerId == request.PartnerId && p.SubscriptionId == request.SubscriptionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/ApiGateways/RemovePartner.cs` around lines 32 - 38, RemovePartner currently identifies links by PartnerId only and uses FirstOrDefault on gateway.Partners, which ignores SubscriptionId and can remove the wrong row; update the RemovePartnerRequest to include SubscriptionId and change the lookup in the RemovePartner handler (the code using gateway.Partners and variable partnerLink) to filter by both PartnerId and SubscriptionId (matching the composite PK in ApiGatewayPartners) before calling _dbContext.Remove(partnerLink), and throw SWNotFoundException if no exact match is found; ensure this mirrors the uniqueness checks in AddPartner.SW.Bitween.Api/Services/ReceivingService.cs-98-99 (1)
98-99:⚠️ Potential issue | 🟠 Major
GetServicemay returnnull— useGetRequiredServiceinstead.On line 98 (and line 122 for the external path),
GetService<XchangeService>()is used, which returnsnullif the service is not registered, leading to aNullReferenceExceptionon the next line. This is inconsistent with the pattern used elsewhere in the file (e.g., line 108 usesGetRequiredService).Proposed fix
- var xchangeService = serviceProvider.GetService<XchangeService>(); + var xchangeService = serviceProvider.GetRequiredService<XchangeService>();Apply the same fix on line 122 for the external adapter path:
- var xchangeService = serviceProvider.GetService<XchangeService>(); + var xchangeService = serviceProvider.GetRequiredService<XchangeService>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Services/ReceivingService.cs` around lines 98 - 99, The code calls serviceProvider.GetService<XchangeService>() which can return null and then immediately calls SubmitSubscriptionXchange causing a potential NullReferenceException; replace GetService<XchangeService>() with GetRequiredService<XchangeService>() in both places (the internal path where xchangeService is acquired and the external adapter path) so the DI system throws a clear error if XchangeService is not registered before invoking XchangeService.SubmitSubscriptionXchange(subId, xchangeFile).SW.Bitween.Api/Services/ReceivingService.cs-139-147 (1)
139-147:⚠️ Potential issue | 🟠 MajorFragile constructor selection — relies on ordering and assumes single-parameter input model.
FirstOrDefault(c => c.GetParameters().Length > 0)picks the first constructor with any parameters, regardless of parameter count. If the adapter has multiple constructors (e.g., one taking DI services), this could select the wrong one. Additionally, only the first parameter's type is used as the input model (line 146), ignoring any additional parameters.Consider filtering for constructors with exactly one parameter, or using a convention/attribute to identify the correct constructor.
Proposed fix — prefer single-parameter constructors
- var constructor = adapterInfo.Type.GetConstructors() - .FirstOrDefault(c => c.GetParameters().Length > 0); + var constructor = adapterInfo.Type.GetConstructors() + .FirstOrDefault(c => c.GetParameters().Length == 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Services/ReceivingService.cs` around lines 139 - 147, The current constructor selection uses adapterInfo.Type.GetConstructors().FirstOrDefault(c => c.GetParameters().Length > 0) which can pick the wrong overload and then uses the first parameter as the input model (inputParameter / inputType); change this to find a constructor with exactly one parameter (e.g., FirstOrDefault(c => c.GetParameters().Length == 1)) and throw the existing BitweenException(adapterId) if none found; alternatively, if you prefer explicit selection, look for a constructor marked by a custom attribute (e.g., [NativeAdapterInput]) on adapterInfo.Type constructors and use that constructor's sole parameter type as inputType.SW.Bitween.Api/Services/ReceivingService.cs-162-173 (1)
162-173:⚠️ Potential issue | 🟠 MajorSilent catch swallows all conversion errors — can mask configuration bugs.
The empty
catchat line 168 swallows every exception fromConvert.ChangeType, not just format mismatches. If the target property is not astring, the value is silently dropped. This makes misconfigured adapter properties very hard to diagnose.At minimum, log a warning so operators can troubleshoot why a property wasn't set.
Proposed fix
catch { - // If conversion fails, set string value directly - if (prop.PropertyType == typeof(string)) - prop.SetValue(inputInstance, value); + if (prop.PropertyType == typeof(string)) + { + prop.SetValue(inputInstance, value); + } + else + { + logger.LogWarning( + "Failed to convert property '{Property}' value '{Value}' to type '{Type}' for adapter '{Adapter}'", + prop.Name, value, prop.PropertyType.Name, adapterId); + } }Note:
loggeris a class field, butInstantiateNativeReceiveris a private method without access to it. Either pass the logger as a parameter or make it accessible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Services/ReceivingService.cs` around lines 162 - 173, The empty catch in InstantiateNativeReceiver around Convert.ChangeType/prop.SetValue swallows all exceptions and drops non-string values; change it to catch specific exceptions (e.g., InvalidCastException, FormatException, OverflowException) and use the class logger to log a warning including the exception and target property name and type before falling back to the string assignment for props of type string; for non-string properties either rethrow or log an error so the misconfiguration is visible. Since InstantiateNativeReceiver currently lacks access to logger, pass the logger into InstantiateNativeReceiver (or make the logger field accessible) so you can log the warning when conversion fails, and keep the Nullable.GetUnderlyingType logic and prop.SetValue usage intact.SW.Bitween.Api/Helpers/StartupValuesFiller.cs-55-66 (1)
55-66:⚠️ Potential issue | 🟠 MajorHandle null
Valuesin GlobalAdapterValuesSet.
If a global set exists butValuesis null, the current lookup will throw instead of preserving the original template.🛠️ Suggested fix
- return globalSet.Values.FirstOrDefault(v => - v.Key.Equals(keyName, StringComparison.OrdinalIgnoreCase)).Value; + if (globalSet.Values == null) + { + return null; + } + return globalSet.Values.FirstOrDefault(v => + v.Key.Equals(keyName, StringComparison.OrdinalIgnoreCase)).Value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Helpers/StartupValuesFiller.cs` around lines 55 - 66, The lookup can throw when a matching GlobalAdapterValuesSet exists but its Values property is null; modify the lambda that finds the replacement so after locating globalSet (via globals.FirstOrDefault(...)) you check for globalSet.Values being null and return null (preserving the original template) before calling FirstOrDefault on Values; update the code paths around globalSet and the keyName lookup to guard against null Values (e.g., if (globalSet?.Values == null) return null) so the call to v.Key.Equals(keyName, ...) is never invoked on a null collection.SW.Bitween.Api/Controllers/GatewayController.cs-76-82 (1)
76-82:⚠️ Potential issue | 🟠 MajorNo upper bound on
Wait-Periodheader — a client can tie up a server thread for an arbitrarily long time.A malicious or misconfigured client could set
Wait-Period: 999999, holding a thread (and polling the DB) for days. Consider clamping to a reasonable maximum (e.g., 300 seconds).🛡️ Proposed fix
+ const int maxWaitPeriod = 300; var waitResponse = 120; var waitResponseHeader = Request.Headers["Wait-Period"].FirstOrDefault(); if (int.TryParse(waitResponseHeader, out var waitResponseValue)) { - waitResponse = waitResponseValue <= 0 ? 120 : waitResponseValue; + waitResponse = waitResponseValue <= 0 ? 120 : Math.Min(waitResponseValue, maxWaitPeriod); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Controllers/GatewayController.cs` around lines 76 - 82, The Wait-Period header handling currently allows arbitrarily large values via waitResponse; modify the parsing logic in GatewayController (the waitResponse variable and the Request.Headers["Wait-Period"] parsing block) to clamp the parsed waitResponseValue to a safe maximum (e.g., 300 seconds) and a minimum >0, preserving the default 120 when parsing fails; use Math.Clamp(waitResponseValue, 1, 300) or equivalent so clients cannot tie up server threads indefinitely.SW.Bitween.Api/Services/XchangeService.cs-207-256 (1)
207-256: 🛠️ Refactor suggestion | 🟠 MajorExtract shared adapter instantiation logic to reduce duplication and improve robustness.
Code duplication: This method duplicates the property-mapping and instantiation logic from
ReceivingService.InstantiateNativeReceiver(lines 130–180). Extract the common logic into a shared helper (e.g., onNativeAdapterDiscoveryService) to maintain a single source of truth.Silent failure on type conversion: The bare
catchblock (lines 240–244) swallows all conversion errors without logging. For non-string properties, values are silently dropped, making runtime issues difficult to diagnose. Log or throw on unexpected conversion failures.Unsafe cast: The cast
(T)adapterat line 255 will throw a genericInvalidCastExceptionif the adapter doesn't implementT. Add an explicit type check with a descriptive error message:♻️ Proposed type check for the cast
var adapter = Activator.CreateInstance(adapterInfo.Type, inputInstance); - - return (T)adapter; + + if (adapter is not T typedAdapter) + throw new BitweenException( + $"Native adapter '{adapterId}' of type {adapterInfo.Type.Name} does not implement {typeof(T).Name}"); + return typedAdapter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Services/XchangeService.cs` around lines 207 - 256, InstantiateNativeAdapter duplicates logic from ReceivingService.InstantiateNativeReceiver, swallows conversion exceptions, and performs an unsafe cast; extract the property-mapping + instantiation logic into a shared helper on NativeAdapterDiscoveryService (or a new internal helper class) and have InstantiateNativeAdapter and ReceivingService.InstantiateNativeReceiver call that helper (move steps that create inputInstance, map properties, and create adapter instance into a single method); replace the bare catch around Convert.ChangeType with error handling that either logs the conversion error (including property name, target type, and value) or rethrows a descriptive exception for non-string targets; and before returning cast (T)adapter in InstantiateNativeAdapter validate that adapter is T (e.g., use is/as checks) and throw a BitweenException with a clear message if the adapter does not implement the expected interface/type.SW.Bitween.Api/Services/XchangeService.cs-110-113 (1)
110-113:⚠️ Potential issue | 🟠 MajorFix spacing and forward gateway context parameters through the xchange creation flow.
The spacing issue is confirmed (line 111: missing space after comma before
GlobalAdapterValuesSet; line 113: missing space after comma beforeglobalAdapterValuesSets).More importantly,
GatewayControllerhas access to bothpartnerandglobalAdapterValuesSet(available from cache and authorization context) but doesn't forward them through the xchange creation chain. Currently:
SubmitSubscriptionXchangeis called without thegatewayPartnerparameterSubmitSubscriptionXchangedoesn't acceptglobalAdapterValuesSetsat all- Both parameters reach
CreateXchangeasnullfor all gateway-originated exchangesIf these parameters are meant to carry partner/global adapter context into the xchange, the entire flow needs updating:
SubmitSubscriptionXchangeshould accept both parameters and forward them toCreateXchange, andGatewayControllershould pass them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Services/XchangeService.cs` around lines 110 - 113, Fix the spacing typos and propagate the gateway context through the call chain: add the missing spaces after commas around GlobalAdapterValuesSet and globalAdapterValuesSets in the CreateXchange signature, update SubmitSubscriptionXchange to accept parameters Partner gatewayPartner and GlobalAdapterValuesSet[] globalAdapterValuesSets, and forward those arguments from SubmitSubscriptionXchange into CreateXchange (preserving existing parameters and ordering). Also update GatewayController to pass the cached/authorized partner and globalAdapterValuesSets into SubmitSubscriptionXchange so gateway-originated exchanges receive non-null gatewayPartner and globalAdapterValuesSets.
🟡 Minor comments (14)
SW.Bitween.PgSql/SW.Bitween.PgSql.csproj-20-23 (1)
20-23:⚠️ Potential issue | 🟡 MinorRemove unnecessary Compile Remove entries that reference non-existent migration files.
The excluded migration files
Migrations\20260212153331_ApiGateway.csandMigrations\20260212153331_ApiGateway.Designer.csdo not exist in the repository. These Compile Remove entries serve no purpose and should be deleted from the csproj file.Csproj snippet
<ItemGroup> <Compile Remove="Migrations\20260212153331_ApiGateway.cs" /> <Compile Remove="Migrations\20260212153331_ApiGateway.Designer.cs" /> </ItemGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.PgSql/SW.Bitween.PgSql.csproj` around lines 20 - 23, The csproj contains two unused Compile Remove entries referencing non-existent migration files (the XML elements Compile Remove="Migrations\20260212153331_ApiGateway.cs" and Compile Remove="Migrations\20260212153331_ApiGateway.Designer.cs"); remove those two <Compile Remove="..."/> lines from the ItemGroup in SW.Bitween.PgSql.csproj so the project file no longer references missing migration sources and save the csproj.SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs-128-147 (1)
128-147:⚠️ Potential issue | 🟡 MinorMissing
defaultcase inReadObjectswitch — unexpected tokens are silently ignored.If an unexpected token type appears (e.g.,
EndArrayinside an object), the loop silently skips it. Add adefaultcase that throwsJsonSerializationExceptionfor consistency withReadValue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs` around lines 128 - 147, In DictionaryConverter.cs inside the ReadObject method add a default case to the switch on reader.TokenType that throws a JsonSerializationException (similar to ReadValue) when an unexpected token is encountered; include the unexpected reader.TokenType (and optionally reader.Path) in the exception message so unexpected tokens (e.g., EndArray) are not silently ignored and debugging matches ReadValue's behavior.SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs-36-49 (1)
36-49:⚠️ Potential issue | 🟡 MinorSilent data loss when value is not
IDictionary<string, object>.If the cast on line 39 fails, an empty
{}is written. Consider throwing or falling back toserializer.Serialize(writer, value)instead of silently writing an empty object. Same concern applies toWriteArray(line 54).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs` around lines 36 - 49, The current WriteObject method silently writes an empty {} when value is not an IDictionary<string, object>; update WriteObject (and similarly WriteArray) to detect a failed cast and either throw a descriptive exception or delegate to the JsonSerializer (e.g., call serializer.Serialize(writer, value)) so the actual value is preserved; locate the methods WriteObject and WriteArray in DictionaryConverter.cs and replace the silent-empty-object behavior with a fallback to serializer.Serialize(writer, value) (or throw) when the value is not the expected dictionary/collection.SW.Bitween.Web/Properties/launchSettings.json-24-24 (1)
24-24:⚠️ Potential issue | 🟡 Minor
launchUrluses HTTPS on the HTTP port.
launchUrlishttps://localhost:5003butapplicationUrlbinds HTTPS to port 5002 and HTTP to port 5003. ThelaunchUrlshould be eitherhttps://localhost:5002orhttp://localhost:5003.Same issue applies to the
PmmDevprofile (Line 50).Also applies to: 45-45, 50-50, 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Web/Properties/launchSettings.json` at line 24, The launchSettings.json has mismatched launchUrl values: update each profile's "launchUrl" to match the scheme and port used in that profile's "applicationUrl" bindings (e.g., if applicationUrl binds HTTPS to port 5002 and HTTP to port 5003, use "https://localhost:5002" for HTTPS launches or "http://localhost:5003" for HTTP launches). Fix the IIS Express profile and the PmmDev profile (and the other occurrences flagged) so launchUrl uses the same scheme and port as the corresponding applicationUrl entries.SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs-37-48 (1)
37-48:⚠️ Potential issue | 🟡 MinorNull-dereference risk when
AuthTypeis set but corresponding credentials are null.For
"Basic"auth, ifLoginUsernameorLoginPasswordis null, the concatenation produces a broken credential string. For"Bearer", ifLoginPasswordis null, theAuthorizationheader value will be"Bearer "with no token. Similarly,ClientId!/ClientSecret!on lines 70-71 suppress null warnings without any guard. Consider validating required fields per auth type early inHandleor in the constructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs` around lines 37 - 48, Validate required credentials for each auth mode before setting headers in HttpHandler.Handle: when _options.AuthType == "ApiKey" ensure _options.ApiKey is non-null/non-empty; when "Bearer" ensure _options.LoginPassword (token) is non-null/non-empty before assigning DefaultRequestHeaders.Authorization; when "Basic" ensure both _options.LoginUsername and _options.LoginPassword are non-null and non-empty before building the Base64 credential string; similarly validate ClientId and ClientSecret (used where ClientId!/ClientSecret! are referenced) and throw or return a clear ArgumentException/invalid-request result if missing. Add these checks at the start of the Handle method (or in the HttpHandler constructor) so header-setting code (DefaultRequestHeaders.Add and DefaultRequestHeaders.Authorization) never receives null values.SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs-56-64 (1)
56-64:⚠️ Potential issue | 🟡 MinorDead code after
EnsureSuccessStatusCode()— the status check on line 59 is unreachable.
EnsureSuccessStatusCode()(line 58) already throwsHttpRequestExceptionfor any non-2xx status. The subsequent check on line 59 will never be true. Also,LoginUrlis null-forgiving but there's no prior validation that it's non-null whenAuthType == "Login".Proposed fix
HttpResponseMessage loginResponse = await client.PostAsync(new Uri(_options.LoginUrl!), new StringContent(loginJson, Encoding.UTF8, "application/json")); loginResponse.EnsureSuccessStatusCode(); - if (loginResponse.StatusCode != HttpStatusCode.OK) - throw new Exception(loginResponse.StatusCode.ToString()); string rs = await loginResponse.Content.ReadAsStringAsync();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs` around lines 56 - 64, The code calls loginResponse.EnsureSuccessStatusCode(), making the subsequent explicit status check on loginResponse.StatusCode unreachable; remove that redundant check and instead validate inputs and response payloads: before calling PostAsync ensure _options.LoginUrl is not null when performing login (check the AuthType == "Login" path and throw a clear exception if LoginUrl is null), keep EnsureSuccessStatusCode() to handle non-success HTTP responses, and after reading and deserializing the body (loginResponse and rsDeserialized) validate rsDeserialized and its Jwt before assigning client.DefaultRequestHeaders.Authorization and throw or return an informative error if Jwt is missing.SW.Bitween.Api/Resources/ApiGateways/Update.cs-34-38 (1)
34-38:⚠️ Potential issue | 🟡 Minor
UrlNameis validated butNameis not.
Namecan be set to null or empty without any guard. IfNameis required, add a similar check. Consider using FluentValidation (as done in other handlers) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/ApiGateways/Update.cs` around lines 34 - 38, The handler currently validates model.UrlName but not model.Name, allowing entity.Name to become null/empty; add a guard for model.Name (e.g., check string.IsNullOrWhiteSpace(model.Name) and throw SWException with an appropriate message) before assigning entity.Name = model.Name, or, for consistency with other handlers, move the validation into the existing FluentValidation validator used for this Update handler so both Name and UrlName are validated together.SW.Bitween.Api/Resources/ApiGateways/Delete.cs-20-26 (1)
20-26:⚠️ Potential issue | 🟡 MinorDeletion will throw a raw DB exception if the gateway has associated partners.
Per the EF config in
BitweenDbContext.cs(lines 29-34),ApiGatewayPartner → ApiGatewayusesDeleteBehavior.Restrict. Attempting to delete a gateway with existing partners will result in an unhandledDbUpdateException. Consider either removing partners first or catching the exception to return a user-friendly error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/ApiGateways/Delete.cs` around lines 20 - 26, Handle currently calls DeleteByKeyAsync<ApiGateway> which will bubble up a DbUpdateException due to the ApiGatewayPartner → ApiGateway relationship configured with DeleteBehavior.Restrict in BitweenDbContext; update Handle to either (a) delete dependent ApiGatewayPartner rows first by querying the ApiGatewayPartner set for partners with the gateway id and removing them before calling DeleteByKeyAsync<ApiGateway>, or (b) wrap the DeleteByKeyAsync<ApiGateway> call in a try/catch for DbUpdateException and return/throw a clear user-facing error indicating the gateway has associated partners (referencing the Handle method, DeleteByKeyAsync<ApiGateway>, ApiGatewayPartner, ApiGateway, and BitweenDbContext to locate the relevant code).SW.Bitween.Api/Domain/Partner/Partner.cs-33-33 (1)
33-33:⚠️ Potential issue | 🟡 Minor
AdapterPropertiesis not initialized and may causeNullReferenceException.Other collections (
_Subscriptions,_ApiCredentials) are initialized in the constructor, butAdapterPropertiesis left asnull. Any code that iterates or accesses this dictionary without a null check will throw.Consider initializing it:
Proposed fix
- public Dictionary<string,string> AdapterProperties { get; set; } + public Dictionary<string,string> AdapterProperties { get; set; } = new();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Domain/Partner/Partner.cs` at line 33, Partner.AdapterProperties is left uninitialized and can cause NullReferenceException; initialize the dictionary in the Partner constructor to ensure safe access. In the Partner class constructor (the method named Partner or the class initializer that already sets _Subscriptions and _ApiCredentials), instantiate AdapterProperties = new Dictionary<string,string>() so any callers can safely iterate or add entries without null checks. Ensure any deserialization paths preserve non-null by keeping this initialization in the constructor or by using a property initializer on the AdapterProperties property.SW.Bitween.Api/Resources/ApiGateways/Get.cs-19-41 (1)
19-41:⚠️ Potential issue | 🟡 MinorMissing access control check; consider making public read intentional.
DeleteandUpdateboth enforce_requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member), but this handler has no access check. SinceGetdoesn't injectIRequestContext, adding the guard would require refactoring the constructor. If public read is intentional, add a comment to clarify. Otherwise, injectIRequestContextand add the guard.Also, the
.Include()/.ThenInclude()calls on lines 23–26 are redundant; EF Core automatically loads these navigation properties when theSelectprojection accesses them (e.g.,p.Partner.Name). Remove them for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/ApiGateways/Get.cs` around lines 19 - 41, The Handle method in Get (public async Task<object> Handle(int key)) lacks the access control present in Delete/Update; either make read explicitly public by adding a clarifying comment above Handle/ApiGateway GET behavior, or inject IRequestContext into the handler constructor and call _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member) at the start of Handle to enforce the same guard as Delete/Update; additionally remove the redundant .Include(...).ThenInclude(...) calls (the projection that uses p.Partner.Name and p.Subscription.Name loads them) to simplify the query.SW.Bitween.Api/Services/ReceivingService.cs-150-150 (1)
150-150:⚠️ Potential issue | 🟡 Minor
Activator.CreateInstance(inputType)will throw if the input model has no parameterless constructor.If the adapter's input model type doesn't have a public parameterless constructor, this call throws a
MissingMethodException. Consider adding a guard or a more descriptive error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Services/ReceivingService.cs` at line 150, The call to Activator.CreateInstance(inputType) in ReceivingService will throw MissingMethodException if the adapter input model lacks a public parameterless constructor; add a guard: check inputType for a public parameterless constructor (e.g., via inputType.GetConstructor(Type.EmptyTypes)) before calling Activator.CreateInstance, and if none exists throw a clear InvalidOperationException that includes inputType.FullName and guidance to add a public parameterless ctor (alternatively wrap Activator.CreateInstance in a try/catch that catches MissingMethodException and rethrows a descriptive exception referencing inputType and the receiving method).SW.Bitween.Api/Resources/ApiGateways/AddPartner.cs-44-57 (1)
44-57:⚠️ Potential issue | 🟡 MinorMissing validation that
PartnerIdexists.The handler validates that the gateway and subscription exist but never checks whether
model.PartnerIdrefers to a valid Partner. If an invalidPartnerIdis supplied, the foreign key constraint atSaveChangesAsyncwill throw a rawDbUpdateException, which is harder for the caller to interpret than a properSWNotFoundException.This is consistent with the pattern used for
SubscriptionIdvalidation (lines 35–39).Proposed fix — add Partner existence check
+ var partner = await _dbContext.Set<Partner>() + .FirstOrDefaultAsync(p => p.Id == model.PartnerId); + + if (partner == null) + throw new SWNotFoundException($"Partner with Id {model.PartnerId} not found"); + // Check if partner already exists var existingPartner = gateway.Partners != null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/ApiGateways/AddPartner.cs` around lines 44 - 57, Add a check that the supplied model.PartnerId refers to an existing Partner before creating ApiGatewayPartner to avoid DbUpdateException; in the AddPartner handler (the method creating partnerLink/gateway variable), query the partners table (e.g., await _dbContext.Partners.AnyAsync(p => p.PartnerId == model.PartnerId) or FindAsync) similar to the SubscriptionId validation, and if not found throw a SWNotFoundException("Partner not found" or similar); place this check before constructing ApiGatewayPartner and before SaveChangesAsync so the error is surfaced as a SWNotFoundException.SW.Bitween.PgSql/Migrations/20260217152930_ApiGateWayAndGlobalValues.cs-47-47 (1)
47-47:⚠️ Potential issue | 🟡 MinorAdd
HasMaxLength(200)constraint to PgSql DbContext configuration forGlobalAdapterValuesSet.Id.The PgSql
BitweenDbContextentity configuration forGlobalAdapterValuesSetis missing the max length constraint. The mainBitweenDbContextspecifies.HasMaxLength(200)on theIdproperty, and the MsSql/MySql migrations both usevarchar(200). The PgSql DbContext must include this configuration to maintain consistency across providers:gav.Property(p => p.Id).IsUnicode(false).HasMaxLength(200);Without this, the property defaults to unbounded
textin PostgreSQL, creating an inconsistency in primary key size constraints across databases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.PgSql/Migrations/20260217152930_ApiGateWayAndGlobalValues.cs` at line 47, The PgSql DbContext is creating GlobalAdapterValuesSet.Id as unbounded text; update the BitweenDbContext PostgreSQL entity configuration for GlobalAdapterValuesSet (the EntityTypeBuilder referenced as gav) to apply .Property(p => p.Id).IsUnicode(false).HasMaxLength(200) so the column maps to varchar(200) like MsSql/MySql; after changing the model, regenerate or adjust the migration so the column definition is varchar(200) instead of text to keep provider consistency.SW.Bitween.Api/Controllers/GatewayController.cs-60-60 (1)
60-60:⚠️ Potential issue | 🟡 Minor
StreamReaderis not disposed — potential resource leak.
new StreamReader(HttpContext.Request.Body)is never disposed. While it may not cause issues in practice (the framework manages the request body lifetime), it's best practice to use ausingstatement. Note: passleaveOpen: trueto avoid closing the underlying request stream prematurely.🛡️ Proposed fix
- var json = await new StreamReader(HttpContext.Request.Body).ReadToEndAsync(); + using var reader = new StreamReader(HttpContext.Request.Body, leaveOpen: true); + var json = await reader.ReadToEndAsync();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Controllers/GatewayController.cs` at line 60, The StreamReader created for HttpContext.Request.Body in GatewayController is not disposed; change the code to use a using declaration or using block (e.g., using var reader = new StreamReader(HttpContext.Request.Body, leaveOpen: true);) and then call await reader.ReadToEndAsync() to get json so the StreamReader is properly disposed while leaving the underlying request stream open.
| var partnerLink = gateway.Partners? | ||
| .FirstOrDefault(p => p.PartnerId == model.PartnerId); | ||
|
|
||
| if (partnerLink == null) | ||
| throw new SWNotFoundException($"Partner with Id {model.PartnerId} not found in gateway {gatewayId}"); | ||
|
|
||
| partnerLink.SubscriptionId = model.SubscriptionId; | ||
|
|
||
| await _dbContext.SaveChangesAsync(); |
There was a problem hiding this comment.
Modifying a primary key component — this will fail at runtime.
ApiGatewayPartner has a composite primary key (ApiGatewayId, PartnerId, SubscriptionId) (see SW.Bitween.PgSql/BitweenDbContext.cs line 134). EF Core does not allow modifying key properties on tracked entities. Setting partnerLink.SubscriptionId on line 50 will throw an InvalidOperationException when SaveChangesAsync() is called.
You need to remove the old entity and add a new one instead.
Proposed fix
var partnerLink = gateway.Partners?
.FirstOrDefault(p => p.PartnerId == model.PartnerId);
if (partnerLink == null)
throw new SWNotFoundException($"Partner with Id {model.PartnerId} not found in gateway {gatewayId}");
- partnerLink.SubscriptionId = model.SubscriptionId;
+ _dbContext.Remove(partnerLink);
+
+ var newPartnerLink = new ApiGatewayPartner
+ {
+ ApiGatewayId = gatewayId,
+ PartnerId = model.PartnerId,
+ SubscriptionId = model.SubscriptionId
+ };
+ _dbContext.Add(newPartnerLink);
await _dbContext.SaveChangesAsync();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var partnerLink = gateway.Partners? | |
| .FirstOrDefault(p => p.PartnerId == model.PartnerId); | |
| if (partnerLink == null) | |
| throw new SWNotFoundException($"Partner with Id {model.PartnerId} not found in gateway {gatewayId}"); | |
| partnerLink.SubscriptionId = model.SubscriptionId; | |
| await _dbContext.SaveChangesAsync(); | |
| var partnerLink = gateway.Partners? | |
| .FirstOrDefault(p => p.PartnerId == model.PartnerId); | |
| if (partnerLink == null) | |
| throw new SWNotFoundException($"Partner with Id {model.PartnerId} not found in gateway {gatewayId}"); | |
| _dbContext.Remove(partnerLink); | |
| var newPartnerLink = new ApiGatewayPartner | |
| { | |
| ApiGatewayId = gatewayId, | |
| PartnerId = model.PartnerId, | |
| SubscriptionId = model.SubscriptionId | |
| }; | |
| _dbContext.Add(newPartnerLink); | |
| await _dbContext.SaveChangesAsync(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SW.Bitween.Api/Resources/ApiGateways/UpdatePartner.cs` around lines 44 - 52,
The code attempts to update a key property on a tracked ApiGatewayPartner
(setting partnerLink.SubscriptionId) which will throw; instead remove the
existing partnerLink entity from the DbContext and add a new ApiGatewayPartner
with the same ApiGatewayId and PartnerId but the new SubscriptionId (preserving
any other non-key fields you need), e.g. call _dbContext.Remove(partnerLink)
then _dbContext.Add(new ApiGatewayPartner { ApiGatewayId = gatewayId, PartnerId
= model.PartnerId, SubscriptionId = model.SubscriptionId, /* copy other fields
if needed */ }) and then await _dbContext.SaveChangesAsync(); ensure you
reference the partnerLink variable and the ApiGatewayPartner type when
implementing the replace.
| b1.HasData( | ||
| new | ||
| { | ||
| PartnerId = 1, | ||
| Id = 1, | ||
| Key = "7facc758283844b49cc4ffd26a75b1de", | ||
| Name = "default" | ||
| }); |
There was a problem hiding this comment.
Remove hardcoded API credential from seed data.
The migration snapshot embeds a real-looking API key in PartnerApiCredentials. This is a secret in source control and will be deployed into databases. Rotate it and source the value from a secrets store or a post-deploy provisioning step instead of migrations.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 963-963: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@SW.Bitween.MsSql/Migrations/20260217150741_ApiGateWayAndGlobalValues.Designer.cs`
around lines 958 - 965, The migration snapshot currently seeds a real API
credential via b1.HasData(...) for PartnerApiCredentials (PartnerId/Id/Key/Name)
in the ApiGateWayAndGlobalValues.Designer.cs migration; remove the hardcoded Key
value from the seed data and replace it with a non-secret placeholder or omit
seeding the Key entirely, then move creation of real credentials into a secure
post-deploy provisioning step or read from a secrets store at runtime (e.g.,
change the b1.HasData call to exclude Key or use a placeholder and document that
PartnerApiCredentials must be populated from secrets management after
deployment).
| b1.HasData( | ||
| new | ||
| { | ||
| PartnerId = 1, | ||
| Id = 1, | ||
| Key = "7facc758283844b49cc4ffd26a75b1de", | ||
| Name = "default" | ||
| }); |
There was a problem hiding this comment.
Remove hardcoded API credential from seed data.
This snapshot embeds a real-looking API key in PartnerApiCredentials. Avoid committing secrets in migrations; rotate the key and inject it via a secrets store or post-deploy provisioning.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 960-960: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@SW.Bitween.MySql/Migrations/20260217151131_ApiGateWayAndGlobalValues.Designer.cs`
around lines 955 - 962, The migration currently seeds a real-looking API
credential via b1.HasData into the PartnerApiCredentials table
(PartnerId/Id/Key/Name); remove the hardcoded Key value from the migration and
replace it with a non-secret placeholder or remove the seeded secret row
entirely, then ensure runtime provisioning uses a secrets store or post-deploy
script to insert rotated credentials; update the migration method that calls
b1.HasData (the block inserting PartnerId = 1, Id = 1, Key = "...", Name =
"default") to omit the actual secret and document that credentials are injected
out-of-band.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Get.cs (2)
1-5: Addusing SW.Bitween.Domain.Accounts;to avoid fully qualifiedAccountRolereferences on Line 22.The sibling
Create.csalready imports this namespace. The verboseDomain.Accounts.AccountRole.*qualified names are purely a result of the missing directive.♻️ Proposed fix
using System.Threading.Tasks; using Microsoft.EntityFrameworkCore; using SW.Bitween.Domain; +using SW.Bitween.Domain.Accounts; using SW.Bitween.Model; using SW.PrimitiveTypes;Then simplify Line 22:
- _requestContext.EnsureAccess(Domain.Accounts.AccountRole.Admin, Domain.Accounts.AccountRole.Member, Domain.Accounts.AccountRole.Viewer); + _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member, AccountRole.Viewer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Get.cs` around lines 1 - 5, Add the missing using directive for the Accounts namespace to remove fully-qualified enum references: add "using SW.Bitween.Domain.Accounts;" at the top of SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Get.cs so you can replace occurrences of Domain.Accounts.AccountRole.* with the simpler AccountRole.* (matching the sibling Create.cs import); update any AccountRole references (e.g., the one on the current Line 22) to the unqualified form after adding the using.
1-5: Addusing SW.Bitween.Domain.Accounts;to avoid fully qualified type names on Line 22.The sibling
Create.csalready imports this namespace. The fully qualifiedDomain.Accounts.AccountRole.*names on Line 22 are purely a result of the missing using directive.♻️ Proposed fix
using System.Threading.Tasks; using Microsoft.EntityFrameworkCore; using SW.Bitween.Domain; +using SW.Bitween.Domain.Accounts; using SW.Bitween.Model; using SW.PrimitiveTypes;Then simplify Line 22:
- _requestContext.EnsureAccess(Domain.Accounts.AccountRole.Admin, Domain.Accounts.AccountRole.Member, Domain.Accounts.AccountRole.Viewer); + _requestContext.EnsureAccess(AccountRole.Admin, AccountRole.Member, AccountRole.Viewer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Get.cs` around lines 1 - 5, Add the missing using directive "SW.Bitween.Domain.Accounts" at the top of Get.cs and then simplify the fully-qualified references on the line using Domain.Accounts.AccountRole.* to just AccountRole.* (mirror the sibling Create.cs import), so occurrences of Domain.Accounts.AccountRole should be replaced with AccountRole to remove fully-qualified type names.SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Create.cs (2)
51-51:Valuesis validated asNotNullbut notNotEmpty— empty dictionaries are accepted.If a
GlobalAdapterValuesSetwith zero entries has no meaningful purpose, add anNotEmpty()rule. Leave as-is if empty sets are a valid initial state (e.g., a template to be filled later).♻️ Proposed addition
- RuleFor(i => i.Values).NotNull(); + RuleFor(i => i.Values).NotNull().NotEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Create.cs` at line 51, The validator currently only enforces RuleFor(i => i.Values).NotNull(), allowing empty dictionaries; update the validation on the Values property in Create (the validator where RuleFor(i => i.Values) is defined) to require content by adding NotEmpty(), e.g. change or extend the rule to RuleFor(i => i.Values).NotNull().NotEmpty(); (or add a separate RuleFor(i => i.Values).NotEmpty() after the NotNull() call) unless empty sets are intentionally allowed.
51-51:Valuesis validated asNotNullbut notNotEmpty— an empty dictionary is accepted.If a
GlobalAdapterValuesSetwith zero entries has no meaningful purpose, addNotEmpty(). Leave as-is if empty sets are valid as an initial template.♻️ Proposed addition
- RuleFor(i => i.Values).NotNull(); + RuleFor(i => i.Values).NotNull().NotEmpty();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Create.cs` at line 51, The validator currently only enforces RuleFor(i => i.Values).NotNull() in Create.cs for GlobalAdapterValuesSet, allowing an empty dictionary; update the rule to also require NotEmpty() so empty sets are rejected when not meaningful—for example change the RuleFor for Values (in the Create validator) to call .NotNull().NotEmpty() (or add a separate RuleFor(i => i.Values).NotEmpty()) so the Values dictionary must contain at least one entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Create.cs`:
- Around line 26-28: The current pre-insert check using AnyAsync on
_dbContext.Set<GlobalAdapterValuesSet>() is vulnerable to TOCTOU races: wrap the
call to SaveChangesAsync (or the method that persists the new
GlobalAdapterValuesSet) in a try-catch that catches DbUpdateException and detect
a unique/PK constraint violation (inspect the inner exception / SQL error codes
for your DB provider); when such a duplicate-key error is detected, rethrow an
SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id
'{request.Id}' already exists") so concurrent duplicate requests return the same
validation error as the pre-check (keep the original AnyAsync check but add this
catch-and-translate around the persistence call).
- Around line 26-28: The current duplicate-ID race (checked via
_dbContext.Set<GlobalAdapterValuesSet>().AnyAsync) can still cause a
DbUpdateException at SaveChangesAsync; wrap the SaveChangesAsync call in a
try/catch that catches DbUpdateException, inspect the exception/inner exception
for a uniqueness/PK violation related to GlobalAdapterValuesSet.Id, and rethrow
a SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id
'{request.Id}' already exists") when detected; otherwise rethrow the original
exception so non-constraint errors still bubble up.
---
Nitpick comments:
In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Create.cs`:
- Line 51: The validator currently only enforces RuleFor(i =>
i.Values).NotNull(), allowing empty dictionaries; update the validation on the
Values property in Create (the validator where RuleFor(i => i.Values) is
defined) to require content by adding NotEmpty(), e.g. change or extend the rule
to RuleFor(i => i.Values).NotNull().NotEmpty(); (or add a separate RuleFor(i =>
i.Values).NotEmpty() after the NotNull() call) unless empty sets are
intentionally allowed.
- Line 51: The validator currently only enforces RuleFor(i =>
i.Values).NotNull() in Create.cs for GlobalAdapterValuesSet, allowing an empty
dictionary; update the rule to also require NotEmpty() so empty sets are
rejected when not meaningful—for example change the RuleFor for Values (in the
Create validator) to call .NotNull().NotEmpty() (or add a separate RuleFor(i =>
i.Values).NotEmpty()) so the Values dictionary must contain at least one entry.
In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Get.cs`:
- Around line 1-5: Add the missing using directive for the Accounts namespace to
remove fully-qualified enum references: add "using SW.Bitween.Domain.Accounts;"
at the top of SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Get.cs so you can
replace occurrences of Domain.Accounts.AccountRole.* with the simpler
AccountRole.* (matching the sibling Create.cs import); update any AccountRole
references (e.g., the one on the current Line 22) to the unqualified form after
adding the using.
- Around line 1-5: Add the missing using directive "SW.Bitween.Domain.Accounts"
at the top of Get.cs and then simplify the fully-qualified references on the
line using Domain.Accounts.AccountRole.* to just AccountRole.* (mirror the
sibling Create.cs import), so occurrences of Domain.Accounts.AccountRole should
be replaced with AccountRole to remove fully-qualified type names.
| var exists = await _dbContext.Set<GlobalAdapterValuesSet>().AnyAsync(x => x.Id == request.Id); | ||
| if (exists) | ||
| throw new SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id '{request.Id}' already exists"); |
There was a problem hiding this comment.
TOCTOU on duplicate-ID check — concurrent duplicate requests will surface an unhandled DbUpdateException instead of SWValidationException.
Between the AnyAsync check and SaveChangesAsync, a concurrent request with the same Id can slip through. The DB PK constraint rejects the second write, but EF Core raises a raw DbUpdateException, producing a 500 instead of a friendly 400/422 validation error.
🛡️ Proposed fix — catch the constraint violation
_dbContext.Add(entity);
- await _dbContext.SaveChangesAsync();
+ try
+ {
+ await _dbContext.SaveChangesAsync();
+ }
+ catch (DbUpdateException)
+ {
+ throw new SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id '{request.Id}' already exists");
+ }
return new
{
entity.Id
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var exists = await _dbContext.Set<GlobalAdapterValuesSet>().AnyAsync(x => x.Id == request.Id); | |
| if (exists) | |
| throw new SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id '{request.Id}' already exists"); | |
| var exists = await _dbContext.Set<GlobalAdapterValuesSet>().AnyAsync(x => x.Id == request.Id); | |
| if (exists) | |
| throw new SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id '{request.Id}' already exists"); | |
| // ... entity construction ... | |
| _dbContext.Add(entity); | |
| try | |
| { | |
| await _dbContext.SaveChangesAsync(); | |
| } | |
| catch (DbUpdateException) | |
| { | |
| throw new SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id '{request.Id}' already exists"); | |
| } | |
| return new | |
| { | |
| entity.Id | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Create.cs` around lines 26 -
28, The current pre-insert check using AnyAsync on
_dbContext.Set<GlobalAdapterValuesSet>() is vulnerable to TOCTOU races: wrap the
call to SaveChangesAsync (or the method that persists the new
GlobalAdapterValuesSet) in a try-catch that catches DbUpdateException and detect
a unique/PK constraint violation (inspect the inner exception / SQL error codes
for your DB provider); when such a duplicate-key error is detected, rethrow an
SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id
'{request.Id}' already exists") so concurrent duplicate requests return the same
validation error as the pre-check (keep the original AnyAsync check but add this
catch-and-translate around the persistence call).
TOCTOU on duplicate-ID check — concurrent duplicate requests will surface a raw DbUpdateException instead of a friendly SWValidationException.
Between the AnyAsync check and SaveChangesAsync, a concurrent request with the same Id can slip through. The database PK constraint will reject the second write, but EF Core surfaces that as an unhandled DbUpdateException, causing a 500 response instead of a 400/422 validation error.
🛡️ Proposed fix — catch the DB uniqueness violation
_dbContext.Add(entity);
- await _dbContext.SaveChangesAsync();
+ try
+ {
+ await _dbContext.SaveChangesAsync();
+ }
+ catch (DbUpdateException)
+ {
+ throw new SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id '{request.Id}' already exists");
+ }
return new
{
entity.Id
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SW.Bitween.Api/Resources/GlobalAdapterValuesSets/Create.cs` around lines 26 -
28, The current duplicate-ID race (checked via
_dbContext.Set<GlobalAdapterValuesSet>().AnyAsync) can still cause a
DbUpdateException at SaveChangesAsync; wrap the SaveChangesAsync call in a
try/catch that catches DbUpdateException, inspect the exception/inner exception
for a uniqueness/PK violation related to GlobalAdapterValuesSet.Id, and rethrow
a SWValidationException("ID_EXISTS", $"GlobalAdapterValuesSet with id
'{request.Id}' already exists") when detected; otherwise rethrow the original
exception so non-constraint errors still bubble up.
…nhance validation logic for subscription types
3 participants
Summary by CodeRabbit