#89: Add CORS Access-Control-Allow-Origin header

[barrycaceres]

Resolves Issue #89

---

Resolves #89
Resolves #87
Resolves senzing-garage/template-docker#135

[github-actions]

🤖 Claude Code Review

I'll perform a comprehensive code review of this pull request against the provided checklist.

Code Review for PR: Version 0.4.0 Release

Code Quality

✅ Code Style Adherence

The code generally follows Java conventions with proper indentation, naming, and structure. The formatting appears consistent with the project's style guidelines.

✅ No Commented-Out Code

No commented-out code blocks detected in the diff.

✅ Meaningful Variable Names

Variable names are clear and descriptive:

• allowedOrigins - clear purpose
• corsDecorator - indicates CORS decorator function
• dataMartReports - descriptive service name

✅ DRY Principle Followed

The code appropriately reuses the corsDecorator for both gRPC and data mart services, avoiding duplication.

⚠️ Potential Defects Identified

1. Line 441 (SzGrpcServer.java): The corsDecorator variable is initialized as null but the logic checks for allowedOrigins != null && allowedOrigins.size() > 0. This could be simplified to allowedOrigins != null && !allowedOrigins.isEmpty() for better readability.

2. Lines 443-456 (SzGrpcServer.java): The CORS configuration uses allowAllRequestHeaders(true) which might be too permissive for production environments. Consider restricting this to specific headers based on security requirements.

3. Line 448 (SzGrpcServer.java): Using allowCredentials() is only applied when specific origins are provided but not for wildcard (*). This is correct behavior, but worth noting that wildcard origin with credentials is not allowed per CORS spec.

4. Lines 842-843 (SzGrpcServerOption.java): The processing logic for ALLOWED_ORIGINS uses .trim() on each parameter but doesn't validate that the origins are valid URLs/domains. Consider adding URL validation.

Testing

❌ Unit Tests for New Functions

Issue: No tests are included in this PR for the new CORS functionality. The --allowed-origins option should have:

• Unit tests for parsing multiple origins
• Unit tests for wildcard origin (*)
• Integration tests verifying CORS headers are present in responses
• Tests for invalid origin formats

Location: Tests should be added for the new functionality in SzGrpcServer.java:441-456 and SzGrpcServerOption.java:117-133, 838-843.

❌ Integration Tests for New Endpoints

No integration tests verify that CORS headers are actually sent in HTTP responses from the server.

❌ Edge Cases Covered

Missing test coverage for:

• Empty string origins
• Invalid URL formats
• Mixed valid/invalid origins
• Null handling in the list

❌ Test Coverage > 80%

Cannot verify coverage without test execution, but the lack of tests for new functionality suggests coverage goals may not be met.

Documentation

⚠️ Readme Updated if Needed

The PR doesn't show changes to README.md. If the CORS feature is significant for users, it should be documented in the README or user guide.

⚠️ API Docs Updated

While Javadoc exists for the new getAllowedOrigins() and setAllowedOrigins() methods (lines 387-408 in SzGrpcServerOptions.java), there's no documentation explaining the security implications or usage examples.

✅ Inline Comments for Complex Logic

The CORS decorator logic is reasonably clear, though additional comments explaining the distinction between wildcard and specific origins would be helpful.

✅ CHANGELOG.md Updated

The CHANGELOG.md has been updated appropriately (lines 8-12) with version 0.4.0 and mentions the CORS support.

❌ Markdown Formatting Issues in CHANGELOG.md

Issue at CHANGELOG.md:12: According to the CommonMark specification and the review requirements, there should be:

• Empty lines before and after lists
• The CHANGELOG is missing reference links that were present in the original template

Original template had:

[Keep a Changelog]: https://keepachangelog.com/en/1.0.0/
[markdownlint]: https://dlaa.me/markdownlint/
[Semantic Versioning]: https://semver.org/spec/v2.0.0.html

