Skip to content

fix: don't prompt for creds for local models #567

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

Merged
merged 2 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 8 additions & 6 deletions pkg/credentials/credential.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,15 @@ func (c Credential) toDockerAuthConfig() (types.AuthConfig, error) {

func credentialFromDockerAuthConfig(authCfg types.AuthConfig) (Credential, error) {
var cred Credential
if err := json.Unmarshal([]byte(authCfg.Password), &cred); err != nil || len(cred.Env) == 0 {
// Legacy: try unmarshalling into just an env map
var env map[string]string
if err := json.Unmarshal([]byte(authCfg.Password), &env); err != nil {
return Credential{}, err
if authCfg.Password != "" {
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a bug where we would try to unmarshal an empty string.

if err := json.Unmarshal([]byte(authCfg.Password), &cred); err != nil || len(cred.Env) == 0 {
// Legacy: try unmarshalling into just an env map
var env map[string]string
if err := json.Unmarshal([]byte(authCfg.Password), &env); err != nil {
return Credential{}, err
}
cred.Env = env
}
cred.Env = env
}

// We used to hardcode the username as "gptscript" before CredentialType was introduced, so
Expand Down
14 changes: 12 additions & 2 deletions pkg/credentials/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package credentials

import (
"errors"
"net/url"
"regexp"
"strings"

Expand Down Expand Up @@ -71,9 +72,18 @@ func (h *HelperStore) GetAll() (map[string]types.AuthConfig, error) {
contextPieces := strings.Split(ctx, ":")
if len(contextPieces) > 1 {
possiblePortNumber := contextPieces[len(contextPieces)-1]
if regexp.MustCompile(`\d+$`).MatchString(possiblePortNumber) {
if regexp.MustCompile(`^\d+$`).MatchString(possiblePortNumber) {
// port number confirmed
toolName = toolName + ":" + possiblePortNumber
toolURL, err := url.Parse(toolName)
if err != nil {
return nil, err
}

// Save the path so we can put it back after removing it.
path := toolURL.Path
toolURL.Path = ""

toolName = toolURL.String() + ":" + possiblePortNumber + path
ctx = strings.TrimSuffix(ctx, ":"+possiblePortNumber)
Comment on lines +77 to 87
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a bug where we were putting the port after the path.
I.e., the string we're trying to fix is http://localhost/v1///default:5555 and we used to make it become http://localhost/v1:5555 rather than http://localhost:5555/v1.

}
}
Expand Down
7 changes: 6 additions & 1 deletion pkg/remote/remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (c *Client) clientFromURL(ctx context.Context, apiURL string) (*openai.Clie
env := "GPTSCRIPT_PROVIDER_" + env2.ToEnvLike(parsed.Hostname()) + "_API_KEY"
key := os.Getenv(env)

if key == "" {
if key == "" && !isLocalhost(apiURL) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, we no longer look for the credential in the store if the model is running locally. Not sure whether we care to support IPv6 (::1) here, but I can add that if we do.

var err error
key, err = c.retrieveAPIKey(ctx, env, apiURL)
if err != nil {
Expand Down Expand Up @@ -179,3 +179,8 @@ func (c *Client) load(ctx context.Context, toolName string) (*openai.Client, err
func (c *Client) retrieveAPIKey(ctx context.Context, env, url string) (string, error) {
return prompt.GetModelProviderCredential(ctx, c.credStore, url, env, fmt.Sprintf("Please provide your API key for %s", url), append(gcontext.GetEnv(ctx), c.envs...))
}

func isLocalhost(url string) bool {
return strings.HasPrefix(url, "http://localhost") || strings.HasPrefix(url, "http://127.0.0.1") ||
strings.HasPrefix(url, "https://localhost") || strings.HasPrefix(url, "https://127.0.0.1")
}
9 changes: 9 additions & 0 deletions pkg/types/tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"path/filepath"
"regexp"
"slices"
"sort"
"strings"
Expand Down Expand Up @@ -283,6 +284,10 @@ func ParseCredentialArgs(toolName string, input string) (string, string, map[str
fields = fields[2:]
}

if alias != "" && !isAlphaNumeric(alias) {
return "", "", nil, fmt.Errorf("credential alias must be alphanumeric")
}
Comment on lines +287 to +289
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to add a constraint that a credential alias needs to be alphanumeric. This is to avoid interfering with credentials for model providers and such.


if len(fields) == 0 { // Nothing left, so just return
return originalName, alias, nil, nil
}
Expand Down Expand Up @@ -780,3 +785,7 @@ func FirstSet[T comparable](in ...T) (result T) {
}
return
}

func isAlphaNumeric(s string) bool {
return regexp.MustCompile(`^[a-zA-Z0-9_.]+$`).MatchString(s)
}
Loading