include other claude rules#2538
Open
tharindu1st wants to merge 1 commit into
Open
Conversation
tharindu1st
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
Thushani-Jayasekera,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
lasanthaS,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
thivindu
as code owners
July 8, 2026 06:08
Contributor
📝 WalkthroughWalkthroughThis PR adds and extends ChangesSecurity rules documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
.claude/rules/js-output-encoding-xss.md (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the CSP example to the portal's current CDN-backed assets.
A literal
script-src 'self'example conflicts with the existing CDN script whitelist inportals/developer-portal/src/utils/util.js:700-720, so copying it as-is would break pages that still load those assets. Either call out that the example assumes self-hosted scripts, or list the required hashes/origins for the current model.As shown in
portals/developer-portal/src/utils/util.js:700-720, the current asset-loading contract still depends on CDN scripts.Also applies to: 136-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/rules/js-output-encoding-xss.md around lines 27 - 29, Scope the CSP guidance in the JS output encoding doc so it matches the portal’s current CDN-backed asset model rather than implying a blanket script-src 'self' requirement. Update the CSP example in the affected section to either explicitly say it applies only to self-hosted scripts or to include the needed CDN origins/hashes already used by the asset-loading logic in util.js, and make sure the wording stays consistent with the related guidance elsewhere in the document..claude/rules/js-xxe-xml-processing.md (1)
22-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDon’t present
stopNodesas a depth limit.This reads like
fast-xml-parserhas a real nesting-cap option here, but that may not be what the API actually provides. Please verify the parser docs and reword this to a genuine depth cap or the manual depth-walk / worker-timeout fallback unless the library confirms a real limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/rules/js-xxe-xml-processing.md around lines 22 - 24, The XML parsing guidance currently treats fast-xml-parser stopNodes as if it were a nesting-depth limit, which may be inaccurate. Reword the Depth Ceiling section in js-xxe-xml-processing.md to reference only a real parser depth-cap if the library docs confirm one; otherwise instruct implementers to add a manual depth-tracking validation pass before parsing or use a worker-thread timeout fallback. Ensure the guidance stays aligned with the symbols and concepts in the existing file, especially the parser depth and parse-timeout recommendations..claude/rules/xxe-xml-processing.md (1)
85-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShow a config-backed ceiling in the example.
maxDocBytesis hardcoded here even though the rule says the XML byte ceiling must come from config. A reader can copy this sample verbatim and silently violate the policy. Please inject the configured limit here instead of baking5 << 20into the example.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/rules/xxe-xml-processing.md around lines 85 - 89, The example for the XML parser settings hardcodes the document byte ceiling instead of showing the config-backed value required by the rule. Update the snippet around the maxDocBytes constant to reference the configured limit used by the real call sites, keeping maxElemDepth and parseTimeout as-is, so readers copy an example that matches the policy and symbols like maxDocBytes remain clearly tied to configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/rules/authentication_authorization.md:
- Around line 632-643: The safeRedirectTarget helper currently accepts host-less
opaque URLs, which can still produce unsafe redirect locations. Update
safeRedirectTarget to only allow path-only targets by requiring an empty scheme
and opaque component in addition to no host and no leading "//", and only return
success when the parsed value starts with "/". Keep the validation and return
logic inside safeRedirectTarget consistent so malformed or non-path URLs are
rejected before being used as a redirect target.
In @.claude/rules/js-authentication-authorization.md:
- Around line 603-614: The authMiddleware example calls jwtVerify(token, ...)
before token is defined, so make the snippet copy-pastable by extracting the
bearer token from req.headers.authorization first and binding it to token before
verification. Update authMiddleware to parse the Authorization header, validate
the Bearer format, then pass the extracted token into jwtVerify while keeping
the existing User.findByPk and req.user assignment logic unchanged.
In @.claude/rules/js-file-access.md:
- Around line 254-262: The icon upload handler reads req.file.buffer before
checking whether a file was actually uploaded, so the route can throw when
upload.single('icon') reaches it without a file. Update the /apis/:id/icon
handler to guard req.file at the start of the async callback before calling
validateUploadContent or ApiIcon.create, and return a generic 400/422 response
when missing; use the existing route handler and req.file reference to place the
fix.
In @.claude/rules/js-ssrf-prevention.md:
- Around line 130-146: The import in the spec URL handler is pulling
assertAllowedScheme from the wrong module, so the example will break when
reused. Update the import in the /api/specs/import-from-url handler to get
assertAllowedScheme from ssrfGuard.js and keep safeHttpClient from
safeHttpClient.js, so the scheme check and the fetch both reference the correct
symbols.
- Line 19: The reserved-range denylist in the SSRF prevention rules contains an
incorrect loopback CIDR example, so update the “Deny Reserved Ranges” entry to
use the proper loopback range instead of 127.0.0.0/0. Fix the list in the
js-ssrf-prevention guidance so the IPv4 loopback CIDR is accurate and the
examples remain safe and correct alongside the other reserved ranges.
- Around line 91-124: The scheme check in assertAllowedScheme currently allows
plain http: by default, which should be opt-in instead. Update
assertAllowedScheme so http: is rejected unless an explicit allowHttp flag is
passed, while still allowing https: and preserving the existing statusCode
behavior for disallowed schemes. Keep the change localized to the URL parsing
logic in assertAllowedScheme and its callers, including safeHttpClient if
needed.
In @.claude/rules/ssrf-prevention.md:
- Line 19: The SSRF deny-list example in the reserved ranges guidance uses an
incorrect loopback CIDR, which would match every IPv4 address instead of just
loopback. Update the reserved-range list in the SSRF prevention rule to use the
correct loopback block, and keep the rest of the deny-list examples in line with
the intended semantics for private, loopback, link-local, and metadata
destinations.
---
Nitpick comments:
In @.claude/rules/js-output-encoding-xss.md:
- Around line 27-29: Scope the CSP guidance in the JS output encoding doc so it
matches the portal’s current CDN-backed asset model rather than implying a
blanket script-src 'self' requirement. Update the CSP example in the affected
section to either explicitly say it applies only to self-hosted scripts or to
include the needed CDN origins/hashes already used by the asset-loading logic in
util.js, and make sure the wording stays consistent with the related guidance
elsewhere in the document.
In @.claude/rules/js-xxe-xml-processing.md:
- Around line 22-24: The XML parsing guidance currently treats fast-xml-parser
stopNodes as if it were a nesting-depth limit, which may be inaccurate. Reword
the Depth Ceiling section in js-xxe-xml-processing.md to reference only a real
parser depth-cap if the library docs confirm one; otherwise instruct
implementers to add a manual depth-tracking validation pass before parsing or
use a worker-thread timeout fallback. Ensure the guidance stays aligned with the
symbols and concepts in the existing file, especially the parser depth and
parse-timeout recommendations.
In @.claude/rules/xxe-xml-processing.md:
- Around line 85-89: The example for the XML parser settings hardcodes the
document byte ceiling instead of showing the config-backed value required by the
rule. Update the snippet around the maxDocBytes constant to reference the
configured limit used by the real call sites, keeping maxElemDepth and
parseTimeout as-is, so readers copy an example that matches the policy and
symbols like maxDocBytes remain clearly tied to configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65486bdd-7a40-4e75-a773-1529a8f54940
📒 Files selected for processing (9)
.claude/rules/authentication_authorization.md.claude/rules/file-access.md.claude/rules/js-authentication-authorization.md.claude/rules/js-file-access.md.claude/rules/js-output-encoding-xss.md.claude/rules/js-ssrf-prevention.md.claude/rules/js-xxe-xml-processing.md.claude/rules/ssrf-prevention.md.claude/rules/xxe-xml-processing.md
Comment on lines
+632
to
+643
| func safeRedirectTarget(raw string) (string, bool) { | ||
| if raw == "" { | ||
| return "/", true | ||
| } | ||
| u, err := url.Parse(raw) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| // Relative paths are safe by construction — no host to validate. | ||
| if u.Host == "" && !strings.HasPrefix(raw, "//") { | ||
| return u.String(), true | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".claude/rules/authentication_authorization.md"
wc -l "$file"
sed -n '620,660p' "$file"Repository: wso2/api-platform
Length of output: 1671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat <<'PY' > /tmp/redirect_probe.py
from urllib.parse import urlparse
tests = [
"",
"/",
"/foo",
"foo",
"//evil.com",
"https://portal.example.com/x",
"javascript:alert(1)",
"mailto:test@example.com",
"?next=/home",
]
for t in tests:
u = urlparse(t)
print(f"{t!r} -> scheme={u.scheme!r} netloc={u.netloc!r} path={u.path!r} params={u.params!r} query={u.query!r} fragment={u.fragment!r}")
PY
python3 /tmp/redirect_probe.py
cat <<'GO' > /tmp/redirect_probe.go
package main
import (
"fmt"
"net/url"
)
func main() {
tests := []string{"", "/", "/foo", "foo", "//evil.com", "https://portal.example.com/x", "javascript:alert(1)", "mailto:test@example.com", "?next=/home"}
for _, t := range tests {
u, err := url.Parse(t)
if err != nil {
fmt.Printf("%q -> err=%v\n", t, err)
continue
}
fmt.Printf("%q -> scheme=%q host=%q opaque=%q path=%q rawQuery=%q fragment=%q string=%q\n", t, u.Scheme, u.Host, u.Opaque, u.Path, u.RawQuery, u.Fragment, u.String())
}
}
GO
go run /tmp/redirect_probe.goRepository: wso2/api-platform
Length of output: 1903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '632,648p' .claude/rules/authentication_authorization.mdRepository: wso2/api-platform
Length of output: 624
Restrict safeRedirectTarget to path-only URLs. url.Parse treats values like javascript:alert(1) as host-less opaque URLs, so the current branch can still return a dangerous Location. Require an empty scheme and opaque component, plus a leading /, before accepting a redirect target.
Suggested fix
- if u.Host == "" && !strings.HasPrefix(raw, "//") {
+ if u.Scheme == "" && u.Host == "" && u.Opaque == "" && strings.HasPrefix(raw, "/") && !strings.HasPrefix(raw, "//") {
return u.String(), true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func safeRedirectTarget(raw string) (string, bool) { | |
| if raw == "" { | |
| return "/", true | |
| } | |
| u, err := url.Parse(raw) | |
| if err != nil { | |
| return "", false | |
| } | |
| // Relative paths are safe by construction — no host to validate. | |
| if u.Host == "" && !strings.HasPrefix(raw, "//") { | |
| return u.String(), true | |
| } | |
| func safeRedirectTarget(raw string) (string, bool) { | |
| if raw == "" { | |
| return "/", true | |
| } | |
| u, err := url.Parse(raw) | |
| if err != nil { | |
| return "", false | |
| } | |
| // Relative paths are safe by construction — no host to validate. | |
| if u.Scheme == "" && u.Host == "" && u.Opaque == "" && strings.HasPrefix(raw, "/") && !strings.HasPrefix(raw, "//") { | |
| return u.String(), true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/authentication_authorization.md around lines 632 - 643, The
safeRedirectTarget helper currently accepts host-less opaque URLs, which can
still produce unsafe redirect locations. Update safeRedirectTarget to only allow
path-only targets by requiring an empty scheme and opaque component in addition
to no host and no leading "//", and only return success when the parsed value
starts with "/". Keep the validation and return logic inside safeRedirectTarget
consistent so malformed or non-path URLs are rejected before being used as a
redirect target.
Comment on lines
+603
to
+614
| async function authMiddleware(req, res, next) { | ||
| try { | ||
| const { payload } = await jwtVerify(token, JWKS, { algorithms: ['RS256'] }); | ||
| const user = await User.findByPk(payload.sub); | ||
| if (!user || user.status === 'locked' || user.tokenVersion !== payload.tokenVersion) { | ||
| return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); | ||
| } | ||
| req.user = { id: user.id, organizationId: user.organizationId, scopes: payload.scopes ?? [] }; | ||
| next(); | ||
| } catch (err) { | ||
| return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define the bearer token before verifying it.
This sample calls jwtVerify(token, ...) without binding token first, so it is not copy-pastable as written. Add the Authorization-header extraction before the verification call.
Suggested fix
async function authMiddleware(req, res, next) {
try {
+ const token = (req.headers.authorization || '').replace(/^Bearer\s+/, '');
const { payload } = await jwtVerify(token, JWKS, { algorithms: ['RS256'] });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function authMiddleware(req, res, next) { | |
| try { | |
| const { payload } = await jwtVerify(token, JWKS, { algorithms: ['RS256'] }); | |
| const user = await User.findByPk(payload.sub); | |
| if (!user || user.status === 'locked' || user.tokenVersion !== payload.tokenVersion) { | |
| return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); | |
| } | |
| req.user = { id: user.id, organizationId: user.organizationId, scopes: payload.scopes ?? [] }; | |
| next(); | |
| } catch (err) { | |
| return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); | |
| } | |
| async function authMiddleware(req, res, next) { | |
| try { | |
| const token = (req.headers.authorization || '').replace(/^Bearer\s+/, ''); | |
| const { payload } = await jwtVerify(token, JWKS, { algorithms: ['RS256'] }); | |
| const user = await User.findByPk(payload.sub); | |
| if (!user || user.status === 'locked' || user.tokenVersion !== payload.tokenVersion) { | |
| return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); | |
| } | |
| req.user = { id: user.id, organizationId: user.organizationId, scopes: payload.scopes ?? [] }; | |
| next(); | |
| } catch (err) { | |
| return res.status(401).json({ error: 'unauthorized', message: 'Invalid or expired credentials.' }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/js-authentication-authorization.md around lines 603 - 614, The
authMiddleware example calls jwtVerify(token, ...) before token is defined, so
make the snippet copy-pastable by extracting the bearer token from
req.headers.authorization first and binding it to token before verification.
Update authMiddleware to parse the Authorization header, validate the Bearer
format, then pass the extracted token into jwtVerify while keeping the existing
User.findByPk and req.user assignment logic unchanged.
Comment on lines
+254
to
+262
| app.post('/apis/:id/icon', upload.single('icon'), async (req, res, next) => { | ||
| try { | ||
| const sniffedMime = await validateUploadContent(req.file.buffer); // Actual bytes, not the header | ||
| await ApiIcon.create({ apiId: req.params.id, data: req.file.buffer, mimeType: sniffedMime }); | ||
| res.status(201).send(); | ||
| } catch (err) { | ||
| next(err); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '240,270p' .claude/rules/js-file-access.md | cat -nRepository: wso2/api-platform
Length of output: 1957
Guard req.file before reading buffer. upload.single('icon') can still reach this handler without a file, so req.file.buffer will throw before validation runs. Return a generic 400/422 when req.file is missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/js-file-access.md around lines 254 - 262, The icon upload
handler reads req.file.buffer before checking whether a file was actually
uploaded, so the route can throw when upload.single('icon') reaches it without a
file. Update the /apis/:id/icon handler to guard req.file at the start of the
async callback before calling validateUploadContent or ApiIcon.create, and
return a generic 400/422 response when missing; use the existing route handler
and req.file reference to place the fix.
|
|
||
| ### 2. Block Private, Loopback, Link-Local, and Cloud Metadata Addresses | ||
|
|
||
| * **Deny Reserved Ranges:** Reject destinations resolving to `10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`, `127.0.0.0/0`, `169.254.0.0/16` (this includes `169.254.169.254`, the AWS/GCP/Azure metadata address), `::1`, and `fe80::/10`. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the loopback CIDR.
127.0.0.0/0 is not loopback; it matches the whole IPv4 space. That makes the denylist example incorrect.
🐛 Proposed fix
- '127.0.0.0/0', '169.254.0.0/16', '::1/128', 'fe80::/10',
+ '127.0.0.0/8', '169.254.0.0/16', '::1/128', 'fe80::/10',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/js-ssrf-prevention.md at line 19, The reserved-range denylist
in the SSRF prevention rules contains an incorrect loopback CIDR example, so
update the “Deny Reserved Ranges” entry to use the proper loopback range instead
of 127.0.0.0/0. Fix the list in the js-ssrf-prevention guidance so the IPv4
loopback CIDR is accurate and the examples remain safe and correct alongside the
other reserved ranges.
Comment on lines
+91
to
+124
| function guardedLookup(hostname, options, callback) { | ||
| dns.lookup(hostname, options, (err, address, family) => { | ||
| if (err) return callback(err); | ||
| if (isDenied(address)) { | ||
| return callback(new Error('Destination is not allowed')); | ||
| } | ||
| callback(null, address, family); | ||
| }); | ||
| } | ||
|
|
||
| function assertAllowedScheme(raw) { | ||
| const { protocol } = new URL(raw); | ||
| if (protocol !== 'https:' && protocol !== 'http:') { | ||
| throw Object.assign(new Error('URL scheme is not allowed'), { statusCode: 422 }); | ||
| } | ||
| } | ||
|
|
||
| module.exports = { guardedLookup, assertAllowedScheme, isDenied }; | ||
|
|
||
| // utils/safeHttpClient.js — GOOD: axios instance with dial-time IP validation, | ||
| // no automatic redirect-following, timeout, and a response-size ceiling. | ||
| const axios = require('axios'); | ||
| const http = require('node:http'); | ||
| const https = require('node:https'); | ||
| const { guardedLookup } = require('./ssrfGuard'); | ||
|
|
||
| const safeHttpClient = axios.create({ | ||
| timeout: 8000, | ||
| maxRedirects: 0, // Directive 2: no silent redirect-following | ||
| maxContentLength: 5 * 1024 * 1024, // 5 MiB response cap | ||
| maxBodyLength: 5 * 1024 * 1024, | ||
| httpAgent: new http.Agent({ lookup: guardedLookup }), | ||
| httpsAgent: new https.Agent({ lookup: guardedLookup }), | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the referenced file and surrounding lines
if [ -f .claude/rules/js-ssrf-prevention.md ]; then
wc -l .claude/rules/js-ssrf-prevention.md
sed -n '1,180p' .claude/rules/js-ssrf-prevention.md | nl -ba | sed -n '80,140p'
else
echo "File not found: .claude/rules/js-ssrf-prevention.md"
fi
# Search for the helper names and related SSRF guidance
rg -n "guardedLookup|assertAllowedScheme|allowHttp|isDenied|URL scheme is not allowed|Destination is not allowed|numeric target|ip-literal|http:" .claude/rules -SRepository: wso2/api-platform
Length of output: 1957
🌐 Web query:
Node.js http.Agent lookup option IP literal hostname dns.lookup called for numeric host
💡 Result:
In Node.js, the http.Agent lookup option allows developers to provide a custom function to resolve hostnames to IP addresses [1][2]. When a request is made, if a custom lookup function is provided, Node.js calls it regardless of whether the hostname is a domain name or an IP literal [1][3]. The dns.lookup() function, which serves as the default implementation, is designed to handle this gracefully [4][3]. Specifically, dns.lookup() contains internal logic that detects if the input is already a valid IP address [3]. If an IP literal is passed to it, dns.lookup() will not perform a network-based DNS query; instead, it returns the IP address immediately [3]. Therefore, if you provide a custom lookup function to http.Agent, your function is invoked for every request [1]. If your custom function calls the built-in dns.lookup() as its final step, it will correctly identify IP literals and avoid unnecessary DNS operations [3]. If you implement a custom lookup function from scratch, you should replicate this behavior (i.e., check for IP literals) if you wish to avoid performing actual resolution for numeric hosts [3].
Citations:
- 1: Feature request: allow a custom dns lookup function to be supplied nodejs/undici#421
- 2: https://nodejs.org/api/http.html
- 3: UDP/dgram performs unnecessary DNS lookups nodejs/node#39468
- 4: https://nodejs.org/docs/latest-v26.x/api/dns.html
Make http: opt-in here.
guardedLookup still runs for IP literals, so there isn’t a numeric-host bypass here. The remaining issue is that assertAllowedScheme accepts plain http: unconditionally; gate it behind an explicit allowHttp flag or reject it by default.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/js-ssrf-prevention.md around lines 91 - 124, The scheme check
in assertAllowedScheme currently allows plain http: by default, which should be
opt-in instead. Update assertAllowedScheme so http: is rejected unless an
explicit allowHttp flag is passed, while still allowing https: and preserving
the existing statusCode behavior for disallowed schemes. Keep the change
localized to the URL parsing logic in assertAllowedScheme and its callers,
including safeHttpClient if needed.
Comment on lines
+130
to
+146
| const { safeHttpClient, assertAllowedScheme } = require('../utils/safeHttpClient'); | ||
|
|
||
| app.post('/api/specs/import-from-url', async (req, res, next) => { | ||
| try { | ||
| assertAllowedScheme(req.body.specUrl); // Directive 1: scheme allowlist | ||
|
|
||
| const response = await safeHttpClient.get(req.body.specUrl); | ||
|
|
||
| // Directive 4: do not auto-dereference nested $ref/server URLs found in the | ||
| // fetched spec — return them to the client for the user to review/open. | ||
| res.json({ spec: response.data }); | ||
| } catch (err) { | ||
| logger.warn('Spec import rejected', { reason: err.message }); // Internal detail only | ||
| return res.status(422).json({ | ||
| error: 'invalid_request', | ||
| message: 'The provided URL could not be used.', // Generic — no resolved-IP detail | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Import the scheme validator from the correct module.
safeHttpClient.js only exports safeHttpClient; assertAllowedScheme is defined in ssrfGuard.js. As written, this example will fail when copied.
♻️ Proposed fix
-const { safeHttpClient, assertAllowedScheme } = require('../utils/safeHttpClient');
+const { safeHttpClient } = require('../utils/safeHttpClient');
+const { assertAllowedScheme } = require('../utils/ssrfGuard');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { safeHttpClient, assertAllowedScheme } = require('../utils/safeHttpClient'); | |
| app.post('/api/specs/import-from-url', async (req, res, next) => { | |
| try { | |
| assertAllowedScheme(req.body.specUrl); // Directive 1: scheme allowlist | |
| const response = await safeHttpClient.get(req.body.specUrl); | |
| // Directive 4: do not auto-dereference nested $ref/server URLs found in the | |
| // fetched spec — return them to the client for the user to review/open. | |
| res.json({ spec: response.data }); | |
| } catch (err) { | |
| logger.warn('Spec import rejected', { reason: err.message }); // Internal detail only | |
| return res.status(422).json({ | |
| error: 'invalid_request', | |
| message: 'The provided URL could not be used.', // Generic — no resolved-IP detail | |
| }); | |
| const { safeHttpClient } = require('../utils/safeHttpClient'); | |
| const { assertAllowedScheme } = require('../utils/ssrfGuard'); | |
| app.post('/api/specs/import-from-url', async (req, res, next) => { | |
| try { | |
| assertAllowedScheme(req.body.specUrl); // Directive 1: scheme allowlist | |
| const response = await safeHttpClient.get(req.body.specUrl); | |
| // Directive 4: do not auto-dereference nested $ref/server URLs found in the | |
| // fetched spec — return them to the client for the user to review/open. | |
| res.json({ spec: response.data }); | |
| } catch (err) { | |
| logger.warn('Spec import rejected', { reason: err.message }); // Internal detail only | |
| return res.status(422).json({ | |
| error: 'invalid_request', | |
| message: 'The provided URL could not be used.', // Generic — no resolved-IP detail | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/js-ssrf-prevention.md around lines 130 - 146, The import in
the spec URL handler is pulling assertAllowedScheme from the wrong module, so
the example will break when reused. Update the import in the
/api/specs/import-from-url handler to get assertAllowedScheme from ssrfGuard.js
and keep safeHttpClient from safeHttpClient.js, so the scheme check and the
fetch both reference the correct symbols.
|
|
||
| ### 2. Block Private, Loopback, Link-Local, and Metadata Addresses | ||
|
|
||
| * **Deny-List Reserved Ranges:** Reject destinations resolving to RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), loopback (`127.0.0.0/8`, `::1`), link-local (`169.254.0.0/16`, `fe80::/10`), and the `169.254.169.254` / `fd00:ec2::254` cloud metadata addresses — regardless of cloud provider. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the loopback CIDR.
127.0.0.0/0 matches all IPv4 addresses, so this example would break the denylist semantics. Use 127.0.0.0/8 here.
🐛 Proposed fix
- * **Deny-List Reserved Ranges:** Reject destinations resolving to RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), loopback (`127.0.0.0/0`, `::1`), link-local (`169.254.0.0/16`, `fe80::/10`), and the `169.254.169.254` / `fd00:ec2::254` cloud metadata addresses — regardless of cloud provider.
+ * **Deny-List Reserved Ranges:** Reject destinations resolving to RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), loopback (`127.0.0.0/8`, `::1`), link-local (`169.254.0.0/16`, `fe80::/10`), and the `169.254.169.254` / `fd00:ec2::254` cloud metadata addresses — regardless of cloud provider.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * **Deny-List Reserved Ranges:** Reject destinations resolving to RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), loopback (`127.0.0.0/8`, `::1`), link-local (`169.254.0.0/16`, `fe80::/10`), and the `169.254.169.254` / `fd00:ec2::254` cloud metadata addresses — regardless of cloud provider. | |
| * **Deny-List Reserved Ranges:** Reject destinations resolving to RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), loopback (`127.0.0.0/8`, `::1`), link-local (`169.254.0.0/16`, `fe80::/10`), and the `169.254.169.254` / `fd00:ec2::254` cloud metadata addresses — regardless of cloud provider. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/ssrf-prevention.md at line 19, The SSRF deny-list example in
the reserved ranges guidance uses an incorrect loopback CIDR, which would match
every IPv4 address instead of just loopback. Update the reserved-range list in
the SSRF prevention rule to use the correct loopback block, and keep the rest of
the deny-list examples in line with the intended semantics for private,
loopback, link-local, and metadata destinations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
include other claude rules