feat(slack): improve app setup and connection management - #272
Conversation
lidge-jun
left a comment
lidge-jun
left a comment
There was a problem hiding this comment.
Reviewed at head c09ad246 against base 92a53506. Targeted Slack tests pass 65/65, backend and frontend typechecks pass, locale key parity holds across all four locales, and verify-counts.sh reports 363 matching entries.
Blocking
The connection reset reports success it cannot deliver, for env-configured installs.
public/js/features/settings-slack.ts clears the persisted Slack settings, and the UI then shows the connection as gone. But applyEnvOverrides() re-applies the environment on every settings load:
// src/core/config.ts:697
if (process.env["SLACK_BOT_TOKEN"]) {
s["slack"] = s["slack"] || {};
s["slack"].botToken = process.env["SLACK_BOT_TOKEN"];
s["slack"].enabled = true; // ← the disable is undone
}That runs from the load path at src/core/config.ts:934. So on any install that injects SLACK_BOT_TOKEN, the sequence is: reset → UI says disconnected → restart → connected again, with the same token.
For a security-shaped action this is worse than an ordinary bug: the user believes the connection is severed and acts on that belief.
Suggested shape: make the reset server-owned so it can detect credential provenance, and either refuse with 409 naming the env vars to remove, or persist an explicit disable override that survives applyEnvOverrides. Either way it needs a test that restarts with the env var set and asserts Slack stays off.
Worth fixing before merge
App display name and bot handle are conflated — src/slack/manifest.ts:17-35
normalizedSlackAppName() enforces /^[a-z0-9._-]+$/ and the result feeds both display_information.name and features.bot_user.display_name. Slack only constrains the bot handle that way; the app display name accepts up to 35 characters of ordinary text. As written, My App and any non-Latin name are rejected. Worth splitting the two, deriving the handle or asking for it separately.
The help text points at UI this PR removed — public/locales/{en,ja,ko,zh}.json
The guidance still tells users to press "Copy manifest" in the guide card, but public/index.html deletes that card and button. Either repoint it at the setup wizard's Generate manifest step or restore the control. The now-orphaned settings.slack.guide.step1Desc and copyManifest* keys can go at the same time.
Reset does not read back authoritative state — settings-slack.ts:48-64
If the server commits the change but the response is lost, the UI reports failure while the credentials are in fact gone. After a confirmed success the transport health row also stays stale. A settings/health read-back after every attempt would resolve both.
The reset test cannot fail for the right reason — tests/unit/slack-connection-reset.test.ts
146 lines, but fetch is mocked, so it exercises frontend branching only — not the real route, the auth and Origin middleware, persistence, transport restart, or the env override. The blocking issue above sits squarely in the gap this test leaves. An API-level test against the actual route would cover it.
channel-onboarding-surface.test.ts:102-145 asserts mostly on source strings and regexes, so it can stay green while click, clipboard, or rerender behavior breaks. One JSDOM interaction test would be worth more than all of it.
Checked and clean
PUT /api/settingsis behindrequireAuth(src/routes/settings.ts:153); remote unauthenticated callers get 401.- Browser CSRF is covered by the global Host/Origin checks (
server.ts:244-270). - No token is logged or echoed back. No XSS: interpolated values are escaped and manifest status uses
textContent.
The loopback auth bypass (server.ts:293-304) means any local process can clear Slack settings, but that predates this PR.
On the CI status
Earlier this run showed no checks at all, which looked like a fork-approval gate. It has since executed: run 31381328166 completed with failure.
That failure is not attributable to this PR. Comparing the failing test names against the same workflow on base dev (31188392986):
devitself fails 41 tests in CI- this PR fails 41
- exactly one name differs:
slackApi timeout alone aborts a stalled request
That test passes 3/3 in isolation locally, so it is order-dependent flake, not a regression introduced here. The base branch is red on its own and needs separate attention; it should not be held against this PR.
Verdict
Blocking on the reset issue only. The manifest naming, locale text, and read-back items are worth folding in while you are here. Everything else looks sound.
|
@lidge-jun I’ve addressed the blocking environment-managed Slack reset behavior, removed the obsolete locale guidance, and added an authoritative settings read-back after reset attempts. I intentionally deferred separating the Slack app display name and bot handle because it would expand the onboarding UI and API contract beyond this PR’s scope. This can be handled in a follow-up issue if needed. Could you please re-review the PR? |
lidge-jun
left a comment
lidge-jun
left a comment
There was a problem hiding this comment.
Re-reviewed at head e978e5e against current dev c8d7c0e. The original env-configured reset false-success blocker is fixed, including server-owned 409/provenance, authoritative read-back, route-level coverage, and UI lock. A second data-loss edge found during re-review—partial SLACK_* ownership clearing unrelated file credentials on boot and watcher reload—is now fixed with RED-to-GREEN regressions and field-specific persistence/API/watcher ownership. Independent Sol-medium re-audit: PASS. Local focused Slack/settings tests, backend/frontend typechecks, backend/frontend builds, dist verification, git diff check, and verify-counts 414/414 pass. All four platform install checks pass. node-tests remains red only on the existing dev baseline: 35 unique failure names on dev run 31492864315 and PR run 31494059766, with an empty diff. Follow-ups #287 and #291 remain intentionally separate. Approved.
…291) The manifest generator validated one input with the bot-handle character class and wrote that same value to both display_information.name and features.bot_user.display_name. Slack documents these differently: the app name allows up to 35 characters with no documented character-set restriction — their own example is "The Very Fantastic Name of Your App" — while the bot display name allows 80 characters from a-z0-9._- only. Applying the narrower rule to both meant a spaced or non-Latin app name was rejected outright. The app name is now validated on length alone, and the bot handle is derived from it: lowercase, replace runs of unsupported characters with a single hyphen, collapse repeats, trim edge separators. Legacy lowercase slugs take a fast path and produce byte-identical output to before, so existing setups are unaffected. A name with no usable ASCII falls back to the cli-jaw handle. That is a real product decision rather than an implementation detail — an app named 데모 gets a bot handle named cli-jaw — so it is no longer silent. The route returns the derived handle as an additive botDisplayName field, onboarding shows it when it differs from the app name, and all four locale hints say the fallback exists. The browser reads that value rather than re-deriving it, because the same rule living in two places is what produced this issue. No truncation branch. The bound is not "derivation only shortens" — that is false, since toLowerCase can expand: 35 copies of U+0130 lowercase to 70 code points and derive a 69-character handle. The real argument is that 35 code points expand to at most 70, which stays under 80. That worst case is pinned as a regression test so a future change to the derivation cannot quietly breach it. The two tests that asserted the old coupling — one requiring both fields to match, one requiring 'demo app', 'Demo' and '데모' to be rejected — were the defect written down, so they are rewritten rather than extended. structure/server_api.md documented this route as returning { yaml }, which has been stale since PR #272 added json. The row now reads { yaml, json, botDisplayName }. Verified against the compiled build: legacy slugs unchanged, Demo -> demo, demo app -> demo-app, 데모 and !!! -> cli-jaw, Slack's own example name accepted. No new test failures; tsc, build, frontend build and verify-counts all clean.
2 participants
Summary
Verification
npm run typecheck:frontendnpm run build:frontend