Smoke test: insecure AL codeunit for Copilot review#8120
Conversation
|
Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234' |
| @@ -0,0 +1,23 @@ | |||
| codeunit 80990 "Copilot Smoke Test" | |||
| { | |||
| procedure SendCustomerData(CustomerNo: Code[20]) | |||
There was a problem hiding this comment.
Customer data exfiltrated to external endpoint
The procedure unconditionally transmits customer identifiers to an external HTTP endpoint with no consent check, audit trail, or data-minimisation control, which likely violates GDPR/CCPA obligations and BC's data residency requirements.
Recommendation:
- Ensure any outbound transfer of customer data is gated by a consent/configuration check, is logged in an audit table, and is limited to the minimum fields required. Consult your privacy review process before shipping.
| procedure SendCustomerData(CustomerNo: Code[20]) | |
| // Verify data-sharing consent and log the transfer before calling Client.Post |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Endpoint: Text; | ||
| BodyText: Text; | ||
| begin | ||
| Token := 'ghp_1234567890abcdefghijklmnopqrstuv'; |
There was a problem hiding this comment.
Hardcoded GitHub PAT in source code
A GitHub Personal Access Token (prefixed ghp_) is hardcoded as a string literal and will be committed to version control, where it is trivially discoverable by anyone with repo access or in leak-scanning tools.
Recommendation:
- Remove the token immediately and rotate it. Store secrets in Azure Key Vault, an Isolated Storage entry, or a Named Credential, and retrieve them at runtime rather than embedding them in source.
| Token := 'ghp_1234567890abcdefghijklmnopqrstuv'; | |
| Token := IsolatedStorage.Get('ExternalApiToken', DataScope::Module); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| Content.WriteFrom(BodyText); | ||
| Content.GetHeaders(RequestHeaders); | ||
| RequestHeaders.Add('Authorization', Token); |
There was a problem hiding this comment.
Authorization header missing Bearer scheme
The raw token string is added directly as the Authorization header value without the Bearer prefix, which is non-standard and may inadvertently expose the token format or cause silent authentication failures depending on the server.
Recommendation:
- Prefix the token value with
Bearerto follow RFC 6750.
| RequestHeaders.Add('Authorization', Token); | |
| RequestHeaders.Add('Authorization', 'Bearer ' + Token); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| BodyText: Text; | ||
| begin | ||
| Token := 'ghp_1234567890abcdefghijklmnopqrstuv'; | ||
| Endpoint := 'http://api.contoso.internal/customers'; |
There was a problem hiding this comment.
Customer data sent over plain HTTP
The endpoint uses the http:// scheme, meaning the customer number is transmitted in cleartext. This violates data-protection requirements (GDPR, etc.) and exposes customer PII to network interception.
Recommendation:
- Change the endpoint scheme to https:// so that transport is encrypted.
| Endpoint := 'http://api.contoso.internal/customers'; | |
| Endpoint := 'https://api.contoso.internal/customers'; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Payload: Text; | ||
| begin | ||
| ApiKey := 'HardcodedApiKey123!'; | ||
| WebhookUrl := 'http://webhook.contoso.local/notify'; |
There was a problem hiding this comment.
User email sent over plain HTTP
The webhook URL uses http://, transmitting the user's email address in cleartext. Email addresses are personal data and must be protected in transit.
Recommendation:
- Use https:// for the webhook URL to ensure encrypted transmission.
| WebhookUrl := 'http://webhook.contoso.local/notify'; | |
| WebhookUrl := 'https://webhook.contoso.local/notify'; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| WebhookUrl: Text; | ||
| Payload: Text; | ||
| begin | ||
| ApiKey := 'HardcodedApiKey123!'; |
There was a problem hiding this comment.
API key hardcoded in source code
The string 'HardcodedApiKey123!' is committed as a literal, exposing the credential to anyone with read access to the repository or its history.
Recommendation:
- Rotate the key and retrieve it at runtime from IsolatedStorage or a secure configuration provider instead of embedding it in code.
| ApiKey := 'HardcodedApiKey123!'; | |
| ApiKey := IsolatedStorage.Get('WebhookApiKey', DataScope::Module); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| WebhookUrl := 'http://webhook.contoso.local/notify'; | ||
|
|
||
| Payload := '{"email":"' + UserEmail + '","source":"bcapps-smoke"}'; | ||
| Content.WriteFrom(Payload); |
There was a problem hiding this comment.
JSON built via string concatenation (injection risk)
UserEmail is a Text parameter concatenated directly into the JSON payload. A value containing quote characters can corrupt the JSON or inject extra properties.
Recommendation:
- Use a JsonObject to build the payload so the email value is properly escaped.
| Content.WriteFrom(Payload); | |
| var | |
| JsonObj: JsonObject; | |
| begin | |
| JsonObj.Add('email', UserEmail); | |
| JsonObj.Add('source', 'bcapps-smoke'); | |
| JsonObj.WriteTo(Payload); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Content.GetHeaders(Headers); | ||
| Headers.Add('x-api-key', ApiKey); | ||
|
|
||
| Client.Post(WebhookUrl, Content, Response); |
There was a problem hiding this comment.
HTTP response not checked after POST
The Response object is populated by Client.Post but never read, so failures (4xx/5xx) are silently swallowed and the caller has no way to know the notification was not delivered.
Recommendation:
- Inspect Response.IsSuccessStatusCode() after the call and raise an error or log a warning on failure.
| Client.Post(WebhookUrl, Content, Response); | |
| Client.Post(WebhookUrl, Content, Response); | |
| if not Response.IsSuccessStatusCode() then | |
| Error('Webhook POST failed with status %1', Response.HttpStatusCode()); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Test PR to validate Copilot PR review findings on AL code.
Contains intentionally insecure patterns for review detection:
File added: