ciimsproxy/plans/2026-07-08-ciims-proxy-fixes-v1.md

CIIMS Proxy Remediation Plan

Objective

Fix all correctness, security, maintainability, and testing issues identified in the code review so the proxy is reliable, safe, and maintainable. Expected outcomes:

• Request timeouts are honored and configurable via CIIMS_TIMEOUT.
• sendMessage no longer panics on SOAP faults and returns a proper error response.
• All user-supplied values are safely XML-escaped before being embedded in SOAP envelopes.
• SOAP response parsing is robust against namespace/formatting changes.
• Dead code and inconsistent logging are removed or unified.
• HTTP handlers have unit/integration tests.
• Dependencies and Go toolchain are upgraded to supported versions.

Implementation Plan

1. Fix Timeout Configuration and Request Handling

• Remove the package-level timeout constant in cmd/main/main.go that always evaluates to 0.
• Introduce an application configuration struct (or closure) to hold the resolved timeout value so handlers receive the configured timeout instead of the global zero value.
• Ensure http.Client.Timeout is set to the configured duration; treat 0 as a validation error or explicitly default to defaultTimeout before constructing the client.
• Validate that CIIMS_TIMEOUT, when provided, parses as a positive integer and fail fast with a clear message on invalid input instead of panicking.

2. Fix sendMessage Error Path

• In cmd/main/main.go, after calling internal.GetErrMsg(resp), use the returned errMsg string for logging and the JSON error payload instead of err.Error().
• Add the missing return statement inside the len(errMsg) > 0 branch so the handler does not return 200 OK {"error":""} after detecting a SOAP fault.
• Ensure consistent error response shape across all handler error paths (e.g., {"error": "..."}).

3. Harden SOAP Message Construction

• XML-escape all user-provided fields inserted into the SOAP templates (user, pass, event, message) using xml.EscapeText or equivalent.
• Audit the sendtpl and receivetpl templates in internal/codec.go to confirm no raw interpolation remains.
• Consider replacing string-template-based SOAP construction with typed encoding/xml structs for the envelope, header, and body, while still embedding the inner message as escaped text.
• Add unit tests that verify payloads containing XML metacharacters (<, >, &, ", ') are escaped correctly.

4. Replace Fragile Response Parsing

• Replace regex-based extraction in GetMsgs and GetErrMsg with XML unmarshaling into properly typed structs, or at minimum use namespace-aware parsing.
• If regex is retained as a short-term fix, switch to non-greedy patterns (e.g., <ns1:string>(.*?)</ns1:string>) and validate slice indices before substring operations.
• Remove hardcoded magic numbers (12, 13, 54, 15) from split and GetErrMsg.
• Add tests covering responses with different namespace prefixes, extra attributes, whitespace variations, and missing elements.

5. Remove Dead Code and Unify Logging

• Delete the unused post function in internal/http.go.
• Remove commented-out default URLs and the commented r.Run() line in cmd/main/main.go.
• Decide on a single logging approach: either use Gin's built-in logger and standard log package, or keep a structured logger consistently; remove the mixed use of github.com/labstack/gommon/log unless it provides required features.
• Replace println startup messages with structured log calls or remove them.

6. Improve Error Handling

• Handle errors from regexp.Compile explicitly; if regex remains, compile patterns once at package init and panic only on init failure, or prefer compile-time-safe approaches.
• Refactor getIntEnv to return (int, error) instead of panicking, and let main decide how to report invalid configuration.
• Ensure all HTTP client errors (network, timeout, non-2xx status) are logged and returned to the client without leaking internal details.

7. Add Security Hardening

• Add configurable authentication for the /send and /receive endpoints (e.g., API key header, basic auth, or TLS client certificates) if the proxy is exposed beyond localhost.
• Document that the proxy should run behind TLS when handling credentials.
• Avoid logging request bodies or credentials; if URL logging is required, log only the host or a sanitized version.
• Add request body size limits and input validation (e.g., max count, max msg length) to prevent abuse.

8. Expand Test Coverage

• Add HTTP handler tests for /send and /receive using net/http/httptest and a mock CIIMS backend.
• Add tests for timeout behavior, including verification that http.Client.Timeout is set correctly.
• Add tests for SOAP fault handling in sendMessage and receiveMessage.
• Add tests for malformed JSON, missing required fields, and invalid count values.
• Ensure existing tests in internal/codec_test.go continue to pass after refactoring, updating expected strings only if the XML format changes intentionally.

9. Upgrade Toolchain and Dependencies

• Update go.mod to a supported Go version (e.g., 1.22 or later) and run go mod tidy.
• Upgrade gin-gonic/gin, labstack/gommon, and stretchr/testify to current stable versions.
• Review release notes for breaking changes in Gin and adjust handler code if necessary.
• Verify the build and all tests pass on the upgraded toolchain.

10. Documentation and Deployment Notes

• Update README.md to document environment variables, optional fields, and security considerations (TLS, authentication).
• Remove the committed main binary from the repository and add it to .gitignore if not already ignored.
• Add a Makefile or build script for consistent compilation and testing (optional but recommended).

Verification Criteria

• CIIMS_TIMEOUT=30 results in outbound requests timing out after 30 seconds; CIIMS_TIMEOUT=0 falls back to the default 240 seconds or fails validation as designed.
• Sending a request that causes a CIIMS SOAP fault returns 500 Internal Server Error with {"error":"<fault text>"} and does not panic.
• A send request with user, pass, event, or msg containing XML metacharacters produces a valid SOAP envelope without breaking XML structure.
• receive responses with different namespace prefixes or whitespace still return the correct decoded messages.
• All existing and new unit tests pass (go test ./...).
• go vet ./... and go build ./... produce no errors on the upgraded Go version.
• The committed main binary is removed from version control.

Potential Risks and Mitigations

1. Regression in SOAP format Mitigation: Keep the existing tests as a baseline and add new tests before refactoring. Compare generated XML with the current expected output to ensure backward compatibility with CIIMS.

2. Namespace changes in CIIMS responses break parsing Mitigation: Move to XML unmarshaling or namespace-agnostic parsing. Add test fixtures covering multiple namespace prefix styles.

3. Authentication requirement breaks existing clients Mitigation: Make authentication optional via environment variable, defaulting to disabled for local development, and document enablement for production.

4. Go/dependency upgrade introduces breaking changes Mitigation: Upgrade dependencies incrementally, run the full test suite after each change, and review Gin migration guides.

5. Timeout behavior change affects long-running CIIMS operations Mitigation: Set a sensible default (e.g., 240 seconds as currently intended) and allow operators to tune CIIMS_TIMEOUT based on observed backend latency.

Alternative Approaches

1. Template-based SOAP vs. struct-based XML marshaling

   • Template approach: Simpler to read and matches the current implementation, but requires careful escaping. Keep if escaping is added and tested.
   • Struct approach: Type-safe and eliminates string-replacement bugs, but more verbose due to mixed namespaces. Recommended for long-term maintainability.

2. Regex parsing vs. XML unmarshaling

   • Regex: Quick to implement and matches current behavior, but fragile. Acceptable only as a short-term fix with non-greedy patterns and bounds checks.
   • XML unmarshaling: Robust and self-documenting. Recommended for production use.

3. Global variables vs. dependency injection for configuration

   • Global variables: Minimal code change, but hard to test. Current code uses this pattern.
   • Dependency injection: Pass a config/handler struct to route registration. Enables better testing and removes hidden state. Recommended during refactoring.