Fix GO-2026-5410: upgrade slack-go/slack v0.15.0 → v0.23.1#629
Open
thiagoalessio wants to merge 1 commit into
Open
Fix GO-2026-5410: upgrade slack-go/slack v0.15.0 → v0.23.1#629thiagoalessio wants to merge 1 commit into
thiagoalessio wants to merge 1 commit into
Conversation
Resolves vulnerability GO-2026-5410 by upgrading github.com/slack-go/slack to v0.23.1. Adapts to breaking API changes: - UploadFileV2Parameters renamed to UploadFileParameters - RichTextPreformatted struct flattened (no longer embeds RichTextSection) - TextBlockObject.Emoji changed from bool to *bool - WorkflowStep types removed (deprecated Steps from Apps feature): defines local types and HTTP client for workflows.updateStep, workflows.stepCompleted, and workflows.stepFailed API calls - InteractionCallback.WorkflowStep field removed: extracts workflow_step_edit_id from raw JSON in handleInteraction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@thiagoalessio: No Jira issue with key GO-2026 exists in the tracker at https://redhat.atlassian.net. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thiagoalessio The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe change upgrades the Slack SDK, replaces file-upload APIs, adds an HTTP client for deprecated workflow endpoints, updates workflow event and modal handling, and adjusts modal payload construction for the newer SDK structures. ChangesSlack compatibility updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Slack
participant WorkflowHandler
participant JiraIssueFiler
participant SlackWorkflowClient
participant SlackAPI
Slack->>WorkflowHandler: workflow_step_execute event
WorkflowHandler->>JiraIssueFiler: file Jira ticket
JiraIssueFiler-->>WorkflowHandler: issue key and link
WorkflowHandler->>SlackWorkflowClient: WorkflowStepCompleted(outputs)
SlackWorkflowClient->>SlackAPI: POST workflows.stepCompleted
SlackAPI-->>SlackWorkflowClient: workflow response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@thiagoalessio: No Jira issue with key GO-2026 exists in the tracker at https://redhat.atlassian.net. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/slack/events/workflowSubmissionEvents/workflow_handler.go (1)
111-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the return statement.
The error check and explicit returns can be collapsed into a single return statement.
♻️ Proposed refactor
- err = client.WorkflowStepCompleted(event.WorkflowStep.WorkflowStepExecuteID, outgoingOutputs) - if err != nil { - return err - } - return nil + return client.WorkflowStepCompleted(event.WorkflowStep.WorkflowStepExecuteID, outgoingOutputs)🤖 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 `@pkg/slack/events/workflowSubmissionEvents/workflow_handler.go` around lines 111 - 115, In the workflow submission handler, simplify the final client.WorkflowStepCompleted call by returning its result directly instead of assigning to err, checking it, and explicitly returning nil.pkg/slack/events/workflowSubmissionEvents/types.go (1)
139-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck HTTP status code before decoding JSON.
If the Slack API returns a non-2xx status (e.g.,
502 Bad Gateway) with an HTML body, the JSON decoder will fail with a cryptic syntax error. Checking the status code first provides a clearer error message for debugging.🛠️ Proposed fix
+ if resp.StatusCode >= 300 { + return fmt.Errorf("slack API %s returned HTTP %d", method, resp.StatusCode) + } + var sr slackResponse🤖 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 `@pkg/slack/events/workflowSubmissionEvents/types.go` at line 139, In the response-handling flow around the slackResponse variable, validate the HTTP response status before attempting JSON decoding. For non-2xx responses, return a clear error containing the status information and skip decoding the body; retain the existing JSON decoding path for successful responses.cmd/ci-chat-bot/slack.go (1)
64-66: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd
ReadTimeoutto the HTTP server.While
ReadHeaderTimeoutmitigates basic Slowloris attacks targeting headers, a malicious client could still slowly trickle the request body and exhaust connections. Adding aReadTimeoutbounds the total time allowed to read the full request (including the body).🛡️ Proposed configuration
mux.Handle("/slack/interactive-endpoint", handler(handleInteraction(bot.BotSigningSecret, interactionrouter.ForModals(slackclient, jobManager, httpclient, bot.BotToken)))) - server := &http.Server{Addr: ":" + strconv.Itoa(bot.Port), Handler: mux, ReadHeaderTimeout: 10 * time.Second} + server := &http.Server{ + Addr: ":" + strconv.Itoa(bot.Port), + Handler: mux, + ReadHeaderTimeout: 10 * time.Second, + ReadTimeout: 30 * time.Second, + }🤖 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 `@cmd/ci-chat-bot/slack.go` around lines 64 - 66, Update the http.Server configuration in the server initialization to include a ReadTimeout that bounds the total request-read duration, including the body, while preserving the existing ReadHeaderTimeout setting.Source: Linters/SAST tools
pkg/slack/modals/common/version_views.go (1)
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the SDK constructor over manual struct initialization.
While using
new(bool)functionally provides the pointer required by the upgradedslack-go/slackSDK, manually instantiatingTextBlockObjectliterals leaves your code vulnerable to future upstream struct alterations. It is highly recommended to use the SDK's native constructor (slackClient.NewTextBlockObject), which safely handles pointer extraction for you and shrinks visual boilerplate (a pattern that is already appropriately used inpkg/slack/modals/common/simple_modals.go).
pkg/slack/modals/common/version_views.go#L107-L112: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "Version Specifications", false, false),pkg/slack/modals/common/version_views.go#L163-L168: Replace the struct literal withslackClient.NewTextBlockObject(slackClient.PlainTextType, config.ContextMetadata, false, false),pkg/slack/modals/common/version_views.go#L241-L246: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "Select a Version", false, false),pkg/slack/modals/common/version_views.go#L270-L275: Replace the struct literal withslackClient.NewTextBlockObject(slackClient.PlainTextType, config.ContextMetadata, false, false),pkg/slack/modals/common/version_views.go#L344-L349: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "There are too many results from the selected Stream. Select a Major.Minor as well", false, false),pkg/slack/modals/common/version_views.go#L373-L378: Replace the struct literal withslackClient.NewTextBlockObject(slackClient.PlainTextType, config.ContextMetadata, false, false),pkg/slack/modals/common/version_views.go#L404-L409: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "Enter A PR", false, false),pkg/slack/modals/common/version_views.go#L428-L433: Replace the struct literal withslackClient.NewTextBlockObject(slackClient.PlainTextType, config.ContextMetadata, false, false),pkg/slack/modals/common/version_views.go#L496-L501: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "Do you want to launch from a PR?", false, false),pkg/slack/modals/common/version_views.go#L522-L527: Replace the struct literal withslackClient.NewTextBlockObject(slackClient.PlainTextType, config.ContextMetadata, false, false),pkg/slack/modals/launch/views.go#L46-L51: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "Select the Launch Platform and Architecture", false, false),pkg/slack/modals/launch/views.go#L139-L144: Replace the struct literal withslackClient.NewTextBlockObject(slackClient.PlainTextType, context, false, false),pkg/slack/modals/list/views.go#L20-L25: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "See who is hogging all the clusters", false, false),pkg/slack/modals/mce/create/views.go#L51-L56: Replace the struct literal withText: slackClient.NewTextBlockObject(slackClient.PlainTextType, "Select the Launch Platform and Duration", false, false),pkg/slack/modals/mce/create/views.go#L117-L122: Replace the struct literal withslackClient.NewTextBlockObject(slackClient.PlainTextType, context, false, false),🤖 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 `@pkg/slack/modals/common/version_views.go` around lines 107 - 112, Replace each manual slackClient.TextBlockObject literal with slackClient.NewTextBlockObject, preserving the existing PlainTextType, text value, and false/false arguments. Apply this in pkg/slack/modals/common/version_views.go at lines 107-112, 163-168, 241-246, 270-275, 344-349, 373-378, 404-409, 428-433, 496-501, and 522-527; pkg/slack/modals/launch/views.go at lines 46-51 and 139-144; pkg/slack/modals/list/views.go at lines 20-25; and pkg/slack/modals/mce/create/views.go at lines 51-56 and 117-122.
🤖 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 `@pkg/slack/events/workflowSubmissionEvents/types.go`:
- Around line 36-43: Remove redundant pointers from WorkflowStepInputs and
WorkflowStepOutput throughout eventWorkflowStep, workflowUpdateStepRequest, and
SaveWorkflowStepConfiguration, using non-pointer map and slice types so nil
values remain safely rangeable. In
pkg/slack/events/workflowSubmissionEvents/workflow_handler.go, remove
dereferences of Inputs and Outputs. In pkg/slack/interactions/router/router.go,
pass input and output values directly and update the slackClient interface
signature to match; apply these changes at types.go lines 36-43, 73-77, and
112-120, workflow_handler.go lines 58-74, 76-116, and 118-144, and router.go
lines 96-101 and 124-127.
- Line 137: Update the defer around resp.Body.Close() to explicitly discard or
handle its returned error, preserving the existing response-body cleanup while
satisfying golangci-lint.
- Line 126: Update the request construction in the client method containing
http.NewRequestWithContext to derive the context with context.WithTimeout
instead of context.Background(), using the appropriate existing or defined
duration and ensuring the cancel function is released. Pass the timed context to
the POST request so unresponsive Slack API calls terminate within the bound.
---
Nitpick comments:
In `@cmd/ci-chat-bot/slack.go`:
- Around line 64-66: Update the http.Server configuration in the server
initialization to include a ReadTimeout that bounds the total request-read
duration, including the body, while preserving the existing ReadHeaderTimeout
setting.
In `@pkg/slack/events/workflowSubmissionEvents/types.go`:
- Line 139: In the response-handling flow around the slackResponse variable,
validate the HTTP response status before attempting JSON decoding. For non-2xx
responses, return a clear error containing the status information and skip
decoding the body; retain the existing JSON decoding path for successful
responses.
In `@pkg/slack/events/workflowSubmissionEvents/workflow_handler.go`:
- Around line 111-115: In the workflow submission handler, simplify the final
client.WorkflowStepCompleted call by returning its result directly instead of
assigning to err, checking it, and explicitly returning nil.
In `@pkg/slack/modals/common/version_views.go`:
- Around line 107-112: Replace each manual slackClient.TextBlockObject literal
with slackClient.NewTextBlockObject, preserving the existing PlainTextType, text
value, and false/false arguments. Apply this in
pkg/slack/modals/common/version_views.go at lines 107-112, 163-168, 241-246,
270-275, 344-349, 373-378, 404-409, 428-433, 496-501, and 522-527;
pkg/slack/modals/launch/views.go at lines 46-51 and 139-144;
pkg/slack/modals/list/views.go at lines 20-25; and
pkg/slack/modals/mce/create/views.go at lines 51-56 and 117-122.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 476b0573-8459-468f-9de0-37e8ba69449c
⛔ Files ignored due to path filters (89)
go.sumis excluded by!**/*.sumvendor/github.com/slack-go/slack/.gitignoreis excluded by!vendor/**vendor/github.com/slack-go/slack/.golangci.ymlis excluded by!vendor/**vendor/github.com/slack-go/slack/CHANGELOG.mdis excluded by!vendor/**vendor/github.com/slack-go/slack/CONTRIBUTING.mdis excluded by!vendor/**vendor/github.com/slack-go/slack/Makefileis excluded by!vendor/**vendor/github.com/slack-go/slack/README.mdis excluded by!vendor/**vendor/github.com/slack-go/slack/TODO.txtis excluded by!vendor/**vendor/github.com/slack-go/slack/admin.gois excluded by!vendor/**vendor/github.com/slack-go/slack/admin_conversations.gois excluded by!vendor/**vendor/github.com/slack-go/slack/admin_conversations_ekm.gois excluded by!vendor/**vendor/github.com/slack-go/slack/admin_conversations_restrictAccess.gois excluded by!vendor/**vendor/github.com/slack-go/slack/admin_roles.gois excluded by!vendor/**vendor/github.com/slack-go/slack/admin_teams.gois excluded by!vendor/**vendor/github.com/slack-go/slack/apps.gois excluded by!vendor/**vendor/github.com/slack-go/slack/assistant.gois excluded by!vendor/**vendor/github.com/slack-go/slack/attachments.gois excluded by!vendor/**vendor/github.com/slack-go/slack/audit.gois excluded by!vendor/**vendor/github.com/slack-go/slack/auth.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_action.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_alert.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_call.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_card.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_carousel.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_context.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_context_actions.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_conv.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_divider.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_element.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_file.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_header.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_image.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_input.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_json.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_markdown.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_object.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_plan.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_rich_text.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_section.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_table.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_task_card.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_unknown.gois excluded by!vendor/**vendor/github.com/slack-go/slack/block_video.gois excluded by!vendor/**vendor/github.com/slack-go/slack/channels.gois excluded by!vendor/**vendor/github.com/slack-go/slack/chat.gois excluded by!vendor/**vendor/github.com/slack-go/slack/chat_stream_chunks.gois excluded by!vendor/**vendor/github.com/slack-go/slack/conversation.gois excluded by!vendor/**vendor/github.com/slack-go/slack/dialog.gois excluded by!vendor/**vendor/github.com/slack-go/slack/dnd.gois excluded by!vendor/**vendor/github.com/slack-go/slack/entity.gois excluded by!vendor/**vendor/github.com/slack-go/slack/files.gois excluded by!vendor/**vendor/github.com/slack-go/slack/function_execute.gois excluded by!vendor/**vendor/github.com/slack-go/slack/huddle.gois excluded by!vendor/**vendor/github.com/slack-go/slack/im.gois excluded by!vendor/**vendor/github.com/slack-go/slack/info.gois excluded by!vendor/**vendor/github.com/slack-go/slack/interactions.gois excluded by!vendor/**vendor/github.com/slack-go/slack/manifests.gois excluded by!vendor/**vendor/github.com/slack-go/slack/messages.gois excluded by!vendor/**vendor/github.com/slack-go/slack/metadata.gois excluded by!vendor/**vendor/github.com/slack-go/slack/migration.gois excluded by!vendor/**vendor/github.com/slack-go/slack/misc.gois excluded by!vendor/**vendor/github.com/slack-go/slack/mise.tomlis excluded by!vendor/**vendor/github.com/slack-go/slack/oauth.gois excluded by!vendor/**vendor/github.com/slack-go/slack/reactions.gois excluded by!vendor/**vendor/github.com/slack-go/slack/remotefiles.gois excluded by!vendor/**vendor/github.com/slack-go/slack/retry.gois excluded by!vendor/**vendor/github.com/slack-go/slack/rtm.gois excluded by!vendor/**vendor/github.com/slack-go/slack/search.gois excluded by!vendor/**vendor/github.com/slack-go/slack/security.gois excluded by!vendor/**vendor/github.com/slack-go/slack/slack.gois excluded by!vendor/**vendor/github.com/slack-go/slack/slackevents/action_events.gois excluded by!vendor/**vendor/github.com/slack-go/slack/slackevents/inner_events.gois excluded by!vendor/**vendor/github.com/slack-go/slack/slackevents/parsers.gois excluded by!vendor/**vendor/github.com/slack-go/slack/socket_mode.gois excluded by!vendor/**vendor/github.com/slack-go/slack/stars.gois excluded by!vendor/**vendor/github.com/slack-go/slack/team.gois excluded by!vendor/**vendor/github.com/slack-go/slack/usergroups.gois excluded by!vendor/**vendor/github.com/slack-go/slack/users.gois excluded by!vendor/**vendor/github.com/slack-go/slack/views.gois excluded by!vendor/**vendor/github.com/slack-go/slack/webhooks.gois excluded by!vendor/**vendor/github.com/slack-go/slack/websocket_groups.gois excluded by!vendor/**vendor/github.com/slack-go/slack/websocket_managed_conn.gois excluded by!vendor/**vendor/github.com/slack-go/slack/websocket_misc.gois excluded by!vendor/**vendor/github.com/slack-go/slack/workflow_step.gois excluded by!vendor/**vendor/github.com/slack-go/slack/workflow_step_execute.gois excluded by!vendor/**vendor/github.com/slack-go/slack/workflows_featured.gois excluded by!vendor/**vendor/github.com/slack-go/slack/workflows_triggers.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (16)
cmd/ci-chat-bot/slack.gogo.modpkg/slack/actions_request_test.gopkg/slack/events/router/router.gopkg/slack/events/workflowSubmissionEvents/types.gopkg/slack/events/workflowSubmissionEvents/workflow_handler.gopkg/slack/interactions/router/router.gopkg/slack/modals/common/simple_modals.gopkg/slack/modals/common/version_views.gopkg/slack/modals/launch/views.gopkg/slack/modals/list/views.gopkg/slack/modals/mce/create/views.gopkg/slack/modals/stepsFromApp/jira_step.gopkg/slack/modals/stepsFromApp/workflow_submit.gopkg/slack/parser/types.gopkg/slack/slack.go
Comment on lines
+36
to
+43
| type eventWorkflowStep struct { | ||
| WorkflowStepExecuteID string `json:"workflow_step_execute_id"` | ||
| WorkflowID string `json:"workflow_id"` | ||
| WorkflowInstanceID string `json:"workflow_instance_id"` | ||
| StepID string `json:"step_id"` | ||
| Inputs *WorkflowStepInputs `json:"inputs,omitempty"` | ||
| Outputs *[]WorkflowStepOutput `json:"outputs,omitempty"` | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Nil-pointer panic hazard: Remove redundant map and slice pointers.
WorkflowStepInputs (a map) and []WorkflowStepOutput (a slice) are being used as pointers. In Go, maps and slices are already reference types, and standard JSON omitempty correctly omits nil maps/slices without requiring pointers. Using pointers here introduces a critical risk: if the optional inputs or outputs fields are omitted in the Slack JSON payload (unmarshaling to nil), the dereferencing operations in workflow_handler.go will cause a runtime panic. Removing the pointers completely resolves the panic hazard because ranging over a nil map or slice is safe in Go.
pkg/slack/events/workflowSubmissionEvents/types.go#L36-L43: ChangeInputstoWorkflowStepInputsandOutputsto[]WorkflowStepOutput(remove the*) ineventWorkflowStep.pkg/slack/events/workflowSubmissionEvents/types.go#L73-L77: ChangeInputstoWorkflowStepInputsandOutputsto[]WorkflowStepOutputinworkflowUpdateStepRequest.pkg/slack/events/workflowSubmissionEvents/types.go#L112-L120: Change theinputsandoutputsparameters inSaveWorkflowStepConfigurationto non-pointers.pkg/slack/events/workflowSubmissionEvents/workflow_handler.go#L58-L74: Remove the*dereference from*event.WorkflowStep.Inputson line 59.pkg/slack/events/workflowSubmissionEvents/workflow_handler.go#L76-L116: Remove the*dereference from*event.WorkflowStep.Outputson line 103.pkg/slack/events/workflowSubmissionEvents/workflow_handler.go#L118-L144: Remove the*dereference from*event.WorkflowStep.Inputson line 122.pkg/slack/interactions/router/router.go#L96-L101: Passinput, outputdirectly on line 99 instead of using addresses&input, &output.pkg/slack/interactions/router/router.go#L124-L127: Update theslackClientinterface signature to match.
📍 Affects 3 files
pkg/slack/events/workflowSubmissionEvents/types.go#L36-L43(this comment)pkg/slack/events/workflowSubmissionEvents/types.go#L73-L77pkg/slack/events/workflowSubmissionEvents/types.go#L112-L120pkg/slack/events/workflowSubmissionEvents/workflow_handler.go#L58-L74pkg/slack/events/workflowSubmissionEvents/workflow_handler.go#L76-L116pkg/slack/events/workflowSubmissionEvents/workflow_handler.go#L118-L144pkg/slack/interactions/router/router.go#L96-L101pkg/slack/interactions/router/router.go#L124-L127
🤖 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 `@pkg/slack/events/workflowSubmissionEvents/types.go` around lines 36 - 43,
Remove redundant pointers from WorkflowStepInputs and WorkflowStepOutput
throughout eventWorkflowStep, workflowUpdateStepRequest, and
SaveWorkflowStepConfiguration, using non-pointer map and slice types so nil
values remain safely rangeable. In
pkg/slack/events/workflowSubmissionEvents/workflow_handler.go, remove
dereferences of Inputs and Outputs. In pkg/slack/interactions/router/router.go,
pass input and output values directly and update the slackClient interface
signature to match; apply these changes at types.go lines 36-43, 73-77, and
112-120, workflow_handler.go lines 58-74, 76-116, and 118-144, and router.go
lines 96-101 and 124-127.
| if err != nil { | ||
| return err | ||
| } | ||
| req, err := http.NewRequestWithContext(context.Background(), "POST", c.endpoint+method, bytes.NewReader(body)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the external API call.
Using context.Background() with http.DefaultClient means the HTTP request has no timeout. If the Slack API is unresponsive, this will block the worker thread indefinitely. Use context.WithTimeout to bound the request duration.
⚡ Proposed fix to add a context timeout
- req, err := http.NewRequestWithContext(context.Background(), "POST", c.endpoint+method, bytes.NewReader(body))
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ req, err := http.NewRequestWithContext(ctx, "POST", c.endpoint+method, bytes.NewReader(body))📝 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.
Suggested change
| req, err := http.NewRequestWithContext(context.Background(), "POST", c.endpoint+method, bytes.NewReader(body)) | |
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |
| defer cancel() | |
| req, err := http.NewRequestWithContext(ctx, "POST", c.endpoint+method, bytes.NewReader(body)) |
🤖 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 `@pkg/slack/events/workflowSubmissionEvents/types.go` at line 126, Update the
request construction in the client method containing http.NewRequestWithContext
to derive the context with context.WithTimeout instead of context.Background(),
using the appropriate existing or defined duration and ensuring the cancel
function is released. Pass the timed context to the POST request so unresponsive
Slack API calls terminate within the bound.
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Handle or explicitly ignore the resp.Body.Close() error.
As per coding guidelines, code must pass make verify lint. The golangci-lint tool flags the unhandled error from resp.Body.Close(). Explicitly handle or discard it to satisfy the linter and prevent CI failures.
🧹 Proposed fix to explicitly discard the error
- defer resp.Body.Close()
+ defer func() {
+ _ = resp.Body.Close()
+ }()📝 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.
Suggested change
| defer resp.Body.Close() | |
| defer func() { | |
| _ = resp.Body.Close() | |
| }() |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 137-137: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 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 `@pkg/slack/events/workflowSubmissionEvents/types.go` at line 137, Update the
defer around resp.Body.Close() to explicitly discard or handle its returned
error, preserving the existing response-body cleanup while satisfying
golangci-lint.
Sources: Coding guidelines, Linters/SAST tools
Contributor
|
@thiagoalessio: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
Resolves vulnerability GO-2026-5410 by upgrading github.com/slack-go/slack to v0.23.1. Adapts to breaking API changes:
/hold
I didn't test locally yet, just want to see if govulncheck will pass with the changes first.
Summary by CodeRabbit