chore(sync): merge upstream main back into dev - #21
Merged
Merged
Conversation
…cution Uploaded assets are served from the application's own origin, so the extension stored in the key decides how a browser interprets the bytes. Nothing validated it: /api/message/upload_attachment accepted any file from an authenticated guest and the storage layer wrote whatever extension the client named, so a planted .html file ran against the dashboard origin the moment a staff member opened the attachment link. - Reject browser-active extensions and payloads (HTML, SVG, XML/XSL, scripts, legacy server-side pages) in AssetService.UploadFile and UploadBytes, and sniff the leading bytes instead of trusting the client-declared Content-Type so renaming a page to .png does not smuggle it through - Reduce client filenames to a bare basename within the 255 character column, cut on a rune boundary, and strip control characters - Derive the stored extension from an explicit MIME table. mime.ExtensionsByType consults the Windows registry, where text/html resolved to .ehtml and escaped the block list, and it differs between dev machines and production - Fall back to an inert .bin when no safe extension can be determined, so the server never sniffs the payload to decide its own Content-Type - Serve local assets with X-Content-Type-Options nosniff and force a download for anything that is not previewable media, which also neutralises document types already sitting in an existing storage root - Add error.e0348 to both backend locales for the rejection
Adds Discord as a sixth channel type, following the same shape as the Telegram
and Zalo OA integrations: a client package, an inbound webhook service, an
outbound service drained through the channel message outbox, a third-party
handler, and the dashboard form fields.
Inbound
POST /api/third/discord/webhook[/:channel_id]
Accepts both a bare interaction/webhook body and one nested under "message".
Author, message id, channel id, guild id, attachments and embeds are all read
from either level, so the same endpoint works with the payload shapes Discord and
common bridges emit. Bot authors and empty messages are dropped. An attachment
becomes a readable "[filename] url" line and an embed falls back to its
description or title, so a customer who sends only an image or a link still
produces a message an agent can act on.
Messages map onto the existing conversation model through ExternalSourceDiscord,
so a Discord user gets the same identity resolution, assignment, handoff and
ticket behaviour as every other channel.
Outbound
Agent and AI replies are queued by EnqueueDiscordMessage and drained by cron
every five seconds, with the same immediate-dispatch goroutine, backoff and
retry ceiling as the other channels. Text and HTML go out as content; an image
message is sent as an embed with the asset's signed URL; an attachment appends
its signed URL to the text. The reply target is the discord_channel_id recorded
on the last inbound message, falling back to a DM channel created from the
customer's Discord user id, so a conversation that started in a server stays in
that server.
Guild scoping
GuildID and ChannelScope are enforced on the way in. A bot invited to several
servers answers only in the guild the channel names, and a channel set to
dm_only ignores guild messages. Without this the two fields are stored
configuration that silently does nothing, and an operator has no way to tell why
a bot is answering somewhere it should not.
Credentials
A channel may carry its own bot token. When it does not, the deployment-wide
token is used, resolved from discord.botToken in config.yaml or DISCORD_BOT_TOKEN
in the environment. New DiscordConfig section plus AGENT_DESK_DISCORD_* and
DISCORD_* bindings, documented in .env.example.
Security
The webhook secret is compared with crypto/subtle.ConstantTimeCompare. A
byte-wise != leaks how much of the prefix matched through response timing, which
matters here because the endpoint is unauthenticated by design. Verification is
skipped only when no secret is configured on the channel, matching how the
Telegram integration treats its webhook secret.
Tests
internal/discord/client_test.go client against an httptest stub
internal/handlers/third/discord_handler_test.go handler routing and response
internal/services/discord_inbound_service_test.go
inbound to conversation to outbox, guild scope matrix (matching guild,
other guild, DM under a guild-scoped channel, DM and guild message under
dm_only, unscoped channel), and rejection of a wrong webhook secret
internal/services/discord_integration_test.go full round trip
Not included
Discord's interactions endpoint signs requests with an Ed25519 signature over the
raw body, verified against the application public key. This integration ingests
through a shared-secret webhook instead, so the public key field is stored but
not yet used to verify anything. Adding Ed25519 verification, and the OAuth flow
that provisions a bot without a pasted token, are both worthwhile follow-ups.
Adds Slack as a sixth channel type, following the same shape as the Telegram and
Zalo OA integrations: a client package, an inbound Events API service, an
outbound service drained through the channel message outbox, a third-party
handler, and the dashboard form fields.
Inbound
POST /api/third/slack/webhook[/:channel_id]
Answers Slack's url_verification handshake before any channel lookup, because
Slack sends it once while the endpoint is being configured and does not retry.
event_callback messages map onto the existing conversation model through
ExternalSourceSlack. Bot authors, bot_message subtypes and empty text are
dropped so the bot cannot talk to itself in a loop. The channel is resolved by
explicit id, then by team_id, then by the single enabled Slack channel.
Outbound
Agent and AI replies are queued by EnqueueSlackMessage and drained by cron every
five seconds, with the same immediate-dispatch goroutine, backoff and retry
ceiling as the other channels. Replies are posted with the inbound thread_ts so
an answer stays in the customer's thread instead of landing in the workspace
timeline. Text and HTML go out as content; an image is sent with its signed asset
URL; an attachment appends the signed URL.
Signing secret verification
Two changes here, both because the endpoint is unauthenticated by design.
A channel with a signing secret now rejects a delivery that arrives without the
X-Slack-Signature and X-Slack-Request-Timestamp headers. Previously the check
only ran when both headers were present, so omitting them bypassed verification
entirely: anyone who learned the webhook URL could post into a workspace
conversation and trigger paid AI replies. Slack always signs once a signing
secret exists, so a missing header means the sender is not Slack.
Request timestamps are now rejected outside a five minute window in either
direction. Slack's own verification guide requires this, and without it a
captured request replays indefinitely: the signature covers the timestamp and the
body, but nothing in it expires.
A channel with no signing secret still accepts deliveries, so this is not a
breaking change for an existing installation. The dashboard field explains what
leaving it empty costs.
Credentials
Everything is per channel: bot token, signing secret, app id, team id, team name
and a default channel. No environment variables are added, so .env.example is
untouched. The bot token is required when creating a channel, the same way
Telegram's is, because without it the channel can receive events but can never
reply.
Tests
internal/slack/client_test.go
threaded reply carries thread_ts, a top-level post omits it, ok:false is
surfaced as an error rather than treated as success, and input validation
internal/handlers/third/slack_handler_test.go
url_verification handshake, an unsigned event is rejected and creates no
customer identity, a correctly signed event resolves the sender
internal/services/slack_inbound_service_test.go
inbound to conversation to outbox; rejection of missing headers, a
signature without a timestamp, a timestamp without a signature, a wrong
signature and a non-numeric timestamp; replayed timestamps at ten minutes,
one hour and ten minutes in the future, each correctly signed for its own
timestamp; fresh and four-minute-old signatures still accepted
Not included
Slack's OAuth install flow, so the bot token is pasted rather than obtained by
authorization. Channel and DM history backfill is also not implemented: the
channel starts receiving from the moment the Events API subscription is live.
fix(storage): stop guest uploads from becoming same-origin script execution
feat(channel): add native Discord channel integration
ensureDefaultOIDCRole assigned the admin role (falling back to super_admin) to any user that arrived without a role, which in practice means every first-time OIDC login: a stranger with an account at the identity provider became an administrator of the support desk. Give the lowest staff role (cs_user) instead; administrative access must be granted deliberately. The regression test seeds all three candidate roles and asserts a first-time OIDC user lands on cs_user and nothing else.
…ckout to it Two coupled defects. The second could not be fixed safely before the first. Gin was never told which proxies to trust. Nothing in the repository calls SetTrustedProxies or sets TrustedPlatform, so the engine ran on Gin's own default of 0.0.0.0/0 and ::/0 - every peer is a trusted proxy. validateHeader then walks X-Forwarded-For right to left and stops at the first address that is *not* a trusted proxy; with everything trusted it never stops, and returns the leftmost value, which is the one the caller supplied. Any client could therefore choose the address recorded in t_login_credential_log.client_ip and t_user.last_login_ip by sending one header. X-Real-IP is in Gin's default RemoteIPHeaders too, so it was a second route to the same result. - ServerConfig gains trustedProxies and trustedPlatform. Left unset, trustedProxies falls back to the loopback, RFC1918, IPv6 unique-local and link-local ranges rather than to Gin's trust-everything default. That covers a sidecar tunnel, a compose network and a local nginx, and a client on the public internet cannot present one of those addresses as its direct peer. - trustedPlatform maps "cloudflare", "fly.io" and "google-app-engine" onto Gin's own constants and passes any other value through as a literal header name. It defaults to empty on purpose: an edge that appends rather than overwrites would turn the header back into a client-controlled value. - NewServer applies both before any middleware is registered, and fails startup on an unparseable CIDR rather than quietly dropping the trust boundary. - Documented in config/config.example.yaml, and .env.example sets TRUSTED_PLATFORM=cloudflare because this deployment sits behind a Cloudflare Tunnel. With an address that can be believed, credential lockout stops being a denial of service. isCredentialLocked keyed on the username alone, so anybody who knew a username could lock the real account out for the whole window, from anywhere, as often as they liked - and the window was renewable indefinitely. - The per-account window is now keyed on principal *and* client address, so one source grinding on one account is still stopped. - A second window counts failures from one address across all principals, which is what credential stuffing looks like. auth.maxFailedAttemptsPerIP configures it and defaults to four times maxFailedAttempts; disabling maxFailedAttempts disables both, matching the existing convention. - An address the server could not determine is normalised to a single "unknown" bucket and excluded from the per-address window, so a missing IP cannot pool every such caller into one lockout. - createLoginCredentialLog normalises on write so the read and write sides cannot drift apart. Tests. TestGinDefaultTrustsEveryProxy pins the vulnerable default, so the fix stays load-bearing and a future Gin that changes the default says so instead of silently making the configuration decorative. Three more cover an untrusted peer's forged header being ignored, a trusted proxy's appended address beating client-prepended junk, and the platform header taking precedence over X-Forwarded-For. Two lockout tests cover the denial of service being closed and the per-address window catching a username spray that no single account trips. Four existing lockout tests seeded credential logs without a client address, which is exactly the assumption this removes, so they now seed one.
…an drive There was no rate limiting anywhere in the application. Every unauthenticated endpoint could be called in a tight loop, which made three things cheap: flooding the message upload endpoints with 20 MB bodies, spraying login and support registration, and burning LLM spend through the conversation endpoints. Add a small in-process fixed-window limiter and attach it to six public routes: login (20 per window), support registration (10), the widget session exchange (120), message attachment and image upload (30, shared between the two), and documentation feedback (20). The window is 60 seconds by default, and both it and the on/off switch are configurable. The limits are sized for a human driving a browser, not for a machine. The session exchange budget in particular has to absorb a whole support office loading the widget from behind one NAT address, which is why it sits an order of magnitude above the others. Deliberately not limited: - /api/third/* channel webhooks. A platform that receives a 429 from its webhook stops retrying and eventually disables the delivery, which takes an entire channel offline - a far worse outcome than the flood the limit was meant to stop. Those endpoints authenticate by signature instead. - /api/ws/* websockets, and /api/webhooks/* which are HMAC verified. - /api/dashboard/* - already behind AuthMiddleware, and staff sharing one office address would throttle each other. Rejections return 429 with a Retry-After header and an ordinary JsonResult body. The status is a real 429 rather than the 200-with-error-code the auth middleware uses, because web/lib/api/client.ts parses the payload before it inspects response.ok and surfaces payload.message, so the localized text still reaches the user - and Retry-After is only meaningful on a 429 or 503. The message is error.e0354 in both backend locales and does not disclose the limit or the remaining budget, which would only tell a caller how much room they have left. Retry-After rounds the remaining window up rather than down. Telling a caller to come back sooner than the window actually resets just earns another 429. Rejections are not logged separately. requestLogMiddleware already records path, status and client address for every request, so a 429 is visible there without giving a flood a second way to fill the log. The limiter keys on ctx.ClientIP(), which is only trustworthy because of the trusted-proxy configuration that landed immediately before this. Without it a caller would pick their own bucket with an X-Forwarded-For header. Counters are per process, and expired buckets are swept lazily from inside Allow rather than by a goroutine, so there is no background lifetime to own and no unbounded growth. Running several replicas gives each its own budget, weakening the bound by the replica count; config.example.yaml says so explicitly. These limits exist to make flooding expensive, not to meter a quota. Tests: seven for the limiter, including an exact-count check across 16 goroutines making 3200 calls against one key; three for the middleware, covering the 429 shape, per-address isolation and a nil limiter allowing everything; and five end to end against a server built by NewServer, including one that fires 1000 requests at the health, config, org-sync and two channel webhook routes and fails if any of them returns 429. The concurrency test could not be run under -race: this repository builds with CGO disabled and go test -race requires cgo. It still has teeth, because an unguarded concurrent map write panics rather than merely miscounting.
…nsport When OIDC is enabled, password login is disabled, and no other provider is configured, /dashboard/login now sends the visitor straight to the provider instead of rendering a page whose only working control is the "sign in with SSO" button. The redirect waits for the session probe, so an already-signed-in visitor still lands on their destination, and it is suppressed when the provider bounced back with ?oidcError= so a failing IdP cannot create a redirect loop (the page then renders with its error toast and the manual button), and in WxWork-only environments.
fix(security): make the client IP trustworthy and scope credential lockout to it
feat(security): rate limit the public endpoints an anonymous caller can drive
…in-role fix(auth): stop granting admin to every first-time OIDC login
Resolve against upstream main through huabeitech#49: - union the Slack and Discord channel additions (dto config types, external source enum, outbox enqueue + dispatch, channel parse and validation, cron dispatch, routes registration, dashboard dialog and channel page) - keep upstream's ServerConfig security fields (trustedProxies, trustedPlatform, rateLimit) which merge in cleanly alongside this branch's Slack config - parseSlackChannelConfig no longer trims a WelcomeMessage field the Slack config never had - the dashboard dialog's EditForm/schema grow both slack and discord fields so one form serves every IM channel - messages locales gain both channel option sets
feat(channel): add native Slack channel integration
feat(auth): auto-redirect to the SSO provider when it is the only transport
Bring the fork back in sync after the upstream merge wave (Slack and Discord channels, OIDC first-login role fix, trusted client IP + lockout, public endpoint rate limiting, login auto-redirect): - keep the fork's hardened Slack webhook gate (ResolveSlack deployment fallback, unsigned-rejection, secret-source warn logs) and its Slack OAuth client flow - upstream's merged variant predates that work - keep upstream's constant-time Discord webhook-secret comparison - restore upstream test coverage the merge had dropped: slack inbound rejection/replay/challenge cases, discord guild-scope and wrong-secret cases, and the OIDC first-login lowest-role case (adapted to the fork's portalOrigin-aware signature) - dedupe the auto-merged login-form redirect block (keep the break-glass-aware variant), the config test set, and a duplicated DiscordConfig declaration - messages: add the slack/discord setup keys the channel dialog renders in all three locales, and the slack bot-token required validation with its key
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![dos[bot]](https://avatars.githubusercontent.com/in/4711427?s=60&v=4){kind=small}
There was a problem hiding this comment.
⏱️ Code Review completed (25 files · 59,292 chars · 2 PR unit(s))
ℹ️ Full-Context Analysis: Analyzed all changed files in a unified context pass to preserve cross-file type definitions, imports, and caller contracts. Deducted 2 PR units.
⏱️ Adversarial Review completed (Model: qwen3.8-27b)
🔍 Verified Adversarial Review Findings
🔵 MINOR
web/messages/zh-CN.json:643-648: Incorrect language content in Chinese localization file- Failure Trace:
- A user selects the
zh-CNlocale in the application. - The user navigates to the Channel Edit page and selects "Discord" or "Slack" as the channel type.
- The UI renders the setup instructions using keys like
channel.discordSetupDescriptionorchannel.slackSetupTitle. - The
next-intllibrary resolves these keys fromweb/messages/zh-CN.json. - The values at lines 643-648 are Vietnamese text (e.g.,
"discordSetupDescription": "Tạo bot trong Discord Developer Portal..."), not Chinese. - The user sees Vietnamese instructions instead of Chinese, resulting in a broken localization experience.
- A user selects the
- Actionable Fix:
- Failure Trace:
"discordSetupDescription": "在 Discord Developer Portal 中创建机器人，启用 Message Content 特权意图，邀请机器人进入服务器，并将交互或桥接端点指向下方的 webhook URL。如果留空 bot token，将使用部署范围的 DISCORD_BOT_TOKEN。",
"discordSetupTitle": "Discord 机器人连接",
"slackSigningSecretHint": "用于验证传入事件。如果没有它，webhook 将接受未签名的请求，任何知道 URL 的人都可以向对话中发送消息。",
"slackRequestUrl": "请求 URL",
"slackSetupDescription": "创建 Slack 应用，将其安装到工作区，然后从基本信息中复制 Bot User OAuth Token 和 Signing Secret。订阅 message.events，然后在事件订阅中设置下方的请求 URL。",
"slackSetupTitle": "Slack 应用连接",
🛡️ Dismissed Claims
- None: No other candidate issues were provided for review.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Slack and Discord channel integrations, adding webhook handlers, configuration logic, database tests, and frontend UI components. The review feedback highlights three key issues: a missing import for Gamepad2Icon in the channels page, a potential timing attack vulnerability in the Discord webhook signature verification when comparing secrets of different lengths, and copy-paste translation errors in the Chinese localization file (zh-CN.json) which contains Vietnamese text.
| return <SlackIcon className="size-4" /> | ||
| } | ||
| if (channelType === "discord") { | ||
| return <Gamepad2Icon className="size-4" /> |
There was a problem hiding this comment.
The Gamepad2Icon component is used here but it does not appear to be imported in the changes for this file. This could lead to a build failure. Please ensure that Gamepad2Icon is imported from lucide-react at the top of the file.
Comment on lines
+48
to
51
| if cfg.WebhookSecret != "" && | ||
| subtle.ConstantTimeCompare([]byte(strings.TrimSpace(secretHeader)), []byte(cfg.WebhookSecret)) != 1 { | ||
|
|
||
| return errorsx.UnauthorizedI18n("error.auth.invalidSignature") |
There was a problem hiding this comment.
While using subtle.ConstantTimeCompare is a great security improvement, the comparison might be susceptible to timing attacks if the lengths of the two byte slices are different. subtle.ConstantTimeCompare requires the slices to be of equal length. An attacker could potentially infer the length of the secret by observing timing differences. It's recommended to ensure the comparison is safe even with inputs of different lengths, for example by hashing the incoming secret before comparison.
Comment on lines
+643
to
+648
| "discordSetupDescription": "Tạo bot trong Discord Developer Portal, bật Message Content privileged intent, mời bot vào server, rồi trỏ endpoint interaction/bridge tới webhook URL bên dưới. Để trống bot token để dùng DISCORD_BOT_TOKEN toàn deployment.", | ||
| "discordSetupTitle": "Kết nối Discord Bot", | ||
| "slackSigningSecretHint": "Bắt buộc để xác minh event đến. Thiếu nó, webhook sẽ nhận request không chữ ký - ai biết URL cũng có thể nhắn vào hội thoại.", | ||
| "slackRequestUrl": "Request URL", | ||
| "slackSetupDescription": "Tạo Slack app, cài vào workspace, rồi copy Bot User OAuth Token và Signing Secret từ mục Basic Information. Đăng ký message.events, rồi đặt Request URL bên dưới vào Event Subscriptions.", | ||
| "slackSetupTitle": "Kết nối Slack App", |
There was a problem hiding this comment.
These new internationalization keys have Vietnamese translations instead of Chinese. It appears they may have been copied from the vi-VN.json file. Please replace them with the correct Chinese translations.
…eam coverage The merge left TestSlackWebhook_Handler declared twice (the fork's whatsapp_slack_handler_test.go and upstream's slack_handler_test.go). Drop the fork's copy - upstream's version is the newer signed-payload form and already covers the unsigned-rejection case - and keep the WhatsApp test in the fork file. Also restore the upstream-only tests the merge had dropped (slack inbound rejection/replay/challenge, discord guild-scope and wrong-secret, OIDC first-login lowest role) and the slack/discord setup i18n keys the channel dialog renders.
There was a problem hiding this comment.
⏱️ Code Review completed (26 files · 65,370 chars · 3 PR unit(s))
ℹ️ Full-Context Analysis: Analyzed all changed files in a unified context pass to preserve cross-file type definitions, imports, and caller contracts. Deducted 3 PR units.
⏱️ Adversarial Review completed (Model: qwen3.8-27b)
🔍 Verified Adversarial Review Findings
🔵 MINOR
web/messages/zh-CN.json:643-648: Vietnamese text in Chinese locale- Failure Trace:
- User selects
zh-CNlocale in the dashboard. - User navigates to Channels -> Edit Slack/Discord channel.
- The UI renders
t("channel.slackSetupDescription")ort("channel.discordSetupDescription"). - The
zh-CN.jsonfile contains Vietnamese strings (e.g., "Tạo bot trong Discord Developer Portal...") for these keys instead of Chinese. - The user sees Vietnamese instructions, which is a localization defect causing confusion.
- User selects
- Actionable Fix:
"discordSetupDescription": "在 Discord Developer Portal 中创建机器人，启用 Message Content 特权意图，邀请机器人到您的服务器，并将交互或桥接端点指向下方的 webhook URL。留空机器人令牌以使用部署范围的 DISCORD_BOT_TOKEN。", "discordSetupTitle": "Discord 机器人连接", "slackSigningSecretHint": "必需以验证传入事件。缺少它，webhook 将接受未签名请求，任何知道 URL 的人都可以向对话中发送消息。", "slackRequestUrl": "请求 URL", "slackSetupDescription": "创建 Slack 应用，安装到工作区，然后从基本信息中复制 Bot User OAuth Token 和 Signing Secret。订阅 message.events，然后在事件订阅中设置下方的请求 URL。", "slackSetupTitle": "Slack 应用连接",
- Failure Trace:
🛡️ Dismissed Claims
- None: No other candidate issues were provided for review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Periodic sync: merge
upstream/mainback intodevafter the upstream merge wave (the Slack and Discord channel PRs, the OIDC first-login role fix, the trusted client IP + lockout hardening, public endpoint rate limiting, and the login auto-redirect - all originally authored here and now merged upstream).Resolution highlights:
ResolveSlackfallback, unsigned-rejection, secret-source warn logs); the fork's version wins, and upstream's replay-window logic (already in ours) is unchanged.portalOrigin-aware signature).slackSetup*/discordSetup*keys that were missing from all three locales (pre-existing fork gap) - added, plus the upstream slack bot-token required validation with its key..gitmodules/qdrantremain deleted (deliberate fork choice from 9a25b41). No upstream file or test dropped by the merge (verified post-commit).Validation
go test -tags devgreen across the CI package list;go vet -tags dev ./...clean;pnpm typecheckclean; lint shows only the pre-existingnameValuewarning. Playwright check of the channels dialog (Slack + Discord sections) follows the merge.