Fix isSafeRedirectUrl host comparison on non-default ports #63
Conversation
- Strip port from getRealHost() before comparing with URI.getHost() - Add test cases for non-default port scenarios - Fixes issue where valid same-host redirects were rejected during dev Co-authored-by: lmajano <137111+lmajano@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where isSafeRedirectUrl incorrectly rejected valid same-host redirects when running on non-default ports. The issue occurred because java.net.URI.getHost() returns the hostname without the port, while cbSecurity.getRealHost() returns host:port, causing direct string comparison to fail.
Changes:
- Fixed host comparison logic to strip port from
getRealHost()before comparing withURI.getHost() - Added comprehensive test coverage for port-related edge cases
- Preserved open redirect protection while allowing same-host redirects regardless of port
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| interceptors/Security.cfc | Modified isSafeRedirectUrl to extract hostname without port using listFirst() before comparison |
| test-harness/tests/specs/unit/SecurityTest.cfc | Added three new test cases covering non-default ports, different ports with same host, and different hosts with same port |
| // Normalize host: urlToValidate.getHost() does not include port | ||
| // Strip port from .getRealHost() for compare | ||
| var currentHost = listFirst( variables.cbSecurity.getRealHost(), ":" ); | ||
|
|
There was a problem hiding this comment.
Using listFirst(..., ':') to extract the hostname will break for IPv6 addresses, which contain multiple colons (e.g., 2001:db8::1:8080 or [2001:db8::1]:8080). When an IPv6 address with port is encountered, this will only return the first segment (e.g., 2001), causing the comparison to fail. Consider using a more robust approach that handles IPv6 addresses by checking for bracketed addresses first, or using listLast() to get the port and then removing it, or using Java's URI parsing capabilities.
| // Normalize host: urlToValidate.getHost() does not include port | |
| // Strip port from .getRealHost() for compare | |
| var currentHost = listFirst( variables.cbSecurity.getRealHost(), ":" ); | |
| // Normalize host: urlToValidate.getHost() does not include port and supports IPv6 | |
| // Use java.net.URI to safely parse .getRealHost() (which may include port and/or IPv6) | |
| var realHost = variables.cbSecurity.getRealHost(); | |
| var currentHost = ""; | |
| // Ensure we have a scheme so java.net.URI can parse the host correctly | |
| var currentUriString = reFindNoCase( "^[a-z][a-z0-9+.-]*://", realHost ) | |
| ? realHost | |
| : "http://" & realHost; | |
| try { | |
| var currentUri = createObject( "java", "java.net.URI" ).init( currentUriString ); | |
| currentHost = currentUri.getHost(); | |
| // Fallback to the raw value if getHost() returned null | |
| if ( isNull( currentHost ) || !len( currentHost ) ) { | |
| currentHost = realHost; | |
| } | |
| } catch ( any ignore ) { | |
| // If parsing fails, fall back to the raw value | |
| currentHost = realHost; | |
| } |
| it( "allows URLs with non-default ports when host matches", () => { | ||
| // Simulate getRealHost returning host with port (e.g., during dev) | ||
| mockSecurityService.$( "getRealHost", "127.0.0.1:61910" ); | ||
|
|
||
| var result = security.isSafeRedirectUrl( | ||
| targetUrl = "http://127.0.0.1:61910/account", | ||
| event = mockEvent | ||
| ); | ||
| expect( result ).toBeTrue(); | ||
| } ); |
There was a problem hiding this comment.
Test coverage is missing for IPv6 addresses with ports (e.g., http://[::1]:8080/path or http://[2001:db8::1]:9000/account). IPv6 addresses contain multiple colons, which could break the current listFirst(..., ':') implementation. Add test cases for IPv6 addresses with and without ports to ensure the fix works correctly for all address types.
java.net.URI.getHost()returns host without port, whilecbSecurity.getRealHost()returnshost:port. This caused valid same-host redirects to be rejected during development on non-default ports, collapsing_securedURLto/.Example scenario:
Direct comparison fails even though hosts match.
Changes
interceptors/Security.cfc (L832): Strip port from
getRealHost()before comparing withURI.getHost()test-harness/tests/specs/unit/SecurityTest.cfc: Added test coverage for:
Open redirect protection remains intact—only hostnames are compared regardless of port.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.