fix(canvas,server): address review findings on 3f11df03
Five review findings from the 3f11df03 six-bug commit:
1. Add TestPeers_DevModeFailOpen_{Allows,ClosedWhenAdminTokenSet,
ClosedInProduction} covering all three gating states for the
security-sensitive dev-mode hatch the prior commit added to
/registry/:id/peers. Previously shipped untested — a future
refactor could have silently inverted polarity or removed the
gate. New tests pin the contract:
* MOLECULE_ENV=development + ADMIN_TOKEN="" → allow bearerless
* MOLECULE_ENV=development + ADMIN_TOKEN set → require token
* MOLECULE_ENV=production → require token
2. ConfigTab handleSave diffs against the RAW parsed YAML / form
config instead of the DEFAULT_CONFIG-merged shape. The previous
code would silently PATCH tier=1 to the DB when a user deleted
the `tier:` line in raw mode (the default-merge substituted 1).
Now: only fields the user actually typed participate in the
diff. Type guards (typeof === "number" / "string") prevent
coercion surprises on malformed YAML.
3. ConfigTab model-save failure no longer lies "Saved". The
/workspaces/:id/model PATCH can reject when the runtime doesn't
support the chosen model; previously we caught + console.warn'd
+ showed green Saved, and the user watched the model revert on
next reload with no explanation. Now the save path collects a
`modelSaveError` and surfaces it via setError with a partial-
success message ("Other fields saved, but model update failed:
…") so the user sees why.
4. ChannelsTab now surfaces BOTH channels-fetch and adapters-fetch
failures, distinguishing them in the error text ("Failed to
load connected channels and platforms — try refreshing").
Previously only an adapters failure was visible; a channels
failure left the user with an apparently-empty list and no
indication the API was unreachable.
5. ChatTab panels drop the redundant aria-hidden attribute. The
`hidden`/`flex` Tailwind class already sets display:none, which
removes the node from the accessibility tree on its own; the
extra aria-hidden invited WAI-ARIA lint warnings if a focusable
descendant ever landed inside an inactive panel.
Tests: 923 canvas + full Go handler suite pass. 3 new Go tests.
No behaviour change on the five prior fixes — this commit tightens
their edges per the independent review.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang
parent
3f11df031c
commit
507696d88a
@ -69,16 +69,25 @@ export function ChannelsTab({ workspaceId }: Props) {
|
||||
api.get<Channel[]>(`/workspaces/${workspaceId}/channels`),
|
||||
api.get<ChannelAdapter[]>(`/channels/adapters`),
|
||||
]);
|
||||
const errors: string[] = [];
|
||||
if (chResult.status === "fulfilled") {
|
||||
setChannels(Array.isArray(chResult.value) ? chResult.value : []);
|
||||
} else {
|
||||
console.warn("ChannelsTab: channels load failed", chResult.reason);
|
||||
errors.push("connected channels");
|
||||
}
|
||||
if (adResult.status === "fulfilled") {
|
||||
setAdapters(Array.isArray(adResult.value) ? adResult.value : []);
|
||||
} else {
|
||||
console.warn("ChannelsTab: adapters load failed", adResult.reason);
|
||||
setError("Failed to load channel platforms — try refreshing");
|
||||
errors.push("platforms");
|
||||
}
|
||||
// Surface BOTH failure modes so the user can distinguish
|
||||
// "no channels configured" from "API unreachable".
|
||||
if (errors.length > 0) {
|
||||
setError(`Failed to load ${errors.join(" and ")} — try refreshing`);
|
||||
} else {
|
||||
setError("");
|
||||
}
|
||||
setLoading(false);
|
||||
}, [workspaceId]);
|
||||
|
||||
@ -152,7 +152,6 @@ export function ChatTab({ workspaceId, data }: Props) {
|
||||
id="chat-panel-my-chat"
|
||||
role="tabpanel"
|
||||
aria-labelledby="chat-tab-my-chat"
|
||||
aria-hidden={subTab !== "my-chat"}
|
||||
className={`flex-1 overflow-hidden flex-col ${
|
||||
subTab === "my-chat" ? "flex" : "hidden"
|
||||
}`}
|
||||
@ -163,7 +162,6 @@ export function ChatTab({ workspaceId, data }: Props) {
|
||||
id="chat-panel-agent-comms"
|
||||
role="tabpanel"
|
||||
aria-labelledby="chat-tab-agent-comms"
|
||||
aria-hidden={subTab !== "agent-comms"}
|
||||
className={`flex-1 overflow-hidden flex-col ${
|
||||
subTab === "agent-comms" ? "flex" : "hidden"
|
||||
}`}
|
||||
|
||||
@ -257,21 +257,26 @@ export function ConfigTab({ workspaceId }: Props) {
|
||||
// the form, hits Save, sees the request succeed, then watches the
|
||||
// values snap back on the next reload because the workspace row
|
||||
// never heard about the change.
|
||||
//
|
||||
// Diff against the RAW parsed YAML (or the form `config` in non-
|
||||
// raw mode) rather than the DEFAULT_CONFIG-merged shape — if the
|
||||
// user deleted a field in raw mode the merge would substitute the
|
||||
// default (e.g. tier=1) and we'd silently PATCH that down from
|
||||
// the stored value. Only fields the user actually typed get sent.
|
||||
const oldParsed = parseYaml(originalYaml);
|
||||
const nextParsed = rawMode ? parseYaml(rawDraft) : null;
|
||||
const effective = nextParsed
|
||||
? { ...DEFAULT_CONFIG, ...nextParsed } as ConfigData
|
||||
: config;
|
||||
const nextSource = rawMode
|
||||
? (parseYaml(rawDraft) as Record<string, unknown>)
|
||||
: (config as unknown as Record<string, unknown>);
|
||||
const dbPatch: Record<string, unknown> = {};
|
||||
if (effective.name && effective.name !== oldParsed.name) {
|
||||
dbPatch.name = effective.name;
|
||||
if (typeof nextSource.name === "string" && nextSource.name && nextSource.name !== oldParsed.name) {
|
||||
dbPatch.name = nextSource.name;
|
||||
}
|
||||
if (effective.tier && effective.tier !== (oldParsed.tier ?? null)) {
|
||||
dbPatch.tier = effective.tier;
|
||||
if (typeof nextSource.tier === "number" && nextSource.tier !== (oldParsed.tier ?? null)) {
|
||||
dbPatch.tier = nextSource.tier;
|
||||
}
|
||||
const oldRuntime = (oldParsed.runtime as string) || "";
|
||||
if (effective.runtime && effective.runtime !== oldRuntime) {
|
||||
dbPatch.runtime = effective.runtime;
|
||||
if (typeof nextSource.runtime === "string" && nextSource.runtime && nextSource.runtime !== oldRuntime) {
|
||||
dbPatch.runtime = nextSource.runtime;
|
||||
}
|
||||
if (Object.keys(dbPatch).length > 0) {
|
||||
await api.patch(`/workspaces/${workspaceId}`, dbPatch);
|
||||
@ -279,14 +284,21 @@ export function ConfigTab({ workspaceId }: Props) {
|
||||
|
||||
// Model has its own endpoint (separate from the general workspace
|
||||
// PATCH) because the runtime may need to validate it against the
|
||||
// template's supported models list.
|
||||
// template's supported models list. A model rejection is a
|
||||
// partial-save state — we report it as a user-visible warning
|
||||
// rather than lying "Saved" and letting the user discover the
|
||||
// revert on next reload.
|
||||
const oldModel = (oldParsed.model as string) || "";
|
||||
if (effective.model && effective.model !== oldModel) {
|
||||
let modelSaveError: string | null = null;
|
||||
if (
|
||||
typeof nextSource.model === "string" &&
|
||||
nextSource.model &&
|
||||
nextSource.model !== oldModel
|
||||
) {
|
||||
try {
|
||||
await api.put(`/workspaces/${workspaceId}/model`, { model: effective.model });
|
||||
await api.put(`/workspaces/${workspaceId}/model`, { model: nextSource.model });
|
||||
} catch (e) {
|
||||
// Non-fatal — log and continue so the rest of the save commits.
|
||||
console.warn("ConfigTab: model PATCH failed", e);
|
||||
modelSaveError = e instanceof Error ? e.message : "Model update was rejected";
|
||||
}
|
||||
}
|
||||
|
||||
@ -302,9 +314,16 @@ export function ConfigTab({ workspaceId }: Props) {
|
||||
} else {
|
||||
useCanvasStore.getState().updateNodeData(workspaceId, { needsRestart: true });
|
||||
}
|
||||
setSuccess(true);
|
||||
clearTimeout(successTimerRef.current);
|
||||
successTimerRef.current = setTimeout(() => setSuccess(false), 2000);
|
||||
if (modelSaveError) {
|
||||
// Partial-save UX: surface the model rejection instead of
|
||||
// showing "Saved" — the user would otherwise watch the model
|
||||
// field revert on next reload with no explanation.
|
||||
setError(`Other fields saved, but model update failed: ${modelSaveError}`);
|
||||
} else {
|
||||
setSuccess(true);
|
||||
clearTimeout(successTimerRef.current);
|
||||
successTimerRef.current = setTimeout(() => setSuccess(false), 2000);
|
||||
}
|
||||
} catch (e) {
|
||||
setError(e instanceof Error ? e.message : "Failed to save");
|
||||
} finally {
|
||||
|
||||
@ -618,3 +618,109 @@ func TestDiscoverHostPeer_Smoke_Success(t *testing.T) {
|
||||
t.Errorf("expected 200, got %d", w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
// ==================== Peers auth — dev-mode fail-open gate ====================
|
||||
//
|
||||
// validateDiscoveryCaller applies a Tier-1b dev-mode hatch so the canvas
|
||||
// user session (which holds no workspace-scoped bearer) can still load
|
||||
// the Details → PEERS list on a local Docker setup. The gate must pass
|
||||
// ONLY when MOLECULE_ENV is development AND ADMIN_TOKEN is empty.
|
||||
// These tests pin that contract against accidental polarity flips.
|
||||
|
||||
// peersAuthFixtureHasLiveToken seeds the mock rows required for the
|
||||
// Peers handler to reach the auth branch: HasAnyLiveToken → true (a
|
||||
// non-zero count so validateDiscoveryCaller has to make the dev-mode
|
||||
// decision instead of grandfathering the request).
|
||||
func peersAuthFixtureHasLiveToken(mock sqlmock.Sqlmock, workspaceID string) {
|
||||
// HasAnyLiveToken issues `SELECT COUNT(*) FROM workspace_auth_tokens ...`
|
||||
mock.ExpectQuery("SELECT COUNT.+workspace_auth_tokens").
|
||||
WithArgs(workspaceID).
|
||||
WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1))
|
||||
}
|
||||
|
||||
func TestPeers_DevModeFailOpen_AllowsBearerlessRequest(t *testing.T) {
|
||||
// Dev mode: MOLECULE_ENV=development AND ADMIN_TOKEN empty. Canvas
|
||||
// sends no bearer token; validateDiscoveryCaller must return nil
|
||||
// (allow) and the handler must proceed to return the peer list.
|
||||
t.Setenv("MOLECULE_ENV", "development")
|
||||
t.Setenv("ADMIN_TOKEN", "")
|
||||
|
||||
mock := setupTestDB(t)
|
||||
setupTestRedis(t)
|
||||
handler := NewDiscoveryHandler()
|
||||
|
||||
peersAuthFixtureHasLiveToken(mock, "ws-dev")
|
||||
|
||||
// Root workspace → children+parent queries still fire but the
|
||||
// parent_id lookup comes first.
|
||||
mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id =").
|
||||
WithArgs("ws-dev").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow(nil))
|
||||
peerCols := []string{"id", "name", "role", "tier", "status", "agent_card", "url", "parent_id", "active_tasks"}
|
||||
mock.ExpectQuery("SELECT w.id.+WHERE w.parent_id IS NULL AND w.id").
|
||||
WithArgs("ws-dev").
|
||||
WillReturnRows(sqlmock.NewRows(peerCols))
|
||||
mock.ExpectQuery("SELECT w.id.+WHERE w.parent_id = \\$1 AND w.status").
|
||||
WithArgs("ws-dev").
|
||||
WillReturnRows(sqlmock.NewRows(peerCols))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Params = gin.Params{{Key: "id", Value: "ws-dev"}}
|
||||
c.Request = httptest.NewRequest("GET", "/registry/ws-dev/peers", nil)
|
||||
|
||||
handler.Peers(c)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("expected 200 under dev-mode hatch, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestPeers_DevModeFailOpen_ClosedWhenAdminTokenSet(t *testing.T) {
|
||||
// An operator with ADMIN_TOKEN set has explicitly opted into #684
|
||||
// closure; dev-mode hatch must NOT open even when MOLECULE_ENV is
|
||||
// "development". This is the SaaS guarantee.
|
||||
t.Setenv("MOLECULE_ENV", "development")
|
||||
t.Setenv("ADMIN_TOKEN", "seven-admin-token")
|
||||
|
||||
mock := setupTestDB(t)
|
||||
setupTestRedis(t)
|
||||
handler := NewDiscoveryHandler()
|
||||
|
||||
peersAuthFixtureHasLiveToken(mock, "ws-prod")
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Params = gin.Params{{Key: "id", Value: "ws-prod"}}
|
||||
c.Request = httptest.NewRequest("GET", "/registry/ws-prod/peers", nil)
|
||||
|
||||
handler.Peers(c)
|
||||
|
||||
if w.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("expected 401 with ADMIN_TOKEN set, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestPeers_DevModeFailOpen_ClosedInProduction(t *testing.T) {
|
||||
// Production MOLECULE_ENV — hatch must stay closed regardless of
|
||||
// ADMIN_TOKEN state. SaaS production rejects the bearerless call.
|
||||
t.Setenv("MOLECULE_ENV", "production")
|
||||
t.Setenv("ADMIN_TOKEN", "")
|
||||
|
||||
mock := setupTestDB(t)
|
||||
setupTestRedis(t)
|
||||
handler := NewDiscoveryHandler()
|
||||
|
||||
peersAuthFixtureHasLiveToken(mock, "ws-prod")
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Params = gin.Params{{Key: "id", Value: "ws-prod"}}
|
||||
c.Request = httptest.NewRequest("GET", "/registry/ws-prod/peers", nil)
|
||||
|
||||
handler.Peers(c)
|
||||
|
||||
if w.Code != http.StatusUnauthorized {
|
||||
t.Fatalf("expected 401 in production, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user