Skip to content

Security hardening and code quality fixes #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rohitg00 merged 3 commits into from
Feb 22, 2026
Merged

Security hardening and code quality fixes #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9db918
Security hardening and code quality fixes
rohitg00 Feb 22, 2026
067d2ad
fix: address CodeRabbit and Devin review findings from PR #74
rohitg00 Feb 22, 2026
67f0410
fix(executor): restore shell feature support in shellExecutor
rohitg00 Feb 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ npm-debug.log*

# Environment
.env
.env.local
.env.*.local
**/.env
**/.env.local
**/.env.*.local

# Package manager
yarn.lock
Expand Down
27 changes: 20 additions & 7 deletions packages/api/src/middleware/rate-limit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Context, Next } from 'hono';
import type { Context, Next } from "hono";

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

return async (c: Context, next: Next): Promise<Response | void> => {
const forwardedFor = c.req.header('x-forwarded-for');
const ip = (forwardedFor?.split(',')[0]?.trim()) || c.req.header('x-real-ip') || 'unknown';
const xRealIp = c.req.header("x-real-ip");
const forwarded = c.req.header("x-forwarded-for");
const forwardedIp = forwarded
? forwarded.split(",").at(-1)?.trim()
: undefined;
Comment on lines +23 to +25
Copy link

@devin-ai-integration devin-ai-integration bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Rate limiter extracts proxy IP instead of client IP from X-Forwarded-For

The rate limiter uses forwarded.split(",").at(-1) to extract the IP from the X-Forwarded-For header, but this retrieves the last entry (the nearest proxy), not the first entry (the original client IP).

Root Cause and Impact

The X-Forwarded-For header format is client, proxy1, proxy2. The old code correctly used split(',')[0] to get the client's IP. The new code at packages/api/src/middleware/rate-limit.ts:24 uses .at(-1), which returns the last proxy in the chain.

This means:

  • All clients behind the same proxy share a single rate-limit bucket (the proxy's IP), making rate limiting ineffective per-client.
  • A single abusive client won't be throttled because their requests are bucketed under the proxy IP along with legitimate traffic.
  • Conversely, a busy proxy could get all its clients rate-limited even if each individual client is well-behaved.

The old code:

const ip = (forwardedFor?.split(',')[0]?.trim()) || c.req.header('x-real-ip') || 'unknown';

The new code:

const forwardedIp = forwarded ? forwarded.split(",").at(-1)?.trim() : undefined;
Suggested change
const forwardedIp = forwarded
? forwarded.split(",").at(-1)?.trim()
: undefined;
const forwardedIp = forwarded
? forwarded.split(",").at(0)?.trim()
: undefined;
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const ip = xRealIp || forwardedIp || "unknown";
const now = Date.now();

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

entry.count++;

c.header('X-RateLimit-Limit', String(maxRequests));
c.header('X-RateLimit-Remaining', String(Math.max(0, maxRequests - entry.count)));
c.header('X-RateLimit-Reset', String(Math.ceil(entry.resetAt / 1000)));
c.header("X-RateLimit-Limit", String(maxRequests));
c.header(
"X-RateLimit-Remaining",
String(Math.max(0, maxRequests - entry.count)),
);
c.header("X-RateLimit-Reset", String(Math.ceil(entry.resetAt / 1000)));

if (entry.count > maxRequests) {
return c.json({ error: 'Too many requests', retryAfter: Math.ceil((entry.resetAt - now) / 1000) }, 429);
return c.json(
{
error: "Too many requests",
retryAfter: Math.ceil((entry.resetAt - now) / 1000),
},
429,
);
}

await next();
Expand Down
69 changes: 50 additions & 19 deletions packages/api/src/routes/save.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Hono } from 'hono';
import { ContentExtractor, SkillGenerator, AutoTagger } from '@skillkit/core';
import { Hono } from "hono";
import { ContentExtractor, SkillGenerator, AutoTagger } from "@skillkit/core";

interface SaveRequest {
url?: string;
Expand All @@ -10,25 +10,40 @@ interface SaveRequest {

const MAX_TEXT_LENGTH = 500_000;

const BLOCKED_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]', '::1', '0.0.0.0']);
const BLOCKED_HOSTS = new Set([
"localhost",
"127.0.0.1",
"[::1]",
"::1",
"0.0.0.0",
]);

function isAllowedUrl(url: string): boolean {
try {
const parsed = new URL(url);
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return false;
if (parsed.protocol !== "http:" && parsed.protocol !== "https:")
return false;

const hostname = parsed.hostname.toLowerCase();
const bare = hostname.replace(/^\[|\]$/g, '');
const bare = hostname.replace(/^\[|\]$/g, "");

if (BLOCKED_HOSTS.has(hostname) || BLOCKED_HOSTS.has(bare)) return false;
if (bare.startsWith('::ffff:')) return isAllowedUrl(`http://${bare.slice(7)}`);
if (bare.startsWith("::ffff:"))
return isAllowedUrl(`http://${bare.slice(7)}`);
if (/^127\./.test(bare) || /^0\./.test(bare)) return false;
if (bare.startsWith('10.') || bare.startsWith('192.168.')) return false;
if (bare.startsWith("10.") || bare.startsWith("192.168.")) return false;
if (/^172\.(1[6-9]|2\d|3[01])\./.test(bare)) return false;
if (bare.startsWith('169.254.')) return false;
if (bare.startsWith('fe80:') || bare.startsWith('fc') || bare.startsWith('fd')) return false;
if (bare.startsWith("169.254.")) return false;
if (bare.includes(":")) {
if (
bare.startsWith("fe80:") ||
bare.startsWith("fc") ||
bare.startsWith("fd")
)
return false;
if (bare.startsWith("ff")) return false;
}
if (/^(22[4-9]|23\d|24\d|25[0-5])\./.test(bare)) return false;
if (bare.startsWith('ff')) return false;
return true;
} catch {
return false;
Expand All @@ -41,24 +56,39 @@ export function saveRoutes() {
const generator = new SkillGenerator();
const tagger = new AutoTagger();

app.post('/save', async (c) => {
app.post("/save", async (c) => {
let body: SaveRequest;
try {
body = await c.req.json();
} catch {
return c.json({ error: 'Invalid JSON body' }, 400);
return c.json({ error: "Invalid JSON body" }, 400);
}

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

if (body.url && !isAllowedUrl(body.url)) {
return c.json({ error: 'URL must be a public HTTP(S) address' }, 400);
return c.json({ error: "URL must be a public HTTP(S) address" }, 400);
}

if (body.name && !/^[a-zA-Z0-9][a-zA-Z0-9._-]*$/.test(body.name)) {
return c.json(
{
error:
"Name must be alphanumeric (hyphens, underscores, dots allowed)",
},
400,
);
}

if (body.text && body.text.length > MAX_TEXT_LENGTH) {
return c.json({ error: `Text exceeds maximum length of ${MAX_TEXT_LENGTH} characters` }, 400);
return c.json(
{
error: `Text exceeds maximum length of ${MAX_TEXT_LENGTH} characters`,
},
400,
);
}

try {
Expand All @@ -78,16 +108,17 @@ export function saveRoutes() {
tags: tagger.detectTags(content),
});
} catch (err) {
const message = err instanceof Error ? err.message : 'Unknown error';
console.error("Save extraction failed:", err);
const isTimeout =
(err instanceof DOMException && err.name === 'TimeoutError') ||
(err instanceof Error && (err.name === 'TimeoutError' || err.name === 'AbortError'));
(err instanceof DOMException && err.name === "TimeoutError") ||
(err instanceof Error &&
(err.name === "TimeoutError" || err.name === "AbortError"));

if (isTimeout) {
return c.json({ error: `Fetch timeout: ${message}` }, 504);
return c.json({ error: "Fetch timed out" }, 504);
}

return c.json({ error: `Extraction failed: ${message}` }, 422);
return c.json({ error: "Extraction failed" }, 422);
}
});

Expand Down
56 changes: 33 additions & 23 deletions packages/api/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import { Hono } from 'hono';
import { cors } from 'hono/cors';
import { MemoryCache } from '@skillkit/core';
import { rateLimiter } from './middleware/rate-limit.js';
import { healthRoutes } from './routes/health.js';
import { searchRoutes } from './routes/search.js';
import { skillRoutes } from './routes/skills.js';
import { trendingRoutes } from './routes/trending.js';
import { categoryRoutes } from './routes/categories.js';
import { docsRoutes } from './routes/docs.js';
import { saveRoutes } from './routes/save.js';
import type { ApiSkill, SearchResponse } from './types.js';
import { Hono } from "hono";
import { cors } from "hono/cors";
import { MemoryCache } from "@skillkit/core";
import { rateLimiter } from "./middleware/rate-limit.js";
import { healthRoutes } from "./routes/health.js";
import { searchRoutes } from "./routes/search.js";
import { skillRoutes } from "./routes/skills.js";
import { trendingRoutes } from "./routes/trending.js";
import { categoryRoutes } from "./routes/categories.js";
import { docsRoutes } from "./routes/docs.js";
import { saveRoutes } from "./routes/save.js";
import type { ApiSkill, SearchResponse } from "./types.js";

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

const app = new Hono();

app.use('*', cors({ origin: options.corsOrigin || '*' }));
app.use('*', rateLimiter(options.rateLimitMax ?? 60));
app.use("*", cors({ origin: options.corsOrigin || "*" }));
app.use("*", async (c, next) => {
await next();
c.header("X-Content-Type-Options", "nosniff");
c.header("X-Frame-Options", "DENY");
c.header(
"Content-Security-Policy",
"default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'",
);
c.header("Referrer-Policy", "strict-origin-when-cross-origin");
});
app.use("*", rateLimiter(options.rateLimitMax ?? 60));

app.route('/', healthRoutes(skills.length, cache));
app.route('/', searchRoutes(skills, cache));
app.route('/', skillRoutes(skills));
app.route('/', trendingRoutes(skills));
app.route('/', categoryRoutes(skills));
app.route('/', docsRoutes());
app.route('/', saveRoutes());
app.route("/", healthRoutes(skills.length, cache));
app.route("/", searchRoutes(skills, cache));
app.route("/", skillRoutes(skills));
app.route("/", trendingRoutes(skills));
app.route("/", categoryRoutes(skills));
app.route("/", docsRoutes());
app.route("/", saveRoutes());

return { app, cache };
}

export async function startServer(options: ServerOptions = {}) {
const port = options.port ?? 3737;
const host = options.host ?? '0.0.0.0';
const host = options.host ?? "127.0.0.1";
const { app, cache } = createApp(options);

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

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