Repository navigation
fix: Validate Access-Control-Max-Age is non-negative (Fetch spec §3.2.1) - #80
Conversation
setMaxage() now throws MisconfigurationException for negative values. Zero is accepted (no header emitted per the existing > 0 guard). Adds core.INVALID_ARGUMENT error code to misconfiguration.properties. Closes #72 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…GUMENT
- Rename field/setter to maxAge (camelCase, consistent with JavaBeans convention and the HTTP header name)
- Fix core.INVALID_ARGUMENT.solution to be self-contained in properties file instead of passing free-text as {2} argument (enables i18n)
- Update test method names and calls accordingly
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ge null - Remove the free-text 4th argument from MisconfigurationException (solution is now self-contained in properties) - Add null guard in handle() to prevent NPE on unboxing when maxAge is null - Add setMaxAgeNullIsAccepted test to cover the null case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds validation to CorsMiddleware.setMaxAge() to enforce that the Access-Control-Max-Age value is non-negative, per the Fetch specification §3.2.1. It also renames the public setter from setMaxage() to setMaxAge() for proper camelCase naming, and introduces a new general-purpose core.INVALID_ARGUMENT error code in misconfiguration.properties.
Changes:
CorsMiddleware.setMaxAge()now throwsMisconfigurationExceptionwith codecore.INVALID_ARGUMENTfor negative values, and a null-check guards themaxAge > 0Lheader-emission condition- New
core.INVALID_ARGUMENTerror code (problem + solution) added toMETA-INF/misconfiguration.properties - Four new tests covering the null, negative, zero, and positive cases for
setMaxAge()
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
enkan-web/src/main/java/enkan/middleware/CorsMiddleware.java |
Renames field/setter to camelCase maxAge, adds null-safety to the header guard, and adds negative-value validation with MisconfigurationException |
enkan-web/src/test/java/enkan/middleware/CorsMiddlewareTest.java |
Adds four new test cases covering the null, negative, zero, and positive paths of setMaxAge() |
enkan-core/src/main/resources/META-INF/misconfiguration.properties |
Introduces the new core.INVALID_ARGUMENT error code with problem/solution messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void setMaxAgeNullIsAccepted() { | ||
| CorsMiddleware sut = new CorsMiddleware(); | ||
| sut.setMaxAge(null); // no exception; header will not be emitted | ||
| } | ||
|
|
||
| @Test | ||
| void setMaxAgeNegativeThrowsMisconfigurationException() { | ||
| CorsMiddleware sut = new CorsMiddleware(); | ||
| assertThatThrownBy(() -> sut.setMaxAge(-1L)) | ||
| .isInstanceOf(MisconfigurationException.class); | ||
| } | ||
|
|
||
| @Test | ||
| void setMaxAgeZeroIsAccepted() { | ||
| CorsMiddleware sut = new CorsMiddleware(); | ||
| sut.setMaxAge(0L); // no exception; header will not be emitted (> 0 guard) | ||
| } |
There was a problem hiding this comment.
The setMaxAgeNullIsAccepted and setMaxAgeZeroIsAccepted tests only verify that no exception is thrown, but neither performs any end-to-end assertion that the Access-Control-Max-Age header is absent from a preflight response when maxAge is null or 0. Since the comment in the test body explicitly states "header will not be emitted", it would be more thorough to also assert doesNotContainKey("Access-Control-Max-Age") after issuing a preflight request, mirroring the approach used in setMaxAgePositiveEmitsHeader.
| */ | ||
| public void setMaxage(Long maxage) { | ||
| this.maxage = maxage; | ||
| public void setMaxAge(Long maxAge) { |
There was a problem hiding this comment.
The PR description's Summary and Test plan consistently refer to setMaxage() (lowercase 'age') and test names like setMaxageNegativeThrowsMisconfigurationException, but the actual code renames the method to setMaxAge() (camelCase 'Age') and the test methods are named setMaxAgeNegativeThrowsMisconfigurationException, setMaxAgeZeroIsAccepted, and setMaxAgePositiveEmitsHeader. The PR description should be updated to reflect the actual method name setMaxAge() and the accurate test names.
| */ | ||
| public void setMaxage(Long maxage) { | ||
| this.maxage = maxage; | ||
| public void setMaxAge(Long maxAge) { |
There was a problem hiding this comment.
Renaming the public method setMaxage(Long) to setMaxAge(Long) is a breaking change for external consumers of this API. Any code outside this repository that calls setMaxage() will fail to compile after this change. If backward compatibility is required, consider keeping the old setMaxage() method as a deprecated delegate to the new setMaxAge() method.
| core.MISSING_IMPLEMENTATION.problem={0} | ||
| core.MISSING_IMPLEMENTATION.solution={0} | ||
| core.INVALID_ARGUMENT.problem=Argument `{0}` has an invalid value: {1}. | ||
| core.INVALID_ARGUMENT.solution=Provide a valid value for `{0}`. See the documentation for allowed values. |
There was a problem hiding this comment.
The solution message "See the documentation for allowed values" is less actionable than other entries in this file, which provide specific guidance (e.g., core.NULL_ARGUMENT.solution says "Provide a non-null value for {0}"). Since the only constraint this error code currently enforces is a non-negative value, the solution could include a hint such as "must be a non-negative integer" directly in the message, or use an additional placeholder so callers can supply the constraint description alongside the argument name and value.
| core.INVALID_ARGUMENT.solution=Provide a valid value for `{0}`. See the documentation for allowed values. | |
| core.INVALID_ARGUMENT.solution=Provide a non-negative integer value for `{0}`. |
- core.INVALID_ARGUMENT.solution: use more actionable message with constraint hint - setMaxAgeNullIsAccepted / setMaxAgeZeroIsAccepted: add preflight assertion that Access-Control-Max-Age header is absent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
setMaxage()now throwsMisconfigurationExceptionwith codecore.INVALID_ARGUMENTwhen a negative value is passedsetMaxage(0L)is accepted (noAccess-Control-Max-Ageheader is emitted, per the existing> 0guard)core.INVALID_ARGUMENTerror code tomisconfiguration.propertiesTest plan
setMaxageNegativeThrowsMisconfigurationException—setMaxage(-1L)throwsMisconfigurationExceptionsetMaxageZeroIsAccepted—setMaxage(0L)does not throwsetMaxagePositiveEmitsHeader—setMaxage(3600L)emitsAccess-Control-Max-Age: 3600in preflight responseCloses #72
🤖 Generated with Claude Code