-
Notifications
You must be signed in to change notification settings - Fork 59
feat: implement HTTP allowed hosts/origins checking #49
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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,11 @@ import ( | |
"fmt" | ||
"log/slog" | ||
"net/http" | ||
"net/url" | ||
"os" | ||
"sort" | ||
"strings" | ||
"unicode" | ||
|
||
"github.com/spf13/cobra" | ||
"github.com/spf13/viper" | ||
|
@@ -58,6 +60,79 @@ func parseAgentType(firstArg string, agentTypeVar string) (AgentType, error) { | |
return AgentTypeCustom, nil | ||
} | ||
|
||
// Validate allowed hosts don't contain whitespace, commas, schemes, or ports. | ||
// Viper/Cobra use different separators (space for env vars, comma for flags), | ||
// so these characters likely indicate user error. | ||
func validateAllowedHosts(input []string) error { | ||
if len(input) == 0 { | ||
return fmt.Errorf("the list must not be empty") | ||
} | ||
// First pass: whitespace & comma checks (surface these errors first) | ||
for _, item := range input { | ||
for _, r := range item { | ||
if unicode.IsSpace(r) { | ||
return fmt.Errorf("'%s' contains whitespace characters, which are not allowed", item) | ||
} | ||
} | ||
if strings.Contains(item, ",") { | ||
return fmt.Errorf("'%s' contains comma characters, which are not allowed", item) | ||
} | ||
} | ||
// Second pass: scheme check | ||
for _, item := range input { | ||
if strings.Contains(item, "http://") || strings.Contains(item, "https://") { | ||
return fmt.Errorf("'%s' must not include http:// or https://", item) | ||
} | ||
} | ||
// Third pass: port check (but allow IPv6 literals without ports) | ||
for _, item := range input { | ||
trimmed := strings.TrimSpace(item) | ||
colonCount := strings.Count(trimmed, ":") | ||
// If bracketed, rely on url.Parse to detect a port in "]:<port>" form. | ||
if strings.HasPrefix(trimmed, "[") { | ||
if u, err := url.Parse("http://" + trimmed); err == nil { | ||
if u.Port() != "" { | ||
return fmt.Errorf("'%s' must not include a port", item) | ||
} | ||
} | ||
continue | ||
} | ||
// Unbracketed IPv6: multiple colons and no brackets; treat as valid (no ports allowed here) | ||
if colonCount >= 2 { | ||
continue | ||
} | ||
// IPv4 or hostname: if URL parsing finds a port or there's a single colon, it's invalid | ||
if u, err := url.Parse("http://" + trimmed); err == nil { | ||
if u.Port() != "" { | ||
return fmt.Errorf("'%s' must not include a port", item) | ||
} | ||
} | ||
if colonCount == 1 { | ||
return fmt.Errorf("'%s' must not include a port", item) | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
// Validate allowed origins don't contain whitespace or commas. | ||
// Origins must include a scheme, validated later by the HTTP layer. | ||
func validateAllowedOrigins(input []string) error { | ||
if len(input) == 0 { | ||
return fmt.Errorf("the list must not be empty") | ||
} | ||
for _, item := range input { | ||
for _, r := range item { | ||
if unicode.IsSpace(r) { | ||
return fmt.Errorf("'%s' contains whitespace characters, which are not allowed", item) | ||
} | ||
} | ||
if strings.Contains(item, ",") { | ||
return fmt.Errorf("'%s' contains comma characters, which are not allowed", item) | ||
} | ||
} | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we validate AllowedOrigins in the HTTP layer but AllowedHosts here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We validate both in both the HTTP layer and the cmd layer. Originally because I wanted to keep cmd-specific validation, like whitespace and comma detection, to the cmd layer. But it did lead to a bunch of duplication, which I'm not happy about. I'll refactor to keep validation in a single place. |
||
} | ||
|
||
func runServer(ctx context.Context, logger *slog.Logger, argsToPass []string) error { | ||
agent := argsToPass[0] | ||
agentTypeValue := viper.GetString(FlagType) | ||
|
@@ -95,12 +170,17 @@ func runServer(ctx context.Context, logger *slog.Logger, argsToPass []string) er | |
} | ||
} | ||
port := viper.GetInt(FlagPort) | ||
srv := httpapi.NewServer(ctx, httpapi.ServerConfig{ | ||
AgentType: agentType, | ||
Process: process, | ||
Port: port, | ||
ChatBasePath: viper.GetString(FlagChatBasePath), | ||
srv, err := httpapi.NewServer(ctx, httpapi.ServerConfig{ | ||
AgentType: agentType, | ||
Process: process, | ||
Port: port, | ||
ChatBasePath: viper.GetString(FlagChatBasePath), | ||
AllowedHosts: viper.GetStringSlice(FlagAllowedHosts), | ||
AllowedOrigins: viper.GetStringSlice(FlagAllowedOrigins), | ||
}) | ||
if err != nil { | ||
return xerrors.Errorf("failed to create server: %w", err) | ||
} | ||
if printOpenAPI { | ||
fmt.Println(srv.GetOpenAPI()) | ||
return nil | ||
|
@@ -150,12 +230,15 @@ type flagSpec struct { | |
} | ||
|
||
const ( | ||
FlagType = "type" | ||
FlagPort = "port" | ||
FlagPrintOpenAPI = "print-openapi" | ||
FlagChatBasePath = "chat-base-path" | ||
FlagTermWidth = "term-width" | ||
FlagTermHeight = "term-height" | ||
FlagType = "type" | ||
FlagPort = "port" | ||
FlagPrintOpenAPI = "print-openapi" | ||
FlagChatBasePath = "chat-base-path" | ||
FlagTermWidth = "term-width" | ||
FlagTermHeight = "term-height" | ||
FlagAllowedHosts = "allowed-hosts" | ||
FlagAllowedOrigins = "allowed-origins" | ||
FlagExit = "exit" | ||
) | ||
|
||
func CreateServerCmd() *cobra.Command { | ||
|
@@ -164,7 +247,22 @@ func CreateServerCmd() *cobra.Command { | |
Short: "Run the server", | ||
Long: fmt.Sprintf("Run the server with the specified agent (one of: %s)", strings.Join(agentNames, ", ")), | ||
Args: cobra.MinimumNArgs(1), | ||
PreRunE: func(cmd *cobra.Command, args []string) error { | ||
allowedHosts := viper.GetStringSlice(FlagAllowedHosts) | ||
if err := validateAllowedHosts(allowedHosts); err != nil { | ||
return xerrors.Errorf("failed to validate allowed hosts: %w", err) | ||
} | ||
allowedOrigins := viper.GetStringSlice(FlagAllowedOrigins) | ||
if err := validateAllowedOrigins(allowedOrigins); err != nil { | ||
return xerrors.Errorf("failed to validate allowed origins: %w", err) | ||
} | ||
return nil | ||
}, | ||
Run: func(cmd *cobra.Command, args []string) { | ||
// The --exit flag is used for testing validation of flags in the test suite | ||
if viper.GetBool(FlagExit) { | ||
return | ||
} | ||
logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) | ||
ctx := logctx.WithLogger(context.Background(), logger) | ||
if err := runServer(ctx, logger, cmd.Flags().Args()); err != nil { | ||
|
@@ -181,6 +279,10 @@ func CreateServerCmd() *cobra.Command { | |
{FlagChatBasePath, "c", "/chat", "Base path for assets and routes used in the static files of the chat interface", "string"}, | ||
{FlagTermWidth, "W", uint16(80), "Width of the emulated terminal", "uint16"}, | ||
{FlagTermHeight, "H", uint16(1000), "Height of the emulated terminal", "uint16"}, | ||
// localhost is the default host for the server. Port is ignored during matching. | ||
{FlagAllowedHosts, "a", []string{"localhost"}, "HTTP allowed hosts (hostnames only, no ports). Use '*' for all, comma-separated list via flag, space-separated list via AGENTAPI_ALLOWED_HOSTS env var", "stringSlice"}, | ||
// localhost:3284 is the default origin when you open the chat interface in your browser. localhost:3000 and 3001 are used during development. | ||
{FlagAllowedOrigins, "o", []string{"http://localhost:3284", "http://localhost:3000", "http://localhost:3001"}, "HTTP allowed origins. Use '*' for all, comma-separated list via flag, space-separated list via AGENTAPI_ALLOWED_ORIGINS env var", "stringSlice"}, | ||
} | ||
|
||
for _, spec := range flagSpecs { | ||
|
@@ -193,6 +295,8 @@ func CreateServerCmd() *cobra.Command { | |
serverCmd.Flags().BoolP(spec.name, spec.shorthand, spec.defaultValue.(bool), spec.usage) | ||
case "uint16": | ||
serverCmd.Flags().Uint16P(spec.name, spec.shorthand, spec.defaultValue.(uint16), spec.usage) | ||
case "stringSlice": | ||
serverCmd.Flags().StringSliceP(spec.name, spec.shorthand, spec.defaultValue.([]string), spec.usage) | ||
default: | ||
panic(fmt.Sprintf("unknown flag type: %s", spec.flagType)) | ||
} | ||
|
@@ -201,6 +305,14 @@ func CreateServerCmd() *cobra.Command { | |
} | ||
} | ||
|
||
serverCmd.Flags().Bool(FlagExit, false, "Exit immediately after parsing arguments") | ||
if err := serverCmd.Flags().MarkHidden(FlagExit); err != nil { | ||
panic(fmt.Sprintf("failed to mark flag %s as hidden: %v", FlagExit, err)) | ||
} | ||
if err := viper.BindPFlag(FlagExit, serverCmd.Flags().Lookup(FlagExit)); err != nil { | ||
panic(fmt.Sprintf("failed to bind flag %s: %v", FlagExit, err)) | ||
} | ||
|
||
viper.SetEnvPrefix("AGENTAPI") | ||
viper.AutomaticEnv() | ||
viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) | ||
|
Uh oh!
There was an error while loading. Please reload this page.