Files

T

zhiqiang feng ce4c2b088f fix: comprehensive bug fixes, hardening, and test coverage

Bug fixes:
- Fix nil-pointer panic in sendMessage SOAP fault handler (used err.Error() on nil)
- Fix missing return after SOAP fault response (caused fall-through to 200 OK)
- Fix timeout=0 shadowing: removed package-level constant, use configured value
- Fix receiveMessage not checking SOAP faults from CIIMS

Codec hardening:
- Replace fragile regex parsing with encoding/xml.Decoder for namespace-agnostic XML
- XML-escape all user inputs (user, pass, event) — not just message body
- Remove magic-number-based string slicing (split, GetErrMsg)

Transport improvements:
- Extract Client type with connection pooling (reuse http.Client across requests)
- Add CIIMSResponse domain type with IsFault(), ErrorMessage(), Messages()
- Add HTTPStatusError and ResponseTooLargeError typed errors
- Replace deprecated ioutil.ReadAll with io.ReadAll
- Add response body size limit (64 MiB)

Security hardening:
- Add MaxBytesReader (1MB request body limit) on both endpoints
- Add URL allowlist via CIIMS_ALLOWED_SERVERS env var
- Add normalizeBaseURL with scheme/host/query validation
- Add count bounds validation (1-1000) on /receive
- Add server-level timeouts (ReadHeaderTimeout, ReadTimeout, IdleTimeout)

Configuration:
- Introduce Config struct to replace package-level globals
- Add loadConfig() with full validation and error propagation
- Add getIntEnv() with positive-value enforcement

Test coverage (75 tests, 35 new):
- Phase 1: 8 internal tests (nil receiver, error types, SOAP+HTTP500, malformed XML)
- Phase 2: 19 pure function tests (normalizeBaseURL, parseAllowedServers, getIntEnv)
- Phase 3: 16 handler tests (send/receive success, SOAP fault, network error, HTTP 500,
  body too large, invalid JSON, missing fields, URL allowlist)
- Phase 4: 8 config/router tests (loadConfig, newServer, newRouter)

Toolchain:
- Upgrade Go 1.13 → 1.22, gin 1.6.3 → 1.10.0, testify 1.5.1 → 1.10.0

Cleanup:
- Remove dead code (unused post() function, commented-out defaults)
- Replace println with structured logging
- Add .gitignore
- Rewrite README with API docs, env vars, security considerations

2026-07-08 16:23:00 +08:00

14 KiB

Raw Permalink Blame History

CIIMS Proxy — Test Coverage Fix Plan

Objective

Close all test coverage gaps identified in the codebase analysis. The internal package has decent coverage (20 tests) but cmd/main/main.go has zero tests across 12 functions. This plan adds 49 new tests organized in 4 phases.

Phase 1: `internal` Package — Remaining Gaps (8 tests)

Objective

Cover the few untested paths in the already well-tested internal package.

Implementation Plan

1.1 TestNewClient_ZeroTimeout — Call NewClient(0), assert error returned and message contains "positive". Also test NewClient(-1). Rationale: The error path for invalid timeout values is untested. loadConfig depends on this to fail fast.
1.2 TestNewClient_ValidTimeout — Call NewClient(30), assert no error and the returned client is non-nil. Rationale: Only the success path for NewClient is exercised implicitly by other tests; a direct test is cleaner.
1.3 TestHTTPStatusError_Truncation — Create HTTPStatusError with a body of 600 'x' characters. Assert Error() returns a string ending in "... and not exceeding ~520 characters (512 + "ciims returned ...: " prefix). Rationale: The truncation logic at http.go:38-40 is untested and could silently break.
1.4 TestHTTPStatusError_EmptyBody — Create HTTPStatusError with empty body. Assert Error() returns "ciims returned <status>" without a colon. Rationale: The empty-body branch at http.go:41-43 is untested.
1.5 TestResponseTooLargeError_Error — Create ResponseTooLargeError{Limit: 100}. Assert Error() returns "ciims response exceeds 100 bytes". Rationale: Simple error type with no existing direct test.
1.6 TestCIIMSResponse_NilReceiver — Call (*CIIMSResponse)(nil).IsFault(), .ErrorMessage(), .Messages(). Assert they return false, "", and nil respectively without panicking. Rationale: The nil-guard branches at http.go:63,67,74 are untested.
1.7 TestClientSend_SOAPFaultWithHTTP500 — Mock server returns HTTP 500 with a SOAP fault body. Assert err is *HTTPStatusError, resp.IsFault() is true, and resp.ErrorMessage() returns the fault text. Rationale: handleSendError in main.go has a code path that extracts SOAP faults from HTTPStatusError.Body — this scenario must be tested at the transport layer first.
1.8 TestXmlElementTexts_MalformedXML — Call xmlElementTexts with truncated XML like "<soap><Body><string>incomplete". Assert it returns gracefully (empty slice) without panicking. Rationale: The decoder.Token() error path at codec.go:136-138 is untested.

