Skip to content

Commit a7a3048

Browse files
committed
feat(security): add input validation module
- Add validate.zig with sanitization for paths, hints, model names, and strings - Integrate validation in CLI argument parsing - Sanitize idea content before processing - Add InvalidHint and InvalidModelName error types with user-friendly messages Signed-off-by: leocavalcante <[email protected]>
1 parent 1951122 commit a7a3048

File tree

4 files changed

+416
-12
lines changed

4 files changed

+416
-12
lines changed

src/cli.zig

Lines changed: 77 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
const std = @import("std");
77
const config = @import("config.zig");
8+
const validate = @import("validate.zig");
89
const Allocator = std.mem.Allocator;
910

1011
/// CLI parsing errors
@@ -14,6 +15,8 @@ pub const ParseError = error{
1415
UnknownProvider,
1516
InvalidProjectDir,
1617
MissingOptionValue,
18+
InvalidHint,
19+
InvalidModelName,
1720
};
1821

1922
/// Result of CLI parsing
@@ -55,9 +58,13 @@ fn parseArgs(allocator: Allocator, args: anytype) ParseError!ParseResult {
5558
planning_model = models.planning;
5659
execution_model = models.execution;
5760
} else if (std.mem.eql(u8, arg, "-P") or std.mem.eql(u8, arg, "--planning-model")) {
58-
planning_model = args.next() orelse return ParseError.MissingOptionValue;
61+
const model = args.next() orelse return ParseError.MissingOptionValue;
62+
validate.validateModelName(model) catch return ParseError.InvalidModelName;
63+
planning_model = model;
5964
} else if (std.mem.eql(u8, arg, "-E") or std.mem.eql(u8, arg, "--execution-model")) {
60-
execution_model = args.next() orelse return ParseError.MissingOptionValue;
65+
const model = args.next() orelse return ParseError.MissingOptionValue;
66+
validate.validateModelName(model) catch return ParseError.InvalidModelName;
67+
execution_model = model;
6168
} else if (std.mem.eql(u8, arg, "-p") or std.mem.eql(u8, arg, "--project")) {
6269
project_dir = args.next() orelse return ParseError.MissingOptionValue;
6370
} else if (std.mem.startsWith(u8, arg, "-")) {
@@ -78,6 +85,11 @@ fn parseArgs(allocator: Allocator, args: anytype) ParseError!ParseResult {
7885
std.posix.getenv("OPENCODER_PROJECT_DIR") orelse
7986
".";
8087

88+
// Validate project directory path for security issues
89+
if (validate.sanitizePathInput(final_project_dir).len == 0) {
90+
return ParseError.InvalidProjectDir;
91+
}
92+
8193
// Validate project directory exists
8294
std.fs.cwd().access(final_project_dir, .{}) catch {
8395
return ParseError.InvalidProjectDir;
@@ -88,10 +100,17 @@ fn parseArgs(allocator: Allocator, args: anytype) ParseError!ParseResult {
88100
return ParseError.InvalidProjectDir;
89101
};
90102

103+
// Sanitize user hint if provided
104+
if (user_hint) |hint| {
105+
const sanitized = validate.sanitizeHintInput(hint, allocator) catch {
106+
return ParseError.InvalidHint;
107+
};
108+
cfg.user_hint = sanitized;
109+
}
110+
91111
cfg.planning_model = planning_model.?;
92112
cfg.execution_model = execution_model.?;
93113
cfg.project_dir = abs_path;
94-
cfg.user_hint = user_hint;
95114

96115
return .{ .run = cfg };
97116
}
@@ -248,6 +267,29 @@ pub fn formatError(err: ParseError, file: std.fs.File) void {
248267
\\For detailed help, run: opencoder --help
249268
\\
250269
,
270+
ParseError.InvalidHint =>
271+
\\Error: User hint is invalid or too long
272+
\\
273+
\\Hints should be:
274+
\\ - Less than 2048 characters
275+
\\ - Free of invalid control characters
276+
\\
277+
\\For detailed help, run: opencoder --help
278+
\\
279+
,
280+
ParseError.InvalidModelName =>
281+
\\Error: Invalid model name format
282+
\\
283+
\\Model names should contain only:
284+
\\ - Letters (a-z, A-Z)
285+
\\ - Numbers (0-9)
286+
\\ - Special chars: - _ . / : @
287+
\\
288+
\\Example: opencoder -P anthropic/claude-sonnet-4
289+
\\
290+
\\For detailed help, run: opencoder --help
291+
\\
292+
,
251293
};
252294
_ = file.write(msg) catch {};
253295
}
@@ -379,7 +421,10 @@ test "parse with project directory long form" {
379421
test "parse with user hint" {
380422
const args = &[_][]const u8{ "--provider", "github", "build a todo app" };
381423
const result = try parseFromSlice(std.testing.allocator, args);
382-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
424+
defer if (result == .run) {
425+
std.testing.allocator.free(result.run.project_dir);
426+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
427+
};
383428

384429
try std.testing.expect(result == .run);
385430
try std.testing.expect(result.run.user_hint != null);
@@ -389,7 +434,10 @@ test "parse with user hint" {
389434
test "parse with all options combined" {
390435
const args = &[_][]const u8{ "--provider", "anthropic", "-v", "-p", ".", "create a REST API" };
391436
const result = try parseFromSlice(std.testing.allocator, args);
392-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
437+
defer if (result == .run) {
438+
std.testing.allocator.free(result.run.project_dir);
439+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
440+
};
393441

394442
try std.testing.expect(result == .run);
395443
try std.testing.expectEqual(true, result.run.verbose);
@@ -401,7 +449,10 @@ test "parse with all options combined" {
401449
test "parse with mixed explicit models and provider (explicit wins)" {
402450
const args = &[_][]const u8{ "--provider", "github", "-P", "custom/planning", "-E", "custom/execution" };
403451
const result = try parseFromSlice(std.testing.allocator, args);
404-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
452+
defer if (result == .run) {
453+
std.testing.allocator.free(result.run.project_dir);
454+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
455+
};
405456

406457
try std.testing.expect(result == .run);
407458
// Explicit models should override provider preset
@@ -412,7 +463,10 @@ test "parse with mixed explicit models and provider (explicit wins)" {
412463
test "parse with options in different order" {
413464
const args = &[_][]const u8{ "-v", "build something", "--provider", "github", "-p", "." };
414465
const result = try parseFromSlice(std.testing.allocator, args);
415-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
466+
defer if (result == .run) {
467+
std.testing.allocator.free(result.run.project_dir);
468+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
469+
};
416470

417471
try std.testing.expect(result == .run);
418472
try std.testing.expectEqual(true, result.run.verbose);
@@ -529,7 +583,10 @@ test "parse edge case: multiple verbose flags" {
529583
test "parse edge case: last user hint wins" {
530584
const args = &[_][]const u8{ "--provider", "github", "first hint", "second hint" };
531585
const result = try parseFromSlice(std.testing.allocator, args);
532-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
586+
defer if (result == .run) {
587+
std.testing.allocator.free(result.run.project_dir);
588+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
589+
};
533590

534591
try std.testing.expect(result == .run);
535592
// Only the last positional argument is used as hint
@@ -539,7 +596,10 @@ test "parse edge case: last user hint wins" {
539596
test "parse edge case: model names with special characters" {
540597
const args = &[_][]const u8{ "-P", "provider/model-v1.2.3", "-E", "provider/model_beta" };
541598
const result = try parseFromSlice(std.testing.allocator, args);
542-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
599+
defer if (result == .run) {
600+
std.testing.allocator.free(result.run.project_dir);
601+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
602+
};
543603

544604
try std.testing.expect(result == .run);
545605
try std.testing.expectEqualStrings("provider/model-v1.2.3", result.run.planning_model);
@@ -549,7 +609,10 @@ test "parse edge case: model names with special characters" {
549609
test "parse edge case: user hint with spaces preserved" {
550610
const args = &[_][]const u8{ "--provider", "github", "build a complex web application" };
551611
const result = try parseFromSlice(std.testing.allocator, args);
552-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
612+
defer if (result == .run) {
613+
std.testing.allocator.free(result.run.project_dir);
614+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
615+
};
553616

554617
try std.testing.expect(result == .run);
555618
try std.testing.expectEqualStrings("build a complex web application", result.run.user_hint.?);
@@ -558,7 +621,10 @@ test "parse edge case: user hint with spaces preserved" {
558621
test "parse edge case: no user hint results in null" {
559622
const args = &[_][]const u8{ "--provider", "github" };
560623
const result = try parseFromSlice(std.testing.allocator, args);
561-
defer if (result == .run) std.testing.allocator.free(result.run.project_dir);
624+
defer if (result == .run) {
625+
std.testing.allocator.free(result.run.project_dir);
626+
if (result.run.user_hint) |hint| std.testing.allocator.free(hint);
627+
};
562628

563629
try std.testing.expect(result == .run);
564630
try std.testing.expectEqual(@as(?[]const u8, null), result.run.user_hint);

src/ideas.zig

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const fs = std.fs;
99
const Allocator = std.mem.Allocator;
1010

1111
const fsutil = @import("fs.zig");
12+
const validate = @import("validate.zig");
1213

1314
/// An idea loaded from the ideas directory
1415
pub const Idea = struct {
@@ -89,13 +90,22 @@ pub fn loadAllIdeas(ideas_dir: []const u8, allocator: Allocator, max_size: usize
8990
errdefer allocator.free(full_path);
9091

9192
// Try to read the file
92-
const content = fsutil.readFile(full_path, allocator, max_size) catch {
93+
const raw_content = fsutil.readFile(full_path, allocator, max_size) catch {
9394
// Skip unreadable files, but delete them
9495
allocator.free(full_path);
9596
removeIdea(ideas_dir, entry.name) catch {};
9697
continue;
9798
};
9899

100+
// Sanitize idea content
101+
const content = validate.sanitizeStringInput(raw_content, 8192, allocator) catch {
102+
allocator.free(raw_content);
103+
allocator.free(full_path);
104+
removeIdea(ideas_dir, entry.name) catch {};
105+
continue;
106+
};
107+
allocator.free(raw_content);
108+
99109
// Skip empty files, delete them
100110
const trimmed = std.mem.trim(u8, content, " \t\n\r");
101111
if (trimmed.len == 0) {

src/main.zig

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,4 +158,5 @@ test {
158158
_ = @import("executor.zig");
159159
_ = @import("evaluator.zig");
160160
_ = @import("loop.zig");
161+
_ = @import("validate.zig");
161162
}

0 commit comments

Comments
 (0)