diff --git a/Makefile b/Makefile index 24c29b6..8fa3efb 100644 --- a/Makefile +++ b/Makefile @@ -168,7 +168,7 @@ test-cli-regressions: build NYLAS_TEST_RATE_LIMIT_BURST=$(NYLAS_TEST_RATE_LIMIT_BURST) \ NYLAS_TEST_BINARY=$(CURDIR)/bin/nylas \ go test ./internal/cli/integration/... -tags=integration -v -timeout 10m -p 1 \ - -run 'TestCLI_(InboundRemoved|InboxAliasRemoved|HelpOmitsInbound|AuthLoginRejectsInboxProvider|ConnectorSurfaces_HideInboxProvider|AdminConnectorsCreate_RejectsInboxProvider|AdminConnectorsShow_HidesInboxProvider)$$' + -run 'TestCLI_(InboundRemoved|InboxAliasRemoved|HelpOmitsInbound|AuthLoginRejectsInboxProvider|ConnectorSurfaces_HideInboxProvider|AdminConnectorsCreate_RejectsInboxProvider|AdminConnectorsShow_HidesInboxProvider|EmailSendValidationShowsFormattedSuggestion)$$' @echo "✓ CLI regression checks passed" # Agent integration checks require explicit credentials plus an agent domain so the lifecycle suites do not self-skip. @@ -268,9 +268,10 @@ test-playwright: @echo "" @echo "Installing Playwright dependencies..." @cd tests && npm install + @cd tests && npm run test:config @echo "" @echo "Running E2E tests..." - @cd tests && npx playwright test + @cd tests && UI_E2E_DEMO=true npx playwright test @echo "" @echo "✓ Playwright E2E tests complete!" @echo " Report: tests/playwright-report/index.html" @@ -294,7 +295,8 @@ test-playwright-ui: } @$(MAKE) --no-print-directory build @cd tests && npm install - @cd tests && npx playwright test --project=ui-chromium + @cd tests && npm run test:config + @cd tests && UI_E2E_DEMO=true npx playwright test --project=ui-chromium @echo "✓ UI E2E tests complete!" test-playwright-interactive: @@ -313,20 +315,7 @@ test-playwright-headed: # Security Targets # ============================================================================ security: - @echo "=== Security Scan ===" - @echo "Checking for hardcoded API keys..." - @grep -rE "nyk_v0[a-zA-Z0-9_]{20,}" --include="*.go" . | grep -v "_test.go" && echo "WARNING: Possible API key found!" || echo "✓ No API keys found" - @echo "" - @echo "Checking for credential patterns..." - @grep -rE "(api_key|password|secret)\s*=\s*\"[^\"]+\"" --include="*.go" . | grep -v "_test.go" | grep -v "mock.go" && echo "WARNING: Possible credentials found!" || echo "✓ No hardcoded credentials" - @echo "" - @echo "Checking for full credential logging..." - @grep -rE "fmt\.(Print|Fprint|Sprint).*[Aa]pi[Kk]ey[^:\[]" --include="*.go" . | grep -v "token.go" | grep -v "doctor.go" && echo "WARNING: Possible credential logging!" || echo "✓ No credential logging" - @echo "" - @echo "Checking staged files..." - @git diff --cached --name-only | grep -E "\.(env|key|pem|json)$$" && echo "WARNING: Sensitive file staged!" || echo "✓ No sensitive files staged" - @echo "" - @echo "=== Security scan complete ===" + @sh scripts/security-scan.sh . # ============================================================================ # CI Targets diff --git a/cmd/nylas/main.go b/cmd/nylas/main.go index eeabc67..a11d61d 100644 --- a/cmd/nylas/main.go +++ b/cmd/nylas/main.go @@ -14,6 +14,7 @@ import ( "github.com/nylas/cli/internal/cli/audit" "github.com/nylas/cli/internal/cli/auth" "github.com/nylas/cli/internal/cli/calendar" + "github.com/nylas/cli/internal/cli/common" "github.com/nylas/cli/internal/cli/config" "github.com/nylas/cli/internal/cli/contacts" "github.com/nylas/cli/internal/cli/dashboard" @@ -68,7 +69,7 @@ func main() { if err := cli.Execute(); err != nil { cli.LogAuditError(err) - fmt.Fprintln(os.Stderr, "Error:", err) + fmt.Fprint(os.Stderr, common.FormatError(err)) os.Exit(1) } } diff --git a/internal/cli/integration/local_regressions_test.go b/internal/cli/integration/local_regressions_test.go index 16998ec..1f40885 100644 --- a/internal/cli/integration/local_regressions_test.go +++ b/internal/cli/integration/local_regressions_test.go @@ -196,7 +196,7 @@ func TestCLI_AuthRemove_UpdatesDefaultGrantAndConfig(t *testing.T) { t.Fatalf("failed to set default grant: %v", err) } - stdout, stderr, err := runCLIWithOverrides(30*time.Second, map[string]string{ + stdout, stderr, err := runCLIWithOverrides(5*time.Second, map[string]string{ "XDG_CONFIG_HOME": configHome, "XDG_CACHE_HOME": cacheHome, "HOME": tempDir, @@ -255,7 +255,7 @@ func TestCLI_AuthList_DoesNotRequireFileStorePassphrase(t *testing.T) { configHome := filepath.Join(tempDir, "xdg") cacheHome := filepath.Join(tempDir, "cache") - stdout, stderr, err := runCLIWithOverrides(30*time.Second, map[string]string{ + stdout, stderr, err := runCLIWithOverrides(5*time.Second, map[string]string{ "XDG_CONFIG_HOME": configHome, "XDG_CACHE_HOME": cacheHome, "HOME": tempDir, @@ -275,6 +275,35 @@ func TestCLI_AuthList_DoesNotRequireFileStorePassphrase(t *testing.T) { } } +func TestCLI_EmailSendValidationShowsFormattedSuggestion(t *testing.T) { + if testBinary == "" { + t.Skip("CLI binary not found") + } + + tempDir := t.TempDir() + stdout, stderr, err := runCLIWithOverrides(5*time.Second, map[string]string{ + "XDG_CONFIG_HOME": filepath.Join(tempDir, "xdg"), + "XDG_CACHE_HOME": filepath.Join(tempDir, "cache"), + "HOME": tempDir, + "NYLAS_API_KEY": "", + "NYLAS_GRANT_ID": "", + "NYLAS_DISABLE_KEYRING": "true", + "NYLAS_FILE_STORE_PASSPHRASE": "integration-test-file-store-passphrase", + }, "email", "send", "--to", "user@example.com", "--body", "hi", "--yes") + if err == nil { + t.Fatalf("expected email send validation to fail\nstdout: %s\nstderr: %s", stdout, stderr) + } + for _, want := range []string{ + "subject is required", + "Suggestion:", + "Use --subject to specify the email subject", + } { + if !strings.Contains(stderr, want) { + t.Fatalf("stderr %q does not contain %q", stderr, want) + } + } +} + func TestCLI_AuthProviders_RequiresFileStorePassphrase(t *testing.T) { if testBinary == "" { t.Skip("CLI binary not found") diff --git a/internal/testutil/security_scan_integration_test.go b/internal/testutil/security_scan_integration_test.go new file mode 100644 index 0000000..c487203 --- /dev/null +++ b/internal/testutil/security_scan_integration_test.go @@ -0,0 +1,22 @@ +//go:build integration + +package testutil + +import ( + "strings" + "testing" +) + +func TestSecurityScanIntegrationFailsClosedForSensitiveJSON(t *testing.T) { + repoDir := initSecurityScanRepo(t) + writeFile(t, repoDir, "credentials.json", "{}\n") + runGit(t, repoDir, "add", "credentials.json") + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if !strings.Contains(output, "Sensitive tracked files found") { + t.Fatalf("security scan output %q does not mention tracked sensitive files", output) + } +} diff --git a/internal/testutil/security_scan_test.go b/internal/testutil/security_scan_test.go new file mode 100644 index 0000000..58ccf7c --- /dev/null +++ b/internal/testutil/security_scan_test.go @@ -0,0 +1,228 @@ +package testutil + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestSecurityScanFailsForHardcodedCredentials(t *testing.T) { + repoDir := initSecurityScanRepo(t) + writeFile(t, repoDir, "leak.go", `package main + +var api_key = "not-a-real-secret" +`) + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if !strings.Contains(output, "Possible credentials found") { + t.Fatalf("security scan output %q does not mention credentials", output) + } +} + +func TestSecurityScanRedactsMatchedSecretValues(t *testing.T) { + repoDir := initSecurityScanRepo(t) + secret := "nyk_v0abcdefghijklmnopqrstuvwx" + writeFile(t, repoDir, "leak.go", `package main + +var apiKey = "`+secret+`" +`) + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if strings.Contains(output, secret) { + t.Fatalf("security scan leaked matched secret value in output:\n%s", output) + } + if !strings.Contains(output, "leak.go:3") { + t.Fatalf("security scan output %q does not include sanitized file location", output) + } +} + +func TestSecurityScanScansNonGoCredentialFiles(t *testing.T) { + repoDir := initSecurityScanRepo(t) + secret := "demo-client-secret-12345" + writeFile(t, repoDir, "settings.yaml", "client_secret: "+secret+"\n") + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if !strings.Contains(output, "Possible credentials found") { + t.Fatalf("security scan output %q does not mention credentials", output) + } + if strings.Contains(output, secret) { + t.Fatalf("security scan leaked matched secret value in output:\n%s", output) + } + if !strings.Contains(output, "settings.yaml:1") { + t.Fatalf("security scan output %q does not include sanitized file location", output) + } +} + +func TestSecurityScanScansTokenCredentialNames(t *testing.T) { + repoDir := initSecurityScanRepo(t) + secret := "tokenvalue1234567890" + writeFile(t, repoDir, "app.go", `package main + +var access_token = "`+secret+`" +`) + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if !strings.Contains(output, "Possible credentials found") { + t.Fatalf("security scan output %q does not mention credentials", output) + } + if strings.Contains(output, secret) { + t.Fatalf("security scan leaked matched secret value in output:\n%s", output) + } + if !strings.Contains(output, "app.go:3") { + t.Fatalf("security scan output %q does not include sanitized file location", output) + } +} + +func TestSecurityScanDetectsFormattedAPIKeyLogging(t *testing.T) { + repoDir := initSecurityScanRepo(t) + writeFile(t, repoDir, "logging.go", `package main + +import "fmt" + +func main() { + apiKey := loadAPIKey() + _ = fmt.Sprintf("loaded key %s", apiKey) +} + +func loadAPIKey() string { + return "from-keyring" +} +`) + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if !strings.Contains(output, "Possible credential logging") { + t.Fatalf("security scan output %q does not mention credential logging", output) + } + if !strings.Contains(output, "logging.go:7") { + t.Fatalf("security scan output %q does not include sanitized file location", output) + } + if strings.Contains(output, "loaded key") || strings.Contains(output, "apiKey") { + t.Fatalf("security scan leaked matched logging line in output:\n%s", output) + } +} + +func TestSecurityScanFailsForTrackedSensitiveFiles(t *testing.T) { + repoDir := initSecurityScanRepo(t) + writeFile(t, repoDir, ".env.production", "NYLAS_API_KEY=not-a-real-key\n") + runGit(t, repoDir, "add", ".env.production") + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if !strings.Contains(output, "Sensitive tracked files found") { + t.Fatalf("security scan output %q does not mention tracked sensitive files", output) + } +} + +func TestSecurityScanFailsForTrackedSensitiveJSONFiles(t *testing.T) { + repoDir := initSecurityScanRepo(t) + writeFile(t, repoDir, "service-account.json", "{}\n") + runGit(t, repoDir, "add", "service-account.json") + + output, err := runSecurityScan(t, repoDir) + if err == nil { + t.Fatalf("security scan passed, want failure. output:\n%s", output) + } + if !strings.Contains(output, "Sensitive tracked files found") { + t.Fatalf("security scan output %q does not mention tracked sensitive files", output) + } +} + +func TestSecurityScanAllowsTestFixtures(t *testing.T) { + repoDir := initSecurityScanRepo(t) + writeFile(t, repoDir, "fixture_test.go", `package main + +var api_key = "not-a-real-secret" +`) + writeFile(t, repoDir, "testdata/settings.yaml", "client_secret: fixture-secret-value\n") + + output, err := runSecurityScan(t, repoDir) + if err != nil { + t.Fatalf("security scan failed, want success. output:\n%s", output) + } +} + +func initSecurityScanRepo(t *testing.T) string { + t.Helper() + + repoDir := t.TempDir() + runGit(t, repoDir, "init") + writeFile(t, repoDir, "main.go", "package main\n") + runGit(t, repoDir, "add", "main.go") + return repoDir +} + +func runSecurityScan(t *testing.T, repoDir string) (string, error) { + t.Helper() + + cmd := exec.Command("sh", securityScanScriptPath(t), repoDir) + output, err := cmd.CombinedOutput() + return string(output), err +} + +func securityScanScriptPath(t *testing.T) string { + t.Helper() + + dir, err := os.Getwd() + if err != nil { + t.Fatalf("os.Getwd returned error: %v", err) + } + + for { + candidate := filepath.Join(dir, "scripts", "security-scan.sh") + if _, err := os.Stat(candidate); err == nil { + return candidate + } + + parent := filepath.Dir(dir) + if parent == dir { + t.Fatalf("could not find scripts/security-scan.sh from %s", dir) + } + dir = parent + } +} + +func writeFile(t *testing.T, repoDir, name, contents string) { + t.Helper() + + path := filepath.Join(repoDir, name) + if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil { + t.Fatalf("os.MkdirAll returned error: %v", err) + } + if err := os.WriteFile(path, []byte(contents), 0600); err != nil { + t.Fatalf("os.WriteFile returned error: %v", err) + } +} + +func runGit(t *testing.T, repoDir string, args ...string) { + t.Helper() + + cmd := exec.Command("git", args...) + cmd.Dir = repoDir + cmd.Env = append(os.Environ(), + "HOME="+t.TempDir(), + "GIT_CONFIG_GLOBAL=/dev/null", + "GIT_CONFIG_SYSTEM=/dev/null", + ) + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s failed: %v\n%s", strings.Join(args, " "), err, output) + } +} diff --git a/internal/tui/attachment_download_integration_test.go b/internal/tui/attachment_download_integration_test.go new file mode 100644 index 0000000..f8081e3 --- /dev/null +++ b/internal/tui/attachment_download_integration_test.go @@ -0,0 +1,30 @@ +//go:build integration + +package tui + +import ( + "path/filepath" + "strings" + "testing" +) + +func TestIntegrationSafeAttachmentDownloadPathStaysInDestination(t *testing.T) { + downloadDir := t.TempDir() + + result, err := safeAttachmentDownloadPath(downloadDir, "../../../Library/LaunchAgents/com.example.plist") + if err != nil { + t.Fatalf("safeAttachmentDownloadPath returned error: %v", err) + } + + if filepath.Base(result) != "com.example.plist" { + t.Fatalf("download path base = %q, want %q", filepath.Base(result), "com.example.plist") + } + + rel, err := filepath.Rel(downloadDir, result) + if err != nil { + t.Fatalf("filepath.Rel returned error: %v", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + t.Fatalf("download path escaped destination: %q", result) + } +} diff --git a/internal/tui/attachments_test.go b/internal/tui/attachments_test.go index d7f9ce4..7823bc5 100644 --- a/internal/tui/attachments_test.go +++ b/internal/tui/attachments_test.go @@ -3,6 +3,7 @@ package tui import ( "os" "path/filepath" + "strings" "testing" "github.com/nylas/cli/internal/domain" @@ -129,6 +130,112 @@ func TestGetUniqueFilename(t *testing.T) { } } +func TestGetUniqueFilenameTreatsDanglingSymlinkAsExisting(t *testing.T) { + app := createTestApp(t) + view := NewMessagesView(app) + tmpDir := t.TempDir() + outsideDir := t.TempDir() + + linkPath := filepath.Join(tmpDir, "invoice.pdf") + if err := os.Symlink(filepath.Join(outsideDir, "outside.pdf"), linkPath); err != nil { + t.Skipf("symlink creation not supported: %v", err) + } + + result := view.getUniqueFilename(linkPath) + if result == linkPath { + t.Fatalf("getUniqueFilename returned dangling symlink path %q", result) + } + if filepath.Dir(result) != tmpDir { + t.Fatalf("getUniqueFilename returned path outside download dir: %q", result) + } + if filepath.Base(result) != "invoice (1).pdf" { + t.Fatalf("getUniqueFilename = %q, want invoice (1).pdf suffix", result) + } +} + +func TestCreateUniqueAttachmentFileDoesNotFollowDanglingSymlink(t *testing.T) { + app := createTestApp(t) + view := NewMessagesView(app) + tmpDir := t.TempDir() + outsideDir := t.TempDir() + + outsideTarget := filepath.Join(outsideDir, "outside.pdf") + linkPath := filepath.Join(tmpDir, "invoice.pdf") + if err := os.Symlink(outsideTarget, linkPath); err != nil { + t.Skipf("symlink creation not supported: %v", err) + } + + file, result, err := view.createUniqueAttachmentFile(linkPath) + if err != nil { + t.Fatalf("createUniqueAttachmentFile returned error: %v", err) + } + if err := file.Close(); err != nil { + t.Fatalf("failed to close created file: %v", err) + } + + if result == linkPath { + t.Fatalf("createUniqueAttachmentFile used dangling symlink path %q", result) + } + if filepath.Base(result) != "invoice (1).pdf" { + t.Fatalf("createUniqueAttachmentFile path = %q, want invoice (1).pdf suffix", result) + } + if _, err := os.Stat(outsideTarget); !os.IsNotExist(err) { + t.Fatalf("outside symlink target exists or returned unexpected error: %v", err) + } + if _, err := os.Stat(result); err != nil { + t.Fatalf("created attachment file missing: %v", err) + } +} + +func TestSafeAttachmentDownloadPathSanitizesTraversal(t *testing.T) { + tmpDir := t.TempDir() + + result, err := safeAttachmentDownloadPath(tmpDir, "../../.ssh/config") + if err != nil { + t.Fatalf("safeAttachmentDownloadPath returned error: %v", err) + } + + expected := filepath.Join(tmpDir, "config") + if result != expected { + t.Fatalf("safeAttachmentDownloadPath = %q, want %q", result, expected) + } + + rel, err := filepath.Rel(tmpDir, result) + if err != nil { + t.Fatalf("filepath.Rel returned error: %v", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + t.Fatalf("download path escaped destination: %q", result) + } +} + +func TestSafeAttachmentDownloadPathUsesFallbackForEmptyBase(t *testing.T) { + tmpDir := t.TempDir() + + tests := []struct { + name string + filename string + }{ + {"empty", ""}, + {"current directory", "."}, + {"parent directory", ".."}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := safeAttachmentDownloadPath(tmpDir, tt.filename) + if err != nil { + t.Fatalf("safeAttachmentDownloadPath returned error: %v", err) + } + + expected := filepath.Join(tmpDir, "attachment") + if result != expected { + t.Fatalf("safeAttachmentDownloadPath = %q, want %q", result, expected) + } + }) + } +} + func TestMessagesViewClearAttachmentsOnCloseDetail(t *testing.T) { app := createTestApp(t) diff --git a/internal/tui/views_messages_actions.go b/internal/tui/views_messages_actions.go index c30614f..251bb41 100644 --- a/internal/tui/views_messages_actions.go +++ b/internal/tui/views_messages_actions.go @@ -2,6 +2,7 @@ package tui import ( "context" + "errors" "fmt" "io" "os" @@ -188,11 +189,15 @@ func (v *MessagesView) downloadAttachment(messageID, attachmentID, filename stri } // Create file with unique name if exists - destPath := filepath.Join(downloadDir, filename) - destPath = v.getUniqueFilename(destPath) - - // #nosec G304 -- destPath is validated through getUniqueFilename() and constrained to downloadDir - file, err := os.Create(destPath) + destPath, err := safeAttachmentDownloadPath(downloadDir, filename) + if err != nil { + v.app.QueueUpdateDraw(func() { + v.app.Flash(FlashError, "Cannot create file: %v", err) + }) + return + } + // #nosec G304 -- destPath is sanitized and constrained to downloadDir by safeAttachmentDownloadPath. + file, destPath, err := v.createUniqueAttachmentFile(destPath) if err != nil { v.app.QueueUpdateDraw(func() { v.app.Flash(FlashError, "Cannot create file: %v", err) @@ -217,22 +222,86 @@ func (v *MessagesView) downloadAttachment(messageID, attachmentID, filename stri }() } +func safeAttachmentDownloadPath(downloadDir, filename string) (string, error) { + safeName := filepath.Base(filename) + if safeName == "." || safeName == ".." || safeName == string(filepath.Separator) { + safeName = "attachment" + } + + // filepath.Abs cleans traversal but intentionally does not resolve symlinked download directories. + downloadDirAbs, err := filepath.Abs(downloadDir) + if err != nil { + return "", fmt.Errorf("resolve download directory: %w", err) + } + + destPath := filepath.Join(downloadDirAbs, safeName) + destPathAbs, err := filepath.Abs(destPath) + if err != nil { + return "", fmt.Errorf("resolve attachment path: %w", err) + } + + rel, err := filepath.Rel(downloadDirAbs, destPathAbs) + if err != nil { + return "", fmt.Errorf("validate attachment path: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + return "", fmt.Errorf("attachment filename %q resolves outside Downloads", filename) + } + + return destPathAbs, nil +} + func (v *MessagesView) getUniqueFilename(path string) string { - if _, err := os.Stat(path); os.IsNotExist(err) { + // Caller must pass a path already sanitized and constrained to the download directory. + for i := 0; i < 1000; i++ { + candidate := uniqueAttachmentFilename(path, i) + if _, err := os.Lstat(candidate); os.IsNotExist(err) { + return candidate + } + } + + // Fallback: append timestamp + return timestampAttachmentFilename(path) +} + +func (v *MessagesView) createUniqueAttachmentFile(path string) (*os.File, string, error) { + // Caller must pass a path already sanitized and constrained to the download directory. + for i := 0; i < 1000; i++ { + candidate := uniqueAttachmentFilename(path, i) + // #nosec G304 -- candidate is derived from a path constrained by safeAttachmentDownloadPath. + file, err := os.OpenFile(candidate, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) + if err == nil { + return file, candidate, nil + } + if errors.Is(err, os.ErrExist) { + continue + } + return nil, "", err + } + + fallback := timestampAttachmentFilename(path) + // #nosec G304 -- fallback is derived from a path constrained by safeAttachmentDownloadPath. + file, err := os.OpenFile(fallback, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) + if err != nil { + return nil, "", err + } + return file, fallback, nil +} + +func uniqueAttachmentFilename(path string, index int) string { + if index == 0 { return path } dir := filepath.Dir(path) ext := filepath.Ext(path) name := strings.TrimSuffix(filepath.Base(path), ext) + return filepath.Join(dir, fmt.Sprintf("%s (%d)%s", name, index, ext)) +} - for i := 1; i < 1000; i++ { - newPath := filepath.Join(dir, fmt.Sprintf("%s (%d)%s", name, i, ext)) - if _, err := os.Stat(newPath); os.IsNotExist(err) { - return newPath - } - } - - // Fallback: append timestamp - return filepath.Join(dir, fmt.Sprintf("%s_%d%s", name, time.Now().Unix(), ext)) +func timestampAttachmentFilename(path string) string { + dir := filepath.Dir(path) + ext := filepath.Ext(path) + name := strings.TrimSuffix(filepath.Base(path), ext) + return filepath.Join(dir, fmt.Sprintf("%s_%d%s", name, time.Now().UnixNano(), ext)) } diff --git a/scripts/security-scan.sh b/scripts/security-scan.sh new file mode 100755 index 0000000..ad35706 --- /dev/null +++ b/scripts/security-scan.sh @@ -0,0 +1,147 @@ +#!/bin/sh +set -eu + +repo_dir=${1:-.} +cd "$repo_dir" + +found=0 + +excluded_dirs='(^|/)(\.git|vendor|node_modules|testdata|playwright-report|test-results|coverage|dist|build)/' + +is_excluded_path() { + printf '%s\n' "$1" | grep -Eq "$excluded_dirs" +} + +is_text_candidate() { + case "$1" in + *_test.go | *.png | *.jpg | *.jpeg | *.gif | *.ico | *.pdf | *.zip | *.tar | *.gz | *.tgz | *.woff | *.woff2 | *.ttf | *.eot | *.mp4 | *.mov | *.sqlite | *.db) + return 1 + ;; + esac + + return 0 +} + +list_scan_files() { + git ls-files -co --exclude-standard 2>/dev/null | while IFS= read -r file; do + [ -n "$file" ] || continue + is_excluded_path "$file" && continue + is_text_candidate "$file" || continue + printf '%s\n' "$file" + done +} + +scan_content_locations() { + pattern=$1 + + list_scan_files | while IFS= read -r file; do + grep -nEI "$pattern" "$file" 2>/dev/null | while IFS=: read -r line_no _; do + [ -n "$line_no" ] || continue + printf '%s:%s\n' "$file" "$line_no" + done + done | sort -u +} + +is_config_candidate() { + case "$1" in + .env | .env.* | *.env | *.yaml | *.yml | *.json | *.toml | *.ini | *.conf | *.properties | *.tf | Dockerfile | docker-compose.yml | docker-compose.yaml) + return 0 + ;; + esac + + return 1 +} + +scan_credential_locations() { + credential_name="(api[_-]?key|apikey|password|passwd|secret|client[_-]?secret|access[_-]?token|refresh[_-]?token|private[_-]?key|bearer|aws[_-]?access[_-]?key[_-]?id)" + source_pattern="$credential_name[[:space:]]*(:[[:space:]]+|=[[:space:]]*)['\"][A-Za-z0-9_./+=:@-]{8,}['\"]" + config_pattern="$credential_name[[:space:]]*(:[[:space:]]+|=[[:space:]]*)['\"]?[A-Za-z0-9_./+=:@-]{8,}" + + list_scan_files | while IFS= read -r file; do + pattern=$source_pattern + if is_config_candidate "$file"; then + pattern=$config_pattern + fi + + grep -nEI "$pattern" "$file" 2>/dev/null | while IFS=: read -r line_no _; do + [ -n "$line_no" ] || continue + printf '%s:%s\n' "$file" "$line_no" + done + done | sort -u +} + +filter_allowed_logging_locations() { + while IFS= read -r location; do + file=${location%:*} + case "$file" in + internal/cli/auth/token.go | internal/cli/doctor.go | internal/cli/ai/config_main.go) + continue + ;; + esac + printf '%s\n' "$location" + done +} + +echo "=== Security Scan ===" +echo "Checking for hardcoded API keys..." +matches=$(scan_content_locations "nyk_v0[a-zA-Z0-9_]{20,}" || true) +if [ -n "$matches" ]; then + printf '%s\n' "$matches" + echo "WARNING: Possible API key found!" + found=1 +else + echo "OK: No API keys found" +fi + +echo "" +echo "Checking for credential patterns..." +matches=$(scan_credential_locations || true) +if [ -n "$matches" ]; then + printf '%s\n' "$matches" + echo "WARNING: Possible credentials found!" + found=1 +else + echo "OK: No hardcoded credentials" +fi + +echo "" +echo "Checking for full credential logging..." +logging_pattern="fmt\.(Print|Fprint|Sprint)[A-Za-z]*\([^)]*([A-Za-z0-9_]*[Aa]pi[Kk]ey|api_key)" +matches=$(scan_content_locations "$logging_pattern" | filter_allowed_logging_locations || true) +if [ -n "$matches" ]; then + printf '%s\n' "$matches" + echo "WARNING: Possible credential logging!" + found=1 +else + echo "OK: No credential logging" +fi + +echo "" +echo "Checking staged files..." +matches=$(git diff --cached --name-only 2>/dev/null | grep -E '(^|/)\.env([^/]*|$)|\.(key|pem|json)$|(^|/)secrets/' || true) +if [ -n "$matches" ]; then + printf '%s\n' "$matches" + echo "WARNING: Sensitive file staged!" + found=1 +else + echo "OK: No sensitive files staged" +fi + +echo "" +echo "Checking tracked files..." +matches=$(git ls-files 2>/dev/null | grep -Ei '(^|/)\.env([^/]*|$)|\.(key|pem)$|(^|/)secrets/|(^|/)[^/]*(secret|credential|credentials|service-account|private-key|private)[^/]*\.json$' || true) +if [ -n "$matches" ]; then + printf '%s\n' "$matches" + echo "WARNING: Sensitive tracked files found!" + found=1 +else + echo "OK: No sensitive tracked files" +fi + +echo "" +if [ "$found" -ne 0 ]; then + echo "Security scan failed" + exit 1 +fi + +echo "=== Security scan complete ===" diff --git a/tests/package.json b/tests/package.json index 22ef4b8..1ce09b5 100644 --- a/tests/package.json +++ b/tests/package.json @@ -9,6 +9,7 @@ "test:air": "playwright test --project=air-chromium", "test:ui": "playwright test --project=ui-chromium", "test:chat": "playwright test --project=chat-chromium", + "test:config": "node --test playwright.config.test.js", "test:air:ui": "playwright test --project=air-chromium --ui", "test:ui:ui": "playwright test --project=ui-chromium --ui", "test:air:headed": "playwright test --project=air-chromium --headed", diff --git a/tests/playwright.config.js b/tests/playwright.config.js index 55b66dc..0cb9c6d 100644 --- a/tests/playwright.config.js +++ b/tests/playwright.config.js @@ -17,6 +17,10 @@ const isCI = !!process.env.CI; const airPort = parseInt(process.env.AIR_PORT || '7365', 10); const uiPort = parseInt(process.env.UI_PORT || '7363', 10); const chatPort = parseInt(process.env.CHAT_PORT || '7367', 10); +const uiE2EDemo = process.env.UI_E2E_DEMO === 'true'; +const uiCommand = uiE2EDemo + ? 'cd .. && go run cmd/nylas/main.go demo ui --no-browser --port ' + uiPort + : 'cd .. && go run cmd/nylas/main.go ui --no-browser --port ' + uiPort; module.exports = defineConfig({ // Run tests in parallel within files @@ -120,10 +124,10 @@ module.exports = defineConfig({ }, // Nylas UI server (port 7363) { - command: 'cd .. && go run cmd/nylas/main.go ui --no-browser --port ' + uiPort, + command: uiCommand, port: uiPort, timeout: 60000, - reuseExistingServer: !isCI, + reuseExistingServer: !isCI && !uiE2EDemo, }, // Nylas Chat server (port 7367) { diff --git a/tests/playwright.config.test.js b/tests/playwright.config.test.js new file mode 100644 index 0000000..f81cf6a --- /dev/null +++ b/tests/playwright.config.test.js @@ -0,0 +1,72 @@ +// @ts-check +const assert = require('node:assert/strict'); +const test = require('node:test'); + +function loadConfigWithEnv(env = {}) { + const configPath = require.resolve('./playwright.config.js'); + const previousEnv = {}; + + for (const [key, value] of Object.entries(env)) { + previousEnv[key] = process.env[key]; + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + + try { + delete require.cache[configPath]; + return require(configPath); + } finally { + delete require.cache[configPath]; + for (const [key, value] of Object.entries(previousEnv)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + } +} + +function projectByName(config, name) { + const project = config.projects.find((candidate) => candidate.name === name); + assert.ok(project, `expected ${name} project config`); + return project; +} + +function uiProjectPort(config) { + const baseURL = projectByName(config, 'ui-chromium').use.baseURL; + assert.ok(baseURL, 'expected UI project baseURL'); + return Number(new URL(baseURL).port); +} + +function uiWebServer(config) { + const uiPort = uiProjectPort(config); + const uiServer = config.webServer.find((server) => server.port === uiPort); + assert.ok(uiServer, 'expected UI web server config'); + return uiServer; +} + +function uiWebServerCommand(config) { + return uiWebServer(config).command; +} + +test('UI E2E uses real UI server by default', () => { + const config = loadConfigWithEnv({ UI_E2E_DEMO: undefined }); + + assert.match(uiWebServerCommand(config), /cmd\/nylas\/main\.go ui --no-browser --port 7363/); +}); + +test('UI E2E can run against deterministic demo UI', () => { + const config = loadConfigWithEnv({ UI_E2E_DEMO: 'true' }); + + assert.match(uiWebServerCommand(config), /cmd\/nylas\/main\.go demo ui --no-browser --port 7363/); +}); + +test('UI E2E demo mode does not reuse a non-demo server', () => { + const config = loadConfigWithEnv({ UI_E2E_DEMO: 'true', CI: undefined }); + + assert.equal(uiWebServer(config).reuseExistingServer, false); +}); diff --git a/tests/ui/e2e/header-controls.spec.js b/tests/ui/e2e/header-controls.spec.js index 8323eed..4544ff0 100644 --- a/tests/ui/e2e/header-controls.spec.js +++ b/tests/ui/e2e/header-controls.spec.js @@ -80,10 +80,14 @@ test.describe('Client Dropdown', () => { await skipIfNotConfigured(page, testInfo); const clientDropdown = page.locator(selectors.header.clientDropdown); - const clientText = await clientDropdown.textContent(); + const selectedClient = clientDropdown.locator(selectors.header.selectedClient); + const clientText = (await selectedClient.textContent())?.trim() || ''; - // Should contain a truncated client ID - expect(clientText).toMatch(/[a-f0-9]{8}/i); + if (process.env.UI_E2E_DEMO === 'true') { + expect(clientText).toBe('demo-cli...'); + } else { + expect(clientText).toMatch(/^[a-f0-9]{8}\.\.\.$/i); + } }); test('client dropdown opens on click', async ({ page }, testInfo) => { diff --git a/tests/ui/e2e/keyboard.spec.js b/tests/ui/e2e/keyboard.spec.js index 02c97a1..83e28cb 100644 --- a/tests/ui/e2e/keyboard.spec.js +++ b/tests/ui/e2e/keyboard.spec.js @@ -1,4 +1,9 @@ // @ts-check +const { spawn } = require('node:child_process'); +const fs = require('node:fs/promises'); +const net = require('node:net'); +const os = require('node:os'); +const path = require('node:path'); const { test, expect } = require('@playwright/test'); const selectors = require('../../shared/helpers/ui-selectors'); @@ -25,6 +30,148 @@ async function skipIfNotConfigured(page, testInfo) { } } +function getAvailablePort() { + return new Promise((resolve, reject) => { + const server = net.createServer(); + server.on('error', reject); + server.listen(0, '127.0.0.1', () => { + const address = server.address(); + if (!address || typeof address === 'string') { + server.close(() => reject(new Error('failed to allocate TCP port'))); + return; + } + server.close(() => resolve(address.port)); + }); + }); +} + +async function waitForServer(url, child, output) { + const deadline = Date.now() + 60000; + + while (Date.now() < deadline) { + if (child.exitCode !== null) { + throw new Error(`unconfigured UI server exited early with ${child.exitCode}\n${output()}`); + } + + try { + const response = await fetch(url); + if (response.ok) { + return; + } + } catch { + // Server is not ready yet. + } + + await new Promise((resolve) => setTimeout(resolve, 250)); + } + + throw new Error(`timed out waiting for unconfigured UI server\n${output()}`); +} + +async function stopServer(child) { + if (child.exitCode !== null) { + return; + } + + const exited = new Promise((resolve) => child.once('exit', resolve)); + child.kill('SIGTERM'); + await Promise.race([ + exited, + new Promise((resolve) => setTimeout(resolve, 5000)), + ]); + + if (child.exitCode === null) { + child.kill('SIGKILL'); + await exited; + } +} + +async function uiServerCommand(repoRoot, port) { + const binary = path.join(repoRoot, 'bin', 'nylas'); + try { + await fs.access(binary); + return { + command: binary, + args: ['ui', '--no-browser', '--port', String(port)], + }; + } catch { + return { + command: 'go', + args: ['run', 'cmd/nylas/main.go', 'ui', '--no-browser', '--port', String(port)], + }; + } +} + +async function startUnconfiguredUIServer(repoRoot, env) { + let lastError; + + for (let attempt = 0; attempt < 3; attempt++) { + const port = await getAvailablePort(); + const { command, args } = await uiServerCommand(repoRoot, port); + const child = spawn( + command, + args, + { + cwd: repoRoot, + env, + stdio: ['ignore', 'pipe', 'pipe'], + } + ); + + let output = ''; + child.stdout.on('data', (chunk) => { + output += chunk.toString(); + }); + child.stderr.on('data', (chunk) => { + output += chunk.toString(); + }); + + try { + const url = `http://127.0.0.1:${port}`; + await waitForServer(url, child, () => output); + return { child, url }; + } catch (err) { + lastError = err; + await stopServer(child); + if (!String(err.message || err).includes('address already in use')) { + throw err; + } + } + } + + throw lastError; +} + +async function withUnconfiguredUIServer(callback) { + const repoRoot = path.resolve(__dirname, '../../..'); + const homeDir = await fs.mkdtemp(path.join(os.tmpdir(), 'nylas-ui-unconfigured-')); + const configDir = path.join(homeDir, 'config'); + await fs.mkdir(configDir, { recursive: true }); + + const env = { + ...process.env, + HOME: homeDir, + XDG_CONFIG_HOME: configDir, + NYLAS_DISABLE_KEYRING: 'true', + }; + delete env.NYLAS_API_KEY; + delete env.NYLAS_CLIENT_ID; + delete env.NYLAS_GRANT_ID; + delete env.UI_E2E_DEMO; + + let server; + try { + server = await startUnconfiguredUIServer(repoRoot, env); + const { child, url } = server; + await callback(url); + } finally { + if (server) { + await stopServer(server.child); + } + await fs.rm(homeDir, { recursive: true, force: true }); + } +} + test.describe('Tab Navigation', () => { test.beforeEach(async ({ page }) => { await page.goto('/'); @@ -362,32 +509,31 @@ test.describe('Form Keyboard Navigation', () => { }); test('setup form fields are keyboard accessible', async ({ page }) => { - const setupView = page.locator(selectors.setup.view); - const isActive = await setupView.evaluate((el) => el.classList.contains('active')); - - if (!isActive) { - test.skip(); - return; - } - - // Tab to API key input - const apiKeyInput = page.locator(selectors.setup.apiKeyInput); - await apiKeyInput.focus(); - - await expect(apiKeyInput).toBeFocused(); - - // The setup form has a "show password" toggle button between the API key - // input and the region select, so tab past it to reach the region select. - const regionSelect = page.locator(selectors.setup.regionSelect); - for (let i = 0; i < 5; i++) { - if (await regionSelect.evaluate((el) => el === document.activeElement)) { - break; + await withUnconfiguredUIServer(async (baseURL) => { + await page.goto(baseURL); + + const setupView = page.locator(selectors.setup.view); + await expect(setupView).toHaveClass(/active/); + + // Tab to API key input + const apiKeyInput = page.locator(selectors.setup.apiKeyInput); + await apiKeyInput.focus(); + + await expect(apiKeyInput).toBeFocused(); + + // The setup form has a "show password" toggle button between the API key + // input and the region select, so tab past it to reach the region select. + const regionSelect = page.locator(selectors.setup.regionSelect); + for (let i = 0; i < 5; i++) { + if (await regionSelect.evaluate((el) => el === document.activeElement)) { + break; + } + await page.keyboard.press('Tab'); + await page.waitForTimeout(50); } - await page.keyboard.press('Tab'); - await page.waitForTimeout(50); - } - await expect(regionSelect).toBeFocused(); + await expect(regionSelect).toBeFocused(); + }); }); });