Phase 2: `cmd/main` — Pure Functions (19 tests)

Objective

Test all functions in main.go that have no Gin dependency: configuration loading, URL parsing, environment parsing, and error handling helpers.

Implementation Plan

2.1 TestNormalizeBaseURL_ValidHTTP — Input "http://example.com/path/", assert returns "http://example.com/path" (trailing slash stripped, scheme lowercased). Rationale: Happy path for the URL normalization function.
2.2 TestNormalizeBaseURL_ValidHTTPS — Input "https://EXAMPLE.COM:8443", assert host lowercased, scheme lowercased. Rationale: Verifies case normalization and port preservation.
2.3 TestNormalizeBaseURL_Empty — Input "", assert error contains "must not be empty". Rationale: Empty input validation.
2.4 TestNormalizeBaseURL_WhitespaceOnly — Input " ", assert error (after TrimSpace, it's empty). Rationale: Edge case for whitespace handling.
2.5 TestNormalizeBaseURL_InvalidScheme — Input "ftp://example.com", assert error contains "scheme must be http or https". Rationale: Scheme allowlist enforcement.
2.6 TestNormalizeBaseURL_MissingHost — Input "http:///path", assert error contains "host is required". Rationale: url.Parse on http:///path sets Host to empty.
2.7 TestNormalizeBaseURL_QueryNotAllowed — Input "http://example.com?a=1", assert error contains "query and fragment are not allowed". Rationale: Query/fragment rejection prevents URL smuggling.
2.8 TestNormalizeBaseURL_FragmentNotAllowed — Input "http://example.com#section", assert error. Rationale: Same as above for fragments.
2.9 TestNormalizeBaseURL_InvalidURL — Input "://bad", assert error (from url.Parse). Rationale: Tests the url.Parse error path.
2.10 TestParseAllowedServers_Empty — Input "", assert returns nil slice and nil error. Rationale: Empty env var is valid (means "no additional servers").
2.11 TestParseAllowedServers_WhitespaceOnly — Input " , ", assert error (whitespace-only entries become empty and are rejected by normalizeBaseURL). Rationale: Edge case for the split logic.
2.12 TestParseAllowedServers_ValidSingle — Input "http://other.example.com", assert returns ["http://other.example.com"]. Rationale: Happy path for single additional server.
2.13 TestParseAllowedServers_ValidMultiple — Input "http://a.com,https://b.com:8443", assert both are normalized and returned. Rationale: Multi-server allowlist parsing.
2.14 TestParseAllowedServers_InvalidEntry — Input "http://good.com,ftp://bad.com", assert error references the bad entry and the env var name. Rationale: Error propagation with context.
2.15 TestGetIntEnv_Unset — Unset the env var, assert returns (0, nil). Rationale: Default behavior when env var is absent.
2.16 TestGetIntEnv_Valid — Set env var to "30", assert returns (30, nil). Rationale: Happy path.
2.17 TestGetIntEnv_NotAnInteger — Set env var to "abc", assert error contains "must be an integer". Rationale: Parse error path.
2.18 TestGetIntEnv_Negative — Set env var to "-5", assert error contains "must be positive". Rationale: Negative value rejection (new validation in current code).
2.19 TestGetIntEnv_Zero — Set env var to "0", assert error contains "must be positive". Rationale: Zero value rejection.]]>

