fix(pr#2): address CodeRabbit round 2 review findings
Go: - cache: fix TOCTOU race in get() — re-verify entry pointer under lock before evicting to prevent corrupting s.currentBytes and removing a newly-set entry - bridge: fix writeErrorResponse recorder out of sync — buffer into w.body/ w.headers and call commit() so Status(), Header(), Size() reflect error response - bridge: fix ValidateResponse number precision — use json.Decoder+UseNumber for initial envelope decode to preserve large integers (matches Validate path) - ratelimit: fix unreachable credential branches — move X-API-Key and Authorization hashing before IP fallback so NAT'd clients are bucketed by key - openapi: gate cacheSuccessHeaders on sb.CacheEnabled flag, not just method==get - openapi: use isNilRouteGroup in prepareRouteGroups to catch typed-nil RouteGroup PHP: - RateLimitExceededException: remove ad-hoc CORS handling — let framework CORS middleware apply correct headers for all responses including errors - SeoReportService.extractCharset: parse charset token from Content-Type value instead of returning the full "text/html; charset=utf-8" string - SeoReportService: validate IP literals directly with filter_var before DNS lookup so ::ffff:127.0.0.1-style hosts don't bypass private-IP checks - SeoReportService.isPrivateIp: extract isPrivateIpv4 helper; handle IPv4-mapped IPv6 (::ffff::/96) by checking embedded IPv4 against private ranges; add 0.0.0.0/8 to private range list Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
Snider
parent
e54dd2e370
commit
372326297e
6 changed files with 121 additions and 58 deletions
25
bridge.go
25
bridge.go
|
|
@ -290,8 +290,14 @@ func (v *toolInputValidator) ValidateResponse(body []byte) error {
|
|||
return nil
|
||||
}
|
||||
|
||||
// Use a decoder with UseNumber so that large integers in the envelope
|
||||
// (including within the data field) are preserved as json.Number rather
|
||||
// than being silently coerced to float64. This matches the behaviour of
|
||||
// the Validate path and avoids precision loss for 64-bit integer values.
|
||||
var envelope map[string]any
|
||||
if err := json.Unmarshal(body, &envelope); err != nil {
|
||||
envDec := json.NewDecoder(bytes.NewReader(body))
|
||||
envDec.UseNumber()
|
||||
if err := envDec.Decode(&envelope); err != nil {
|
||||
return coreerr.E("ToolBridge.ValidateResponse", "invalid JSON response", err)
|
||||
}
|
||||
|
||||
|
|
@ -711,14 +717,19 @@ func (w *toolResponseRecorder) writeErrorResponse(status int, resp Response[any]
|
|||
return
|
||||
}
|
||||
|
||||
// Update recorder state so middleware observing c.Writer.Status() or
|
||||
// Written() sees the correct values after an error response is emitted.
|
||||
// Keep recorder state aligned with the replacement response so that
|
||||
// Status(), Written(), Header() and Size() all reflect the error
|
||||
// response. Post-handler middleware and metrics must observe correct
|
||||
// values, not stale state from the reset() call above.
|
||||
w.status = status
|
||||
w.wroteHeader = true
|
||||
|
||||
w.ResponseWriter.Header().Set("Content-Type", "application/json")
|
||||
w.ResponseWriter.WriteHeader(status)
|
||||
_, _ = w.ResponseWriter.Write(data)
|
||||
if w.headers == nil {
|
||||
w.headers = make(http.Header)
|
||||
}
|
||||
w.headers.Set("Content-Type", "application/json")
|
||||
w.body.Reset()
|
||||
_, _ = w.body.Write(data)
|
||||
w.commit()
|
||||
}
|
||||
|
||||
func typeError(path, want string, value any) error {
|
||||
|
|
|
|||
9
cache.go
9
cache.go
|
|
@ -62,6 +62,15 @@ func (s *cacheStore) get(key string) *cacheEntry {
|
|||
}
|
||||
if time.Now().After(entry.expires) {
|
||||
s.mu.Lock()
|
||||
// Re-verify the entry pointer is unchanged before evicting. Another
|
||||
// goroutine may have called set() between us releasing and re-acquiring
|
||||
// the lock, replacing the entry with a fresh one. Evicting the new
|
||||
// entry would corrupt s.currentBytes and lose valid cached data.
|
||||
currentEntry, stillExists := s.entries[key]
|
||||
if !stillExists || currentEntry != entry {
|
||||
s.mu.Unlock()
|
||||
return nil
|
||||
}
|
||||
if elem, exists := s.index[key]; exists {
|
||||
s.order.Remove(elem)
|
||||
delete(s.index, key)
|
||||
|
|
|
|||
|
|
@ -366,7 +366,7 @@ func (sb *SpecBuilder) buildPaths(groups []preparedRouteGroup) map[string]any {
|
|||
"summary": rd.Summary,
|
||||
"description": rd.Description,
|
||||
"operationId": operationID(method, fullPath, operationIDs),
|
||||
"responses": operationResponses(method, rd.StatusCode, rd.Response, rd.ResponseExample, rd.ResponseHeaders, security, deprecated, rd.SunsetDate, rd.Replacement, deprecationHeaders),
|
||||
"responses": operationResponses(method, rd.StatusCode, rd.Response, rd.ResponseExample, rd.ResponseHeaders, security, deprecated, rd.SunsetDate, rd.Replacement, deprecationHeaders, sb.CacheEnabled),
|
||||
}
|
||||
if deprecated {
|
||||
operation["deprecated"] = true
|
||||
|
|
@ -497,10 +497,10 @@ func normaliseOpenAPIPath(path string) string {
|
|||
// operationResponses builds the standard response set for a documented API
|
||||
// operation. The framework always exposes the common envelope responses, plus
|
||||
// middleware-driven 429 and 504 errors.
|
||||
func operationResponses(method string, statusCode int, dataSchema map[string]any, example any, responseHeaders map[string]string, security []map[string][]string, deprecated bool, sunsetDate, replacement string, deprecationHeaders map[string]any) map[string]any {
|
||||
func operationResponses(method string, statusCode int, dataSchema map[string]any, example any, responseHeaders map[string]string, security []map[string][]string, deprecated bool, sunsetDate, replacement string, deprecationHeaders map[string]any, cacheEnabled bool) map[string]any {
|
||||
documentedHeaders := documentedResponseHeaders(responseHeaders)
|
||||
successHeaders := mergeHeaders(standardResponseHeaders(), rateLimitSuccessHeaders(), deprecationHeaders, documentedHeaders)
|
||||
if method == "get" {
|
||||
if method == "get" && cacheEnabled {
|
||||
successHeaders = mergeHeaders(successHeaders, cacheSuccessHeaders())
|
||||
}
|
||||
|
||||
|
|
@ -1554,7 +1554,7 @@ func prepareRouteGroups(groups []RouteGroup) []preparedRouteGroup {
|
|||
|
||||
out := make([]preparedRouteGroup, 0, len(groups))
|
||||
for _, g := range groups {
|
||||
if g == nil {
|
||||
if g == nil || isNilRouteGroup(g) {
|
||||
continue
|
||||
}
|
||||
if isHiddenRouteGroup(g) {
|
||||
|
|
|
|||
20
ratelimit.go
20
ratelimit.go
|
|
@ -202,15 +202,9 @@ func clientRateLimitKey(c *gin.Context) string {
|
|||
}
|
||||
}
|
||||
|
||||
// Fall back to IP address.
|
||||
if ip := c.ClientIP(); ip != "" {
|
||||
return "ip:" + ip
|
||||
}
|
||||
if c.Request != nil && c.Request.RemoteAddr != "" {
|
||||
return "ip:" + c.Request.RemoteAddr
|
||||
}
|
||||
|
||||
// Last resort: hash credential headers so raw secrets are not retained.
|
||||
// Fall back to credential headers before the IP so that different API
|
||||
// keys coming from the same NAT address are bucketed independently. The
|
||||
// raw secret is never stored — it is hashed with SHA-256 first.
|
||||
if apiKey := strings.TrimSpace(c.GetHeader("X-API-Key")); apiKey != "" {
|
||||
h := sha256.Sum256([]byte(apiKey))
|
||||
return "cred:sha256:" + hex.EncodeToString(h[:])
|
||||
|
|
@ -220,6 +214,14 @@ func clientRateLimitKey(c *gin.Context) string {
|
|||
return "cred:sha256:" + hex.EncodeToString(h[:])
|
||||
}
|
||||
|
||||
// Last resort: fall back to IP address.
|
||||
if ip := c.ClientIP(); ip != "" {
|
||||
return "ip:" + ip
|
||||
}
|
||||
if c.Request != nil && c.Request.RemoteAddr != "" {
|
||||
return "ip:" + c.Request.RemoteAddr
|
||||
}
|
||||
|
||||
return "ip:unknown"
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -39,7 +39,11 @@ class RateLimitExceededException extends HttpException
|
|||
*/
|
||||
public function render(?Request $request = null): JsonResponse
|
||||
{
|
||||
$response = $this->errorResponse(
|
||||
// Return the rate-limit error response with rate-limit headers attached.
|
||||
// CORS headers are intentionally omitted here; they are applied by the
|
||||
// framework's CORS middleware (or PublicApiCors) which handles patterns,
|
||||
// credentials, and Vary correctly for all responses — including errors.
|
||||
return $this->errorResponse(
|
||||
errorCode: 'rate_limit_exceeded',
|
||||
message: $this->getMessage(),
|
||||
meta: [
|
||||
|
|
@ -49,22 +53,6 @@ class RateLimitExceededException extends HttpException
|
|||
],
|
||||
status: 429,
|
||||
)->withHeaders($this->rateLimitResult->headers());
|
||||
|
||||
if ($request !== null) {
|
||||
$origin = $request->headers->get('Origin');
|
||||
$allowedOrigins = (array) config('cors.allowed_origins', []);
|
||||
if ($origin !== null && in_array($origin, $allowedOrigins, true)) {
|
||||
$response->headers->set('Access-Control-Allow-Origin', $origin);
|
||||
}
|
||||
|
||||
$existingVary = $response->headers->get('Vary');
|
||||
$response->headers->set(
|
||||
'Vary',
|
||||
$existingVary ? $existingVary.', Origin' : 'Origin'
|
||||
);
|
||||
}
|
||||
|
||||
return $response;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -216,7 +216,18 @@ class SeoReportService
|
|||
}
|
||||
}
|
||||
|
||||
return $this->extractMetaContent($xpath, 'content-type', 'http-equiv');
|
||||
// The http-equiv Content-Type meta returns a full value such as
|
||||
// "text/html; charset=utf-8". Extract only the charset token so that
|
||||
// callers receive a bare encoding label (e.g. "utf-8"), not the whole
|
||||
// content-type string.
|
||||
$contentType = $this->extractMetaContent($xpath, 'content-type', 'http-equiv');
|
||||
if ($contentType !== null) {
|
||||
if (preg_match('/charset\s*=\s*["\']?([^\s;"\']+)/i', $contentType, $matches)) {
|
||||
return $matches[1];
|
||||
}
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -383,6 +394,21 @@ class SeoReportService
|
|||
}
|
||||
|
||||
$host = $parsed['host'];
|
||||
|
||||
// If the host is an IP literal (IPv4 or bracketed IPv6), validate it
|
||||
// directly. dns_get_record returns nothing for IP literals and
|
||||
// gethostbyname returns the same value, so both would silently skip
|
||||
// the private-range check without this explicit guard.
|
||||
$normalised = ltrim(rtrim($host, ']'), '['); // strip IPv6 brackets
|
||||
if (filter_var($normalised, FILTER_VALIDATE_IP) !== false) {
|
||||
if ($this->isPrivateIp($normalised)) {
|
||||
throw new RuntimeException('The supplied URL resolves to a private or reserved address.');
|
||||
}
|
||||
|
||||
// Valid public IP literal — no DNS lookup required.
|
||||
return;
|
||||
}
|
||||
|
||||
$records = dns_get_record($host, DNS_A | DNS_AAAA) ?: [];
|
||||
|
||||
// Fall back to gethostbyname for hosts not returned by dns_get_record.
|
||||
|
|
@ -417,28 +443,23 @@ class SeoReportService
|
|||
}
|
||||
|
||||
if (strlen($packed) === 4) {
|
||||
// IPv4 checks.
|
||||
$long = ip2long($ip);
|
||||
if ($long === false) {
|
||||
return true;
|
||||
}
|
||||
$privateRanges = [
|
||||
['start' => ip2long('127.0.0.0'), 'end' => ip2long('127.255.255.255')], // loopback
|
||||
['start' => ip2long('10.0.0.0'), 'end' => ip2long('10.255.255.255')], // RFC-1918
|
||||
['start' => ip2long('172.16.0.0'), 'end' => ip2long('172.31.255.255')], // RFC-1918
|
||||
['start' => ip2long('192.168.0.0'), 'end' => ip2long('192.168.255.255')], // RFC-1918
|
||||
['start' => ip2long('169.254.0.0'), 'end' => ip2long('169.254.255.255')], // link-local
|
||||
];
|
||||
foreach ($privateRanges as $range) {
|
||||
if ($long >= $range['start'] && $long <= $range['end']) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
return $this->isPrivateIpv4($ip);
|
||||
}
|
||||
|
||||
// IPv6 checks: loopback (::1), link-local (fe80::/10), ULA (fc00::/7).
|
||||
// IPv6 checks.
|
||||
|
||||
// ::ffff:0:0/96 — IPv4-mapped addresses (e.g. ::ffff:127.0.0.1).
|
||||
// The first 10 bytes are 0x00, bytes 10-11 are 0xff 0xff, then 4
|
||||
// bytes of IPv4. Evaluate the embedded IPv4 address against the
|
||||
// standard private ranges.
|
||||
if (str_repeat("\x00", 10) . "\xff\xff" === substr($packed, 0, 12)) {
|
||||
$ipv4 = inet_ntop(substr($packed, 12, 4));
|
||||
if ($ipv4 !== false && $this->isPrivateIpv4($ipv4)) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
// Loopback (::1).
|
||||
if ($ip === '::1') {
|
||||
return true;
|
||||
}
|
||||
|
|
@ -458,6 +479,38 @@ class SeoReportService
|
|||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Return true when an IPv4 address string falls within a private,
|
||||
* loopback, link-local, or reserved range.
|
||||
*
|
||||
* Handles 0.0.0.0/8 (RFC 1122 "this network"), 127/8 (loopback),
|
||||
* 10/8, 172.16/12, 192.168/16 (RFC 1918), and 169.254/16 (link-local).
|
||||
*/
|
||||
protected function isPrivateIpv4(string $ip): bool
|
||||
{
|
||||
$long = ip2long($ip);
|
||||
if ($long === false) {
|
||||
return true; // Treat unparsable as unsafe.
|
||||
}
|
||||
|
||||
$privateRanges = [
|
||||
['start' => ip2long('0.0.0.0'), 'end' => ip2long('0.255.255.255')], // 0.0.0.0/8 (RFC 1122)
|
||||
['start' => ip2long('127.0.0.0'), 'end' => ip2long('127.255.255.255')], // loopback
|
||||
['start' => ip2long('10.0.0.0'), 'end' => ip2long('10.255.255.255')], // RFC-1918
|
||||
['start' => ip2long('172.16.0.0'), 'end' => ip2long('172.31.255.255')], // RFC-1918
|
||||
['start' => ip2long('192.168.0.0'), 'end' => ip2long('192.168.255.255')], // RFC-1918
|
||||
['start' => ip2long('169.254.0.0'), 'end' => ip2long('169.254.255.255')], // link-local
|
||||
];
|
||||
|
||||
foreach ($privateRanges as $range) {
|
||||
if ($long >= $range['start'] && $long <= $range['end']) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Quote a literal for XPath queries.
|
||||
*/
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue