feat(googlechat): Google Chat channel integration#148
feat(googlechat): Google Chat channel integration#148tuntran wants to merge 3 commits intonextlevelbuilder:mainfrom
Conversation
viettranx
left a comment
There was a problem hiding this comment.
Code Review: Google Chat Channel Integration
Risk Level: Medium | Verdict: Request Changes
Overall the implementation follows existing channel patterns well. However, there are 2 critical security issues and several important items to address before merge.
Critical (must fix before merge)
C1. UI default policy is "open" instead of "pairing"
ui/web/src/pages/channels/channel-schemas.ts — The GoogleChat schema uses inline policy options with default "open" for both group_policy and dm_policy. All other channels use the shared groupPolicyOptions/dmPolicyOptions with default "pairing". This is a security regression — anyone in the Google Workspace can talk to the bot without approval.
Fix: Use the shared groupPolicyOptions/dmPolicyOptions arrays and set default to "pairing", consistent with all other channels.
C2. Config-based path missing default GroupPolicy "pairing"
cmd/gateway_channels_setup.go — The GoogleChat config registration doesn't set a default GroupPolicy: "pairing". When a user leaves the field empty, bot.go will default to "open".
Fix: Add GroupPolicy: "pairing" default in the config-based registration path, matching what Discord/Feishu/Slack do.
Important (should fix)
H1. Dedup goroutine leak potential
channel.go — Each incoming message spawns a new goroutine with go func() { time.Sleep(5*time.Minute); cache.Delete(key) }() for dedup cleanup. Under high traffic this can accumulate thousands of sleeping goroutines.
Fix: Use time.AfterFunc(5*time.Minute, func() { cache.Delete(key) }) instead — it uses the runtime timer heap without blocking a goroutine.
H2. ServiceAccountFile allows arbitrary file reads
credentials.go — os.ReadFile(path) with no path validation. This is a potential path traversal vector. Since ServiceAccountJSON inline already works, consider removing the file-based path entirely, or at minimum validate the path is within an expected directory.
H3. Unsafe type assertion
channel.go — val.(*reactionState) will panic if the sync.Map contains a value of unexpected type. Use the comma-ok pattern:
rs, ok := val.(*reactionState)
if !ok {
return
}Suggestions
M1. No test coverage — There are zero unit tests for this channel. Other channels like Slack have tests. At minimum, consider tests for extractSenderInfo, isBotMentioned, resolveServiceAccountJSON, and the message chunking logic.
M2. ReactionLevel not validated — Invalid values fall through silently. Consider logging a warning for unrecognized levels.
M3. io.ReadAll error ignored in api.go — The error from reading the response body should be checked.
M4. Trailing blank line at end of channel.go — minor cleanup.
Nice work on the overall structure — the modular split (api/bot/channel/handler/factory/types/credentials) is clean and follows the project's file-size guidelines well. Just need to nail the security defaults and the items above before merging. 👍
- Security: default dm_policy and group_policy to "pairing" instead of "open" in both UI schema and bot runtime fallback - Replace goroutine+sleep dedup cleanup with time.AfterFunc to prevent goroutine leak - Remove ServiceAccountFile path support, only keep inline JSON - Use comma-ok type assertion in removeReaction to prevent panic - Check io.ReadAll error in API client instead of discarding - Warn on unrecognized reaction_level value - Remove trailing blank line in channel.go - Add unit tests for credentials, handler helpers, and dedup logic
|
All review feedback addressed in c29b075: Critical
Important
Suggestions
Ready for re-review. |
…ad registration Channels registered after Manager.Reload() were not getting webhook routes mounted, causing 404 errors for their webhook handlers. Fixed by delegating webhook requests through Manager's webhookMap instead of mounting static routes at startup. - Add webhookMap tracking active webhook paths and their Channel handlers - Add WebhookServeHTTP() to dynamically route webhook requests to registered channels - Add MountNewWebhookRoutes() to mount handlers for newly registered channels - Update Reload() to trigger webhook route mounting for new channels - Add comprehensive unit tests for webhook mounting and delegation Fixes: Google Chat Add-on returning 404 on /googlechat/events after channel reload
019d018 to
6c3cec0
Compare
Summary
google_chatto valid channel types allowlistConvention Fixes (vs PR #135)
"pairing"for DB instances (security parity with Discord/Feishu/Slack)channels.TypeGoogleChatconstant ("google_chat")project_number)resolveServiceAccountJSONtocredentials.go(channel.go under 200 lines)Changes
internal/channels/googlechat/— full channel implementation (webhook handler, bot API, factory, types, credentials)internal/config/config_channels.go— addGoogleChatConfigstruct withservice_account_jsonfieldinternal/channels/channel.go— addTypeGoogleChatconstantcmd/gateway.go— register GoogleChat factory in instance loadercmd/gateway_channels_setup.go— add GoogleChat to config-based channel registrationui/web/src/pages/channels/— UI fields for GoogleChat channel setupinternal/config/config_secrets.go— mark SA JSON as sensitive