Conversation
| result = BaseEndpoint.build_url( | ||
| url, domain="example.com", method="get", domain_name="example.com" | ||
| ) | ||
| assert "example.com" in result |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to avoid incomplete URL substring sanitization, the code should parse the URL and check the host (and possibly path) using a URL parser rather than using "example.com" in url or url.endswith("example.com"). In tests, this means asserting on parsed components instead of generic substring presence.
Here, the best fix is to change the test so that it asserts that the hostname of the built URL equals the expected domain (example.com), rather than that "example.com" appears somewhere in the full URL string. We can do this by importing urlparse from Python’s standard library urllib.parse and using it to extract the hostname from result.
Concretely:
- Add an import at the top of
tests/unit/test_client.py:from urllib.parse import urlparse
- Update
test_build_url_domains_with_domain:- Replace
assert "example.com" in resultwith parsing the URL and assertingurlparse(result).hostname == "example.com"(or at least that the hostname equals the expected value). This keeps the functional intent (verifying that the domain is correctly embedded) but in a more precise and secure way.
- Replace
- No other tests need to change, and this does not alter production behavior—only the test assertion becomes stricter and more correct.
| @@ -5,6 +5,7 @@ | ||
|
|
||
| import pytest | ||
| import requests | ||
| from urllib.parse import urlparse | ||
|
|
||
| from mailgun.client import BaseEndpoint | ||
| from mailgun.client import Client | ||
| @@ -51,7 +52,8 @@ | ||
| result = BaseEndpoint.build_url( | ||
| url, domain="example.com", method="get", domain_name="example.com" | ||
| ) | ||
| assert "example.com" in result | ||
| parsed = urlparse(result) | ||
| assert parsed.hostname == "example.com" | ||
|
|
||
| def test_build_url_domainlist(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v4/", "keys": ["domainlist"]} |
| def test_builds_url_with_keys(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["events"]} | ||
| result = handle_default(url, "example.com", "get") | ||
| assert "example.com" in result |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix incomplete URL substring checks, you should either (a) assert the full expected URL string, or (b) parse the URL and assert specific structural components (scheme, host, path, etc.) instead of using "...example.com..." in url. In tests, this means expressing stricter expectations so implementations must construct URLs in a well-defined, safe way.
For this concrete case, the best fix that does not change existing functionality is:
- For
TestHandleDefault.test_builds_url_with_keys, replace the two substring assertions:
assert "example.com" in result
assert "events" in resultwith a single assertion on the exact URL that should be produced, consistent with the preceding test test_builds_url_with_domain. That test shows that with base "https://api.mailgun.net/v3/", domain "example.com", and keys ["messages"], the expected URL is "https://api.mailgun.net/v3/example.com/messages". By analogy, when keys is ["events"], the expected URL is "https://api.mailgun.net/v3/example.com/events".
No new imports or helper methods are needed; we only tighten the assertion to compare against the full URL string. This removes the substring-based URL “sanitization” pattern that CodeQL is warning about while preserving the intended functional check.
| @@ -40,10 +40,10 @@ | ||
| def test_builds_url_with_keys(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["events"]} | ||
| result = handle_default(url, "example.com", "get") | ||
| assert "example.com" in result | ||
| assert "events" in result | ||
| assert result == "https://api.mailgun.net/v3/example.com/events" | ||
|
|
||
|
|
||
|
|
||
| class TestHandleDomainlist: | ||
| """Tests for handle_domainlist.""" | ||
|
|
| def test_with_domain_and_keys(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v4/domains/", "keys": ["webhooks"]} | ||
| result = handle_domains(url, "example.com", "get") | ||
| assert "example.com" in result |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the problem is that the test uses a substring match on the full URL ("example.com" in result) instead of asserting the expected URL structure. To avoid incomplete substring checks, tests should compare against a fully specified expected URL, or parse the URL and assert on its components (scheme, netloc, path). This makes the intention explicit and avoids patterns that resemble unsafe domain checks.
For this codebase, the safest, least invasive fix is to change the test to assert the full expected URL for handle_domains when called with base https://api.mailgun.net/v4/domains/, domain example.com, method "get", and key "webhooks". The other assertion assert "webhooks" in result can either remain or also be made more precise; however, the primary CodeQL complaint is about "example.com" in result, so replacing that with an exact equality is sufficient and does not change the behavior the test is trying to enforce (it still passes only if the domain is included correctly). No new imports or helpers are necessary: we update just the assertion line (and optionally its neighbors) in tests/unit/test_handlers.py.
Concretely:
- In
TestHandleDomains.test_with_domain_and_keys, replace the substring assertion on"example.com"with an equality check that the entire URL is"https://api.mailgun.net/v4/domains/example.com/webhooks". - Leave the rest of the tests unchanged.
| @@ -59,7 +59,7 @@ | ||
| def test_with_domain_and_keys(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v4/domains/", "keys": ["webhooks"]} | ||
| result = handle_domains(url, "example.com", "get") | ||
| assert "example.com" in result | ||
| assert result == "https://api.mailgun.net/v4/domains/example.com/webhooks" | ||
| assert "webhooks" in result | ||
|
|
||
| def test_requires_domain_when_keys_present(self) -> None: |
| result = handle_domains( | ||
| url, None, "get", domain_name="my-domain.com" | ||
| ) | ||
| assert "my-domain.com" in result |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the problem here is that the test only verifies that "my-domain.com" appears somewhere in the result string, which mirrors the insecure “substring containment” pattern discouraged by the guidelines. The fix is to make the test assert the exact URL returned, or at least the exact path component where the domain must appear, instead of a loose substring check.
The best fix without changing functionality is to change the assertion in TestHandleDomains.test_with_domain_name_kwarg_get so it expects a specific URL formed from the known base and the domain_name argument. The test constructs url = {"base": "https://api.mailgun.net/v4/domains/", "keys": []} and then calls handle_domains(url, None, "get", domain_name="my-domain.com"). From the neighboring tests and naming, the most consistent expected behavior is that handle_domains returns a URL like https://api.mailgun.net/v4/domains/my-domain.com. We should assert equality with that full string instead of checking for substring containment. No new imports or helper methods are needed; we only modify the relevant assertion line in tests/unit/test_handlers.py.
Concretely:
- In
tests/unit/test_handlers.py, inTestHandleDomains.test_with_domain_name_kwarg_get, replaceassert "my-domain.com" in resultwithassert result == "https://api.mailgun.net/v4/domains/my-domain.com"(preserving indentation). This keeps the test focused and precise and removes the problematic substring check.
| @@ -77,7 +77,7 @@ | ||
| result = handle_domains( | ||
| url, None, "get", domain_name="my-domain.com" | ||
| ) | ||
| assert "my-domain.com" in result | ||
| assert result == "https://api.mailgun.net/v4/domains/my-domain.com" | ||
|
|
||
| def test_verify_requires_true(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v4/domains/", "keys": []} |
| def test_builds_tags_url_with_domain(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["tags"]} | ||
| result = handle_tags(url, "example.com", None) | ||
| assert "example.com" in result |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to avoid incomplete URL substring checks, you should either (a) compare the full expected URL string, or (b) parse the URL and assert on specific components such as scheme, host, path, and query, instead of checking that a domain substring appears anywhere.
Here, the best fix that does not change functional behavior is to make the test assert the exact URL that handle_tags is expected to produce, rather than asserting that "example.com" appears somewhere in the result. The other tests in this file already use stronger assertions such as assert result == "https://api.mailgun.net/v3/example.com/messages" and assert result == "https://api.mailgun.net/v3/ips", so aligning this test with that pattern both removes the substring check and makes the test stricter and clearer.
Concretely, in tests/unit/test_handlers.py, within TestHandleTags.test_builds_tags_url_with_domain, replace:
assert "example.com" in result
assert "tags" in resultwith a single assertion that the entire URL is exactly what is expected, e.g.:
assert result == "https://api.mailgun.net/v3/example.com/tags"This assumes handle_tags constructs URLs consistently with the other handlers: base + domain + key. No additional imports or helper methods are needed.
| @@ -137,8 +137,7 @@ | ||
| def test_builds_tags_url_with_domain(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["tags"]} | ||
| result = handle_tags(url, "example.com", None) | ||
| assert "example.com" in result | ||
| assert "tags" in result | ||
| assert result == "https://api.mailgun.net/v3/example.com/tags" | ||
|
|
||
| def test_with_tag_name_kwarg(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["tags"]} |
| def test_with_domain(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["bounces"]} | ||
| result = handle_bounces(url, "example.com", None) | ||
| assert "example.com" in result |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to avoid incomplete URL substring sanitization, you should parse or deterministically construct URLs and validate specific components (scheme, host, path), rather than searching for substrings anywhere within the URL string. Tests should mirror this by asserting full, structured expectations instead of using broad substring membership checks.
For this specific test, we can strengthen the assertion to check that handle_bounces returns exactly the URL we expect for the given inputs. Other tests in the same file (e.g., test_builds_url_with_domain in TestHandleDefault and test_base_without_trailing_slash in TestHandleIps) already assert the full URL equality, so this change aligns with existing style and doesn’t alter intended functionality. Assuming handle_bounces constructs URLs as "{base}{domain}/{key}" with base = "https://api.mailgun.net/v3/" and key = "bounces", the expected URL string is "https://api.mailgun.net/v3/example.com/bounces".
Concretely:
- In
tests/unit/test_handlers.py, withinTestHandleBounces.test_with_domain, replace:assert "example.com" in resultassert "bounces" in result
- With a single equality assertion:
assert result == "https://api.mailgun.net/v3/example.com/bounces"
No new imports or helper methods are needed.
| @@ -152,8 +152,7 @@ | ||
| def test_with_domain(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["bounces"]} | ||
| result = handle_bounces(url, "example.com", None) | ||
| assert "example.com" in result | ||
| assert "bounces" in result | ||
| assert result == "https://api.mailgun.net/v3/example.com/bounces" | ||
|
|
||
| def test_with_bounce_address(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["bounces"]} |
| def test_with_domain(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["whitelists"]} | ||
| result = handle_whitelists(url, "example.com", None) | ||
| assert "example.com" in result |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 hours ago
In general, to fix incomplete URL substring sanitization, you should parse and inspect structured components of the URL (scheme, host, path, query) instead of searching for substrings. In tests, a more precise and safer pattern is to assert the exact URL or at least well-defined URL components, not just that a particular string appears somewhere in the result.
For this specific test file, we can avoid substring checks that mention the domain and instead assert either the full URL returned by the handler or, if that is not known, at least that the URL begins with the expected base and path sequence. We must not change how the handlers behave, just how the tests verify them. The minimal change that removes the flagged pattern while preserving intent is:
- In
TestHandleWhitelists.test_with_domain, replace:assert "example.com" in resultassert "whitelists" in result
- With a single assertion of the exact URL we expect based on the existing patterns in the file:
"https://api.mailgun.net/v3/example.com/whitelists".
Other tests in this file already assert full URLs (for example, the TestHandleDefault and TestHandleAddressValidate tests). Aligning TestHandleWhitelists with these tests makes behavior clearer and more robust. No new imports or helper methods are needed; we only adjust the assertion text in the shown region.
| @@ -191,8 +191,7 @@ | ||
| def test_with_domain(self) -> None: | ||
| url = {"base": "https://api.mailgun.net/v3/", "keys": ["whitelists"]} | ||
| result = handle_whitelists(url, "example.com", None) | ||
| assert "example.com" in result | ||
| assert "whitelists" in result | ||
| assert result == "https://api.mailgun.net/v3/example.com/whitelists" | ||
|
|
||
|
|
||
| class TestHandleAddressValidate: |
f043857 to
29ac88b
Compare
29ac88b to
fe56a9a
Compare
a50e086 to
4406bc0
Compare
Existing tests in the repo were basically e2e/integration tests and they require real API key. Some of the tests even require access to some paid features.
Updated test framework to include 2 parts: unit tests and integration tests.
Added some specific unit tests.
Coverage is currently 80%.
Updated Makefile commands to run unit tests and different tests sets (including or excluding coverage check)