Phase 3: `cmd/main` — Handler-Level Tests (16 tests)

Objective

Test the Gin HTTP handlers end-to-end using httptest.NewServer + a mock CIIMS backend. This is the most critical gap.

Implementation Plan

3.1 TestSendSuccess — POST to /send with valid JSON. Mock CIIMS returns testSendOK. Assert 200, {"error":""}. Rationale: Core happy path for the send endpoint.
3.2 TestSendSOAPFault — POST to /send with valid JSON. Mock CIIMS returns testErrMsg. Assert 500, response contains the fault message. Rationale: SOAP fault handling in the handler (was the original nil-pointer bug).
3.3 TestSendNetworkError — POST to /send with valid JSON. Mock CIIMS is closed before the request. Assert 500, response contains error. Rationale: Network error path through handleSendError.
3.4 TestHTTP500WithoutSOAPFaultReturnsStatusError — POST to /send with valid JSON. Mock CIIMS returns HTTP 500 with plain text body. Assert 500, response contains the status error. Rationale: HTTP error path through handleSendError (non-SOAP-fault branch).
3.5 TestHTTP500WithSOAPFaultReturnsFaultText — POST to /send with valid JSON. Mock CIIMS returns HTTP 500 with a SOAP fault body. Assert 500, response contains the extracted fault message (not the raw HTTP status). Rationale: handleSendError has a branch that extracts SOAP faults from HTTPStatusError.Body — this is the most complex error path.
3.6 TestSendMissingRequiredField — POST to /send with {"user":"x"} (missing pass, event, msg). Assert 400. Rationale: Gin binding validation.
3.7 TestSendOversizedBody — POST to /send with body > 1MB. Assert 413. Rationale: MaxBytesReader enforcement.
3.8 TestSendInvalidJSON — POST to /send with not json. Assert 400. Rationale: Malformed JSON handling.
3.9 TestAllowedRequestURLSucceeds — POST to /send with {"url":"http://allowed.example.com",...} where the URL is in AllowedServers. Assert 200 (request proxied). Rationale: Allowlist pass-through.
3.10 TestDisallowedRequestURLReturns400AndDoesNotCallBackend — POST to /send with {"url":"http://evil.com",...}. Assert 400, error contains "not allowed", and backend is never called. Rationale: Allowlist rejection.
3.11 TestReceiveSuccess — POST to /receive with valid JSON. Mock CIIMS returns testReceiveResp. Assert 200, response contains messages. Rationale: Core happy path for receive.
3.12 TestReceiveSOAPFault — POST to /receive with valid JSON. Mock CIIMS returns a SOAP fault. Assert 500. Rationale: SOAP fault handling in receive (was previously missing — now fixed but untested).
3.13 TestReceiveInvalidCount — POST to /receive with "count":0. Assert 400, error contains "between 1 and". Also test count=1001. Rationale: Count lower-bound and upper-bound validation.
3.14 (merged with 3.13) — Already covered by TestReceiveInvalidCount.
3.15 TestReceiveCountValidBoundary — POST to /receive with "count":1000. Assert 200 (boundary value). Rationale: Maximum allowed count should succeed.
3.16 TestPing — GET /ping. Assert 200, {"message":"pong"}. Rationale: Health check endpoint.

Phase 4: `cmd/main` — Config & Router Tests (8 tests)

Objective

Test loadConfig, newServer, and newRouter with environment variable manipulation.

Implementation Plan

