Skip to content

Commit ef82e69

Browse files
mpgempge
committed
Remaining fixes from analysis + Go tests + auth token wiring
PHP fixes: - MakeTaskCommand uses dynamic app namespace (not hardcoded App\Tasks) - GrpcDriver parseStatusCode defaults to 0 (error) not 200 - GovelJob logs payload keys only (not sensitive values) - GrpcDriver auth token support (config + Authorization header) - MakeWorkerCommand name validation test (10 invalid patterns) Go tests: - govel-server: isValidTaskName, isPathWithin, filterEnv - govel-sdk: Parse, Require - CI runs go test for server and SDK Test totals: 53 PHP tests, 12 Go tests
1 parent 4b0fc6f commit ef82e69

9 files changed

Lines changed: 282 additions & 6 deletions

File tree

‎.github/workflows/tests.yml‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,11 @@ jobs:
5050
with:
5151
go-version: '1.21'
5252

53-
- name: Vet and build govel-server
53+
- name: Vet, test, and build govel-server
5454
run: |
5555
cd bin/workers/govel-server
5656
go vet ./...
57+
go test ./...
5758
go build -o /dev/null .
5859
5960
- name: Vet and build process-image
@@ -62,7 +63,8 @@ jobs:
6263
go vet ./...
6364
go build -o /dev/null .
6465
65-
- name: Vet govel-sdk
66+
- name: Vet and test govel-sdk
6667
run: |
6768
cd bin/workers/govel-sdk
6869
go vet ./...
70+
go test ./...

‎bin/workers/govel-sdk/govel_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package govel
2+
3+
import (
4+
"encoding/json"
5+
"os"
6+
"os/exec"
7+
"testing"
8+
)
9+
10+
func TestParse(t *testing.T) {
11+
type sample struct {
12+
Name string `json:"name"`
13+
Count int `json:"count"`
14+
}
15+
16+
t.Run("valid JSON", func(t *testing.T) {
17+
payload := json.RawMessage(`{"name":"alice","count":42}`)
18+
result, err := Parse[sample](payload)
19+
if err != nil {
20+
t.Fatalf("unexpected error: %v", err)
21+
}
22+
if result.Name != "alice" {
23+
t.Errorf("expected name alice, got %q", result.Name)
24+
}
25+
if result.Count != 42 {
26+
t.Errorf("expected count 42, got %d", result.Count)
27+
}
28+
})
29+
30+
t.Run("invalid JSON returns error", func(t *testing.T) {
31+
payload := json.RawMessage(`{not valid json}`)
32+
_, err := Parse[sample](payload)
33+
if err == nil {
34+
t.Fatal("expected error for invalid JSON, got nil")
35+
}
36+
})
37+
38+
t.Run("empty payload returns error", func(t *testing.T) {
39+
payload := json.RawMessage(``)
40+
_, err := Parse[sample](payload)
41+
if err == nil {
42+
t.Fatal("expected error for empty payload, got nil")
43+
}
44+
})
45+
}
46+
47+
// TestRequire_True verifies that Require with a true condition does not exit.
48+
func TestRequire_True(t *testing.T) {
49+
// This should simply not panic or exit.
50+
Require(true, "this should not trigger")
51+
}
52+
53+
// TestRequire_False uses a subprocess test pattern to verify that
54+
// Require(false, ...) calls os.Exit(1).
55+
func TestRequire_False(t *testing.T) {
56+
if os.Getenv("TEST_REQUIRE_FALSE") == "1" {
57+
Require(false, "condition failed")
58+
return
59+
}
60+
61+
cmd := exec.Command(os.Args[0], "-test.run=^TestRequire_False$")
62+
cmd.Env = append(os.Environ(), "TEST_REQUIRE_FALSE=1")
63+
err := cmd.Run()
64+
65+
if err == nil {
66+
t.Fatal("expected process to exit with non-zero status")
67+
}
68+
69+
exitErr, ok := err.(*exec.ExitError)
70+
if !ok {
71+
t.Fatalf("expected *exec.ExitError, got %T: %v", err, err)
72+
}
73+
if exitErr.ExitCode() != 1 {
74+
t.Errorf("expected exit code 1, got %d", exitErr.ExitCode())
75+
}
76+
}

