Add admin API token fallback for watcher notifications#1298
Add admin API token fallback for watcher notifications#1298nevyangelova wants to merge 2 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughcheckIssueWatchers now treats connected-user fetch errors as terminal; if no connected Jira client is available and AdminAPIToken is configured, it falls back to fetching issue watchers via a new exported GetWatchersWithAPIToken HTTP call to the Jira REST API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant checkIssueWatchers
participant ConnectedUserClient
participant AdminTokenClient
participant JiraAPI
Client->>checkIssueWatchers: trigger watcher check
checkIssueWatchers->>ConnectedUserClient: fetch connected user client
alt fetchConnectedUser returns error
checkIssueWatchers-->>Client: return (terminal error)
else connected client != nil
checkIssueWatchers->>ConnectedUserClient: GetWatchers(issueID)
ConnectedUserClient->>JiraAPI: GET /rest/api/2/issue/{issueID}/watchers (user auth)
JiraAPI-->>ConnectedUserClient: 200 + watchers JSON
ConnectedUserClient-->>checkIssueWatchers: watchers
else connected client is nil and AdminAPIToken set
checkIssueWatchers->>AdminTokenClient: GetWatchersWithAPIToken(issueID, instanceID)
AdminTokenClient->>JiraAPI: GET /rest/api/2/issue/{issueID}/watchers (admin token)
JiraAPI-->>AdminTokenClient: 200 + watchers JSON
AdminTokenClient->>AdminTokenClient: validate + unmarshal JSON
AdminTokenClient-->>checkIssueWatchers: jira.Watches
end
checkIssueWatchers-->>Client: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request introduces a critical potential Server-Side Request Forgery (SSRF) in server/issue.go: GetWatchersWithAPIToken constructs and requests a URL using an unvalidated instanceID parameter, allowing an attacker-controlled value (including internal IPs) to trigger arbitrary HTTP requests. The finding is rated critical but non-blocking and should be fixed by validating/restricting instanceID, enforcing allowed hosts or schemes, and using safe HTTP client patterns.
🔴 Potential Server-Side Request Forgery (SSRF) in
|
| Vulnerability | Potential Server-Side Request Forgery (SSRF) |
|---|---|
| Description | GetWatchersWithAPIToken constructs a URL using the instanceID parameter as the base URL and performs an HTTP request (httpClient.Do) without validating or restricting the instanceID value. If instanceID is attacker-controllable and contains an arbitrary URL (including internal IPs), this allows SSRF. |
mattermost-plugin-jira/server/issue.go
Lines 1913 to 1916 in a0ade1c
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/issue.go`:
- Around line 1684-1694: The current branch treats a non-nil err from the
connected-user lookup the same as a nil client (no linked user) and falls back
to the admin token; change the logic so that if err != nil you log and return
immediately (using p.errorf and the existing error message), and only when err
== nil && client == nil (i.e., no linked Jira user) should you attempt the
admin-token fallback via p.GetWatchersWithAPIToken; keep references to
GetWatchersWithAPIToken, p.getConfig().AdminAPIToken, and p.errorf to locate and
update the branch accordingly.
- Around line 1907-1919: The http.Client used in the watcher fetch is created
without a timeout which can hang; update the code in GetIssueWatchers (and
similarly in GetIssueDataWithAPIToken and GetProjectListWithAPIToken) to
construct an http.Client with a sensible Timeout (e.g., time.Second * X) before
calling http.NewRequest/Do, and reuse that client variable (httpClient) for the
request; ensure you import time if not present and preserve existing error
handling (SetAdminAPITokenRequestHeader, errors.Wrapf) while setting the timeout
on the client prior to httpClient.Do.
🪄 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 (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5b04062e-10db-4a05-b6d1-71d7370808b8
📒 Files selected for processing (1)
server/issue.go
…ent timeout - Only fall back to admin API token when no connected user is found (client == nil), not on hard errors like instance store failures - Add 30s timeout to HTTP client in GetWatchersWithAPIToken to prevent indefinite blocking on unresponsive Jira servers Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/issue.go`:
- Around line 1911-1915: The request URL in GetWatchersWithAPIToken currently
uses the store key parameter instanceID instead of the Jira base URL; update the
function to resolve the instance object (via the existing LoadInstance or
equivalent on Plugin) using the instanceID key and use the instance's
BaseURL/URL (e.g., instance.BaseURL or instance.URL) when formatting the request
to "/rest/api/2/issue/{issueID}/watchers"; ensure you still handle LoadInstance
errors and preserve the existing error wrapping and http client usage.
🪄 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 (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f7e43f74-f92b-43b9-ac05-d6cc380303bc
📒 Files selected for processing (1)
server/issue.go
| func (p *Plugin) GetWatchersWithAPIToken(issueID, instanceID string) (*jira.Watches, error) { | ||
| httpClient := &http.Client{Timeout: 30 * time.Second} | ||
| req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/rest/api/2/issue/%s/watchers", instanceID, issueID), nil) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to create http request for fetching watchers. IssueID: %s", issueID) |
There was a problem hiding this comment.
Use the Jira base URL here, not the instance ID.
Line 1694 passes instanceID.String(), and this helper formats that value directly into the request URL on Line 1913. In this file, instanceID is the store key used by LoadInstance, not the Jira host, so the new fallback can build an invalid URL and never reach Jira.
🔧 Suggested fix
func (p *Plugin) GetWatchersWithAPIToken(issueID, instanceID string) (*jira.Watches, error) {
+ instance, err := p.instanceStore.LoadInstance(types.ID(instanceID))
+ if err != nil {
+ return nil, errors.Wrapf(err, "failed to load instance. InstanceID: %s", instanceID)
+ }
+
httpClient := &http.Client{Timeout: 30 * time.Second}
- req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/rest/api/2/issue/%s/watchers", instanceID, issueID), nil)
+ req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/rest/api/2/issue/%s/watchers", instance.GetJiraBaseURL(), issueID), nil)
if err != nil {
return nil, errors.Wrapf(err, "failed to create http request for fetching watchers. IssueID: %s", issueID)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *Plugin) GetWatchersWithAPIToken(issueID, instanceID string) (*jira.Watches, error) { | |
| httpClient := &http.Client{Timeout: 30 * time.Second} | |
| req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/rest/api/2/issue/%s/watchers", instanceID, issueID), nil) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "failed to create http request for fetching watchers. IssueID: %s", issueID) | |
| func (p *Plugin) GetWatchersWithAPIToken(issueID, instanceID string) (*jira.Watches, error) { | |
| instance, err := p.instanceStore.LoadInstance(types.ID(instanceID)) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "failed to load instance. InstanceID: %s", instanceID) | |
| } | |
| httpClient := &http.Client{Timeout: 30 * time.Second} | |
| req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/rest/api/2/issue/%s/watchers", instance.GetJiraBaseURL(), issueID), nil) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "failed to create http request for fetching watchers. IssueID: %s", issueID) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/issue.go` around lines 1911 - 1915, The request URL in
GetWatchersWithAPIToken currently uses the store key parameter instanceID
instead of the Jira base URL; update the function to resolve the instance object
(via the existing LoadInstance or equivalent on Plugin) using the instanceID key
and use the instance's BaseURL/URL (e.g., instance.BaseURL or instance.URL) when
formatting the request to "/rest/api/2/issue/{issueID}/watchers"; ensure you
still handle LoadInstance errors and preserve the existing error wrapping and
http client usage.
There was a problem hiding this comment.
This is a false positive. No fix needed.
In this codebase, types.ID for instances is the Jira base URL (e.g., https://jira-stage.veevadev.com), not an opaque store key. The evidence:
instances.go line 267: instanceID := types.ID(instanceURL) — it's literally constructed from the normalized Jira URL
GetIssueDataWithAPIToken uses instanceID the exact same way: fmt.Sprintf("%s/rest/api/2/issue/%s", instanceID, issueID)
GetProjectListWithAPIToken uses it the same way: fmt.Sprintf("%s/rest/api/3/project/search", instanceID)
The original bug report even shows the URL in the logs: "error while fetching connected users for the instanceID https://jira-stage.veevadev.com"
All three WithAPIToken functions follow the identical pattern.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that instanceID for instances is the Jira base URL
rg -n "instanceID.*types.ID\|types.ID.*instanceURL\|instanceURL" --type go -C2 server/instances.go | head -60
# Also check the WithAPIToken pattern usage
rg -n "WithAPIToken" --type go -C3
Length of output: 2785
@nang2049, you're right — my comment was incorrect. The script output confirms the pattern across the codebase: GetIssueDataWithAPIToken and GetProjectListWithAPIToken both use instanceID directly as the URL base in the same way, and webhook_jira.go Line 76 shows instance.GetID().String() being passed as the instanceID argument to GetIssueDataWithAPIToken, confirming the instance ID is the normalized Jira base URL. GetWatchersWithAPIToken correctly follows the same established pattern. Apologies for the noise.
✏️ Learnings added
Learnt from: nang2049
Repo: mattermost/mattermost-plugin-jira PR: 1298
File: server/issue.go:1911-1915
Timestamp: 2026-04-15T12:42:52.679Z
Learning: In the mattermost-plugin-jira codebase (server/), `types.ID` for a Jira instance is the normalized Jira base URL (e.g., "https://jira-stage.veevadev.com"), not an opaque store key. All three `*WithAPIToken` helpers (`GetIssueDataWithAPIToken`, `GetProjectListWithAPIToken`, `GetWatchersWithAPIToken`) correctly use `instanceID` directly as the URL base when building REST API request URLs. Do not flag this pattern as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
The changes LGTM! It might also be good to cover this new fallback scenario with test cases in issue_test.go (I'd consider it non-blocking though)
3 participants
Summary
When none of the issue's Creator, Assignee, or Reporter have connected Mattermost accounts, watcher notifications silently fail because the plugin cannot obtain a Jira API client to fetch the watcher list. This adds a fallback to use the configured Admin API Token to retrieve watchers, matching the existing fallback pattern used for issue data and project list fetching.
Ticket Link
https://mattermost.atlassian.net/browse/MM-68049
Change Impact: 🟠 Medium
Reasoning: The PR adds a fallback mechanism to retrieve issue watchers using an admin API token when no connected user client is available, following existing patterns in the codebase. However, the new
GetWatchersWithAPITokenmethod and its integration into thecheckIssueWatchersflow lack dedicated test coverage for the token-based fallback paths.Regression Risk: Low to moderate. The change follows established fallback patterns already used for
GetIssueDataandGetProjectListData. Error handling is explicit—hard errors fromfetchConnectedUserreturn immediately, while the fallback only triggers whenclient == nil. The token-based HTTP call uses a 30-second timeout to prevent indefinite blocking. However, the new code paths (token-based watcher retrieval) have not been explicitly tested, creating untested notification flow scenarios.QA Recommendation: Manual QA should focus on watcher notification scenarios where no connected user exists but an admin token is configured. Test both success and failure cases: (1) valid admin token retrieving watchers successfully, (2) missing/invalid token, (3) unresponsive Jira server (timeout handling), and (4) malformed responses. Automated test coverage for the new
GetWatchersWithAPITokenmethod and its integration paths should be added before merging. Risk of skipping manual QA is moderate due to the gap in test coverage for token-based fallback flows.