These reference links should be maintained for proper markdown compliance.

Security

✅ No Hardcoded Credentials

No credentials detected in the code.

⚠️ Input Validation Issues

Line 838-843 (SzGrpcServerOption.java): The ALLOWED_ORIGINS processing only trims whitespace but doesn't validate:

• URL format correctness
• Protocol presence (http/https)
• Prevention of injection attacks through malformed origins

Recommendation: Add validation like:

case ALLOWED_ORIGINS:
    List<String> origins = new LinkedList<>();
    for (String param : params) {
        String trimmed = param.trim();
        if (trimmed.isEmpty()) continue;
        // Add URL validation here
        if (!isValidOrigin(trimmed)) {
            throw new IllegalArgumentException("Invalid origin format: " + trimmed);
        }
        origins.add(trimmed);
    }
    return origins;

✅ Proper Error Handling

Error handling appears consistent with existing patterns in the codebase.

✅ No Sensitive Data in Logs

No sensitive data logging detected.

✅ No License Files

No .lic files or AQAAAD strings detected in the diff.

---

Summary

Critical Issues

None

Important Issues

1. Missing Tests (Lines 441-490 in SzGrpcServer.java, 117-133 & 838-843 in SzGrpcServerOption.java): No unit or integration tests for the new CORS functionality
2. Input Validation (SzGrpcServerOption.java:838-843): Origins are not validated for proper URL format
3. CHANGELOG.md Formatting (CHANGELOG.md:1-12): Missing reference links from template

Recommendations

1. Add comprehensive test coverage for CORS functionality
2. Implement origin URL validation in the parameter processing
3. Restore reference links in CHANGELOG.md
4. Consider documenting CORS usage in README or separate guide
5. Review if allowAllRequestHeaders(true) is appropriate for your security requirements
6. Consider adding comments explaining the wildcard vs specific origin distinction in the CORS setup logic

Positive Aspects

• Clean implementation following existing patterns
• Proper separation of concerns
• Good code organization and naming
• Appropriate reuse of the CORS decorator

---

Automated code review analyzing defects and coding standards

[github-actions]

Code Coverage

Overall Project	39.14% -0.28%	🔴
Files changed	66.81%	🔴

File	Coverage
SzGrpcServerOption.java	54.25% -3.62%	🟢
SzGrpcServer.java	50.86% -11.31%	🔴
SzGrpcServerOptions.java	37.29% -6.95%	🔴

[github-actions]

🤖 Claude Code Review

---

Summary of Findings

❌ Critical Issues

1. No Tests - Zero test coverage for CORS functionality
2. CHANGELOG Date Error - Future date (2025-12-18) instead of actual release date
3. Missing URL Validation - Allowed origins not validated (SzGrpcServerOption.java:838-842)
4. Security Risk - Wildcard Origins - No warning about * allowing all origins (SzGrpcServer.java:444-447)

⚠️ Major Issues

5. Missing Documentation:

   • README not updated with --allowed-origins option
   • No Javadoc for new public methods
   • No inline comments for CORS logic

6. CHANGELOG Quality - Removed important references, vague change description

7. Edge Cases Not Handled:

   • Empty string origins
   • Duplicate origins
   • Invalid URL formats

✅ Positive Aspects

• Clean implementation of CORS using Armeria's built-in support
• Proper separation between wildcard and specific origin handling
• Consistent code style with existing codebase
• Appropriate use of decorator pattern for CORS

---

Recommendations

Must Fix Before Merge:

1. Add comprehensive unit and integration tests
2. Fix CHANGELOG date to actual release date
3. Add URL validation for allowed origins
4. Document security implications of wildcard CORS

Should Fix:

5. Update README with new command-line option
6. Add Javadoc to all new public methods
7. Restore CHANGELOG references
8. Add input validation for edge cases

Nice to Have:

9. Refactor CORS configuration logic for clarity (extract method)
10. Add inline comments explaining CORS decorator application

