zhiqiang feng
ce4c2b088f
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
125 lines
8.7 KiB
Markdown
125 lines
8.7 KiB
Markdown
# 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
|
|
|
|
- [x] Remove the package-level `timeout` constant in `cmd/main/main.go` that always evaluates to `0`.
|
|
- [x] 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.
|
|
- [x] 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.
|
|
- [x] 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
|
|
|
|
- [x] 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()`.
|
|
- [x] 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.
|
|
- [x] Ensure consistent error response shape across all handler error paths (e.g., `{"error": "..."}`).
|
|
|
|
### 3. Harden SOAP Message Construction
|
|
|
|
- [x] XML-escape all user-provided fields inserted into the SOAP templates (`user`, `pass`, `event`, `message`) using `xml.EscapeText` or equivalent.
|
|
- [x] 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.
|
|
- [x] 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.
|
|
- [x] 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.
|
|
- [x] Remove hardcoded magic numbers (`12`, `13`, `54`, `15`) from `split` and `GetErrMsg`.
|
|
- [x] Add tests covering responses with different namespace prefixes, extra attributes, whitespace variations, and missing elements.
|
|
|
|
### 5. Remove Dead Code and Unify Logging
|
|
|
|
- [x] Delete the unused `post` function in `internal/http.go`.
|
|
- [x] Remove commented-out default URLs and the commented `r.Run()` line in `cmd/main/main.go`.
|
|
- [x] 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.
|
|
- [x] Replace `println` startup messages with structured log calls or remove them.
|
|
|
|
### 6. Improve Error Handling
|
|
|
|
- [x] 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.
|
|
- [x] Refactor `getIntEnv` to return `(int, error)` instead of panicking, and let `main` decide how to report invalid configuration.
|
|
- [x] 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.
|
|
- [x] Document that the proxy should run behind TLS when handling credentials.
|
|
- [x] Avoid logging request bodies or credentials; if URL logging is required, log only the host or a sanitized version.
|
|
- [x] Add request body size limits and input validation (e.g., max `count`, max `msg` length) to prevent abuse.
|
|
|
|
### 8. Expand Test Coverage
|
|
|
|
- [x] Add HTTP handler tests for `/send` and `/receive` using `net/http/httptest` and a mock CIIMS backend.
|
|
- [x] Add tests for timeout behavior, including verification that `http.Client.Timeout` is set correctly.
|
|
- [x] Add tests for SOAP fault handling in `sendMessage` and `receiveMessage`.
|
|
- [ ] Add tests for malformed JSON, missing required fields, and invalid `count` values.
|
|
- [x] 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
|
|
|
|
- [x] Update `go.mod` to a supported Go version (e.g., `1.22` or later) and run `go mod tidy`.
|
|
- [x] Upgrade `gin-gonic/gin`, `labstack/gommon`, and `stretchr/testify` to current stable versions.
|
|
- [x] Review release notes for breaking changes in Gin and adjust handler code if necessary.
|
|
- [x] Verify the build and all tests pass on the upgraded toolchain.
|
|
|
|
### 10. Documentation and Deployment Notes
|
|
|
|
- [x] Update `README.md` to document environment variables, optional fields, and security considerations (TLS, authentication).
|
|
- [x] 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.
|