Skip to content

Commit a312640

Browse files
rohitg00rohitg00
authored
fix: security hardening and code quality fixes
* Security hardening and code quality fixes - Replace execSync with execFileSync to prevent shell injection (providers, executor, fetcher, publish) - Add security headers: X-Content-Type-Options, X-Frame-Options, Content-Security-Policy, Referrer-Policy - Add input validation for save route name field to prevent path traversal - Fix rate limiter IP extraction (x-forwarded-for + x-real-ip fallback) - Default server bind to 127.0.0.1 instead of 0.0.0.0 - CORS fallback to * (configurable via options.corsOrigin) - Use execFileSync in publish submit (avoid shell injection when opening browser) - TLS-aware health checks and HTTPS support in RemoteTransport - Conditional rejectUnauthorized for TLS (self-signed configurable) - Add **/.env.local patterns to .gitignore - Formatting: double quotes, trailing commas (Prettier) * fix: address CodeRabbit and Devin review findings from PR #74 - TLS: default allowSelfSigned=true to preserve self-signed cert workflow; enforce rejectUnauthorized=true when trustedCAs are supplied in client context - Signature verification: verifySecureMessage now cryptographically verifies signatures using PeerIdentity.verifyHex instead of only checking fingerprints - WebSocket: fix timeout handle leak in performClientHandshake; add error logging to empty catch blocks; add per-socket error handler; pre-compute signature once in broadcast(); add send error callbacks; reject startup if TLS configured but certInfo missing - Command parsing: replace broken regex tokenizer with shared splitCommand() utility that correctly handles --flag="value with spaces" - Rate limiter: prefer x-real-ip, fall back to rightmost x-forwarded-for entry - Save route: scope IPv6 prefix checks to actual IPv6 addresses; stop leaking internal error messages to clients - Publish: extract shared parseSkillFrontmatter(); fix getRepoInfo to use async execFileSync; fix path containment checks with separator - Remote transport: add rejectUnauthorized option; bracket IPv6 in URLs; fix host:port splitting for IPv6 with lastIndexOf - Health checks: add tls/tlsAllowSelfSigned to Host type; pass https options - Fetcher: validate owner/repo; derive indexDir from INDEX_PATH - .gitignore: add recursive **/.env pattern, remove redundant entries * fix(executor): restore shell feature support in shellExecutor The previous security hardening replaced execSync with execFileSync, which broke commands using pipes, redirects, and subshells. Now detects shell metacharacters and falls back to sh -c for those commands while keeping execFileSync for simple commands.
1 parent 39b9453 commit a312640

File tree

22 files changed

+1430
-900
lines changed

22 files changed

+1430
-900
lines changed

‎.gitignore‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,9 @@ npm-debug.log*
2424

2525
# Environment
2626
.env
27-
.env.local
28-
.env.*.local
27+
**/.env
28+
**/.env.local
29+
**/.env.*.local
2930

3031
# Package manager
3132
yarn.lock

‎packages/api/src/middleware/rate-limit.ts‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Context, Next } from 'hono';
1+
import type { Context, Next } from "hono";
22

33
interface RateLimitEntry {
44
count: number;
@@ -18,8 +18,12 @@ export function rateLimiter(maxRequests = 60, windowMs = 60_000) {
1818
}, windowMs).unref();
1919

