Skip to content

fix(security): 2 improvements across 2 files #8980

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

Open
tomaioo wants to merge 5 commits into
base: main
Choose a base branch
from
+10 −13
Open

fix(security): 2 improvements across 2 files #8980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a04a4f3
fix(security): 2 improvements across 2 files
tomaioo Jul 5, 2026
91f1b82
fix(security): 2 improvements across 2 files
tomaioo Jul 5, 2026
4c756be
fix: address review feedback — Removing this line leaves `crypto/tls`…
tomaioo Jul 12, 2026
1aefbb5
fix: address review feedback — This `CreateTemp` then Close then `Ope…
tomaioo Jul 12, 2026
d7e85d1
fix: address review feedback — This refactor is functionally identica…
tomaioo Jul 12, 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
14 changes: 7 additions & 7 deletions cli/azd/cmd/deps_record.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (

func createHttpClient() *http.Client {
transport := http.DefaultTransport.(*http.Transport).Clone()

@jongio jongio Jul 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Removing this line leaves crypto/tls imported but unused (line 9), which causes a compile error under the record build tag. Beyond that, this InsecureSkipVerify is required: the recording proxy uses a self-signed certificate. Without it (and without adding the proxy's CA to a custom cert pool), every HTTPS request through the proxy will fail with a TLS verification error. The same pattern exists in extensions/azure.ai.agents/internal/pkg/recordproxy/transport_record.go:41-44 for the same reason.

Since deps_record.go only compiles with //go:build record (test infrastructure), it never runs in production. The security risk described in the PR body doesn't apply here.

// Allow for self-signed certificates, which is what the recording proxy uses.
transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
// AZD_TEST_HTTPS_PROXY is the proxy setting that only affects azd in record mode.
// This is useful since the recording proxy server isn't trusted by other processes currently.
if val, ok := os.LookupEnv("AZD_TEST_HTTPS_PROXY"); ok {
Expand All @@ -29,12 +27,14 @@ func createHttpClient() *http.Client {
}

transport.Proxy = http.ProxyURL(proxyUrl)
http.DefaultTransport = transport
// The recording proxy uses a self-signed certificate, so we must skip TLS verification.
// This code only compiles under the "record" build tag and never runs in production.
transport.TLSClientConfig = &tls.Config{
InsecureSkipVerify: true,
}
}

http.DefaultClient.Transport = transport

return http.DefaultClient
return &http.Client{Transport: transport}
Comment on lines 20 to +37
}

func createClock() clock.Clock {
Expand All @@ -59,4 +59,4 @@ func fixedClock() (clock.Clock, bool) {

mockClock.Set(time.Unix(unixSec, 0))
return mockClock, true
}
}
9 changes: 3 additions & 6 deletions cli/azd/extensions/azure.ai.agents/internal/cmd/debug.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,8 @@ func setupDebugLogging(flags *pflag.FlagSet) func() {
return func() {}
}

@jongio jongio Jul 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This CreateTemp then Close then OpenFile pattern is a TOCTOU (time-of-check-time-of-use) race: between Close() and the subsequent OpenFile(), another process could modify or remove the file. This is actually less secure than the original code.

The original filename (azd-ai-agents-YYYY-MM-DD.log in CWD) was deliberately predictable so developers can find debug logs. Moving to a temp directory with a random suffix (azd-ai-agents-XYZ123.log in os.TempDir()) makes logs effectively unfindable.

The //nolint:gosec annotation on the original was correct: the G304 warning (file path from variable) doesn't apply when the variable is derived entirely from time.Now().Format(), not user input.

currentDate := time.Now().Format("2006-01-02")
logFileName := fmt.Sprintf("azd-ai-agents-%s.log", currentDate)

//nolint:gosec // log file name is generated locally from date and not user-controlled
logFile, err := os.OpenFile(logFileName, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600)
logFileName := fmt.Sprintf("azd-ai-agents-%s.log", time.Now().Format("2006-01-02"))
logFile, err := os.OpenFile(logFileName, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) //nolint:gosec // path derived from time.Now(), not user input

var w io.Writer
var closeFile func()
Expand Down Expand Up @@ -68,4 +65,4 @@ func isDebug(flags *pflag.FlagSet) bool {

debug, _ := strconv.ParseBool(os.Getenv("AZD_EXT_DEBUG"))
return debug
}
}
Loading