fix: drop -f from curl to avoid GitHub API rate-limit flakiness#1267
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates integration tests that exercise domain allowlist / wildcard matching and blocked-domain behavior by changing the curl flags used in several test commands.
Changes:
- Replaces
curl -fsSwithcurl -sSin multiple wildcard allowlist tests hittinghttps://api.github.com/zen. - Removes
curl -ffrom several blocked-domains allowlist tests (includingbash -c "curl ... && echo success"cases).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/integration/wildcard-patterns.test.ts | Adjusts curl invocation flags in wildcard allowlist tests. |
| tests/integration/blocked-domains.test.ts | Adjusts curl invocation flags in allowed-domain / edge-case tests and a multi-command success chain. |
Comments suppressed due to low confidence (10)
tests/integration/wildcard-patterns.test.ts:60
- Dropping curl’s
-fmakes the test pass on HTTP error responses becausetoSucceed()only checks exit code. This can hide regressions where the request is actually blocked. Keep-f(e.g.,-fsS) or assert the HTTP status explicitly.
const result = await runner.runWithSudo(
'curl -sS https://api.github.com/zen',
{
tests/integration/wildcard-patterns.test.ts:75
- Without curl
-f, HTTP 4xx/5xx responses still produce exit code 0, sotoSucceed()may pass even when the request is blocked or rate-limited. Keep-f(e.g.,-fsS) or validate the HTTP status code explicitly.
const result = await runner.runWithSudo(
'curl -sS https://API.GITHUB.COM/zen',
{
tests/integration/wildcard-patterns.test.ts:88
- Removing curl
-fmeans the command can succeed (exit 0) even when the response is an HTTP error, which undermines whattoSucceed()is asserting here. Reintroduce-for assert on the HTTP status code.
const result = await runner.runWithSudo(
'curl -sS https://API.GITHUB.COM/zen',
{
tests/integration/wildcard-patterns.test.ts:116
- This test’s success criteria is process exit code; without curl
-fit can return 0 on HTTP errors, masking blocked/denied behavior. Prefercurl -fsSor output/verify%{http_code}.
const result = await runner.runWithSudo(
'curl -sS https://api.github.com/zen',
{
tests/integration/wildcard-patterns.test.ts:131
- In a
bash -c "curl ... && echo success"chain, removing curl-fallowsecho successto run even if curl gets an HTTP 403/429/500 response (curl still exits 0). This makes the test a false positive. Keep-for explicitly check the HTTP status code before echoing success.
const result = await runner.runWithSudo(
'bash -c "curl -sS https://api.github.com/zen && echo success"',
{
tests/integration/wildcard-patterns.test.ts:145
- Same issue as above: without curl
-f, the&& echo successchain will succeed even when the HTTP response is an error, so the test may pass while access is actually blocked. Re-add-for check%{http_code}.
const result = await runner.runWithSudo(
'bash -c "curl -sS https://api.github.com/zen && echo success"',
{
tests/integration/blocked-domains.test.ts:52
- Test correctness: without curl
-f, HTTP error responses still return exit code 0, sotoSucceed()may pass even if the request is blocked or GitHub returns an error. Prefercurl -f/-fsSor assert the HTTP status code.
test('should allow requests to allowed domains', async () => {
const result = await runner.runWithSudo(
'curl --max-time 10 https://api.github.com/zen',
{
tests/integration/blocked-domains.test.ts:80
- In this
curl ... && echo successchain, dropping-fmeansecho successwill still run even when curl receives an HTTP error response, making this test a false positive. Re-add-for explicitly check the HTTP status code before echoing success.
const result = await runner.runWithSudo(
'bash -c "curl --max-time 10 https://api.github.com/zen && echo success"',
{
tests/integration/blocked-domains.test.ts:135
- Without curl
-f, an HTTP error response won’t fail the command, so thistoSucceed()assertion can become a false positive. Keep-f(or assert on%{http_code}) to ensure the request truly succeeded.
// Test that domains are matched case-insensitively
const result = await runner.runWithSudo(
'curl --max-time 10 https://API.GITHUB.COM/zen',
{
tests/integration/blocked-domains.test.ts:164
- Removing curl
-fhere can mask blocked/denied behavior because curl exits 0 on HTTP 4xx/5xx. Since the test asserts only on exit code viatoSucceed(), this weakens the test. Reintroduce-for validate the HTTP status code explicitly.
test('should handle domains with leading/trailing whitespace in config', async () => {
const result = await runner.runWithSudo(
'curl --max-time 10 https://api.github.com/zen',
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const result = await runner.runWithSudo( | ||
| 'curl -fsS https://api.github.com/zen', | ||
| 'curl -sS https://api.github.com/zen', | ||
| { |
There was a problem hiding this comment.
These tests rely on the command’s exit code (via toSucceed) to assert that the request was allowed. Dropping curl’s -f means curl will exit 0 even for HTTP 4xx/5xx responses (including Squid block pages), which can turn this into a false positive. Keep -f (e.g., -fsS) or explicitly assert the HTTP status code in the test output.
This issue also appears in the following locations of the same file:
- line 58
- line 73
- line 86
- line 114
- line 129
- ...and 1 more
| const result = await runner.runWithSudo( | ||
| 'curl -f --max-time 10 https://api.github.com/zen', | ||
| 'curl --max-time 10 https://api.github.com/zen', | ||
| { |
There was a problem hiding this comment.
This test uses toSucceed() (exit code 0) as its signal that the network request was allowed. Removing curl -f means HTTP error responses (including potential proxy block pages) won’t fail the command, causing false positives. Keep -f (or validate the HTTP status code explicitly).
This issue also appears in the following locations of the same file:
- line 49
- line 78
- line 132
- line 161
| test('should block specific domain even when parent is allowed', async () => { | ||
| // Allow github.com but block a specific subdomain | ||
| // Note: Currently blocked domains are checked against the ACL, so this tests | ||
| // that the blocking mechanism is properly configured | ||
| const result = await runner.runWithSudo( | ||
| 'curl -f --max-time 10 https://api.github.com/zen', | ||
| 'curl --max-time 10 https://api.github.com/zen', |
There was a problem hiding this comment.
This test name (and the surrounding comment) says it’s validating blocking behavior, but blockDomains isn’t set in the options passed to runWithSudo(), and the assertion expects success. Either rename/update the test description to match what’s being asserted, or pass blockDomains and assert failure to actually test blocked-domain precedence.
…c endpoint detection (#1264) * Initial plan * feat(proxy): add validation for --copilot-api-target flag Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * test: add docker-manager tests for COPILOT_API_TARGET Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * feat(proxy): handle GHEC domains in deriveCopilotApiTarget Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * Potential fix for pull request finding 'CodeQL / Log injection' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * docs: add enterprise configuration guide for GHEC/GHES Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: add missing formatItem and program imports in cli.test.ts (#1265) * Initial plan * fix: add missing formatItem and program imports in cli.test.ts Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> * fix: drop -f from curl to avoid GitHub API rate-limit flakiness (#1267) * Initial plan * fix: drop -f from curl to avoid GitHub API rate-limit flakiness Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Co-authored-by: Landon Cox <landon.cox@microsoft.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
api.github.com/zenintermittently returns HTTP 403 from GitHub's server due to rate limiting on shared CI runner IPs (60 unauthenticated req/hr per IP). Tests usingcurl -ftreated this server-side 403 as a test failure, even though Squid correctly allowed the CONNECT tunnel.Evidence: curl showed a progress meter with bytes received before exiting with code 22 — confirming the proxy tunnel succeeded and the 403 came from GitHub, not Squid.
Changes
tests/integration/blocked-domains.test.ts— 5curl -f --max-time 10→curl --max-time 10on tests accessingapi.github.comthat expect successtests/integration/wildcard-patterns.test.ts— 7curl -fsS→curl -sSfor the same reasonWithout
-f, curl exits 0 when the server returns 4xx (proxy allowed the connection — the actual invariant being tested), but still exits non-zero for proxy CONNECT denials (connection-level failure, unaffected by-f). Tests expectingtoFail()and non-API URLs likeraw.githubusercontent.comare unchanged.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.