2020
return async (c: Context, next: Next): Promise<Response | void> => {
21-
const forwardedFor = c.req.header('x-forwarded-for');
22-
const ip = (forwardedFor?.split(',')[0]?.trim()) || c.req.header('x-real-ip') || 'unknown';
21+
const xRealIp = c.req.header("x-real-ip");
22+
const forwarded = c.req.header("x-forwarded-for");
23+
const forwardedIp = forwarded
24+
? forwarded.split(",").at(-1)?.trim()
25+
: undefined;
26+
const ip = xRealIp || forwardedIp || "unknown";
2327
const now = Date.now();
2428

2529
let entry = windows.get(ip);
@@ -30,12 +34,21 @@ export function rateLimiter(maxRequests = 60, windowMs = 60_000) {
3034

3135
entry.count++;
3236

33-
c.header('X-RateLimit-Limit', String(maxRequests));
34-
c.header('X-RateLimit-Remaining', String(Math.max(0, maxRequests - entry.count)));
35-
c.header('X-RateLimit-Reset', String(Math.ceil(entry.resetAt / 1000)));
37+
c.header("X-RateLimit-Limit", String(maxRequests));
38+
c.header(
39+
"X-RateLimit-Remaining",
40+
String(Math.max(0, maxRequests - entry.count)),
41+
);
42+
c.header("X-RateLimit-Reset", String(Math.ceil(entry.resetAt / 1000)));
3643

3744
if (entry.count > maxRequests) {
38-
return c.json({ error: 'Too many requests', retryAfter: Math.ceil((entry.resetAt - now) / 1000) }, 429);
45+
return c.json(
46+
{
47+
error: "Too many requests",
48+
retryAfter: Math.ceil((entry.resetAt - now) / 1000),
49+
},
50+
429,
51+
);
3952
}
4053

4154
await next();

‎packages/api/src/routes/save.ts‎

Lines changed: 50 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import { Hono } from 'hono';
2-
import { ContentExtractor, SkillGenerator, AutoTagger } from '@skillkit/core';
1+
import { Hono } from "hono";
2+
import { ContentExtractor, SkillGenerator, AutoTagger } from "@skillkit/core";
33

44
interface SaveRequest {
55
url?: string;
@@ -10,25 +10,40 @@ interface SaveRequest {
1010

1111
const MAX_TEXT_LENGTH = 500_000;
1212

13-
const BLOCKED_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]', '::1', '0.0.0.0']);
13+
const BLOCKED_HOSTS = new Set([
14+
"localhost",
15+
"127.0.0.1",
16+
"[::1]",
17+
"::1",
18+
"0.0.0.0",
19+
]);
1420

1521
function isAllowedUrl(url: string): boolean {
1622
try {
1723
const parsed = new URL(url);
18-
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return false;
24+
if (parsed.protocol !== "http:" && parsed.protocol !== "https:")
25+
return false;
1926

2027
const hostname = parsed.hostname.toLowerCase();
21-
const bare = hostname.replace(/^\[|\]$/g, '');
28+
const bare = hostname.replace(/^\[|\]$/g, "");
2229

2330
if (BLOCKED_HOSTS.has(hostname) || BLOCKED_HOSTS.has(bare)) return false;
24-
if (bare.startsWith('::ffff:')) return isAllowedUrl(`http://${bare.slice(7)}`);
31+
if (bare.startsWith("::ffff:"))
32+
return isAllowedUrl(`http://${bare.slice(7)}`);
2533
if (/^127\./.test(bare) || /^0\./.test(bare)) return false;
26-
if (bare.startsWith('10.') || bare.startsWith('192.168.')) return false;
34+
if (bare.startsWith("10.") || bare.startsWith("192.168.")) return false;
2735
if (/^172\.(1[6-9]|2\d|3[01])\./.test(bare)) return false;
28-
if (bare.startsWith('169.254.')) return false;
29-
if (bare.startsWith('fe80:') || bare.startsWith('fc') || bare.startsWith('fd')) return false;
36+
if (bare.startsWith("169.254.")) return false;
37+
if (bare.includes(":")) {
38+
if (
39+
bare.startsWith("fe80:") ||
40+
bare.startsWith("fc") ||
41+
bare.startsWith("fd")
42+
)
43+
return false;
44+
if (bare.startsWith("ff")) return false;
45+
}
3046
if (/^(22[4-9]|23\d|24\d|25[0-5])\./.test(bare)) return false;
31-
if (bare.startsWith('ff')) return false;
3247
return true;
3348
} catch {
3449
return false;
@@ -41,24 +56,39 @@ export function saveRoutes() {
4156
const generator = new SkillGenerator();
4257
const tagger = new AutoTagger();
4358

44-
app.post('/save', async (c) => {
59+
app.post("/save", async (c) => {
4560
let body: SaveRequest;
4661
try {
4762
body = await c.req.json();
4863
} catch {
49-
return c.json({ error: 'Invalid JSON body' }, 400);
64+
return c.json({ error: "Invalid JSON body" }, 400);
5065
}
5166

5267
if (!body.url && !body.text) {
5368
return c.json({ error: 'Either "url" or "text" is required' }, 400);
5469
}
5570

5671
if (body.url && !isAllowedUrl(body.url)) {
57-
return c.json({ error: 'URL must be a public HTTP(S) address' }, 400);
72+
return c.json({ error: "URL must be a public HTTP(S) address" }, 400);
73+
}
74+
75+
if (body.name && !/^[a-zA-Z0-9][a-zA-Z0-9._-]*$/.test(body.name)) {
76+
return c.json(
77+
{
78+
error:
79+
"Name must be alphanumeric (hyphens, underscores, dots allowed)",
80+
},
81+
400,
82+
);
5883
}
5984

6085
if (body.text && body.text.length > MAX_TEXT_LENGTH) {
61-
return c.json({ error: `Text exceeds maximum length of ${MAX_TEXT_LENGTH} characters` }, 400);
86+
return c.json(
87+
{
88+
error: `Text exceeds maximum length of ${MAX_TEXT_LENGTH} characters`,
89+
},
90+
400,
91+
);
6292
}
6393

6494
try {
@@ -78,16 +108,17 @@ export function saveRoutes() {
78108
tags: tagger.detectTags(content),
79109
});
80110
} catch (err) {
81-
const message = err instanceof Error ? err.message : 'Unknown error';
111+
console.error("Save extraction failed:", err);
82112
const isTimeout =
83-
(err instanceof DOMException && err.name === 'TimeoutError') ||
84-
(err instanceof Error && (err.name === 'TimeoutError' || err.name === 'AbortError'));
113+
(err instanceof DOMException && err.name === "TimeoutError") ||
114+
(err instanceof Error &&
115+
(err.name === "TimeoutError" || err.name === "AbortError"));
85116

86117
if (isTimeout) {
87-
return c.json({ error: `Fetch timeout: ${message}` }, 504);
118+
return c.json({ error: "Fetch timed out" }, 504);
88119
}
89120

90-
return c.json({ error: `Extraction failed: ${message}` }, 422);
121+
return c.json({ error: "Extraction failed" }, 422);
91122
}
92123
});
93124

‎packages/api/src/server.ts‎

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
1-
import { Hono } from 'hono';
2-
import { cors } from 'hono/cors';
3-
import { MemoryCache } from '@skillkit/core';
4-
import { rateLimiter } from './middleware/rate-limit.js';
5-
import { healthRoutes } from './routes/health.js';
6-
import { searchRoutes } from './routes/search.js';
7-
import { skillRoutes } from './routes/skills.js';
8-
import { trendingRoutes } from './routes/trending.js';
9-
import { categoryRoutes } from './routes/categories.js';
10-
import { docsRoutes } from './routes/docs.js';
11-
import { saveRoutes } from './routes/save.js';
12-
import type { ApiSkill, SearchResponse } from './types.js';
1+
import { Hono } from "hono";
2+
import { cors } from "hono/cors";
3+
import { MemoryCache } from "@skillkit/core";
4+
import { rateLimiter } from "./middleware/rate-limit.js";
5+
import { healthRoutes } from "./routes/health.js";
6+
import { searchRoutes } from "./routes/search.js";
7+
import { skillRoutes } from "./routes/skills.js";
8+
import { trendingRoutes } from "./routes/trending.js";
9+
import { categoryRoutes } from "./routes/categories.js";
10+
import { docsRoutes } from "./routes/docs.js";
11+
import { saveRoutes } from "./routes/save.js";
12+
import type { ApiSkill, SearchResponse } from "./types.js";
1313

1414
export interface ServerOptions {
1515
port?: number;
@@ -29,26 +29,36 @@ export function createApp(options: ServerOptions = {}) {
2929

3030
const app = new Hono();
3131

32-
app.use('*', cors({ origin: options.corsOrigin || '*' }));
33-
app.use('*', rateLimiter(options.rateLimitMax ?? 60));
32+
app.use("*", cors({ origin: options.corsOrigin || "*" }));
33+
app.use("*", async (c, next) => {
34+
await next();
35+
c.header("X-Content-Type-Options", "nosniff");
36+
c.header("X-Frame-Options", "DENY");
37+
c.header(
38+
"Content-Security-Policy",
39+
"default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'",
40+
);
41+
c.header("Referrer-Policy", "strict-origin-when-cross-origin");
42+
});
43+
app.use("*", rateLimiter(options.rateLimitMax ?? 60));
3444

35-
app.route('/', healthRoutes(skills.length, cache));
36-
app.route('/', searchRoutes(skills, cache));
37-
app.route('/', skillRoutes(skills));
38-
app.route('/', trendingRoutes(skills));
39-
app.route('/', categoryRoutes(skills));
40-
app.route('/', docsRoutes());
41-
app.route('/', saveRoutes());
45+
app.route("/", healthRoutes(skills.length, cache));
46+
app.route("/", searchRoutes(skills, cache));
47+
app.route("/", skillRoutes(skills));
48+
app.route("/", trendingRoutes(skills));
49+
app.route("/", categoryRoutes(skills));
50+
app.route("/", docsRoutes());
51+
app.route("/", saveRoutes());
4252

4353
return { app, cache };
4454
}
4555

4656
export async function startServer(options: ServerOptions = {}) {
4757
const port = options.port ?? 3737;
48-
const host = options.host ?? '0.0.0.0';
58+
const host = options.host ?? "127.0.0.1";
4959
const { app, cache } = createApp(options);
5060

51-
const { serve } = await import('@hono/node-server');
61+
const { serve } = await import("@hono/node-server");
5262

5363
const server = serve({ fetch: app.fetch, port, hostname: host }, () => {
5464
console.log(`SkillKit API server running at http://${host}:${port}`);

0 commit comments

Comments
 (0)