4.1 TestLoadConfig_Minimal — Set only CIIMS_SERVER=http://example.com, unset others. Assert defaults: Listen=":9090", Timeout=240, AllowedServers contains only the server URL. Rationale: Default configuration path.
4.2 TestLoadConfig_Full — Set all env vars. Assert all values are parsed correctly including AllowedServers. Rationale: Full configuration path.
4.3 TestLoadConfigRequiresCIIMSServer — Set CIIMS_SERVER=. Assert error contains "CIIMS_SERVER". Rationale: Configuration validation propagates errors.
4.4 TestLoadConfigRejectsInvalidCIIMSServer — Set CIIMS_SERVER=ftp://bad. Assert error contains "scheme". Rationale: Invalid URL causes load failure.
4.5 TestLoadConfigRejectsNonPositiveTimeout — Set CIIMS_TIMEOUT=0. Assert error contains "positive". Rationale: Zero timeout rejection propagates through NewClient.
4.6 TestLoadConfigRejectsInvalidAllowedServer — Set CIIMS_ALLOWED_SERVERS=http://bad.example.com?x=1. Assert error. Rationale: Invalid allowed server entry causes load failure.
4.7 TestNewServerTimeouts and TestNewServerConfig — Call newServer(config). Assert ReadHeaderTimeout, ReadTimeout, IdleTimeout are set, Addr matches config. Rationale: Server construction verification.
4.8 TestNewRouter_RoutesExist — Call newRouter(config). Use httptest.NewServer with the Gin engine. Assert GET /ping, POST /send, POST /receive all return non-404. Rationale: Route registration verification.

Additional Tests (beyond original plan)

TestDefaultURLUsesCIIMSServer — Verifies the service prefix is appended to the default URL.
TestRequestURLWithQueryReturns400 — Verifies URLs with query params are rejected.
TestLoadConfigAllowedServers — Verifies case normalization and multiple allowed servers.
TestLoadConfig_InvalidTimeout — Verifies non-integer timeout produces error.

Verification Criteria

All 49 new tests pass with go test ./... -count=1
go vet ./... reports no issues
Handler tests use t.Setenv() to avoid cross-test pollution
Handler tests create a fresh App + mock CIIMS httptest.Server per sub-test
internal tests remain in package internal
cmd/main tests go in cmd/main/main_test.go (package main)
No test depends on external network access

Potential Risks and Mitigations

Environment variable pollution between tests Mitigation: Use t.Setenv() which automatically restores the original value after the test. For loadConfig tests, set all relevant vars in each test case.
Gin mode pollution (debug vs release logging) Mitigation: Call gin.SetMode(gin.TestMode) in TestMain or at the top of each test function.
Port conflicts in parallel handler tests Mitigation: Use httptest.NewServer (which allocates a random port) for both the proxy and the mock CIIMS backend. Do not use t.Parallel() for handler tests that share mock servers.
getIntEnv tests need os.Setenv Mitigation: Use t.Setenv() — this is the standard approach since Go 1.17.
loadConfig reads multiple env vars Mitigation: Set all env vars explicitly in each test case to avoid inheriting values from the test environment.

Alternative Approaches

Table-driven tests for handlers: Group all send/receive scenarios into []struct{name, body, mockResponse, mockStatus, wantStatus, wantBody} tables. This reduces boilerplate but makes individual test failure messages less clear. Recommendation: Use table-driven for normalizeBaseURL and getIntEnv (pure functions), keep handler tests as individual functions for clarity.
Refactor handlers to accept http.Handler interface: Extract handler logic from Gin to standard http.Handler for easier testing. Recommendation: Not necessary — httptest.NewServer with Gin works fine and tests the actual routing layer.
Use httptest.NewRecorder directly on Gin: Call router.ServeHTTP(w, req) instead of httptest.NewServer. Recommendation: Use httptest.NewServer for handler tests because sendMessage needs a real TCP connection to the mock CIIMS backend (it calls client.Send with a URL).

14 KiB Raw Permalink Blame History

CIIMS Proxy — Test Coverage Fix Plan

Objective

Phase 1: internal Package — Remaining Gaps (8 tests)

Objective

Implementation Plan

Phase 2: cmd/main — Pure Functions (19 tests)

Objective

Implementation Plan

Phase 3: cmd/main — Handler-Level Tests (16 tests)

Objective

Implementation Plan

Phase 4: cmd/main — Config & Router Tests (8 tests)

Objective

Implementation Plan

Additional Tests (beyond original plan)

Verification Criteria

Potential Risks and Mitigations

Alternative Approaches

14 KiB

Raw Permalink Blame History

Phase 1: `internal` Package — Remaining Gaps (8 tests)

Phase 2: `cmd/main` — Pure Functions (19 tests)

Phase 3: `cmd/main` — Handler-Level Tests (16 tests)

Phase 4: `cmd/main` — Config & Router Tests (8 tests)