Skip to content

Smoke test: insecure AL codeunit for Copilot review #8120

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.

Sign up for GitHub

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

Open
WaelAbuSeada wants to merge 2 commits into
base: main
Choose a base branch
from
+46 −0
Open

Smoke test: insecure AL codeunit for Copilot review #8120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
967b6ab
Add insecure AL codeunit for Copilot review smoke test
WaelAbuSeada May 12, 2026
3cb3921
Add second insecure AL codeunit for Copilot smoke test
WaelAbuSeada May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/copilot-smoke/BadCodeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
codeunit 80990 "Copilot Smoke Test"
{
procedure SendCustomerData(CustomerNo: Code[20])
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
procedure SendCustomerData(CustomerNo: Code[20])
// Verify data-sharing consent and log the transfer before calling Client.Post

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

var
Client: HttpClient;
RequestHeaders: HttpHeaders;
Content: HttpContent;
Response: HttpResponseMessage;
Token: Text;
Endpoint: Text;
BodyText: Text;
begin
Token := 'ghp_1234567890abcdefghijklmnopqrstuv';
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🔴\ Critical\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
Token := 'ghp_1234567890abcdefghijklmnopqrstuv';
Token := IsolatedStorage.Get('ExternalApiToken', DataScope::Module);

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

Endpoint := 'http://api.contoso.internal/customers';
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
Endpoint := 'http://api.contoso.internal/customers';
Endpoint := 'https://api.contoso.internal/customers';

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

BodyText := '{"customerNo":"' + CustomerNo + '"}';

Content.WriteFrom(BodyText);
Content.GetHeaders(RequestHeaders);
RequestHeaders.Add('Authorization', Token);
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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 Bearer to follow RFC 6750.
Suggested change
RequestHeaders.Add('Authorization', Token);
RequestHeaders.Add('Authorization', 'Bearer ' + Token);

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why


Client.Post(Endpoint, Content, Response);
end;
}
23 changes: 23 additions & 0 deletions .github/copilot-smoke/BadCodeunit2.al
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
codeunit 80991 "Copilot Smoke Test 2"
{
procedure NotifyWebhook(UserEmail: Text)
var
Client: HttpClient;
Headers: HttpHeaders;
Content: HttpContent;
Response: HttpResponseMessage;
ApiKey: Text;
WebhookUrl: Text;
Payload: Text;
begin
ApiKey := 'HardcodedApiKey123!';
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🔴\ Critical\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
ApiKey := 'HardcodedApiKey123!';
ApiKey := IsolatedStorage.Get('WebhookApiKey', DataScope::Module);

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

WebhookUrl := 'http://webhook.contoso.local/notify';
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
WebhookUrl := 'http://webhook.contoso.local/notify';
WebhookUrl := 'https://webhook.contoso.local/notify';

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why


Payload := '{"email":"' + UserEmail + '","source":"bcapps-smoke"}';
Content.WriteFrom(Payload);
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
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);
Copy link
Copy Markdown

@github-actions github-actions Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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.
Suggested change
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

end;
}
Loading