‎bin/workers/govel-server/main_test.go‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
package main
2+
3+
import (
4+
"strings"
5+
"testing"
6+
)
7+
8+
func TestIsValidTaskName(t *testing.T) {
9+
t.Run("valid names", func(t *testing.T) {
10+
valid := []string{
11+
"process-image",
12+
"my_task",
13+
"abc123",
14+
"A",
15+
"z",
16+
"TaskName",
17+
"a-b_c-d",
18+
strings.Repeat("a", 128),
19+
}
20+
for _, name := range valid {
21+
if !isValidTaskName(name) {
22+
t.Errorf("expected %q to be valid", name)
23+
}
24+
}
25+
})
26+
27+
t.Run("invalid names", func(t *testing.T) {
28+
invalid := []string{
29+
"../etc/passwd",
30+
"foo/bar",
31+
"bad.name",
32+
"has.dot",
33+
"",
34+
strings.Repeat("a", 129),
35+
"has space",
36+
"special!char",
37+
"semi;colon",
38+
"back\\slash",
39+
"at@sign",
40+
"hash#tag",
41+
}
42+
for _, name := range invalid {
43+
if isValidTaskName(name) {
44+
t.Errorf("expected %q to be invalid", name)
45+
}
46+
}
47+
})
48+
}
49+
50+
func TestIsPathWithin(t *testing.T) {
51+
t.Run("path inside base", func(t *testing.T) {
52+
cases := []struct {
53+
path, base string
54+
}{
55+
{"/opt/bin/task", "/opt/bin"},
56+
{"/opt/bin/sub/task", "/opt/bin"},
57+
{"/opt/bin", "/opt/bin"},
58+
}
59+
for _, tc := range cases {
60+
if !isPathWithin(tc.path, tc.base) {
61+
t.Errorf("expected isPathWithin(%q, %q) = true", tc.path, tc.base)
62+
}
63+
}
64+
})
65+
66+
t.Run("path outside base", func(t *testing.T) {
67+
cases := []struct {
68+
path, base string
69+
}{
70+
{"/opt/bin/../etc/passwd", "/opt/bin"},
71+
{"/opt/other/task", "/opt/bin"},
72+
{"/opt/bi", "/opt/bin"},
73+
}
74+
for _, tc := range cases {
75+
if isPathWithin(tc.path, tc.base) {
76+
t.Errorf("expected isPathWithin(%q, %q) = false", tc.path, tc.base)
77+
}
78+
}
79+
})
80+
}
81+
82+
func TestFilterEnv(t *testing.T) {
83+
t.Run("allows permitted vars", func(t *testing.T) {
84+
input := []string{
85+
"PATH=/usr/bin",
86+
"HOME=/home/user",
87+
"LANG=en_US.UTF-8",
88+
"USER=testuser",
89+
"GOPATH=/go",
90+
"TMPDIR=/tmp",
91+
"TEMP=/tmp",
92+
"TMP=/tmp",
93+
"LC_ALL=C",
94+
}
95+
result := filterEnv(input)
96+
if len(result) != len(input) {
97+
t.Errorf("expected %d vars, got %d", len(input), len(result))
98+
}
99+
for i, v := range result {
100+
if v != input[i] {
101+
t.Errorf("expected %q, got %q", input[i], v)
102+
}
103+
}
104+
})
105+
106+
t.Run("filters sensitive vars", func(t *testing.T) {
107+
input := []string{
108+
"PATH=/usr/bin",
109+
"AWS_SECRET_ACCESS_KEY=supersecret",
110+
"DATABASE_URL=postgres://...",
111+
"GOVEL_AUTH_TOKEN=mytoken",
112+
"API_KEY=abc123",
113+
"HOME=/home/user",
114+
}
115+
result := filterEnv(input)
116+
if len(result) != 2 {
117+
t.Errorf("expected 2 vars, got %d: %v", len(result), result)
118+
}
119+
expected := map[string]bool{
120+
"PATH=/usr/bin": true,
121+
"HOME=/home/user": true,
122+
}
123+
for _, v := range result {
124+
if !expected[v] {
125+
t.Errorf("unexpected env var in output: %q", v)
126+
}
127+
}
128+
})
129+
130+
t.Run("empty input", func(t *testing.T) {
131+
result := filterEnv([]string{})
132+
if len(result) != 0 {
133+
t.Errorf("expected empty result, got %v", result)
134+
}
135+
})
136+
137+
t.Run("malformed entries ignored", func(t *testing.T) {
138+
input := []string{
139+
"NOEQUALS",
140+
"PATH=/usr/bin",
141+
}
142+
result := filterEnv(input)
143+
if len(result) != 1 {
144+
t.Errorf("expected 1 var, got %d: %v", len(result), result)
145+
}
146+
})
147+
}

