microcks / microcks/microcks-cli

pkg/util/rand helpers panic on invalid input instead of returning errors

Open Beginner friendly
#359 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
52
Forks
68
Avg merge
6h 54m
Merged PRs (30d)
10

Description

### Describe the bug

`pkg/util/rand` provides two helpers — `String` and `StringFromCharset` — that both return `(string, error)`. The signature implies invalid inputs should surface as a Go `error`, but some invalid inputs currently cause the process to panic instead:

- `StringFromCharset(1, "")` panics in `crypto/rand.Int` because the upper bound is `<= 0`.
- `String(-1)` panics in `make([]byte, -1)` because the slice length is negative.

Since both functions already declare an `error` return, callers naturally expect that path to be used. Today they cannot recover — the helpers crash the host process before any caller code can react.

---

### Expected behavior

Invalid inputs should return a clear error instead of panicking:

```text
StringFromCharset(1, "") -> "", error("rand: cannot generate ... from empty charset")
String(-1) -> "", error("rand: requested length -1 is negative")
```

`n == 0` should keep returning `("", nil)` regardless of the charset, since that's the natural no-op result for both helpers.

---

### Actual behavior

`StringFromCharset(1, "")` panics with:

```text
panic: crypto/rand: argument to Int is <= 0
```

`String(-1)` panics with:

```text
panic: runtime error: makeslice: len out of range
```

In both cases the process crashes; the `error` return value is never set.

---

### How to Reproduce?

Reproduced locally on the latest `master`.

**Reproducer for empty charset:**

```go
package main

import (
"fmt"

r "github.com/microcks/microcks-cli/pkg/util/rand"
)

func main() {
fmt.Println("Testing empty charset...")
value, err := r.StringFromCharset(1, "")
fmt.Printf("value=%q err=%v\n", value, err)
}
```

Output:

```text
Testing empty charset...
panic: crypto/rand: argument to Int is <= 0
```

**Reproducer for negative length:**

```go
package main

import (
"fmt"

r "github.com/microcks/microcks-cli/pkg/util/rand"
)

func main() {
fmt.Println("Testing negative length...")
value, err := r.String(-1)
fmt.Printf("value=%q err=%v\n", value, err)
}
```

Output:

```text
Testing negative length...
panic: runtime error: makeslice: len out of range
```

In both cases the program exits via panic before the `err=` line can print.

---

### Microcks version or git rev

_No response_

---

### Install method (`docker-compose`, `helm chart`, `operator`, `docker-desktop extension`,...)

_No response_

---

### Additional information

**Root cause.** `StringFromCharset` in `pkg/util/rand/rand.go` does not validate `n` or `charset` before allocating the output slice and calling `crypto/rand.Int`:

```go
func StringFromCharset(n int, charset string) (string, error) {
b := make([]byte, n)
maxIdx := big.NewInt(int64(len(charset)))
for i := 0; i < n; i++ {
randIdx, err := rand.Int(rand.Reader, maxIdx)
...
}
...
}
```

- `n < 0` → `make([]byte, n)` panics with `makeslice: len out of range`.
- `n > 0` with `charset == ""` → `rand.Int(rand.Reader, big.NewInt(0))` panics with `argument to Int is <= 0`.
- `n == 0` → already works correctly (loop doesn't execute, returns `("", nil)`).

`String` calls `StringFromCharset` and inherits the negative-length panic.

**Proposed fix.** Add two guard clauses at the top of `StringFromCharset`:

```go
if n < 0 {
return "", fmt.Errorf("rand: requested length %d is negative", n)
}
if n > 0 && len(charset) == 0 {
return "", fmt.Errorf("rand: cannot generate %d-character string from empty charset", n)
}
```

This preserves every currently-working input (including `n == 0`) and turns both panic paths into descriptive errors. The public signatures stay the same, so no callers (including the SSO PKCE flow in `cmd/login.go`) need to change.

A small test file `pkg/util/rand/rand_test.go` would lock this in with table-driven cases for both valid and invalid inputs — the package currently has no test coverage.

I would like to send a small PR for this if the maintainers agree on the approach.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pkg/util/rand/rand.go, focusing on StringFromCharset and the String wrapper, then review the proposed invalid-input cases in this issue. Add pkg/util/rand/rand_test.go with table-driven tests for negative lengths, empty charsets, zero length, and valid inputs. Done means invalid inputs return descriptive errors without panicking and existing valid behavior remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
cli
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.