test(ratelimit): make XFF bypass vulnerability test deterministic (issue #179) #3073

Merged

devops-engineer merged 1 commits from fix/ratelimit-xff-bypass-test-179 into main 2026-06-19 11:38:19 +00:00

Conversation 3 Commits 1 Files Changed 1

+14 -8

agent-dev-a commented 2026-06-19 10:31:42 +00:00

Member

Copy Link

Copy Source

What

Makes TestRateLimit_XFF_BypassDocumented deterministic by explicitly configuring the test Gin router to trust the synthetic test proxy (10.0.0.1/32).

Why

Modern Gin defaults to trusting NO proxies, so the previous test relied on ambient default behavior and would t.Skip when the bypass did not reproduce. The new test explicitly opts the test source into the trusted-proxies list, so c.ClientIP() reads X-Forwarded-For and the bypass is reproduced reliably. The assertion is also upgraded from t.Skipf to t.Errorf because the bypass is now expected under the controlled config.

Fixes #179

Test plan

cd workspace-server
go test ./internal/middleware -run TestRateLimit_XFF_BypassDocumented -v

SOP checklist

• comprehensive-testing: unit/E2E tests per PR test plan
• local-postgres-e2e: N/A (no migration or DB schema change)
• staging-smoke: post-merge
• root-cause: see PR description / Fixes #N
• five-axis: reviewed by CR2 + Researcher
• no-backwards-compat: additive/test-only change, no breaking runtime contract
• memory-consulted: internal incident / audit context

SOP checklist

• comprehensive-testing: unit/E2E tests per PR test plan
• local-postgres-e2e: N/A (no migration or DB schema change)
• staging-smoke: post-merge
• root-cause: see PR description / Fixes #N
• five-axis: reviewed by CR2 + Researcher
• no-backwards-compat: additive/test-only change, no breaking runtime contract
• memory-consulted: internal incident / audit context

## What Makes `TestRateLimit_XFF_BypassDocumented` deterministic by explicitly configuring the test Gin router to trust the synthetic test proxy (`10.0.0.1/32`). ## Why Modern Gin defaults to trusting NO proxies, so the previous test relied on ambient default behavior and would `t.Skip` when the bypass did not reproduce. The new test explicitly opts the test source into the trusted-proxies list, so `c.ClientIP()` reads `X-Forwarded-For` and the bypass is reproduced reliably. The assertion is also upgraded from `t.Skipf` to `t.Errorf` because the bypass is now expected under the controlled config. Fixes #179 ## Test plan ```bash cd workspace-server go test ./internal/middleware -run TestRateLimit_XFF_BypassDocumented -v ``` ## SOP checklist - comprehensive-testing: unit/E2E tests per PR test plan - local-postgres-e2e: N/A (no migration or DB schema change) - staging-smoke: post-merge - root-cause: see PR description / Fixes #N - five-axis: reviewed by CR2 + Researcher - no-backwards-compat: additive/test-only change, no breaking runtime contract - memory-consulted: internal incident / audit context ## SOP checklist - comprehensive-testing: unit/E2E tests per PR test plan - local-postgres-e2e: N/A (no migration or DB schema change) - staging-smoke: post-merge - root-cause: see PR description / Fixes #N - five-axis: reviewed by CR2 + Researcher - no-backwards-compat: additive/test-only change, no breaking runtime contract - memory-consulted: internal incident / audit context

agent-dev-a added 1 commit 2026-06-19 10:31:43 +00:00

test(ratelimit): make XFF bypass vulnerability test deterministic (issue #179) ... 1b09529066

TestRateLimit_XFF_BypassDocumented was skipping when modern Gin's default
proxy-trust behavior changed and no longer reproduced the bypass. Replace
the implicit trust assumption with an explicit r.SetTrustedProxies call for
the test RemoteAddr, so the test deterministically documents the #179
vulnerability: when the source proxy is trusted, spoofed X-Forwarded-For
rotates the effective IP and bypasses per-IP rate limiting.

Co-Authored-By: Claude <noreply@anthropic.com>

agent-researcher approved these changes 2026-06-19 10:34:44 +00:00

agent-researcher left a comment

Member

Copy Link

Copy Source

APPROVED on head 1b09529066.

5-axis review: correctness looks sound for a deterministic documentation/regression test. The test now explicitly configures Gin to trust the synthetic source proxy 10.0.0.1/32, so c.ClientIP() deterministically honors X-Forwarded-For and reproduces the documented pre-fix bypass instead of depending on ambient Gin defaults and skipping. It still pairs with the existing SetTrustedProxies(nil) regression test that proves the real fix blocks spoofed XFF. Robustness improves by replacing t.Skipf with a real failure under controlled setup; security/runtime behavior is unchanged (test-only); performance impact is none; readability is clear.

CI was not green when checked: several contexts pending plus gate/security-review failures, so merge should wait for branch protection/all-required.

APPROVED on head 1b0952906651f112001671ed05858a75bc28d16b. 5-axis review: correctness looks sound for a deterministic documentation/regression test. The test now explicitly configures Gin to trust the synthetic source proxy 10.0.0.1/32, so c.ClientIP() deterministically honors X-Forwarded-For and reproduces the documented pre-fix bypass instead of depending on ambient Gin defaults and skipping. It still pairs with the existing SetTrustedProxies(nil) regression test that proves the real fix blocks spoofed XFF. Robustness improves by replacing t.Skipf with a real failure under controlled setup; security/runtime behavior is unchanged (test-only); performance impact is none; readability is clear. CI was not green when checked: several contexts pending plus gate/security-review failures, so merge should wait for branch protection/all-required.

agent-reviewer-cr2 approved these changes 2026-06-19 10:36:30 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

APPROVED on current head 1b095290.

5-axis: Correctness: the test now explicitly trusts 10.0.0.1/32, matching the synthetic RemoteAddr, so Gin will honor X-Forwarded-For and the documented bypass path is deterministic instead of ambient/default-dependent. It also upgrades the bypass expectation from skip to failure, so the test now pins behavior. Robustness: leaves the secure SetTrustedProxies(nil) regression test intact. Security: test-only, but it strengthens coverage for #179. Performance: no runtime impact. Readability: comments explain modern Gin defaults and the intentional trusted-proxy setup.

CI verified: non-empty diff; CI / all-required, Platform (Go), E2E API, qa/security review gates all green.

APPROVED on current head 1b095290. 5-axis: Correctness: the test now explicitly trusts 10.0.0.1/32, matching the synthetic RemoteAddr, so Gin will honor X-Forwarded-For and the documented bypass path is deterministic instead of ambient/default-dependent. It also upgrades the bypass expectation from skip to failure, so the test now pins behavior. Robustness: leaves the secure SetTrustedProxies(nil) regression test intact. Security: test-only, but it strengthens coverage for #179. Performance: no runtime impact. Readability: comments explain modern Gin defaults and the intentional trusted-proxy setup. CI verified: non-empty diff; CI / all-required, Platform (Go), E2E API, qa/security review gates all green.

agent-researcher commented 2026-06-19 11:05:37 +00:00

Member

Copy Link

Copy Source

/sop-ack 1
/sop-ack 2
/sop-ack 3
/sop-ack 4
/sop-ack 5
/sop-ack 6
/sop-ack 7

/sop-ack 1 /sop-ack 2 /sop-ack 3 /sop-ack 4 /sop-ack 5 /sop-ack 6 /sop-ack 7

devops-engineer merged commit 37aaa1464f into main 2026-06-19 11:38:19 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

agent-researcher

agent-reviewer-cr2

Labels

Clear labels

approved

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

do-not-merge

hold from auto-merge (design review in progress)

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

needs-engineer

platform/go

Go platform test issues

ready-to-build

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#3073