MM-68853: Confluence Cloud install path via OAuth 2.0 (3LO) + Forge bridge POC#228
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Forge-based Confluence event bridge, Cloud 3LO onboarding and Forge registration, Forge webtrigger handlers (enqueue/drain/register), a clustered poller that drains signed events and posts acks, Forge event serialization, mention extraction and DM dispatch, plugin config/settings and lifecycle wiring, CI/manifest validation, and removal of the legacy Atlassian Connect descriptor. ChangesConfluence Cloud Forge Bridge GA Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
.gitignore (1)
91-91: ⚡ Quick winDo not ignore
forge/package-lock.jsonfor this deployable Node project.Ignoring the lockfile makes Forge CI/deploy dependency resolution non-deterministic. Keep the lockfile versioned and remove this ignore rule.
Suggested diff
# Forge bridge forge/dist/ forge/scripts-dist/ forge/node_modules/ forge/.forge/ -forge/package-lock.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore at line 91, The .gitignore currently excludes the project's Forge lockfile causing non-deterministic installs; remove the ignore entry for "forge/package-lock.json" so the lockfile is tracked. Edit the .gitignore and delete the line "forge/package-lock.json" (or comment it out) to ensure package-lock.json under the forge directory is committed and versioned, then commit the change so CI/deploy uses the deterministic lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/forge-ci.yml:
- Around line 23-29: The workflow uses mutable action tags and leaves checkout
credentials persisted; update the checkout and setup-node steps to pin each
action to a full commit SHA (replace actions/checkout@v4 and
actions/setup-node@v4 with their respective commit SHAs) and add
persist-credentials: false to the actions/checkout step so credentials are not
kept in the runner; locate the checkout and setup-node usages in the workflow
and make these changes for supply-chain and token-leak hardening.
In `@forge/README.md`:
- Line 67: Update the README sentence to use the correct product name casing by
changing "Github" (or any lowercase variant) to "GitHub" in the line mentioning
`server/` and `webapp/` builds and CI (`.github/workflows/forge-ci.yml`) so the
product name is properly capitalized.
- Around line 103-104: The README line referencing the Cloud OAuth file is
stale: locate the actual Cloud 3LO OAuth implementation (search for the Cloud
OAuth handler/flow code referenced near "Plugin-side Cloud 3LO OAuth" and the
implementation in the repository) and update the README so it points to the
correct source file instead of the outdated server/cloud_oauth_flow.go
reference; ensure the line mentions the confirmed file that contains the Cloud
OAuth flow (and keep the mention of server/instance_cloud.go only if that file
is still accurate).
In `@forge/src/index.ts`:
- Around line 41-46: Validate and clamp the incoming body.limit before calling
storage.query().limit: parse it to a number (e.g., Number(body.limit) or
parseInt), guard against NaN and non-integers with Math.floor, then clamp the
result to the range [1, MAX_DRAIN_BATCH] (default to MAX_DRAIN_BATCH when
missing/invalid). Replace the current const limit = Math.min(...) with a
validatedLimit variable and pass validatedLimit into
storage.query().limit(limit) so .limit always receives a positive integer
between 1 and MAX_DRAIN_BATCH; reference symbols: body.limit, MAX_DRAIN_BATCH,
QUEUE_PREFIX, storage.query().limit(...).
- Around line 37-39: The code currently deletes every key in body.ack via
storage.delete without validating ownership; change it to only delete keys that
belong to the target queue by validating each ack key before deletion. For each
key in body.ack, call a validation (e.g., storage.get(k) and check returned
metadata.owner === queueId or implement an isQueueKey(k, queueId) helper that
checks the queue-specific prefix/owner), filter the ack list to only those keys
that pass validation, then await Promise.all(filtered.map(k =>
storage.delete(k))); keep references to body.ack and storage.delete and add
storage.get or isQueueKey/queueId validation as needed.
In `@server/flow.go`:
- Around line 741-752: The config isn't being marked as Cloud before saving the
Cloud OAuth credentials, so ensure you set the configuration's IsCloud flag to
true on the cfg returned by fm.getConfiguration() (e.g., cfg.IsCloud = true)
prior to calling cfg.Sanitize(), cfg.ToMap(), and
fm.client.Configuration.SavePluginConfig; update the block that sets
cfg.ConfluenceOAuthClientID/Secret to also set cfg.IsCloud = true so the
subsequent saved config routes to the Cloud oauth handlers (affecting
fm.getConfiguration(), cfg.Sanitize, cfg.ToMap, and SavePluginConfig usage).
In `@server/forge_poller.go`:
- Around line 137-148: The loop is acking Forge events even when delivery may
have failed because dispatch currently returns before
SendConfluenceNotifications completes; change dispatch so it only returns nil
after SendConfluenceNotifications has finished successfully (or return an
error/result/future that this loop waits on), then only append evt.Key to
ackKeys when dispatch returns no error; update the same logic at the other
occurrence (lines around 194-195) so ackBody/json.Marshal and fp.post are only
called for successfully delivered events and errors from
SendConfluenceNotifications are propagated back and logged via
fp.plugin.client.Log.Warn.
In `@server/instance_cloud.go`:
- Around line 118-126: The refreshed Cloud token is being stored under cloudID
but should be persisted under the configured instance key (instance URL) so
later lookups by instanceURL see the updated token; update the call site in this
block to pass both the configured instance URL and the resolved cloudID to
refreshAndStoreToken (or adjust refreshAndStoreToken's signature) and ensure the
implementation writes the refreshed token into the KV record keyed by the
instance URL (the same key used by connectUser), while leaving the rest of the
function (e.g. httpClient creation and
newCloudClient(fmt.Sprintf(cloudAPIBaseFmt, cloudID), httpClient)) unchanged.
- Around line 43-51: The OAuth scopes list is missing the read:confluence-user
scope required by client.GetSelf(); update the scopes slice in
completeCloudOAuth2 to include "read:confluence-user" (so the GET
/wiki/rest/api/user/current will be allowed) and also update the
confluenceCloudScopes constant in server/flow.go to include the same
"read:confluence-user" entry so both places mirror each other; touch the scopes
defined where completeCloudOAuth2 builds the scopes and the
confluenceCloudScopes constant to ensure they match.
In `@server/user.go`:
- Around line 271-273: The current fallback that returns resources[0].ID when no
resource URL matched the configured ConfluenceURL silently masks a config
mismatch; instead, change the behavior in the function handling the resources
slice (the block that currently checks if len(resources) == 1 and returns
resources[0].ID) to fail fast: do not return the sole resource when URL matching
failed—return an explicit error that includes the configured ConfluenceURL and
the list of returned resource URLs (or at least the single resource URL) so
callers can detect and surface the configuration mismatch. Ensure the error
replaces the current silent return path and preserves the original
matched-return behavior when a proper URL match exists.
---
Nitpick comments:
In @.gitignore:
- Line 91: The .gitignore currently excludes the project's Forge lockfile
causing non-deterministic installs; remove the ignore entry for
"forge/package-lock.json" so the lockfile is tracked. Edit the .gitignore and
delete the line "forge/package-lock.json" (or comment it out) to ensure
package-lock.json under the forge directory is committed and versioned, then
commit the change so CI/deploy uses the deterministic lockfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1ae89d3-4c20-433c-904e-ece254192d7a
📒 Files selected for processing (24)
.github/workflows/forge-ci.yml.gitignoreassets/templates/atlassian-connect.jsonforge/README.mdforge/manifest.ymlforge/package.jsonforge/scripts/tsconfig.jsonforge/scripts/validate-manifest.tsforge/src/index.tsforge/tsconfig.jsonforge/tsconfig.scripts.jsonplugin.jsonserver/atlassian_connect.goserver/client_cloud.goserver/command.goserver/config/main.goserver/controller.goserver/flow.goserver/forge_event_mapping.goserver/forge_poller.goserver/instance_cloud.goserver/plugin.goserver/user.goserver/util/util.go
💤 Files with no reviewable changes (4)
- assets/templates/atlassian-connect.json
- server/util/util.go
- server/atlassian_connect.go
- server/controller.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/user.go (1)
350-358:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
StoreConnectioncalls will double-write unnecessarily.Lines 350-353 and 355-358 are identical—both store the connection under
(instanceID, mattermostUserID). This appears to be a copy-paste error. The second call should be removed.🐛 Proposed fix to remove duplicate call
if err = store.StoreConnection(instanceID, mattermostUserID, connection); err != nil { p.client.Log.Error("Error storing connection", "InstanceID", instanceID, "UserID", mattermostUserID, "error", err.Error()) return err } - if err = store.StoreConnection(instanceID, mattermostUserID, connection); err != nil { - p.client.Log.Error("Error storing connection", "InstanceID", instanceID, "UserID", mattermostUserID, "error", err.Error()) - return err - } - if err = store.StoreConnection(instanceID, AdminMattermostUserID, connection); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/user.go` around lines 350 - 358, The code duplicates a call to store.StoreConnection with identical parameters and identical error handling, causing a double-write; edit the function where these lines appear to remove the second/duplicate store.StoreConnection(...) block (the repeated block that also calls p.client.Log.Error on failure) so only a single call to store.StoreConnection(instanceID, mattermostUserID, connection) remains with its error check and p.client.Log.Error logging.
♻️ Duplicate comments (1)
forge/src/index.ts (1)
118-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure
clampLimitnever returns 0 for fractional positive input.For
raw = 0.5,Math.floormakes the limit0. That breaks the intended positive clamp behavior.Suggested fix
const clampLimit = (raw: unknown): number => { const n = typeof raw === 'number' ? raw : Number(raw); if (!Number.isFinite(n) || n <= 0) return MAX_DRAIN_BATCH; - return Math.min(Math.floor(n), MAX_DRAIN_BATCH); + return Math.max(1, Math.min(Math.floor(n), MAX_DRAIN_BATCH)); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forge/src/index.ts` around lines 118 - 122, clampLimit currently floors positive fractional inputs which makes 0.5 become 0; update the final return to ensure a minimum of 1 for any positive finite input. In the clampLimit function, replace return Math.min(Math.floor(n), MAX_DRAIN_BATCH) with return Math.min(Math.max(Math.floor(n), 1), MAX_DRAIN_BATCH) (keep the existing early check using MAX_DRAIN_BATCH for non-finite or non-positive n).
🧹 Nitpick comments (3)
server/flow.go (3)
609-632: 💤 Low valueCloud instance URL step also reuses
stepConfluenceURLname.Similar to the welcome step,
stepCloudInstanceURL()returns a step with namestepConfluenceURL(line 610), which is the same as the generic instance URL step. This means both flows have steps with the same identifier but different behavior (different dialog handlers).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/flow.go` around lines 609 - 632, The stepCloudInstanceURL function is creating a step with the duplicate identifier stepConfluenceURL which collides with the generic instance URL step; rename the step identifier to a unique name (e.g., stepCloudConfluenceURL) in the flow.NewStep call inside stepCloudInstanceURL and update any places that reference that step name to use the new identifier, and ensure the dialog submit handler (submitCloudConfluenceURL) remains wired to the renamed step so the cloud-specific dialog and handler are correctly isolated from the generic step.
593-607: 💤 Low valueCloud welcome step reuses
stepWelcomename, may conflict with generic welcome step.
stepCloudWelcome()returns a step with namestepWelcome(line 602), which is the same constant used bystepWelcome()(line 246). While different flows can have steps with the same name, this could cause confusion or unexpected behavior if both steps are ever used in the same flow context or during debugging.Consider using a distinct step name like
stepCloudWelcomefor the cloud-specific welcome step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/flow.go` around lines 593 - 607, The cloud-specific welcome step currently reuses the generic step name constant stepWelcome in stepCloudWelcome(), which can cause confusion or conflicts; change the step name passed to flow.NewStep from stepWelcome to a distinct constant (e.g., stepCloudWelcome) and create that new constant, then update any cloud-flow-specific references/handlers that expect the old name (e.g., in stepCloudWelcome, any transition targets or lookups) so the cloud welcome is uniquely identified from the generic stepWelcome.
761-781: 💤 Low valueForge shared secret displayed in plaintext in the wizard instructions.
The wizard renders the
ForgeSharedSecretdirectly in the step text (line 773) and passes it through state (lines 202, 757). While the secret is only visible to the admin running the wizard, be aware that:
- The secret will appear in the Mattermost channel/DM message history
- It could be logged or captured in screenshots
This may be acceptable for a setup flow, but consider noting in the instructions that the admin should treat this as sensitive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/flow.go` around lines 761 - 781, The stepCloudForgeBridge step currently injects ForgeSharedSecret into the public step text (and passes it via flow state), which exposes the secret in message history/screenshots; remove the plaintext {{.ForgeSharedSecret}} from the step text in stepCloudForgeBridge and instead either (a) show a masked placeholder and add a clear sensitive-handling note telling the admin where to copy the secret from (e.g., plugin config/secure storage), or (b) provide an explicit "Reveal secret" action that fetches the secret directly from secure storage and displays it only in an ephemeral modal; also stop including the secret in the generic flow/state payloads (references: ForgeSharedSecret, stepCloudForgeBridge, and the places where state is set/passed around) so the secret isn't stored or transmitted in step text/history.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/user.go`:
- Around line 350-358: The code duplicates a call to store.StoreConnection with
identical parameters and identical error handling, causing a double-write; edit
the function where these lines appear to remove the second/duplicate
store.StoreConnection(...) block (the repeated block that also calls
p.client.Log.Error on failure) so only a single call to
store.StoreConnection(instanceID, mattermostUserID, connection) remains with its
error check and p.client.Log.Error logging.
---
Duplicate comments:
In `@forge/src/index.ts`:
- Around line 118-122: clampLimit currently floors positive fractional inputs
which makes 0.5 become 0; update the final return to ensure a minimum of 1 for
any positive finite input. In the clampLimit function, replace return
Math.min(Math.floor(n), MAX_DRAIN_BATCH) with return
Math.min(Math.max(Math.floor(n), 1), MAX_DRAIN_BATCH) (keep the existing early
check using MAX_DRAIN_BATCH for non-finite or non-positive n).
---
Nitpick comments:
In `@server/flow.go`:
- Around line 609-632: The stepCloudInstanceURL function is creating a step with
the duplicate identifier stepConfluenceURL which collides with the generic
instance URL step; rename the step identifier to a unique name (e.g.,
stepCloudConfluenceURL) in the flow.NewStep call inside stepCloudInstanceURL and
update any places that reference that step name to use the new identifier, and
ensure the dialog submit handler (submitCloudConfluenceURL) remains wired to the
renamed step so the cloud-specific dialog and handler are correctly isolated
from the generic step.
- Around line 593-607: The cloud-specific welcome step currently reuses the
generic step name constant stepWelcome in stepCloudWelcome(), which can cause
confusion or conflicts; change the step name passed to flow.NewStep from
stepWelcome to a distinct constant (e.g., stepCloudWelcome) and create that new
constant, then update any cloud-flow-specific references/handlers that expect
the old name (e.g., in stepCloudWelcome, any transition targets or lookups) so
the cloud welcome is uniquely identified from the generic stepWelcome.
- Around line 761-781: The stepCloudForgeBridge step currently injects
ForgeSharedSecret into the public step text (and passes it via flow state),
which exposes the secret in message history/screenshots; remove the plaintext
{{.ForgeSharedSecret}} from the step text in stepCloudForgeBridge and instead
either (a) show a masked placeholder and add a clear sensitive-handling note
telling the admin where to copy the secret from (e.g., plugin config/secure
storage), or (b) provide an explicit "Reveal secret" action that fetches the
secret directly from secure storage and displays it only in an ephemeral modal;
also stop including the secret in the generic flow/state payloads (references:
ForgeSharedSecret, stepCloudForgeBridge, and the places where state is
set/passed around) so the secret isn't stored or transmitted in step
text/history.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1252f1c-4fb4-4832-a3d4-679384f8ea30
📒 Files selected for processing (6)
forge/README.mdforge/src/index.tsplugin.jsonserver/flow.goserver/instance_cloud.goserver/user.go
|
@nang2049 I've been looking through this and getting acquainted with Forge a little bit more, I think you got the right direction for the architecture with us directly maintaining the Forge bridge app and customers just use the private distribution link to set it up with their instances 👍 I guess now we should have a test app deployed and we can set it up against multiple Jira/MM instance pairs to confirm all is working well before moving onto doing more specific code-level reviews (and likely security reviews) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
forge/src/index.ts (1)
68-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
payload.secrettype before one-shot registration.Non-string values can pass the current check and be stored as the secret, which can permanently break signed
drainrequests after registration.Suggested fix
- if (!payload.secret || payload.secret.length < 32) { + if (typeof payload.secret !== 'string' || payload.secret.length < 32) { return jsonResponse(400, { error: 'secret must be at least 32 characters' }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forge/src/index.ts` around lines 68 - 70, The handler currently only checks payload.secret.length but doesn't verify payload.secret is a string, allowing non-string values to bypass validation; update the validation in the registration logic (the code around payload.secret check in forge/src/index.ts) to first ensure typeof payload.secret === 'string' (and not null/undefined) and only then verify length >= 32, returning a 400 JSON response if the type check fails or the length is insufficient so non-string secrets cannot be stored and break signed drain requests.server/flow.go (2)
658-688:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-Cloud hosts before setting
IsCloud.This path only normalizes the URL, so any HTTPS host is accepted and persisted with
IsCloud = true. A self-hosted or mistyped URL will send/confluence connectdown the 3LO path and fail later instead of being rejected here.Minimal fix
normalized, err := service.NormalizeConfluenceURL(confluenceURL) if err != nil { return "", nil, map[string]string{"confluence_url": err.Error()}, nil } + parsed, err := url.Parse(normalized) + if err != nil || !strings.HasSuffix(parsed.Hostname(), ".atlassian.net") { + return "", nil, map[string]string{ + "confluence_url": "Use your Confluence Cloud site URL (`*.atlassian.net/wiki`).", + }, nil + } if normalized == strings.TrimSuffix(fm.MMSiteURL, "/") { return "", nil, map[string]string{"confluence_url": "This is the Mattermost site URL. Please use the Confluence Cloud URL."}, nil } confluenceURL = normalized
63-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the server wizard server-only.
setupFlowis now only started from/confluence install serverinserver/command.go, but it still branches intostepEditionQuestionand the Cloud-only steps. If an admin clicks Cloud here, the flow skipsstepCloudInstanceURLand reuses the URL already saved bysubmitConfluenceURL, which leaves the config on the Cloud path against the wrong site.Minimal fix
setupFlow.WithSteps( fm.stepWelcome(), fm.stepInstanceURL(), - fm.stepEditionQuestion(), - fm.stepCloudOAuthConfigure(), - fm.stepCloudForgeBridge(), fm.stepServerVersionQuestion(), fm.stepCSversionGreaterthan9(), fm.stepCSversionLessthan9(), fm.stepOAuthInput(), @@ - return stepEditionQuestion, flow.State{ + return stepServerVersionQuestion, flow.State{ keyConfluenceURL: cfg.GetConfluenceBaseURL(), }, nil, nilAlso applies to: 410-412
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/flow.go` around lines 63 - 78, The flow currently includes Cloud-specific steps (fm.stepEditionQuestion, fm.stepCloudOAuthConfigure, fm.stepCloudForgeBridge, etc.) even though setupFlow is only invoked for the server installer, causing Cloud selection to reuse the wrong URL saved by submitConfluenceURL; update the setupFlow sequence to be server-only by removing or replacing fm.stepEditionQuestion with a server-only edition step (or remove Cloud branch entirely) and remove the Cloud-specific steps (fm.stepCloudOAuthConfigure, fm.stepCloudForgeBridge, any stepCloudInstanceURL references) so only server-related steps (e.g., fm.stepInstanceURL, fm.stepServerVersionQuestion, fm.stepCSversionGreaterthan9, fm.stepCSversionLessthan9, fm.stepOAuthInput, fm.stepOAuthConnect, fm.stepAnnouncementQuestion, fm.stepAnnouncementConfirmation, fm.stepDone, fm.stepCancel) remain; ensure submitConfluenceURL is not reused by Cloud steps by keeping Cloud flow in a separate flow invoked only from Cloud commands.
♻️ Duplicate comments (1)
server/forge_poller.go (1)
176-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeliver notifications synchronously before acking the Forge key.
dispatchreturns immediately becauseSendConfluenceNotificationsruns in a goroutine, butdrainOncestill acknowledges the key right afterdispatchreturns. That makes this path drop events from Forge before delivery has actually finished.Possible minimal fix
func (fp *ForgePoller) dispatch(evt forgeDrainEvent) error { forgeEvent, err := serializer.ForgeEventFromJSON(bytes.NewReader(evt.Value.Event)) if err != nil { return errors.Wrap(err, "deserialize forge event") } internal, ok := forgeToInternalEvent[forgeEvent.EventType] if !ok { return fmt.Errorf("unmapped forge event type %q", forgeEvent.EventType) } forgeEvent.BaseURL = config.GetConfig().GetConfluenceBaseURL() - go service.SendConfluenceNotifications(forgeEvent, internal) + service.SendConfluenceNotifications(forgeEvent, internal) return nil }for _, evt := range resp.Events { if err := fp.dispatch(evt); err != nil { fp.plugin.client.Log.Warn("forge drain: dropping event", "key", evt.Key, "error", err.Error()) + continue } ackKeys = append(ackKeys, evt.Key) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/forge_poller.go` around lines 176 - 189, The dispatch path currently launches notifications asynchronously (using "go service.SendConfluenceNotifications") so drainOnce can ack the Forge key before delivery completes; change this to perform notifications synchronously by removing the goroutine and calling service.SendConfluenceNotifications(forgeEvent, internal) directly, and ensure SendConfluenceNotifications returns an error (or add a blocking wrapper) so you can check its result and return that error instead of nil — this guarantees delivery completes (or fails) before returning from the dispatch path that acknowledges the key (references: forgeEvent, internal, service.SendConfluenceNotifications).
🧹 Nitpick comments (1)
server/plugin_test.go (1)
91-198: ⚡ Quick winAssert the persisted secret values, not just that a save happened.
These cases would still pass if
OnConfigurationChangesaved the wrong field or wrote back a non-32-char value. Please capture theSavePluginConfigpayload and verify the regenerated fields are 32 chars while untouched fields stay unchanged; adding one non-empty wrong-length input would also cover the actuallen(...) != 32branch.Example of a stronger assertion pattern
+ var saved map[string]interface{} - mockAPI.On("SavePluginConfig", mock.AnythingOfType("map[string]interface {}")).Return(nil) + mockAPI.On("SavePluginConfig", mock.AnythingOfType("map[string]interface {}")).Run(func(args mock.Arguments) { + saved = args.Get(0).(map[string]interface{}) + }).Return(nil) err := p.OnConfigurationChange() require.NoError(t, err) + require.NotNil(t, saved) + assert.Len(t, saved["encryptionkey"], 32) + assert.Equal(t, valid, saved["secret"]) + assert.Equal(t, valid, saved["forgesharedsecret"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/plugin_test.go` around lines 91 - 198, Update the tests in TestOnConfigurationChange_AutoGenerateSecrets to assert the actual persisted config values instead of only checking SavePluginConfig was called: in each subtest, use the plugintest API's mock SavePluginConfig handler to capture the map[string]interface{} payload passed to SavePluginConfig (when OnConfigurationChange calls SavePluginConfig), then assert the regenerated fields (EncryptionKey, Secret, ForgeSharedSecret) are exactly 32 chars long and that any pre-existing valid fields remain unchanged; for the "regenerates all three" case assert all three are 32 chars, and for the "does not overwrite existing valid secrets" case assert SavePluginConfig is not called or, if called, that the fields equal the original values. Ensure you still load the config via LoadPluginConfiguration and reference OnConfigurationChange, LoadPluginConfiguration, SavePluginConfig and the config.Configuration struct when locating code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/flow.go`:
- Around line 833-851: The current validation only checks scheme/host so secrets
and polling targets can be sent to arbitrary HTTPS endpoints; update validation
around isHTTPSURL, the register flow that calls postForgeRegister, and any code
that persists cfg.ForgeDrainURL (and used by forgePollInterval poller) to
enforce a strict hostname allowlist (e.g., only the official webtrigger host(s)
like webtrigger.atlassian.app), reject unknown hosts/ports, and disallow
redirects: when validating registerURL and drainURL resolve and follow any
redirects (or perform a HEAD/GET without sending secrets) and ensure the final
URL host exactly matches an allowed host; if it does not, return an error in
errorList and do not call postForgeRegister or persist cfg.ForgeDrainURL. Ensure
the same checks are applied to the code paths around postForgeRegister,
cfg.ForgeSharedSecret, and the poller in forge_poller.go so secrets and HMACs
are only sent to approved hosts.
In `@server/serializer/confluence_forge.go`:
- Around line 46-58: ForgeContent.ID is defined as string but Confluence Forge
events can emit content.id/container.id as JSON numbers, causing unmarshalling
failures; update the ForgeContent type to accept both string and numeric JSON
for ID by replacing the ID field with a tolerant type (e.g., json.Number or a
custom type) or implement a custom UnmarshalJSON for ForgeContent (or a
dedicated ID type) that accepts either a quoted string or a number and sets the
ID string field accordingly so both content.id and nested content.container.id
deserialize correctly; ensure the change preserves the exported symbol
ForgeContent and its use sites.
---
Outside diff comments:
In `@forge/src/index.ts`:
- Around line 68-70: The handler currently only checks payload.secret.length but
doesn't verify payload.secret is a string, allowing non-string values to bypass
validation; update the validation in the registration logic (the code around
payload.secret check in forge/src/index.ts) to first ensure typeof
payload.secret === 'string' (and not null/undefined) and only then verify length
>= 32, returning a 400 JSON response if the type check fails or the length is
insufficient so non-string secrets cannot be stored and break signed drain
requests.
In `@server/flow.go`:
- Around line 63-78: The flow currently includes Cloud-specific steps
(fm.stepEditionQuestion, fm.stepCloudOAuthConfigure, fm.stepCloudForgeBridge,
etc.) even though setupFlow is only invoked for the server installer, causing
Cloud selection to reuse the wrong URL saved by submitConfluenceURL; update the
setupFlow sequence to be server-only by removing or replacing
fm.stepEditionQuestion with a server-only edition step (or remove Cloud branch
entirely) and remove the Cloud-specific steps (fm.stepCloudOAuthConfigure,
fm.stepCloudForgeBridge, any stepCloudInstanceURL references) so only
server-related steps (e.g., fm.stepInstanceURL, fm.stepServerVersionQuestion,
fm.stepCSversionGreaterthan9, fm.stepCSversionLessthan9, fm.stepOAuthInput,
fm.stepOAuthConnect, fm.stepAnnouncementQuestion,
fm.stepAnnouncementConfirmation, fm.stepDone, fm.stepCancel) remain; ensure
submitConfluenceURL is not reused by Cloud steps by keeping Cloud flow in a
separate flow invoked only from Cloud commands.
---
Duplicate comments:
In `@server/forge_poller.go`:
- Around line 176-189: The dispatch path currently launches notifications
asynchronously (using "go service.SendConfluenceNotifications") so drainOnce can
ack the Forge key before delivery completes; change this to perform
notifications synchronously by removing the goroutine and calling
service.SendConfluenceNotifications(forgeEvent, internal) directly, and ensure
SendConfluenceNotifications returns an error (or add a blocking wrapper) so you
can check its result and return that error instead of nil — this guarantees
delivery completes (or fails) before returning from the dispatch path that
acknowledges the key (references: forgeEvent, internal,
service.SendConfluenceNotifications).
---
Nitpick comments:
In `@server/plugin_test.go`:
- Around line 91-198: Update the tests in
TestOnConfigurationChange_AutoGenerateSecrets to assert the actual persisted
config values instead of only checking SavePluginConfig was called: in each
subtest, use the plugintest API's mock SavePluginConfig handler to capture the
map[string]interface{} payload passed to SavePluginConfig (when
OnConfigurationChange calls SavePluginConfig), then assert the regenerated
fields (EncryptionKey, Secret, ForgeSharedSecret) are exactly 32 chars long and
that any pre-existing valid fields remain unchanged; for the "regenerates all
three" case assert all three are 32 chars, and for the "does not overwrite
existing valid secrets" case assert SavePluginConfig is not called or, if
called, that the fields equal the original values. Ensure you still load the
config via LoadPluginConfiguration and reference OnConfigurationChange,
LoadPluginConfiguration, SavePluginConfig and the config.Configuration struct
when locating code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ea6f152-5767-41fb-896f-d794653e61cc
📒 Files selected for processing (9)
forge/manifest.ymlforge/src/index.tsserver/command.goserver/flow.goserver/forge_poller.goserver/plugin.goserver/plugin_test.goserver/serializer/confluence_forge.goserver/user.go
💤 Files with no reviewable changes (1)
- server/user.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/flow.go (1)
63-78:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't skip the Cloud URL step in the shared setup flow.
setupFlowfirst saves the genericsubmitConfluenceURLresult withIsCloud = false, then the Cloud button jumps straight tostepCloudOAuthConfigure. That bypassessubmitCloudConfluenceURL, so the Cloud-only URL normalization/persistence never runs and a self-hosted URL can reach the 3LO/Forge path.Also applies to: 374-412, 691-698
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/flow.go` around lines 63 - 78, The shared setup flow skips the Cloud URL normalization because the Cloud button jumps directly to fm.stepCloudOAuthConfigure after submitting the generic submitConfluenceURL (IsCloud = false), so ensure the cloud path runs the cloud-specific URL handler: modify the flow so the cloud branch invokes the cloud URL step/handler (e.g., call submitCloudConfluenceURL or insert a cloud-specific step such as fm.stepCloudInstanceURL / fm.stepInstanceURL-cloud) before fm.stepCloudOAuthConfigure; update setupFlow and any other locations flagged (the other blocks around lines noted) so that submitCloudConfluenceURL is executed for Cloud users prior to fm.stepCloudOAuthConfigure to guarantee Cloud-only normalization/persistence runs.
♻️ Duplicate comments (2)
server/forge_poller.go (2)
117-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRevalidate
ForgeDrainURLbefore polling.The wizard now validates pasted URLs, but the poller still trusts whatever is stored in config. Since
ForgeDrainURLis also admin-configurable, this path will keep sending HMAC-signed requests to any host present there unless you enforce the same Forge-webtrigger check here beforefp.post.Also applies to: 155-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/forge_poller.go` around lines 117 - 129, Revalidate the admin-provided ForgeDrainURL inside drainOnce before calling fp.post: check that cfg.ForgeDrainURL passes the same forge/webtrigger validation used by the wizard (call the shared validation function the wizard uses, e.g. ValidateForgeURL/IsValidForgeWebtrigger) and return nil or an error if it fails; do this check in drainOnce (and the similar block at lines 155-163) so HMAC-signed requests are only sent to validated hosts even when the URL is stored in config, and only proceed to fp.post when the URL is accepted.
137-150:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAck Forge events only after notification delivery finishes.
dispatchstill returnsnilas soon as it spawnsSendConfluenceNotifications, and this loop still appends every key toackKeys. Any downstream send failure or shutdown race turns into a permanent drop instead of a retry.Also applies to: 181-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/forge_poller.go` around lines 137 - 150, The current loop acknowledges all Forge events immediately because fp.dispatch returns nil after spawning SendConfluenceNotifications; change the logic so that you only append an event's key to ackKeys after its notification delivery completes successfully. Modify fp.dispatch (or provide a new blocking variant) so it returns an error only after SendConfluenceNotifications finishes (or return a result/channel you wait on), then in the loop over resp.Events wait for that completion and only ack (append evt.Key) on success; log and skip ack on failures so the post call (fp.post with forgeDrainRequest) only includes keys for confirmed deliveries. Apply the same fix to the duplicate ack loop referenced at lines 181-195.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/flow.go`:
- Around line 63-78: The shared setup flow skips the Cloud URL normalization
because the Cloud button jumps directly to fm.stepCloudOAuthConfigure after
submitting the generic submitConfluenceURL (IsCloud = false), so ensure the
cloud path runs the cloud-specific URL handler: modify the flow so the cloud
branch invokes the cloud URL step/handler (e.g., call submitCloudConfluenceURL
or insert a cloud-specific step such as fm.stepCloudInstanceURL /
fm.stepInstanceURL-cloud) before fm.stepCloudOAuthConfigure; update setupFlow
and any other locations flagged (the other blocks around lines noted) so that
submitCloudConfluenceURL is executed for Cloud users prior to
fm.stepCloudOAuthConfigure to guarantee Cloud-only normalization/persistence
runs.
---
Duplicate comments:
In `@server/forge_poller.go`:
- Around line 117-129: Revalidate the admin-provided ForgeDrainURL inside
drainOnce before calling fp.post: check that cfg.ForgeDrainURL passes the same
forge/webtrigger validation used by the wizard (call the shared validation
function the wizard uses, e.g. ValidateForgeURL/IsValidForgeWebtrigger) and
return nil or an error if it fails; do this check in drainOnce (and the similar
block at lines 155-163) so HMAC-signed requests are only sent to validated hosts
even when the URL is stored in config, and only proceed to fp.post when the URL
is accepted.
- Around line 137-150: The current loop acknowledges all Forge events
immediately because fp.dispatch returns nil after spawning
SendConfluenceNotifications; change the logic so that you only append an event's
key to ackKeys after its notification delivery completes successfully. Modify
fp.dispatch (or provide a new blocking variant) so it returns an error only
after SendConfluenceNotifications finishes (or return a result/channel you wait
on), then in the loop over resp.Events wait for that completion and only ack
(append evt.Key) on success; log and skip ack on failures so the post call
(fp.post with forgeDrainRequest) only includes keys for confirmed deliveries.
Apply the same fix to the duplicate ack loop referenced at lines 181-195.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c08f3ad-e5da-4c18-a46e-28be106521b3
📒 Files selected for processing (3)
server/flow.goserver/forge_poller.goserver/serializer/confluence_forge.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Tremendous work! I had a few general questions about how we want to prepare for production rollout plus some smaller comments but things are looking great!
| }; | ||
| }; | ||
|
|
||
| const PLACEHOLDER_APP_ID = 'REPLACE_WITH_FORGE_APP_ID'; |
There was a problem hiding this comment.
Wondering if we want this to be and env var that we inject via the repository variables since it's mainly something to run in CI
There was a problem hiding this comment.
This was from previous implementation so i am dropping it as the check was dead code. forge create always writes a real ID. CI substitution of the prod ID into manifest.yml belongs in the deploy workflow
|
|
||
| We `forge deploy` this app once into the Mattermost Atlassian developer | ||
| account, then share a private install link. Customers do NOT run | ||
| `forge deploy` themselves. |
There was a problem hiding this comment.
I imagine we'll be adding a workflow with secret env vars that can potentially handle the forge deployments to a specific version for the production deployments? I'm wondering whether we want to have something made in the delivery platform repo that can be shared between Confluence and Jira but we can figure this out as a follow-up
There was a problem hiding this comment.
thats also a good point, we need to keep in mind before merging and when we have the Forge app. Question is who might own this?
There was a problem hiding this comment.
I could look into that (likely with some help from someone in SRE), we might need to discuss the current game plan for how we'll deploy and maintain this in the next team meeting beforehand
|
|
||
| const confluenceCloudScopes = "offline_access, read:confluence-user, read:confluence-content.summary, read:confluence-content.all, read:confluence-space.summary, write:confluence-content" | ||
|
|
||
| var forgeInstallURL = "https://developer.atlassian.com/console/install/mattermost-confluence-bridge" |
There was a problem hiding this comment.
Wondering if this is a value we want to have configurable via the system console, just to both simplify testing and to make possible for customers to deploy and connect to their own forge bridge apps if they so choose (but mainly the testing bit :D)
There was a problem hiding this comment.
yes :D turned it into a required System Console setting. If no URL is configured since today we don't have a prod Forge app deployed yet the wizard shows a warning. Once we have the prod app live sysadmin pastes the install URL once!
| ":wave: Welcome — let's connect Mattermost to a Confluence **Cloud** site.\n\n"+ | ||
| "This sets up two things:\n"+ | ||
| "1. **OAuth 2.0** so Mattermost can act on Confluence as your users.\n"+ | ||
| "2. A small **Forge bridge app** that delivers Confluence events into Mattermost (the GA replacement for the Connect descriptor Atlassian removed on March 31, 2026).\n\n"+ |
There was a problem hiding this comment.
I guess we'll want a task to eventually removed the text in parenthesis once it's no longer relevant (or just remove it altogether now and document this change in other places)
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
The code changes themselves look good! I think we can start QAing and getting some eyes from security on this while we also define how to go about production deployments
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
* MM-67455: DM connected users when @-mentioned in Confluence * PR feedback * Linter * coderabbitai feedback * README.md --------- Co-authored-by: Nevyana Angelova <nevyangelova@192.168.100.47>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
server/service/mention_settings.go (1)
15-21: 💤 Low valueConsider logging KV errors for observability.
The fail-open behavior (defaulting to enabled on error) is reasonable for mention notifications, but silently ignoring
appErron line 17 could mask KV infrastructure issues during troubleshooting.📊 Suggested observability enhancement
func IsMentionNotificationEnabled(mattermostUserID string) bool { data, appErr := config.Mattermost.KVGet(mentionNotifKey(mattermostUserID)) - if appErr != nil || len(data) == 0 { + if appErr != nil { + config.Mattermost.LogDebug("mention settings: KV get failed, defaulting enabled", "user_id", mattermostUserID, "error", appErr.Error()) + return true + } + if len(data) == 0 { return true } return string(data) != "0" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/mention_settings.go` around lines 15 - 21, In IsMentionNotificationEnabled, keep the current fail-open behavior but surface KV errors by logging appErr when config.Mattermost.KVGet(mentionNotifKey(mattermostUserID)) returns an error; add a log call that includes the error, the mattermostUserID and the key (mentionNotifKey(...)) for context (use the project's standard logger used elsewhere) and then continue to return true on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forge/src/index.ts`:
- Around line 153-155: The force re-registration path currently skips any proof
of ownership (the check around storage.get(REGISTERED_KEY) and payload.force in
the register handler), allowing anyone with the URL to overwrite the secret;
change the handler so that when payload.force is true you validate a proof
derived from the existing stored secret (e.g., verify an HMAC or signature
included in the request using the current secret fetched via
storage.get(REGISTERED_KEY)) before accepting the rotation, and reject the
request with 403 if the HMAC/signature verification fails; apply the same
verification to the other forced-overwrite branch referenced near lines 166-170
so all forced resets require the existing-secret proof.
- Around line 36-38: The enqueue logic (around storage.set in function enqueue)
never bounds queued items; before calling storage.set(key, …) add queue-size
control: define a MAX_QUEUE_LENGTH constant and query existing queued items
(e.g., storage.list or storage.get with the same key prefix) to count entries,
and if the count >= MAX_QUEUE_LENGTH either evict the oldest entries (sort by
enqueuedAt and delete via storage.delete) to make room or return an
error/backpressure instead of writing; ensure you use the same key prefix and
the stored enqueuedAt timestamp to identify oldest items, update logging to
reflect evictions or rejection, and keep storage.set only after space has been
guaranteed.
In `@server/confluence_server.go`:
- Around line 423-434: The code is constructing Cloud-style page URLs in the
Page/Comment branches (variables pageURL, eventData.Page, eventData.Comment)
instead of using the Server/DC-provided webui link in the payload; change each
branch that sets pageURL to first use the payload's _links.webui (e.g.,
eventData.Page._links.webui or eventData.Comment._links.webui) when present and
only fall back to the fmt.Sprintf("/spaces/%s/pages/%s", ...) logic if the webui
link is missing, and keep the existing calls to
client.MentionAccountIDsInPage/MentionAccountIDsInComment unchanged.
In `@server/forge_poller.go`:
- Around line 223-234: The debug logs currently emit raw Atlassian account IDs
(accountIDs and evt.ActorAccountID()) which is PII; update the log.Debug calls
in the mention handling (the one referencing internalEvent, evt.Content.ID,
accountIDs and the later one referencing evt.ActorAccountID()) to omit the
actual ID slices/strings and only log non-PII values such as mention_count
(len(accountIDs)) and other content metadata (e.g., internalEvent,
evt.Content.ID, pageURL, kind, instanceID); remove or replace any direct
references to accountIDs and evt.ActorAccountID() in the log arguments while
preserving the rest of the diagnostic fields.
In `@server/store/store.go`:
- Around line 166-170: The delete path currently removes the reverse AccountID
-> mattermostUserID mapping even for the admin sentinel, undoing the store
guard; in DeleteConnectionFromKVStore add the same sentinel check (compare
mattermostUserID to AdminMattermostUserID) and skip deleting the reverse mapping
when it's the admin sentinel so the real user's reverse mapping is preserved;
locate the reverse-mapping cleanup logic in DeleteConnectionFromKVStore and wrap
it with the same conditional used where mappings are stored.
---
Nitpick comments:
In `@server/service/mention_settings.go`:
- Around line 15-21: In IsMentionNotificationEnabled, keep the current fail-open
behavior but surface KV errors by logging appErr when
config.Mattermost.KVGet(mentionNotifKey(mattermostUserID)) returns an error; add
a log call that includes the error, the mattermostUserID and the key
(mentionNotifKey(...)) for context (use the project's standard logger used
elsewhere) and then continue to return true on error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: daa55d30-65ff-4aa4-b5c5-a359dfe5ef81
📒 Files selected for processing (21)
forge/README.mdforge/manifest.ymlforge/src/index.tsplugin.jsonserver/client.goserver/client_cloud.goserver/client_server.goserver/command.goserver/confluence_server.goserver/flow.goserver/forge_poller.goserver/serializer/confluence_forge.goserver/service/adf.goserver/service/adf_test.goserver/service/mention_settings.goserver/service/mentions.goserver/service/mentions_test.goserver/service/notification_test.goserver/service/storage_xhtml.goserver/service/storage_xhtml_test.goserver/store/store.go
✅ Files skipped from review due to trivial changes (1)
- forge/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- plugin.json
- server/client_cloud.go
- forge/manifest.yml
- server/serializer/confluence_forge.go
- server/command.go
- server/flow.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forge/src/index.ts`:
- Around line 153-154: The current unconditional 409 when
storage.get(REGISTERED_KEY) is present prevents idempotent re-registration and
hides secret drift; in the handler around storage.get(REGISTERED_KEY) replace
the unconditional jsonResponse(409, ...) with logic that reads the stored
mm.drainSecret and compares it to the posted secret: if they match, return 200
(idempotent success), if they differ return a non-409 error (e.g., 400/422 with
a clear message that mm.registered must be cleared) so the setup flow halts;
update the check that uses REGISTERED_KEY and use the posted secret vs stored
mm.drainSecret comparison to decide the response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f6b2d02a-58ad-425b-900e-3fa107b44777
📒 Files selected for processing (6)
forge/README.mdforge/src/index.tsserver/confluence_server.goserver/forge_poller.goserver/serializer/confluence_forge.goserver/store/store.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/forge_poller.go
- server/serializer/confluence_forge.go
- server/confluence_server.go
- forge/README.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tested upgrade scenario and fresh install, both working as expected. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
3 participants
Summary
Atlassian removed the "Install a private app" path for new Confluence Cloud sites on March 31, 2026 and new customers can no longer connect Mattermost to Confluence Cloud at all and existing installs keep working until Atlassian's Q4 2026 end of support.
This POC replaces the dead Connect descriptor path with OAuth 2.0 (3LO) for user auth and a small Forge app for event
delivery. Legacy Connect installs are untouched and the webhook receiver at
/cloud/{event}stays so events from pre-cutoff customers keep flowing until Q4 2026.
IsCloudconfig flag, set by the wizard, that branches/confluence connectbetween Atlassian's 3LO endpoints and the existing Server OAuth path.
/confluence install cloud) that walks admins through registering a 3LO app at developer.atlassian.com/console, pasting client ID and installing the Forge bridge.CompleteOAuth2split intocompleteServerOAuth2/completeCloudOAuth2.ForgeSharedSecret(auto-generated) andForgeDrainURL(admin pastes the URL from the Forge install).forge deployonce into the Mattermost developer account and put the link in the wizard.server/atlassian_connect.goandassets/templates/atlassian-connect.jsoninstallCloudHelpconst andGetAtlassianConnectURLPath()helper.Ticket Link
https://mattermost.atlassian.net/browse/MM-68853
Change Impact: 🔴 High
Reasoning: The PR introduces Cloud OAuth2 (3LO) onboarding, a Mattermost-managed Forge bridge with HMAC-signed poll/drain mechanics, new configuration/secret handling (IsCloud, ForgeSharedSecret, ForgeDrainURL), and removes the legacy Connect descriptor—changes spanning authentication, event ingestion, background processing, and installation lifecycles.
Regression Risk: Very High — it touches authentication/token exchange, cross-service HMAC-signed communication, scheduled poller lifecycle, configuration auto-generation, event serialization/dispatch, and public plugin endpoints; defects could disrupt installs, event delivery, or plugin startup.
** QA Recommendation:** Do not skip manual QA. Perform comprehensive end-to-end tests for Cloud and Server OAuth flows, Forge install/register/drain (signature verification, conflict/timeout handling, replay/ack behavior), poller scheduling/stop on activation/deactivation, secrets auto-generation and persistence, and regression tests for existing Server/DC webhook and mention flows. Validate the Forge bridge in staging with representative Confluence Cloud instances before production rollout.
Generated by CodeRabbitAI