‎config/govel.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,12 @@
114114
| Delay in milliseconds between retry attempts.
115115
*/
116116
'retry_delay' => (int) env('GOVEL_GRPC_RETRY_DELAY', 100),
117+
118+
/*
119+
| Optional authentication token sent as a Bearer token
120+
| in the Authorization header to the gRPC server.
121+
*/
122+
'auth_token' => env('GOVEL_AUTH_TOKEN'),
117123
],
118124

119125
/*

‎src/Console/MakeTaskCommand.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,12 @@ public function handle(): int
3939

4040
protected function buildStub(string $name, string $kebab): string
4141
{
42+
$rootNamespace = rtrim($this->laravel->getNamespace(), '\\');
43+
4244
return <<<PHP
4345
<?php
4446
45-
namespace App\Tasks;
47+
namespace {$rootNamespace}\Tasks;
4648
4749
use Mpge\Govel\Contracts\Task;
4850

‎src/Drivers/GrpcDriver.php‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ public function __construct(
2727
protected int $retries = 0,
2828
protected int $retryDelay = 100,
2929
protected int $maxPayloadSize = 0,
30+
protected ?string $authToken = null,
3031
) {}
3132

3233
public function run(Task $task, array $payload = []): Result
@@ -90,10 +91,16 @@ protected function request(string $taskName, array $payload, bool $async = false
9091
);
9192
}
9293

94+
$headers = "Content-Type: application/json\r\nAccept: application/json\r\n";
95+
96+
if ($this->authToken !== null && $this->authToken !== '') {
97+
$headers .= "Authorization: Bearer {$this->authToken}\r\n";
98+
}
99+
93100
$context = stream_context_create([
94101
'http' => [
95102
'method' => 'POST',
96-
'header' => "Content-Type: application/json\r\nAccept: application/json\r\n",
103+
'header' => $headers,
97104
'content' => $body,
98105
'timeout' => max($this->connectTimeout, $this->timeout),
99106
'ignore_errors' => true,
@@ -115,6 +122,14 @@ protected function request(string $taskName, array $payload, bool $async = false
115122

116123
$statusCode = $this->parseStatusCode($http_response_header ?? []);
117124

125+
if ($statusCode === 0) {
126+
throw TaskExecutionException::processError(
127+
$taskName,
128+
'Unable to determine HTTP status from response',
129+
0,
130+
);
131+
}
132+
118133
if ($statusCode >= 400) {
119134
$decoded = json_decode($response, true);
120135
$error = $decoded['error'] ?? "HTTP {$statusCode}";
@@ -137,7 +152,7 @@ protected function parseStatusCode(array $headers): int
137152
}
138153
}
139154

140-
return 200;
155+
return 0;
141156
}
142157

143158
}

‎src/Queue/GovelJob.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public function handle(GoManager $manager): void
3636
if (! $result->success) {
3737
Log::error("Govel queued task [{$this->taskClass}] failed: {$result->error}", [
3838
'task' => $this->taskClass,
39-
'payload' => $this->payload,
39+
'payload_keys' => array_keys($this->payload),
4040
'duration' => $result->duration,
4141
]);
4242

‎src/Services/GoManager.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ protected function createGrpcDriver(): GrpcDriver
138138
retries: (int) ($config['retries'] ?? 0),
139139
retryDelay: (int) ($config['retry_delay'] ?? 100),
140140
maxPayloadSize: (int) ($this->container['config']['govel.max_payload_size'] ?? 0),
141+
authToken: $config['auth_token'] ?? null,
141142
);
142143
}
143144

‎tests/Unit/Console/MakeWorkerCommandTest.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,31 @@ public function it_has_correct_name(): void
2323

2424
$this->assertSame('govel:make-worker', $command->getName());
2525
}
26+
27+
#[Test]
28+
public function it_rejects_invalid_names_via_regex(): void
29+
{
30+
$pattern = '/^[a-z0-9][a-z0-9_-]*$/';
31+
32+
$invalidNames = [
33+
'../../etc',
34+
'../passwd',
35+
'',
36+
'UPPERCASE',
37+
'-starts-with-dash',
38+
'_starts-with-underscore',
39+
'has spaces',
40+
'has/slash',
41+
'has\\backslash',
42+
'.dotfile',
43+
];
44+
45+
foreach ($invalidNames as $name) {
46+
$this->assertDoesNotMatchRegularExpression(
47+
$pattern,
48+
$name,
49+
"Name '{$name}' should be rejected by the validation regex."
50+
);
51+
}
52+
}
2653
}

0 commit comments

Comments
 (0)