feat[Go]: implement agent session/download/logs/get-agent APIs#15661
Open
hunnyboy1217 wants to merge 1 commit into
Open
feat[Go]: implement agent session/download/logs/get-agent APIs#15661hunnyboy1217 wants to merge 1 commit into
hunnyboy1217 wants to merge 1 commit into
Conversation
dosubot
Bot
added
the
size:XL
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds AgentSession model and DAO; extends AgentService with session listing, retrieval, deletion, Redis log retrieval, and storage-backed file/attachment downloads; implements HTTP handlers and registers routes under /api/v1/agents. ChangesAgent Session Management and File Download APIs
Sequence DiagramsequenceDiagram
participant AgentHandler
participant AgentService
participant AgentSessionDAO
participant Redis
participant Storage
AgentHandler->>AgentService: GetAgent(userID, agentID)
AgentService->>AgentService: Check ownership / team permission
AgentHandler->>AgentService: ListAgentSessions(agentID, params)
AgentService->>AgentSessionDAO: ListByAgentIDPaged(agentID, params)
AgentSessionDAO-->>AgentService: sessions, total
AgentService-->>AgentHandler: sessions, total
AgentHandler->>AgentService: DeleteAgentSessionByID(agentID, sessionID)
AgentService->>AgentSessionDAO: BelongsToAgent(sessionID, agentID)
AgentService->>AgentSessionDAO: DeleteByID(sessionID)
AgentHandler->>AgentService: GetAgentLogs(agentID, messageID)
AgentService->>Redis: GET formatted logs key
Redis-->>AgentService: raw payload or nil
AgentHandler->>AgentService: DownloadAgentFile(tenantID, fileID)
AgentService->>Storage: Initialize & Download blob(fileID)
Storage-->>AgentService: blob bytes
AgentService-->>AgentHandler: blob bytes, filename
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/dao/agent_session.go`:
- Around line 72-83: AgentSessionDAO.ListByAgentIDPaged currently assigns
user-controlled orderby directly to order and passes it into base.Order(order),
enabling SQL injection; replace that by validating orderby against a hardcoded
allowlist (e.g., map[string]string
{"id":"id","name":"name","create_time":"create_time","update_time":"update_time"})
and mapping the input to the canonical column name, then build order as
mappedColumn + (desc ? " DESC" : " ASC") and pass only that into base.Order;
ensure the code references the existing orderby variable, the desc boolean, and
the call site base.Order(order) so no raw user input flows into the ORDER BY
clause.
In `@internal/handler/agent.go`:
- Around line 672-684: The handler currently trusts the client-supplied ext and
serves blobs inline, enabling MIME spoofing; update the attachment download
logic (around h.agentService.DownloadAttachment, ext, utility.GetContentType,
contentType and c.Data) to ignore or not trust the ext query for determining
Content-Type, instead sniff the blob (e.g., use http.DetectContentType or
equivalent) to derive contentType and fall back to "application/octet-stream" if
unknown, map/normalize unsafe types (e.g., downgrade "text/html",
"application/javascript" to "text/plain" or "application/octet-stream"), and
force a Content-Disposition: attachment header (with a safe filename) so
responses are downloaded rather than rendered inline.
- Around line 467-470: The handler currently treats any error from agentService
as "Session not found"; update the service and handlers so not-found and
internal errors are distinguishable: in internal/service/agent.go return a
sentinel exported error (e.g. ErrSessionNotFound) or wrap gorm.ErrRecordNotFound
with %w when a record is absent in functions like
GetAgentSession/DeleteAgentSessionItem, and update the handler code that calls
h.agentService.GetAgentSession and DeleteAgentSessionItem to use errors.Is(err,
service.ErrSessionNotFound) to return common.CodeNotFound with the "Session not
found" response, otherwise log the error and return common.CodeServerError (and
an appropriate message) for internal failures; ensure service functions preserve
the original error via wrapping so errors.Is works.
- Around line 547-550: DeleteAgentSessions currently treats any error from
c.ShouldBindJSON(&body) as an empty/no-op and returns common.CodeSuccess;
instead detect genuine empty body vs malformed JSON and return an error for
malformed input. Change the c.ShouldBindJSON(&body) error handling: if the error
is an EOF/empty-body case (use errors.Is(err, io.EOF) or check for empty body)
then keep the no-op behavior, otherwise respond with HTTP 400 and a JSON error
(non-success code) describing the malformed JSON; update the branch that
currently returns common.CodeSuccess so only empty-body errors produce that
response and all other binding errors return an error response. Ensure you
modify the error handling around c.ShouldBindJSON(&body) in DeleteAgentSessions
accordingly.
In `@internal/service/agent.go`:
- Around line 304-307: The current check conflates DAO errors and ownership
falsehoods for s.agentSessionDAO.BelongsToAgent; change the logic to first
handle a non-nil err returned by BelongsToAgent (e.g. append a distinct error
like "failed to verify ownership for session %s: <err>" or record it as an
internal/DAO error) and only when err == nil evaluate belongs and append the
existing "the agent doesn't own the session %s" message; update the block around
s.agentSessionDAO.BelongsToAgent, belongs, and err to separate these two cases
so transient DB failures are not reported as ownership failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c8a59e4-9076-428e-83ca-8ee3235c28ef
📒 Files selected for processing (5)
internal/dao/agent_session.gointernal/entity/canvas.gointernal/handler/agent.gointernal/router/router.gointernal/service/agent.go
Comment on lines
+72
to
+83
| order := orderby | ||
| if order == "" { | ||
| order = "update_time" | ||
| } | ||
| if desc { | ||
| order += " DESC" | ||
| } else { | ||
| order += " ASC" | ||
| } | ||
|
|
||
| query := base.Order(order) | ||
| if page > 0 && pageSize > 0 { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GORM v1.25.x, does db.Order(string) treat input as raw SQL, requiring allowlisting/safe column construction for user-provided sort fields?
💡 Result:
Yes, in GORM v1.25.x, the db.Order(string) method treats its input as raw SQL and is not automatically sanitized against SQL injection [1][2]. Because SQL standards do not allow column names or directions to be parameterized in an ORDER BY clause, GORM does not provide built-in escaping for this method [3][4]. To securely handle user-provided sort fields, you must use an allowlist approach [1][4]. Never pass raw user input directly into db.Order [4]. Instead, map the user input to a predefined set of safe, hardcoded column names [4]. Example of secure pattern: // SECURE: Use a map to validate allowed columns allowedOrders:= map[string]string{ "id": "id", "name": "name", "price": "price", } if col, ok:= allowedOrders[userInput]; ok { db.Order(col + " asc").Find(&products) } else { // Default fallback db.Order("id asc").Find(&products) } The official GORM security documentation explicitly lists db.Order among the methods that are not escaped and requires developer caution to avoid SQL injection [1].
Citations:
- 1: https://gorm.io/docs/security.html
- 2: https://www.bookstack.cn/read/gorm-1.25.2-en/8379980f02364084.md
- 3: prevent SQL injection with Order, support ? syntax go-gorm/gorm#4579
- 4: https://www.apiposture.com/blogs/api-security-posture-management-aspm/go-api-security-securing-gin-echo-and-fiber-ecosystems/securing-go-os-exec-and-sql-preventing-injection/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers
sed -n '50,120p' internal/dao/agent_session.go
# Find where orderby/desc are defined/received in this file
rg -n "orderby|desc\b|Order\\(" internal/dao/agent_session.go
# Show DAO method signature around the snippet (best effort)
rg -n "ListByAgentIDPaged|Paged|ByAgent" internal/dao/agent_session.goRepository: infiniflow/ragflow
Length of output: 2190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Where ListByAgentIDPaged is called
rg -n "ListByAgentIDPaged\\(" -S internal | head -n 50
# Trace how orderby/desc are populated (look for "orderby" string or query params)
rg -n "\\borderby\\b" -S internal | head -n 80
rg -n "\\bdesc\\b" -S internal/handler internal/service 2>/dev/null | head -n 80Repository: infiniflow/ragflow
Length of output: 16829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' internal/handler/agent.go
sed -n '200,320p' internal/service/agent.go
# Narrow search for any allowlist/sanitization of orderby in agent flow
rg -n "orderby.*(allowed|allow|whitelist|map|switch|case)" internal/handler/agent.go internal/service/agent.go internal/dao/agent_session.go || trueRepository: infiniflow/ragflow
Length of output: 9598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect internal/dao/agent_session.go imports + the full ListByAgentIDPaged function for exact context
sed -n '1,120p' internal/dao/agent_session.go
sed -n '120,220p' internal/dao/agent_session.go
# 2) Inspect the handler code around where orderby/desc are set for agent session listing
sed -n '330,470p' internal/handler/agent.go
# 3) Check entity fields for AgentSession to ensure allowlist column names match actual struct/columns
fd -a -e go internal/entity | head -n 50
rg -n "type AgentSession struct" internal/entity || true
ls internal/entity | rg -i "agent|session" || true
fd -a "agent_session.go" internal/entity || true
fd -a "session.go" internal/entity || trueRepository: infiniflow/ragflow
Length of output: 8416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' internal/service/agent.go
sed -n '220,320p' internal/service/agent.go
rg -n "ListAgentSessions\\(" internal/service/agent.go
rg -n "Orderby" internal/service/agent.goRepository: infiniflow/ragflow
Length of output: 9737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check GORM version from go.mod
cat go.mod | rg -n "gorm\\.io/gorm" || true
cat go.mod | rg -n "gorm\\.io/datatypes|gorm\\.io/driver" || true
# See if clause.OrderByColumn is used anywhere in repo
rg -n "gorm\\.io/gorm/clause|OrderByColumn" internal | head -n 50Repository: infiniflow/ragflow
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the AgentSession entity definition
rg -n "type AgentSession struct" internal/entity -S || true
fd --full-path "*session*.go" internal/entity | head -n 50
fd --full-path "*agent*session*.go" internal/entity | head -n 50Repository: infiniflow/ragflow
Length of output: 428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' internal/entity/canvas.go
rg -n "type AgentSession struct" -n internal/entity/canvas.go
sed -n '1,80p' internal/entity/canvas.go | nl -ba | sed -n '1,120p'Repository: infiniflow/ragflow
Length of output: 4439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "type BaseModel struct" internal/entity -S
rg -n "create_time|update_time" internal/entity -S
sed -n '1,200p' internal/entity/base_model.go 2>/dev/null || true
fd --full-path "base*model*.go" internal/entity | head -n 20Repository: infiniflow/ragflow
Length of output: 3139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,160p' internal/entity/base.go
rg -n "gorm\\.io/gorm/clause" -S . || true
rg -n "OrderByColumn" -S . || trueRepository: infiniflow/ragflow
Length of output: 4482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Best-effort: look into module cache for clause.OrderByColumn definition
if command -v go >/dev/null 2>&1; then
GOPATH="$(go env GOPATH 2>/dev/null || true)"
if [ -n "${GOPATH:-}" ] && [ -d "$GOPATH/pkg/mod" ]; then
rg -n "type OrderByColumn" "$GOPATH/pkg/mod" 2>/dev/null | head -n 20 || true
rg -n "OrderByColumn" "$GOPATH/pkg/mod/gorm.io/gorm@"*/clause/* 2>/dev/null | head -n 50 || true
else
echo "GOPATH/pkg/mod not found"
fi
else
echo "go not available"
fi
# Confirm clause import usage in repo (should be none)
rg -n "gorm\\.io/gorm/clause" -S . || trueRepository: infiniflow/ragflow
Length of output: 87
Prevent SQL injection from user-controlled orderby in AgentSessionDAO.ListByAgentIDPaged.
internal/dao/agent_session.go builds order := orderby (only defaulting when empty) and then calls query := base.Order(order), so the orderby query param reaches GORM as raw ORDER BY SQL (not identifier-escaped) via internal/handler/agent.go → service → DAO.
Fix: allowlist orderby to a fixed set of permitted column names (e.g., id, name, create_time, update_time, etc.) and construct ORDER BY <mapped_column> <ASC|DESC> using only the mapped value and the desc boolean.
🤖 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 `@internal/dao/agent_session.go` around lines 72 - 83,
AgentSessionDAO.ListByAgentIDPaged currently assigns user-controlled orderby
directly to order and passes it into base.Order(order), enabling SQL injection;
replace that by validating orderby against a hardcoded allowlist (e.g.,
map[string]string
{"id":"id","name":"name","create_time":"create_time","update_time":"update_time"})
and mapping the input to the canonical column name, then build order as
mappedColumn + (desc ? " DESC" : " ASC") and pass only that into base.Order;
ensure the code references the existing orderby variable, the desc boolean, and
the call site base.Order(order) so no raw user input flows into the ORDER BY
clause.
Comment on lines
+467
to
+470
| session, err := h.agentService.GetAgentSession(agentID, sessionID) | ||
| if err != nil { | ||
| c.JSON(http.StatusOK, gin.H{"code": common.CodeNotFound, "data": false, "message": "Session not found!"}) | ||
| return |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the referenced handler code
sed -n '430,540p' internal/handler/agent.go | cat -n
# Find definitions/usages of GetAgentSession / DeleteAgentSessionItem handlers
rg -n "GetAgentSession|DeleteAgentSessionItem|DeleteAgentSessions" internal/handler/agent.go
# Inspect imports to see available error checks (gorm, errors.Is, etc.)
sed -n '1,120p' internal/handler/agent.go | cat -n
# Search for gorm.ErrRecordNotFound and common CodeServerError usage across the repo
rg -n "gorm\.ErrRecordNotFound|CodeServerError" -S internal | head -n 50
# Inspect the agentService implementations that return errors for session read/delete
# (search for GetAgentSession / DeleteAgentSessionByID)
rg -n "func \\(.*\\) GetAgentSession\\b|GetAgentSession\\(" -S internal | head -n 50
rg -n "func \\(.*\\) DeleteAgentSessionByID\\b|DeleteAgentSessionByID\\(" -S internal | head -n 50Repository: infiniflow/ragflow
Length of output: 15393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- internal/handler/agent.go (lines ~450-520) ----"
sed -n '440,520p' internal/handler/agent.go | cat -n
echo "---- internal/handler/agent.go (lines ~500-520) ----"
sed -n '490,520p' internal/handler/agent.go | cat -n
echo "---- internal/service/agent.go (GetAgentSession / DeleteAgentSessionByID) ----"
sed -n '220,310p' internal/service/agent.go | cat -nRepository: infiniflow/ragflow
Length of output: 8498
Differentiate not-found from internal failures in agent session handlers.
GetAgentSession and DeleteAgentSessionItem currently return common.CodeNotFound for any agentService error, masking backend/DB/infra failures as “Session not found!” and breaking client retry/diagnostics.
Additionally, internal/service/agent.go turns gorm.ErrRecordNotFound into fmt.Errorf("session not found") (without wrapping), so handler-level errors.Is(err, gorm.ErrRecordNotFound) can’t reliably distinguish not-found vs internal errors—use a sentinel/typed error or wrap the original error so handlers can map non-not-found errors to common.CodeServerError (and log).
🤖 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 `@internal/handler/agent.go` around lines 467 - 470, The handler currently
treats any error from agentService as "Session not found"; update the service
and handlers so not-found and internal errors are distinguishable: in
internal/service/agent.go return a sentinel exported error (e.g.
ErrSessionNotFound) or wrap gorm.ErrRecordNotFound with %w when a record is
absent in functions like GetAgentSession/DeleteAgentSessionItem, and update the
handler code that calls h.agentService.GetAgentSession and
DeleteAgentSessionItem to use errors.Is(err, service.ErrSessionNotFound) to
return common.CodeNotFound with the "Session not found" response, otherwise log
the error and return common.CodeServerError (and an appropriate message) for
internal failures; ensure service functions preserve the original error via
wrapping so errors.Is works.
Comment on lines
+547
to
+550
| if err := c.ShouldBindJSON(&body); err != nil { | ||
| // Empty / missing body is OK — means no-op. | ||
| c.JSON(http.StatusOK, gin.H{"code": common.CodeSuccess, "data": true, "message": ""}) | ||
| return |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the reported region with line numbers
sed -n '520,590p' internal/handler/agent.go | nl -ba | sed -n '1,120p'
# Show the full import block to see whether io is already imported
rg -n "import \\(" -n internal/handler/agent.go
sed -n '1,120p' internal/handler/agent.go | nl -ba
# Find all ShouldBindJSON occurrences in this file
rg -n "ShouldBindJSON\\(" internal/handler/agent.goRepository: infiniflow/ragflow
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== internal/handler/agent.go lines 520-590 ==="
sed -n '520,590p' internal/handler/agent.go | cat -n
echo "=== import block (first 140 lines) ==="
sed -n '1,140p' internal/handler/agent.go | cat -n
echo "=== all ShouldBindJSON occurrences in file ==="
rg -n "ShouldBindJSON\\(" internal/handler/agent.goRepository: infiniflow/ragflow
Length of output: 8115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for existing patterns around ShouldBindJSON error handling
rg -n "ShouldBindJSON\\(" internal | head -n 50
# Search for io.EOF handling in the codebase
rg -n "io\\.EOF" . || true
# Search for invalid request / argument error responses near binding errors
rg -n "invalid request body|CodeArgumentError.*bind|ShouldBind.*CodeArgumentError|CodeArgumentError.*ShouldBind" internal || trueRepository: infiniflow/ragflow
Length of output: 4679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tenant.go around the CodeBadRequest invalid request body pattern
sed -n '560,630p' internal/handler/tenant.go | cat -n
# Inspect one representative handler that just returns on bind errors
sed -n '300,470p' internal/handler/file.go | cat -n
# Inspect internal/handler/search.go around one bind error handler
sed -n '60,120p' internal/handler/search.go | cat -n
# Inspect internal/handler/user.go around one bind error handler
sed -n '40,90p' internal/handler/user.go | cat -nRepository: infiniflow/ragflow
Length of output: 12192
Bulk-delete should return an error for malformed JSON
In internal/handler/agent.go (lines 547-550), DeleteAgentSessions treats any c.ShouldBindJSON(&body) error as an “empty/missing body” no-op, so invalid JSON returns common.CodeSuccess (data: true) while skipping deletion.
Suggested fix
import (
"errors"
"fmt"
+ "io"
"mime/multipart"
"net/http"
"net/url"
@@
if err := c.ShouldBindJSON(&body); err != nil {
- // Empty / missing body is OK — means no-op.
- c.JSON(http.StatusOK, gin.H{"code": common.CodeSuccess, "data": true, "message": ""})
+ if errors.Is(err, io.EOF) {
+ // Empty / missing body is OK — means no-op.
+ c.JSON(http.StatusOK, gin.H{"code": common.CodeSuccess, "data": true, "message": ""})
+ return
+ }
+ c.JSON(http.StatusOK, gin.H{"code": common.CodeArgumentError, "data": false, "message": "invalid request body"})
return
}📝 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
| if err := c.ShouldBindJSON(&body); err != nil { | |
| // Empty / missing body is OK — means no-op. | |
| c.JSON(http.StatusOK, gin.H{"code": common.CodeSuccess, "data": true, "message": ""}) | |
| return | |
| if err := c.ShouldBindJSON(&body); err != nil { | |
| if errors.Is(err, io.EOF) { | |
| // Empty / missing body is OK — means no-op. | |
| c.JSON(http.StatusOK, gin.H{"code": common.CodeSuccess, "data": true, "message": ""}) | |
| return | |
| } | |
| c.JSON(http.StatusOK, gin.H{"code": common.CodeArgumentError, "data": false, "message": "invalid request body"}) | |
| return | |
| } |
🤖 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 `@internal/handler/agent.go` around lines 547 - 550, DeleteAgentSessions
currently treats any error from c.ShouldBindJSON(&body) as an empty/no-op and
returns common.CodeSuccess; instead detect genuine empty body vs malformed JSON
and return an error for malformed input. Change the c.ShouldBindJSON(&body)
error handling: if the error is an EOF/empty-body case (use errors.Is(err,
io.EOF) or check for empty body) then keep the no-op behavior, otherwise respond
with HTTP 400 and a JSON error (non-success code) describing the malformed JSON;
update the branch that currently returns common.CodeSuccess so only empty-body
errors produce that response and all other binding errors return an error
response. Ensure you modify the error handling around c.ShouldBindJSON(&body) in
DeleteAgentSessions accordingly.
Comment on lines
+672
to
+684
| ext := c.DefaultQuery("ext", "markdown") | ||
|
|
||
| blob, err := h.agentService.DownloadAttachment(user.ID, attachmentID) | ||
| if err != nil { | ||
| c.JSON(http.StatusOK, gin.H{"code": common.CodeServerError, "data": false, "message": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| contentType := utility.GetContentType(ext, "") | ||
| if contentType == "" { | ||
| contentType = "application/octet-stream" | ||
| } | ||
| c.Data(http.StatusOK, contentType, blob) |
Contributor
There was a problem hiding this comment.
Prevent client-driven MIME spoofing in attachment downloads.
ext directly controls response Content-Type, and response is served inline. A crafted request can force active content types for user-controlled blobs.
Suggested fix
- ext := c.DefaultQuery("ext", "markdown")
+ ext := utility.GetFileExtension(attachmentID)
@@
contentType := utility.GetContentType(ext, "")
if contentType == "" {
contentType = "application/octet-stream"
}
+ c.Header("X-Content-Type-Options", "nosniff")
+ c.Header("Content-Disposition", "attachment; filename*=UTF-8''"+url.QueryEscape(attachmentID))
c.Data(http.StatusOK, contentType, blob)🤖 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 `@internal/handler/agent.go` around lines 672 - 684, The handler currently
trusts the client-supplied ext and serves blobs inline, enabling MIME spoofing;
update the attachment download logic (around h.agentService.DownloadAttachment,
ext, utility.GetContentType, contentType and c.Data) to ignore or not trust the
ext query for determining Content-Type, instead sniff the blob (e.g., use
http.DetectContentType or equivalent) to derive contentType and fall back to
"application/octet-stream" if unknown, map/normalize unsafe types (e.g.,
downgrade "text/html", "application/javascript" to "text/plain" or
"application/octet-stream"), and force a Content-Disposition: attachment header
(with a safe filename) so responses are downloaded rather than rendered inline.
Comment on lines
+304
to
+307
| belongs, err := s.agentSessionDAO.BelongsToAgent(id, agentID) | ||
| if err != nil || !belongs { | ||
| res.Errors = append(res.Errors, fmt.Sprintf("the agent doesn't own the session %s", id)) | ||
| continue |
Contributor
There was a problem hiding this comment.
Separate DAO errors from “not owned” results in bulk delete.
Line 305 currently treats DB failures the same as !belongs, so transient/data-layer issues get misreported as ownership errors instead of actual verification failures.
🔧 Suggested fix
belongs, err := s.agentSessionDAO.BelongsToAgent(id, agentID)
- if err != nil || !belongs {
+ if err != nil {
+ res.Errors = append(res.Errors, fmt.Sprintf("failed to verify ownership for session %s: %v", id, err))
+ continue
+ }
+ if !belongs {
res.Errors = append(res.Errors, fmt.Sprintf("the agent doesn't own the session %s", id))
continue
}📝 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
| belongs, err := s.agentSessionDAO.BelongsToAgent(id, agentID) | |
| if err != nil || !belongs { | |
| res.Errors = append(res.Errors, fmt.Sprintf("the agent doesn't own the session %s", id)) | |
| continue | |
| belongs, err := s.agentSessionDAO.BelongsToAgent(id, agentID) | |
| if err != nil { | |
| res.Errors = append(res.Errors, fmt.Sprintf("failed to verify ownership for session %s: %v", id, err)) | |
| continue | |
| } | |
| if !belongs { | |
| res.Errors = append(res.Errors, fmt.Sprintf("the agent doesn't own the session %s", id)) | |
| continue | |
| } |
🤖 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 `@internal/service/agent.go` around lines 304 - 307, The current check
conflates DAO errors and ownership falsehoods for
s.agentSessionDAO.BelongsToAgent; change the logic to first handle a non-nil err
returned by BelongsToAgent (e.g. append a distinct error like "failed to verify
ownership for session %s: <err>" or record it as an internal/DAO error) and only
when err == nil evaluate belongs and append the existing "the agent doesn't own
the session %s" message; update the block around
s.agentSessionDAO.BelongsToAgent, belongs, and err to separate these two cases
so transient DB failures are not reported as ownership failures.
hunnyboy1217
force-pushed
the
feat/go-agent-session-download-logs-api
branch
from
6d87f69 to
37f704f
Compare
Contributor
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 `@internal/handler/agent.go`:
- Around line 420-423: The code treats both service errors and permission
denials the same when calling h.agentService.CheckCanvasAccess(user.ID,
agentID); change the logic to first check if err != nil and return an
internal/server error response (e.g., http.StatusInternalServerError with an
appropriate error code and log the err) and only if err == nil and ok == false
return the existing permission-denied response; apply this fix to every call
site that invokes CheckCanvasAccess (the calls using
h.agentService.CheckCanvasAccess(user.ID, agentID) so the handler returns a 5xx
on service/DB/tenant errors and a 403/permission response when access is
explicitly false).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54b3ae1a-84cb-4b4d-98ef-e53d136b2950
📒 Files selected for processing (5)
internal/dao/agent_session.gointernal/entity/canvas.gointernal/handler/agent.gointernal/router/router.gointernal/service/agent.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/entity/canvas.go
- internal/router/router.go
- internal/dao/agent_session.go
- internal/service/agent.go
Comment on lines
+420
to
+423
| ok, err := h.agentService.CheckCanvasAccess(user.ID, agentID) | ||
| if err != nil || !ok { | ||
| c.JSON(http.StatusOK, gin.H{"code": common.CodeOperatingError, "data": false, "message": "Make sure you have permission to access the agent."}) | ||
| return |
Contributor
There was a problem hiding this comment.
Split access-check errors from real permission denials.
Lines 420, 493, 531, and 627 all treat CheckCanvasAccess(... ) failures and !ok the same way. That service already returns (false, err) for DAO/tenant lookup failures, so DB or tenant-service outages get reported back as “no permission” instead of server errors.
Suggested fix
ok, err := h.agentService.CheckCanvasAccess(user.ID, agentID)
- if err != nil || !ok {
+ if err != nil {
+ common.Warn("check canvas access failed", zap.String("agent_id", agentID), zap.Error(err))
+ c.JSON(http.StatusOK, gin.H{"code": common.CodeServerError, "data": false, "message": "Internal server error"})
+ return
+ }
+ if !ok {
c.JSON(http.StatusOK, gin.H{"code": common.CodeOperatingError, "data": false, "message": "Make sure you have permission to access the agent."})
return
}Also applies to: 493-496, 531-534, 627-630
🤖 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 `@internal/handler/agent.go` around lines 420 - 423, The code treats both
service errors and permission denials the same when calling
h.agentService.CheckCanvasAccess(user.ID, agentID); change the logic to first
check if err != nil and return an internal/server error response (e.g.,
http.StatusInternalServerError with an appropriate error code and log the err)
and only if err == nil and ok == false return the existing permission-denied
response; apply this fix to every call site that invokes CheckCanvasAccess (the
calls using h.agentService.CheckCanvasAccess(user.ID, agentID) so the handler
returns a 5xx on service/DB/tenant errors and a 403/permission response when
access is explicitly false).
Ports the following Python endpoints from agent_api.py to the Go layer, closing subtask infiniflow#15654: - GET /api/v1/agents/:agent_id (GetAgent) - GET /api/v1/agents/:agent_id/sessions (ListAgentSessions) - GET /api/v1/agents/:agent_id/sessions/:session_id (GetAgentSession) - DELETE /api/v1/agents/:agent_id/sessions/:session_id (DeleteAgentSessionItem) - DELETE /api/v1/agents/:agent_id/sessions (DeleteAgentSessions – bulk) - GET /api/v1/agents/:agent_id/logs/:message_id (GetAgentLogs – Redis) - GET /api/v1/agents/download (DownloadAgentFile) - GET /api/v1/agents/attachments/:attachment_id/download (DownloadAttachment) New files: internal/dao/agent_session.go – DAO for conversation table (agent view) Changed files: internal/entity/canvas.go – AgentSession entity (conversation table) internal/service/agent.go – service methods for all new endpoints internal/handler/agent.go – HTTP handlers for all new endpoints internal/router/router.go – route registration Ownership checks mirror Python: - canvas access (team permission or owner) gates session read/single-delete - canvas ownership gates bulk session delete - attachment download uses caller's tenant ID as the storage bucket
hunnyboy1217
force-pushed
the
feat/go-agent-session-download-logs-api
branch
from
37f704f to
f5defcb
Compare
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
1 participant
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.
Closes #15654:
New files:
internal/dao/agent_session.go – DAO for conversation table (agent view)
Changed files:
internal/entity/canvas.go – AgentSession entity (conversation table)
internal/service/agent.go – service methods for all new endpoints
internal/handler/agent.go – HTTP handlers for all new endpoints
internal/router/router.go – route registration
Ownership checks mirror Python:
What problem does this PR solve?
Briefly describe what this PR aims to solve. Include background context that will help reviewers understand the purpose of the PR.
Type of change