Skip to content
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

### Fixed

- Redacted configured Semaphore API tokens from provider response diagnostics. Thanks @coygeek.
- Kept GitHub Actions runner registration tokens off remote SSH command arguments. Thanks @coygeek.
- Redacted Cloudflare runner bearer tokens from HTTP and streamed error diagnostics. Thanks @coygeek.
- Confined remote failure-bundle links to the generated archive subtree and omitted unsafe special entries. Thanks @coygeek.
Expand Down
3 changes: 2 additions & 1 deletion docs/features/semaphore.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ Defaults when unset: `machine: f1-standard-2`, `osImage: ubuntu2204`,
| OS image | `semaphore.osImage` | `--semaphore-os-image` | `CRABBOX_SEMAPHORE_OS_IMAGE` |
| Idle timeout| `semaphore.idleTimeout` | `--semaphore-idle-timeout` | `CRABBOX_SEMAPHORE_IDLE_TIMEOUT` |

The token has no flag; pass it via env or config only.
The token has no flag; pass it via env or config only. Semaphore API error
bodies redact the configured token before they reach CLI diagnostics.

Equivalent one-off invocations:

Expand Down
25 changes: 22 additions & 3 deletions internal/providers/semaphore/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ func (c *apiClient) getWithHeaders(ctx context.Context, path string) ([]byte, ht
defer resp.Body.Close()
body, _ := io.ReadAll(resp.Body)
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return nil, nil, &semaphoreAPIError{Path: path, StatusCode: resp.StatusCode, Body: string(body)}
return nil, nil, c.responseError(path, resp.StatusCode, body)
}
return body, resp.Header, nil
}
Expand All @@ -323,6 +323,25 @@ func (e *semaphoreAPIError) Error() string {
return fmt.Sprintf("semaphore API %s returned %d: %s", e.Path, e.StatusCode, e.Body)
}

func (c *apiClient) responseError(path string, statusCode int, body []byte) *semaphoreAPIError {
return &semaphoreAPIError{
Path: path,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Redact secrets before storing the error path

When resolving a project, path can come from the API's Link header via nextLinkPath; if a bad or reflective Semaphore response puts the configured token in that next URL and the follow-up request returns non-2xx, semaphoreAPIError.Error() prints this Path verbatim even though the body is redacted. Please run the same redaction over path before storing it so response-controlled pagination diagnostics cannot leak the token.

Useful? React with 👍 / 👎.

StatusCode: statusCode,
Body: redactSemaphoreSecrets(strings.TrimSpace(string(body)), c.token),
}
}

func redactSemaphoreSecrets(value string, secrets ...string) string {
redacted := value
for _, secret := range secrets {
secret = strings.TrimSpace(secret)
if secret != "" {
redacted = strings.ReplaceAll(redacted, secret, "[redacted]")
}
}
return redacted
}

func (c *apiClient) get(ctx context.Context, path string, target any) error {
req, err := http.NewRequestWithContext(ctx, "GET", "https://"+c.host+path, nil)
if err != nil {
Expand All @@ -339,7 +358,7 @@ func (c *apiClient) get(ctx context.Context, path string, target any) error {
body, _ := io.ReadAll(resp.Body)

if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return &semaphoreAPIError{Path: path, StatusCode: resp.StatusCode, Body: string(body)}
return c.responseError(path, resp.StatusCode, body)
}
if target != nil {
return json.Unmarshal(body, target)
Expand Down Expand Up @@ -372,7 +391,7 @@ func (c *apiClient) post(ctx context.Context, path string, payload any, target a
body, _ := io.ReadAll(resp.Body)

if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return &semaphoreAPIError{Path: path, StatusCode: resp.StatusCode, Body: string(body)}
return c.responseError(path, resp.StatusCode, body)
}
if target != nil {
return json.Unmarshal(body, target)
Expand Down
88 changes: 88 additions & 0 deletions internal/providers/semaphore/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ type testClock struct{}

func (testClock) Now() time.Time { return time.Now() }

type semaphoreRoundTripFunc func(*http.Request) (*http.Response, error)

func (f semaphoreRoundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {
return f(req)
}

func testConfig(host string) core.Config {
cfg := core.BaseConfig()
cfg.Provider = providerName
Expand Down Expand Up @@ -222,6 +228,88 @@ func TestStripLeasePrefix(t *testing.T) {

// --- API client tests with httptest ---

func TestAPIClientRedactsTokenFromAllResponseErrors(t *testing.T) {
const token = "semaphore-secret-token"
tests := []struct {
name string
path string
call func(*apiClient, string) error
}{
{
name: "get with headers",
path: "/get-with-headers",
call: func(client *apiClient, path string) error {
_, _, err := client.getWithHeaders(context.Background(), path)
return err
},
},
{name: "get", path: "/get", call: func(client *apiClient, path string) error {
return client.get(context.Background(), path, nil)
}},
{name: "post", path: "/post", call: func(client *apiClient, path string) error {
return client.post(context.Background(), path, map[string]string{"value": "test"}, nil)
}},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
client := &apiClient{
host: "semaphore.example.test",
token: token,
http: &http.Client{Transport: semaphoreRoundTripFunc(func(req *http.Request) (*http.Response, error) {
if got := req.Header.Get("Authorization"); got != "Token "+token {
t.Fatalf("Authorization=%q", got)
}
return &http.Response{
StatusCode: http.StatusUnauthorized,
Body: io.NopCloser(strings.NewReader("bad Authorization: Token " + token)),
Header: make(http.Header),
}, nil
})},
}
err := tc.call(client, tc.path)
if err == nil {
t.Fatal("API call returned nil")
}
if strings.Contains(err.Error(), token) {
t.Fatalf("API error leaked token: %v", err)
}
for _, want := range []string{tc.path, "401", "Token [redacted]"} {
if !strings.Contains(err.Error(), want) {
t.Fatalf("API error=%q, want %q", err, want)
}
}
})
}
if got := redactSemaphoreSecrets("keep body", " "); got != "keep body" {
t.Fatalf("empty secret changed response body: %q", got)
}
}

func TestAPIClientTLSRedactsReflectedToken(t *testing.T) {
const token = "semaphore-secret-token"
server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
if got := req.Header.Get("Authorization"); got != "Token "+token {
t.Fatalf("Authorization=%q", got)
}
w.WriteHeader(http.StatusUnauthorized)
_, _ = io.WriteString(w, "bad "+req.Header.Get("Authorization"))
}))
defer server.Close()

client := &apiClient{
host: strings.TrimPrefix(server.URL, "https://"),
token: token,
http: server.Client(),
}
err := client.get(context.Background(), "/api/v1alpha/projects", nil)
if err == nil {
t.Fatal("API call returned nil")
}
if strings.Contains(err.Error(), token) || !strings.Contains(err.Error(), "Token [redacted]") {
t.Fatalf("TLS API error was not redacted: %v", err)
}
}

func TestCreateJob(t *testing.T) {
var receivedBody map[string]any
server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Expand Down