---

Automated code review analyzing defects and coding standards

[github-actions]

🤖 Claude Code Review

---

Summary

Critical Issues (Must Fix)

1. ❌ Missing Tests - No unit or integration tests for the CORS functionality

   • Add tests to verify CORS headers are correctly applied
   • Test wildcard and specific origin scenarios
   • Test that CORS is disabled when not configured

2. ⚠️ Security: Input Validation - ALLOWED_ORIGINS parameter lacks validation (src/main/java/com/senzing/sdk/grpc/server/SzGrpcServerOption.java:838-842)

   • Validate origin URLs are well-formed
   • Consider rejecting obviously invalid inputs

3. ⚠️ Security: Overly Permissive Headers - allowAllRequestHeaders(true) (src/main/java/com/senzing/sdk/grpc/server/SzGrpcServer.java:447, 453)

   • Consider restricting to specific headers
   • Document why all headers are allowed

Major Issues (Should Fix)

4. ❌ Missing README Documentation - No usage examples or security guidance

   • Add --allowed-origins to README
   • Include security warnings about wildcard usage
   • Provide usage examples

5. ⚠️ Inconsistent Null Handling - (src/main/java/com/senzing/sdk/grpc/server/SzGrpcServerOptions.java:405 vs SzGrpcServer.java:443)

   • Clarify whether null or empty list represents "disabled"
   • Make checks consistent

Minor Issues (Consider Fixing)

6. ⚠️ Vague CHANGELOG - Entry lacks detail (CHANGELOG.md:11)

   • Expand description with usage guidance

7. ⚠️ Missing PATCH Method - CORS configuration doesn't include PATCH (src/main/java/com/senzing/sdk/grpc/server/SzGrpcServer.java:446, 452)

   • Verify if PATCH is needed for your API

8. ⚠️ Missing Security Documentation - Javadoc should warn about wildcard security implications (src/main/java/com/senzing/sdk/grpc/server/SzGrpcServerOption.java:117-133)

---

Recommendation

DO NOT MERGE until:

1. Tests are added for the new CORS functionality
2. Input validation is implemented for origin URLs
3. README is updated with usage examples and security guidance
4. Security implications of allowAllRequestHeaders(true) are documented or the implementation is restricted

---

Automated code review analyzing defects and coding standards

[github-actions]

🤖 Claude Code Review

---

📋 Summary

Critical Issues (Must Fix):

1. ❌ Missing Tests - No unit or integration tests for CORS functionality
2. ❌ Incomplete Documentation - README not updated with new option usage

Recommendations:

3. ⚠️ Enhance CHANGELOG - Make the description more specific about the feature
4. ⚠️ Add Input Validation - Validate origin URL formats (should be valid URLs/domains)
5. ⚠️ Security Warning - Consider adding validation/warning for wildcard origin usage

Positive Aspects:

• ✅ Clean implementation following existing patterns
• ✅ Proper separation of concerns (decorator pattern)
• ✅ Version bumps are consistent across files
• ✅ Javadoc properly added for new public methods
• ✅ CORS implementation correctly handles wildcard vs specific origins with credentials

Recommended Actions:

1. Add test coverage in AbstractGrpcTest or new test class:

   • Test CORS headers present when --allowed-origins specified
   • Test CORS headers absent when option not specified
   • Test wildcard origin behavior
   • Test multiple specific origins
   • Test with credentials

2. Update README.md with:

   ## CORS Configuration
   To enable CORS, use the `--allowed-origins` option:
   ```bash
   --allowed-origins https://example.com
   --allowed-origins * # Allow all origins (use with caution)

3. Improve CHANGELOG.md line 12:

   - Adds CORS support via `--allowed-origins` command-line option and `SENZING_TOOLS_ALLOWED_ORIGINS` environment variable. Enables `Access-Control-Allow-Origin` header configuration.

---

Automated code review analyzing defects and coding standards