From 7b5d62373738fc59b2e92ab7741d436b78955670 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Sun, 14 Jun 2026 17:06:03 +0200 Subject: [PATCH] fix: introduce lint-single-response to prevent control flow continuing past a `ctx.Error(...)`-style method (#13087) This PR adds a new linter to the codebase and addresses all the problems that it identified (including a small number of false positives). The lint-single-response Go analyzer attempts to prevent a common problem in Forgejo where it is possible for a web handler to provide a response to a request, and then continue code execution unintentionally. For example: ```go err := json.Unmarshal(data, &claims) if err != nil { ctx.Error(http.StatusInternalServerError, "Error in unmarshal", err) // Oops, I forgot to `return` here... } // ... more work occurs ... ctx.JSON(http.StatusOK, resp) ``` In order to detect these cases, lint-single-response contains a list of functions that deliver a web response. When any of those functions are used within a function, the control flow must not perform any work after the function is invoked -- it can only return and exit the function. ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Documentation - [x] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - Documentation on the new linter is included inline, in `build/lint-single-response/README.md`. - [ ] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13087 Reviewed-by: Andreas Ahlenstorf --- .golangci.yml | 3 + Makefile | 6 +- build/lint-single-response/README.md | 44 ++++ build/lint-single-response/cmd/main.go | 14 ++ build/lint-single-response/singleresponse.go | 218 ++++++++++++++++++ .../singleresponse_test.go | 14 ++ .../testdata/src/a/api.go | 70 ++++++ .../testdata/src/a/flow.go | 127 ++++++++++ .../testdata/src/a/web.go | 98 ++++++++ .../src/forgejo.org/services/context/api.go | 19 ++ .../src/forgejo.org/services/context/base.go | 41 ++++ .../forgejo.org/services/context/context.go | 33 +++ go.mod | 2 +- routers/api/actions/id_token.go | 2 + routers/api/v1/org/member.go | 1 + routers/api/v1/repo/avatar.go | 2 + routers/api/v1/repo/issue.go | 1 + routers/api/v1/repo/issue_attachment.go | 1 + .../api/v1/repo/issue_comment_attachment.go | 1 + routers/api/v1/repo/issue_stopwatch.go | 27 +-- routers/api/v1/repo/issue_tracked_time.go | 3 + routers/api/v1/repo/mirror.go | 1 + routers/api/v1/repo/repo.go | 1 + routers/api/v1/repo/tag.go | 1 + routers/api/v1/repo/teams.go | 1 + routers/api/v1/shared/quota.go | 3 + routers/api/v1/shared/runners.go | 2 + routers/api/v1/user/gpg_key.go | 1 + routers/api/v1/user/star.go | 1 + routers/api/v1/user/watch.go | 2 + routers/private/actions.go | 1 + routers/private/hook_post_receive.go | 1 + routers/private/manager.go | 1 + routers/private/serv.go | 1 + routers/web/auth/auth.go | 7 +- routers/web/moderation/report.go | 4 +- routers/web/repo/badges/badges.go | 1 + routers/web/repo/editor.go | 1 + routers/web/repo/issue.go | 2 + routers/web/repo/issue_dependency.go | 11 +- routers/web/repo/projects.go | 1 + routers/web/repo/pull.go | 3 +- routers/web/repo/setting/setting.go | 2 +- routers/web/repo/view.go | 1 + routers/web/repo/wiki.go | 47 ++-- routers/web/shared/storage_overview.go | 1 + routers/web/user/notification.go | 1 + routers/web/user/setting/keys.go | 3 + routers/web/webfinger.go | 6 +- services/context/base.go | 7 +- 50 files changed, 781 insertions(+), 61 deletions(-) create mode 100644 build/lint-single-response/README.md create mode 100644 build/lint-single-response/cmd/main.go create mode 100644 build/lint-single-response/singleresponse.go create mode 100644 build/lint-single-response/singleresponse_test.go create mode 100644 build/lint-single-response/testdata/src/a/api.go create mode 100644 build/lint-single-response/testdata/src/a/flow.go create mode 100644 build/lint-single-response/testdata/src/a/web.go create mode 100644 build/lint-single-response/testdata/src/forgejo.org/services/context/api.go create mode 100644 build/lint-single-response/testdata/src/forgejo.org/services/context/base.go create mode 100644 build/lint-single-response/testdata/src/forgejo.org/services/context/context.go diff --git a/.golangci.yml b/.golangci.yml index 2d9ed5e098..e02ef6a55b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -165,6 +165,9 @@ linters: - linters: - forbidigo path: cmd + - linters: + - forbidigo + path: build/lint-single-response - linters: - dupl text: (?i)webhook diff --git a/Makefile b/Makefile index 9f5aee8902..3ce0cdba4e 100644 --- a/Makefile +++ b/Makefile @@ -443,7 +443,7 @@ lint-frontend: lint-js tsc lint-css lint-frontend-fix: lint-js-fix lint-css-fix .PHONY: lint-backend -lint-backend: lint-go lint-go-vet lint-editorconfig lint-renovate lint-locale lint-locale-usage lint-disposable-emails +lint-backend: lint-go lint-go-vet lint-editorconfig lint-renovate lint-locale lint-locale-usage lint-disposable-emails lint-single-response .PHONY: lint-backend-fix lint-backend-fix: lint-go-fix lint-go-vet lint-editorconfig lint-disposable-emails-fix @@ -530,6 +530,10 @@ lint-disposable-emails: lint-disposable-emails-fix: $(GO) run build/generate-disposable-email.go -r $(DISPOSABLE_EMAILS_SHA) +.PHONY: lint-single-response +lint-single-response: + $(GO) run ./build/lint-single-response/cmd ./... + .PHONY: security-check security-check: $(GO) run $(GOVULNCHECK_PACKAGE) -show color ./... diff --git a/build/lint-single-response/README.md b/build/lint-single-response/README.md new file mode 100644 index 0000000000..eb9cfe09b0 --- /dev/null +++ b/build/lint-single-response/README.md @@ -0,0 +1,44 @@ +# lint-single-response + +The lint-single-response Go analyzer attempts to prevent a common problem in Forgejo where it is possible for a web handler to provide a response to a request, and then continue code execution unintentionally. For example: + +```go +err := json.Unmarshal(data, &claims) +if err != nil { + ctx.Error(http.StatusInternalServerError, "Error in unmarshal", err) + // Oops, I forgot to `return` here... +} +// ... more work occurs ... +ctx.JSON(http.StatusOK, resp) +``` + +In order to detect these cases, lint-single-response contains a list of functions that deliver a web response. When any of those functions are used within a function, the control flow must not perform any work after the function is invoked -- it can only return and exit the function. + +Methods named `Test...` are omitted from analysis, as this naming scheme suggests a test case where an error would have no user impact, and such methods sometimes invoke web response methods in unusual but safe patterns. + +## Limitations + +lint-single-response only works within the control-flow of a single function. If a web handler calls another function that invokes `ctx.Error(...)`, then there is no guarantee that the web handler doesn't go on to do more work. This could be addressed in the future but would require a multi-pass analysis -- all functions that invoke web responses would need to be identified, then all functions that invoke those functions would need to be identified, recursively, until no new functions are identified. And then lint-single-response's current behaviour would need to be implemented against that entire set of functions. + +## Usage + +Direct invocation: + +``` +go run ./build/lint-single-response/cmd ./... +``` + +It is also integrated into Forgejo's `Makefile`, and can be run directly as the target `make lint-single-response`, or as part of `make lint-backend` or `make pr-go`. + +## Testing + +lint-single-response contains internal tests to verify that it works correctly. These tests are included in `make test-backend`, but, Go tends to think that they're cached even if data in `testdata` is changed. For development and testing of lint-single-response, it is recommended to run the tests with `-count 1` to avoid caching: + +``` +GOTESTFLAGS="-count 1" GO_TEST_PACKAGES=forgejo.org/build/lint-single-response make test-backend +``` + +Testing is done with the [`analysistest` package](https://pkg.go.dev/golang.org/x/tools@v0.46.0/go/analysis/analysistest#Run). In short, comments `// want ...` indicate that a lint diagnostic must be produced on that line for the test to pass. + +An empty implementation of `context.Base`, `context.Context`, and `context.APIContext` are included in the test package so that the exact method signatures being used in Forgejo can be covered in the tests. + diff --git a/build/lint-single-response/cmd/main.go b/build/lint-single-response/cmd/main.go new file mode 100644 index 0000000000..6f2b42ac82 --- /dev/null +++ b/build/lint-single-response/cmd/main.go @@ -0,0 +1,14 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package main + +import ( + singleresponse "forgejo.org/build/lint-single-response" + + "golang.org/x/tools/go/analysis/singlechecker" +) + +func main() { + singlechecker.Main(singleresponse.Analyzer) +} diff --git a/build/lint-single-response/singleresponse.go b/build/lint-single-response/singleresponse.go new file mode 100644 index 0000000000..c665955eeb --- /dev/null +++ b/build/lint-single-response/singleresponse.go @@ -0,0 +1,218 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package singleresponse + +import ( + "fmt" + "go/ast" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/ctrlflow" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/cfg" +) + +var Analyzer = &analysis.Analyzer{ + Name: "singleresponse", + Doc: "checks that Forgejo web response methods are only invoked once in a control flow", + Requires: []*analysis.Analyzer{inspect.Analyzer, ctrlflow.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs) + + webFuncs := map[string]map[string]any{ + "*forgejo.org/services/context.APIContext": { + "Error": true, + "InternalServerError": true, + "NotFound": true, + "NotFoundOrServerError": true, + "ServerError": true, + }, + "*forgejo.org/services/context.Base": { + "Error": true, + "JSON": true, + "JSONWithContentType": true, + "PlainText": true, + "PlainTextBytes": true, + "Redirect": true, + "ServeContent": true, + }, + "*forgejo.org/services/context.Context": { + "HTML": true, + "JSONError": true, + "JSONOK": true, + "JSONRedirect": true, + "JSONTemplate": true, + "NotFound": true, + "NotFoundOrServerError": true, + "RedirectToFirst": true, + "RenderWithErr": true, + "ServerError": true, + }, + // Future: RedirectToUser does not accept a ctx LHS, but rather a first parameter -- needs different + // implementation of detection, or, refactoring: "RedirectToUser": true, + } + + insp.Nodes([]ast.Node{ + (*ast.FuncDecl)(nil), + (*ast.FuncLit)(nil), + }, func(n ast.Node, push bool) bool { + switch fn := n.(type) { + case *ast.FuncDecl: + // Skip test methods which are assumed to know what they're doing. + if strings.HasPrefix(fn.Name.Name, "Test") { + return false + } + cfg := cfgs.FuncDecl(fn) + if cfg == nil { + return true + } + inspectFunction(cfg, pass, webFuncs) + case *ast.FuncLit: + cfg := cfgs.FuncLit(fn) + if cfg == nil { + return true + } + inspectFunction(cfg, pass, webFuncs) + } + return false + }) + + return nil, nil //nolint:nilnil +} + +func inspectFunction(cfg *cfg.CFG, pass *analysis.Pass, webFuncs map[string]map[string]any) { + for _, block := range cfg.Blocks { + for nodeIdx, node := range block.Nodes { + ast.Inspect(node, func(n ast.Node) bool { + // Don't recurse inside of a function literal inside of a function declaration, as this isn't + // related to the control flow that we're currently iterating through. + _, isFuncLit := n.(*ast.FuncLit) + if isFuncLit { + return false + } + + call, isCall := n.(*ast.CallExpr) + if !isCall { + return true + } + + // SelectorExpr: "an expression followed by a selector", like "ctx.Error". All the functions + // we're interested in match this pattern. + selector, isSelector := call.Fun.(*ast.SelectorExpr) + if !isSelector { + return false + } + + // We almost get the right information easily from the selector by using + // pass.TypesInfo.Uses[selector.X] -- but that will be the type of the variable that we're + // invoking a method on, and not the type of the method receiver. eg. on `ctx + // *context.Context`, `ctx.ServerError(...)` will always be `*context.Context`, even if + // `ServerError` is actually implemented on `*context.Base`. + // + // We need to dig a little deeper here to get the function type, then its signature, and then + // it's receiver type, and we'll really have the method that will be invoked rather than just + // the variable that it is called upon. + selection, hasSelection := pass.TypesInfo.Selections[selector] + if !hasSelection { + return false + } + objFn, ok := selection.Obj().(*types.Func) + if !ok { + return false + } + fnSig, ok := objFn.Type().(*types.Signature) + if !ok { + return false + } + callType := fnSig.Recv().Type().String() + + typeMap, inTypeMap := webFuncs[callType] + if inTypeMap { + callName := selector.Sel.Name + _, inFuncMap := typeMap[callName] + if inFuncMap { + // OK... we've found a call to a terminating function at + // cfg.Blocks[blockIdx].Nodes[nodeIdx]. + trace := false + // For code-time debugging/analysis, set trace=true when digging into why something isn't + // working: + // if callName == "InternalServerError" { + // trace = true + // } + sketchy := inspectCallSite(block, nodeIdx, trace) + if sketchy != nil { + pass.Reportf(node.Pos(), "Invocation of %s / %s, and control flow continues afterwards.", callType, callName) + } + } + } + + return false + }) + } + } +} + +type sketchyCall struct{} + +func inspectCallSite(callingBlock *cfg.Block, callingNodeIndex int, trace bool) *sketchyCall { + // Inspect the remainder of the block passed in, after callingNodeIndex, for "bad" statements + if trace { + println("remainder of block...") + } + for _, nextStmt := range callingBlock.Nodes[callingNodeIndex+1:] { + if trace { + println(fmt.Sprintf("\tnextStmt = %#v", nextStmt)) + } + // Only `return` is permitted after one of the web return functions; maybe this needs to expand in the future + // but haven't identified any cases in Forgejo yet. + _, stmtOk := nextStmt.(*ast.ReturnStmt) + if !stmtOk { + if trace { + println(fmt.Sprintf("\tfound sketchy statement = %#v", nextStmt)) + } + // Future: add information about what was following the call, so that the diagnostic can be more specific + // about the problematic next statement identified... but so far it seems pretty easy to analyze and fix. + return &sketchyCall{} + } + } + if trace { + println("nothing found in remainder of block") + println(fmt.Sprintf("%d Succs blocks will be investigated", len(callingBlock.Succs))) + } + + // Now, assuming that there was nothing problematic found in the remainder of the block, use the control-flow graph + // to identify where code execution would continue and see if there's anything inappropriate in it. + // + // https://pkg.go.dev/golang.org/x/tools@v0.46.0/go/cfg#Block -> A block may have 0-2 successors: zero for a return + // block or a block that calls a function such as panic that never returns; one for a normal (jump) block; and two + // for a conditional (if) block. + // + // It's possible for the next block to have either no nodes, or, no nodes that continue to do work and trigger + // detection... but then to proceed into *another* block that does. So this investigation has to be done + // recursively. Control-flow graph should prevent us from needing to stop this recursive detection; we'll hit a + // return statement or end of function and that's the end of the CFG, and that's also the time we'd want to stop + // looking, so no additional exit logic should be needed. + for i, succ := range callingBlock.Succs { + if trace { + println(fmt.Sprintf("Succs[%d], block index %d, recursing:", i, succ.Index)) + } + // `-1` is used to start at index 0 in the nodes. + sketchy := inspectCallSite(succ, -1, trace) + if trace { + println(fmt.Sprintf("Succs[%d], block index %d, had sketchy = %#v", i, succ.Index, sketchy)) + } + if sketchy != nil { + return sketchy + } + } + + return nil +} diff --git a/build/lint-single-response/singleresponse_test.go b/build/lint-single-response/singleresponse_test.go new file mode 100644 index 0000000000..8b61a5bd22 --- /dev/null +++ b/build/lint-single-response/singleresponse_test.go @@ -0,0 +1,14 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package singleresponse + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestSingleResponse(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), Analyzer, "a") +} diff --git a/build/lint-single-response/testdata/src/a/api.go b/build/lint-single-response/testdata/src/a/api.go new file mode 100644 index 0000000000..b737ef4875 --- /dev/null +++ b/build/lint-single-response/testdata/src/a/api.go @@ -0,0 +1,70 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package a + +import ( + "errors" + + "forgejo.org/services/context" +) + +func work() {} + +func directApiCallFine(ctx *context.APIContext) { + ctx.Error(500, "title", nil) +} + +// Directly call APIContext functions, then "do work", triggering a linting error: + +func directApiCallError(ctx *context.APIContext) { + ctx.Error(500, "title", nil) // want "Invocation of (.*) / Error, and control flow continues afterwards." + work() +} + +func directApiCallInternalServerError(ctx *context.APIContext) { + ctx.InternalServerError(errors.New("something")) // want "Invocation of (.*) / InternalServerError, and control flow continues afterwards." + work() +} + +func directApiCallNotFound(ctx *context.APIContext) { + ctx.NotFound("title") // want "Invocation of (.*) / NotFound, and control flow continues afterwards." + work() +} + +func directApiCallNotFoundOrServerError(ctx *context.APIContext) { + ctx.NotFoundOrServerError("logMsg", func(err error) bool { return false }, errors.New("something")) // want "Invocation of (.*) / NotFoundOrServerError, and control flow continues afterwards." + work() +} + +func directApiCallServerError(ctx *context.APIContext) { + ctx.ServerError("something", errors.New("something")) // want "Invocation of (.*) / ServerError, and control flow continues afterwards." + work() +} + +// Call methods on ctx that will go to the `*Base` implementation: + +func indirectApiCallJSON(ctx *context.APIContext) { + ctx.JSON(200, "something") // want "Invocation of (.*).Base / JSON, and control flow continues afterwards." + work() +} + +func indirectApiCallPlainText(ctx *context.APIContext) { + ctx.PlainText(200, "something") // want "Invocation of (.*).Base / PlainText, and control flow continues afterwards." + work() +} + +func indirectApiCallPlainTextBytes(ctx *context.APIContext) { + ctx.PlainTextBytes(200, []byte{}) // want "Invocation of (.*).Base / PlainTextBytes, and control flow continues afterwards." + work() +} + +func indirectApiCallRedirect(ctx *context.APIContext) { + ctx.Redirect("/somewhere") // want "Invocation of (.*).Base / Redirect, and control flow continues afterwards." + work() +} + +func indirectApiCallServeContent(ctx *context.APIContext) { + ctx.ServeContent(nil, nil) // want "Invocation of (.*).Base / ServeContent, and control flow continues afterwards." + work() +} diff --git a/build/lint-single-response/testdata/src/a/flow.go b/build/lint-single-response/testdata/src/a/flow.go new file mode 100644 index 0000000000..10ee55aa83 --- /dev/null +++ b/build/lint-single-response/testdata/src/a/flow.go @@ -0,0 +1,127 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package a + +import ( + "errors" + "html/template" + "math/rand/v2" + + "forgejo.org/services/context" +) + +func controlFlowIf(ctx *context.APIContext) { + if rand.Float64() > 0.1 { + ctx.Error(500, "title", nil) // want "Invocation of (.*) and control flow continues afterwards." + } + + work() + + if rand.Float64() > 0.1 { + ctx.Error(500, "title", nil) // no want + return + } + + work() + + if rand.Float64() > 0.1 { + ctx.Error(500, "title", nil) // no want + return + } else { + ctx.Error(500, "title", nil) // want "Invocation of (.*) and control flow continues afterwards." + } + + if rand.Float64() > 0.1 { + ctx.Error(500, "title", nil) // no want + return + } else if rand.Float64() > 0.1 { + ctx.Error(500, "title", nil) // no want + return + } else if rand.Float64() > 0.1 { + ctx.InternalServerError(errors.New("something")) // want "Invocation of (.*) and control flow continues afterwards." + } else { + ctx.Error(500, "title", nil) // no want + return + } + + work() + + if rand.Float64() > 0.1 { + ctx.Error(500, "title", nil) // no want -- method ends either way + } else { + ctx.Error(500, "title", nil) // no want -- method ends either way + } +} + +func controlFlowSwitch(ctx *context.APIContext) { + switch { + case rand.Float64() > 0.1: + ctx.Error(500, "title", nil) // want "Invocation of (.*) and control flow continues afterwards." + case rand.Float64() > 0.1: + ctx.Error(500, "title", nil) // want "Invocation of (.*) and control flow continues afterwards." + case rand.Float64() > 0.1: + ctx.Error(500, "title", nil) // no want + return + } + + work() + + switch { + case rand.Float64() > 0.1: + ctx.Error(500, "title", nil) // want "Invocation of (.*) and control flow continues afterwards." + fallthrough + case rand.Float64() > 0.1: + ctx.Error(500, "title", nil) // no want + return + } + + work() + + switch { + case rand.Float64() > 0.1: + ctx.Error(500, "title", nil) // no want -- method ends either way + case rand.Float64() > 0.1: + ctx.Error(500, "title", nil) // no want -- method ends either way + } +} + +func controlFlowLoop(ctx *context.APIContext) { + for range []int{1, 2, 3} { + ctx.Error(500, "title", nil) // want "Invocation of (.*) and control flow continues afterwards." + } + + for range []int{1, 2, 3} { + ctx.Error(500, "title", nil) + return + } + + for range []int{1, 2, 3} { + ctx.Error(500, "title", nil) + break + } + return +} + +func controlFlowInternalDecl(ctx *context.Context) { + work() + renderWithError := func(msg template.HTML) { + ctx.RenderWithErr(msg, "tplAccessTokenEdit", nil) // no want -- within the context of `renderWithError` this is fine + } + if rand.Float64() > 0.1 { + renderWithError("") + return + } + if rand.Float64() > 0.1 { + // Future: would love if call to another method which calls ctx.*, followed by no `return`, could cause a + // diagnostic -- but that may require a multi-pass analysis to do a good job generally. With a local function + // declaration it's more feasible but it's also not very common, may not be worth that effort. + renderWithError("") + } + work() +} + +var localFunc = func(ctx *context.Context) { + ctx.PlainText(200, "title") // want "Invocation of (.*) / PlainText, and control flow continues afterwards." + work() +} diff --git a/build/lint-single-response/testdata/src/a/web.go b/build/lint-single-response/testdata/src/a/web.go new file mode 100644 index 0000000000..0034d5f655 --- /dev/null +++ b/build/lint-single-response/testdata/src/a/web.go @@ -0,0 +1,98 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package a + +import ( + "errors" + + "forgejo.org/services/context" +) + +func directWebCallFine(ctx *context.Context) { + ctx.HTML(200, "tmpl") +} + +// Directly call Context functions, then "do work", triggering a linting error: + +func directWebCallHTML(ctx *context.Context) { + ctx.HTML(200, "tmpl") // want "Invocation of (.*) / HTML, and control flow continues afterwards." + work() +} + +func directWebCallJSONError(ctx *context.Context) { + ctx.JSONError("something") // want "Invocation of (.*) / JSONError, and control flow continues afterwards." + work() +} + +func directWebCallJSONOK(ctx *context.Context) { + ctx.JSONOK() // want "Invocation of (.*) / JSONOK, and control flow continues afterwards." + work() +} + +func directWebCallJSONRedirect(ctx *context.Context) { + ctx.JSONRedirect("/somewhere") // want "Invocation of (.*) / JSONRedirect, and control flow continues afterwards." + work() +} + +func directWebCallJSONTemplate(ctx *context.Context) { + ctx.JSONTemplate("tmpl") // want "Invocation of (.*) / JSONTemplate, and control flow continues afterwards." + work() +} + +func directWebCallNotFound(ctx *context.Context) { + ctx.NotFound("something", errors.New("something")) // want "Invocation of (.*) / NotFound, and control flow continues afterwards." + work() +} + +func directWebCallNotFoundOrServerError(ctx *context.Context) { + ctx.NotFoundOrServerError("something", func(err error) bool { return false }, errors.New("something")) // want "Invocation of (.*) / NotFoundOrServerError, and control flow continues afterwards." + work() +} + +func directWebCallRedirectToFirst(ctx *context.Context) { + ctx.RedirectToFirst("/somewhere") // want "Invocation of (.*) / RedirectToFirst, and control flow continues afterwards." + work() +} + +func directWebCallRenderWithErr(ctx *context.Context) { + ctx.RenderWithErr("something", "tmpl", errors.New("something")) // want "Invocation of (.*) / RenderWithErr, and control flow continues afterwards." + work() +} + +func directWebCallServerError(ctx *context.Context) { + ctx.ServerError("something", errors.New("something")) // want "Invocation of (.*) / ServerError, and control flow continues afterwards." + work() +} + +// Call methods on ctx that will go to the `*Base` implementation: + +func indirectWebCallError(ctx *context.Context) { + ctx.Error(500, "something") // want "Invocation of (.*).Base / Error, and control flow continues afterwards." + work() +} + +func indirectWebCallJSON(ctx *context.Context) { + ctx.JSON(200, "something") // want "Invocation of (.*).Base / JSON, and control flow continues afterwards." + work() +} + +func indirectWebCallPlainText(ctx *context.Context) { + ctx.PlainText(200, "something") // want "Invocation of (.*).Base / PlainText, and control flow continues afterwards." + work() +} + +func indirectWebCallPlainTextBytes(ctx *context.Context) { + ctx.PlainTextBytes(200, []byte{}) // want "Invocation of (.*).Base / PlainTextBytes, and control flow continues afterwards." + work() +} + +func indirectWebCallRedirect(ctx *context.Context) { + ctx.Redirect("/somewhere") // want "Invocation of (.*).Base / Redirect, and control flow continues afterwards." + work() +} + +func indirectWebCallServeContent(ctx *context.Context) { + ctx.ServeContent(nil, nil) // want "Invocation of (.*).Base / ServeContent, and control flow continues afterwards." + work() +} diff --git a/build/lint-single-response/testdata/src/forgejo.org/services/context/api.go b/build/lint-single-response/testdata/src/forgejo.org/services/context/api.go new file mode 100644 index 0000000000..c253601575 --- /dev/null +++ b/build/lint-single-response/testdata/src/forgejo.org/services/context/api.go @@ -0,0 +1,19 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package context + +type APIContext struct { + *Base +} + +func (ctx *APIContext) Error(status int, title string, obj any) {} + +func (ctx *APIContext) InternalServerError(err error) {} + +func (ctx *APIContext) NotFound(objs ...any) {} + +func (ctx *APIContext) NotFoundOrServerError(logMsg string, errCheck func(error) bool, logErr error) { +} + +func (ctx *APIContext) ServerError(title string, err error) {} diff --git a/build/lint-single-response/testdata/src/forgejo.org/services/context/base.go b/build/lint-single-response/testdata/src/forgejo.org/services/context/base.go new file mode 100644 index 0000000000..3e6705caf1 --- /dev/null +++ b/build/lint-single-response/testdata/src/forgejo.org/services/context/base.go @@ -0,0 +1,41 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +// Skeletal implementation of forgejo.org/services/context which allows for test data to access realistic methods on +// Base, Context, APIContext, etc. + +package context + +import ( + "io" + "net/http" + "time" +) + +type Base struct{} + +func (*Base) Status(status int) {} + +func (*Base) Error(status int, contents ...string) {} + +func (*Base) JSON(status int, content any) {} + +func (*Base) PlainTextBytes(status int, bs []byte) {} + +func (*Base) PlainText(status int, text string) {} + +func (*Base) Redirect(location string, status ...int) {} + +func (*Base) ServeContent(r io.ReadSeeker, opts *ServeHeaderOptions) {} + +type ServeHeaderOptions struct { + ContentType string // defaults to "application/octet-stream" + ContentTypeCharset string + ContentLength *int64 + Disposition string // defaults to "attachment" + Filename string + CacheDuration time.Duration // defaults to 5 minutes + LastModified time.Time + AdditionalHeaders http.Header + RedirectStatusCode int +} diff --git a/build/lint-single-response/testdata/src/forgejo.org/services/context/context.go b/build/lint-single-response/testdata/src/forgejo.org/services/context/context.go new file mode 100644 index 0000000000..ef87a2085f --- /dev/null +++ b/build/lint-single-response/testdata/src/forgejo.org/services/context/context.go @@ -0,0 +1,33 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +// Skeletal implementation of forgejo.org/services/context which allows for test data to access realistic methods on +// Base, Context, APIContext, etc. + +package context + +type Context struct { + *Base +} + +func (*Context) HTML(status int, name string) {} + +func (*Context) JSONError(msg any) {} + +func (*Context) JSONOK() {} + +func (*Context) JSONRedirect(redirect string) {} + +func (*Context) JSONTemplate(tmpl string) {} + +func (*Context) NotFound(logMsg string, logErr error) {} + +func (*Context) NotFoundOrServerError(logMsg string, errCheck func(error) bool, logErr error) {} + +func (*Context) RedirectToFirst(location ...string) string { + return "" +} + +func (*Context) RenderWithErr(msg any, tpl string, form any) {} + +func (*Context) ServerError(logMsg string, logErr error) {} diff --git a/go.mod b/go.mod index 223c945964..9485f4faa1 100644 --- a/go.mod +++ b/go.mod @@ -110,6 +110,7 @@ require ( golang.org/x/sync v0.21.0 golang.org/x/sys v0.46.0 golang.org/x/text v0.38.0 + golang.org/x/tools v0.45.0 google.golang.org/protobuf v1.36.11 gopkg.in/gomail.v2 v2.0.0-20160411212932-81ebce5c23df gopkg.in/ini.v1 v1.67.3 @@ -262,7 +263,6 @@ require ( go4.org v0.0.0-20230225012048-214862532bf5 // indirect golang.org/x/mod v0.36.0 // indirect golang.org/x/time v0.15.0 // indirect - golang.org/x/tools v0.45.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20251202230838-ff82c1b0f217 // indirect google.golang.org/grpc v1.79.3 // indirect gopkg.in/alexcesaro/quotedprintable.v3 v3.0.0-20150716171945-2caba252f4dc // indirect diff --git a/routers/api/actions/id_token.go b/routers/api/actions/id_token.go index ab9b2c8d22..678c334cdf 100644 --- a/routers/api/actions/id_token.go +++ b/routers/api/actions/id_token.go @@ -130,6 +130,7 @@ func generateIDToken(ctx *IDTokenContext) { err := json.Unmarshal(inrec, &claims) if err != nil { ctx.Error(http.StatusInternalServerError, "Error generating token") + return } now := time.Now() @@ -143,6 +144,7 @@ func generateIDToken(ctx *IDTokenContext) { signedToken, err := jwtSigningKey.JWT(claims) if err != nil { ctx.Error(http.StatusInternalServerError, "Error signing token") + return } resp := IDTokenResponse{ diff --git a/routers/api/v1/org/member.go b/routers/api/v1/org/member.go index 48785f1656..66007fc3cf 100644 --- a/routers/api/v1/org/member.go +++ b/routers/api/v1/org/member.go @@ -324,6 +324,7 @@ func DeleteMember(ctx *context.APIContext) { } if err := models.RemoveOrgUser(ctx, ctx.Org.Organization.ID, member.ID); err != nil { ctx.Error(http.StatusInternalServerError, "RemoveOrgUser", err) + return } ctx.Status(http.StatusNoContent) } diff --git a/routers/api/v1/repo/avatar.go b/routers/api/v1/repo/avatar.go index 12308ce2df..ef97c4e23b 100644 --- a/routers/api/v1/repo/avatar.go +++ b/routers/api/v1/repo/avatar.go @@ -51,6 +51,7 @@ func UpdateAvatar(ctx *context.APIContext) { err = repo_service.UploadAvatar(ctx, ctx.Repo.Repository, content) if err != nil { ctx.Error(http.StatusInternalServerError, "UploadAvatar", err) + return } ctx.Status(http.StatusNoContent) @@ -82,6 +83,7 @@ func DeleteAvatar(ctx *context.APIContext) { err := repo_service.DeleteAvatar(ctx, ctx.Repo.Repository) if err != nil { ctx.Error(http.StatusInternalServerError, "DeleteAvatar", err) + return } ctx.Status(http.StatusNoContent) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 00908296b3..0855f8d9fb 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -488,6 +488,7 @@ func ListIssues(ctx *context.APIContext) { continue } ctx.Error(http.StatusInternalServerError, "GetMilestoneByRepoID", err) + return } } diff --git a/routers/api/v1/repo/issue_attachment.go b/routers/api/v1/repo/issue_attachment.go index 0cb1875af1..1f8eba880f 100644 --- a/routers/api/v1/repo/issue_attachment.go +++ b/routers/api/v1/repo/issue_attachment.go @@ -289,6 +289,7 @@ func EditIssueAttachment(ctx *context.APIContext) { if err := repo_model.UpdateAttachment(ctx, attachment); err != nil { ctx.Error(http.StatusInternalServerError, "UpdateAttachment", err) + return } ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attachment)) diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index b2e0b57145..7d43c4c152 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -292,6 +292,7 @@ func EditIssueCommentAttachment(ctx *context.APIContext) { if err := repo_model.UpdateAttachment(ctx, attach); err != nil { ctx.Error(http.StatusInternalServerError, "UpdateAttachment", attach) + return } ctx.JSON(http.StatusCreated, convert.ToAPIAttachment(ctx.Repo.Repository, attach)) } diff --git a/routers/api/v1/repo/issue_stopwatch.go b/routers/api/v1/repo/issue_stopwatch.go index 83bcabbe15..b3d3bb52e5 100644 --- a/routers/api/v1/repo/issue_stopwatch.go +++ b/routers/api/v1/repo/issue_stopwatch.go @@ -4,7 +4,6 @@ package repo import ( - "errors" "net/http" issues_model "forgejo.org/models/issues" @@ -49,8 +48,8 @@ func StartIssueStopwatch(ctx *context.APIContext) { // "409": // description: Cannot start a stopwatch again if it already exists - issue, err := prepareIssueStopwatch(ctx, false) - if err != nil { + issue := prepareIssueStopwatch(ctx, false) + if ctx.Written() { return } @@ -98,8 +97,8 @@ func StopIssueStopwatch(ctx *context.APIContext) { // "409": // description: Cannot stop a non existent stopwatch - issue, err := prepareIssueStopwatch(ctx, true) - if err != nil { + issue := prepareIssueStopwatch(ctx, true) + if ctx.Written() { return } @@ -147,8 +146,8 @@ func DeleteIssueStopwatch(ctx *context.APIContext) { // "409": // description: Cannot cancel a non existent stopwatch - issue, err := prepareIssueStopwatch(ctx, true) - if err != nil { + issue := prepareIssueStopwatch(ctx, true) + if ctx.Written() { return } @@ -160,7 +159,7 @@ func DeleteIssueStopwatch(ctx *context.APIContext) { ctx.Status(http.StatusNoContent) } -func prepareIssueStopwatch(ctx *context.APIContext, shouldExist bool) (*issues_model.Issue, error) { +func prepareIssueStopwatch(ctx *context.APIContext, shouldExist bool) *issues_model.Issue { issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if issues_model.IsErrIssueNotExist(err) { @@ -169,31 +168,29 @@ func prepareIssueStopwatch(ctx *context.APIContext, shouldExist bool) (*issues_m ctx.Error(http.StatusInternalServerError, "GetIssueByIndex", err) } - return nil, err + return nil } if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { ctx.Status(http.StatusForbidden) - return nil, errors.New("Unable to write to PRs") + return nil } if !ctx.Repo.CanUseTimetracker(ctx, issue, ctx.Doer) { ctx.Status(http.StatusForbidden) - return nil, errors.New("Cannot use time tracker") + return nil } if issues_model.StopwatchExists(ctx, ctx.Doer.ID, issue.ID) != shouldExist { if shouldExist { ctx.Error(http.StatusConflict, "StopwatchExists", "cannot stop/cancel a non existent stopwatch") - err = errors.New("cannot stop/cancel a non existent stopwatch") } else { ctx.Error(http.StatusConflict, "StopwatchExists", "cannot start a stopwatch again if it already exists") - err = errors.New("cannot start a stopwatch again if it already exists") } - return nil, err + return nil } - return issue, nil + return issue } // GetStopwatches get all stopwatches diff --git a/routers/api/v1/repo/issue_tracked_time.go b/routers/api/v1/repo/issue_tracked_time.go index 1b4f19fb66..1837fb184c 100644 --- a/routers/api/v1/repo/issue_tracked_time.go +++ b/routers/api/v1/repo/issue_tracked_time.go @@ -101,6 +101,7 @@ func ListTrackedTimes(ctx *context.APIContext) { user, err := user_model.GetUserByName(ctx, qUser) if user_model.IsErrUserNotExist(err) { ctx.Error(http.StatusNotFound, "User does not exist", err) + return } else if err != nil { ctx.Error(http.StatusInternalServerError, "GetUserByName", err) return @@ -212,6 +213,7 @@ func AddTime(ctx *context.APIContext) { user, err = user_model.GetUserByName(ctx, form.User) if err != nil { ctx.Error(http.StatusInternalServerError, "GetUserByName", err) + return } } } @@ -531,6 +533,7 @@ func ListTrackedTimesByRepository(ctx *context.APIContext) { user, err := user_model.GetUserByName(ctx, qUser) if user_model.IsErrUserNotExist(err) { ctx.Error(http.StatusNotFound, "User does not exist", err) + return } else if err != nil { ctx.Error(http.StatusInternalServerError, "GetUserByName", err) return diff --git a/routers/api/v1/repo/mirror.go b/routers/api/v1/repo/mirror.go index 834c1d20e3..fbf171e245 100644 --- a/routers/api/v1/repo/mirror.go +++ b/routers/api/v1/repo/mirror.go @@ -58,6 +58,7 @@ func MirrorSync(ctx *context.APIContext) { if !ctx.Repo.CanWrite(unit.TypeCode) { ctx.Error(http.StatusForbidden, "MirrorSync", "Must have write access") + return } if !setting.Mirror.Enabled { diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index a0c21f3c40..4899174161 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -296,6 +296,7 @@ func CreateUserRepo(ctx *context.APIContext, owner *user_model.User, opt api.Cre repo, err = repo_model.GetRepositoryByID(ctx, repo.ID) if err != nil { ctx.Error(http.StatusInternalServerError, "GetRepositoryByID", err) + return } ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, repo, access_model.Permission{AccessMode: perm.AccessModeOwner})) diff --git a/routers/api/v1/repo/tag.go b/routers/api/v1/repo/tag.go index 80e34938a7..721426f871 100644 --- a/routers/api/v1/repo/tag.go +++ b/routers/api/v1/repo/tag.go @@ -120,6 +120,7 @@ func GetAnnotatedTag(ctx *context.APIContext) { commit, err := tag.Commit(ctx.Repo.GitRepo) if err != nil { ctx.Error(http.StatusBadRequest, "GetAnnotatedTag", err) + return } convertedAnnotatedTag, err := convert.ToAnnotatedTag(ctx, ctx.Repo.GitRepo, ctx.Repo.Repository, tag, commit) diff --git a/routers/api/v1/repo/teams.go b/routers/api/v1/repo/teams.go index d41ca46757..417136f58a 100644 --- a/routers/api/v1/repo/teams.go +++ b/routers/api/v1/repo/teams.go @@ -189,6 +189,7 @@ func DeleteTeam(ctx *context.APIContext) { func changeRepoTeam(ctx *context.APIContext, add bool) { if !ctx.Repo.Owner.IsOrganization() { ctx.Error(http.StatusMethodNotAllowed, "noOrg", "repo is not owned by an organization") + return } if !ctx.Repo.Owner.RepoAdminChangeTeamAccess && !ctx.Repo.IsOwner() { ctx.Error(http.StatusForbidden, "noAdmin", "user is nor repo admin nor owner") diff --git a/routers/api/v1/shared/quota.go b/routers/api/v1/shared/quota.go index ceba9fea57..3fc6eceaf5 100644 --- a/routers/api/v1/shared/quota.go +++ b/routers/api/v1/shared/quota.go @@ -58,6 +58,7 @@ func ListQuotaAttachments(ctx *context.APIContext, userID int64) { result, err := convert.ToQuotaUsedAttachmentList(ctx, *attachments) if err != nil { ctx.Error(http.StatusInternalServerError, "convert.ToQuotaUsedAttachmentList", err) + return } ctx.SetLinkHeader(int(count), opts.PageSize) @@ -76,6 +77,7 @@ func ListQuotaPackages(ctx *context.APIContext, userID int64) { result, err := convert.ToQuotaUsedPackageList(ctx, *packages) if err != nil { ctx.Error(http.StatusInternalServerError, "convert.ToQuotaUsedPackageList", err) + return } ctx.SetLinkHeader(int(count), opts.PageSize) @@ -94,6 +96,7 @@ func ListQuotaArtifacts(ctx *context.APIContext, userID int64) { result, err := convert.ToQuotaUsedArtifactList(ctx, *artifacts) if err != nil { ctx.Error(http.StatusInternalServerError, "convert.ToQuotaUsedArtifactList", err) + return } ctx.SetLinkHeader(int(count), opts.PageSize) diff --git a/routers/api/v1/shared/runners.go b/routers/api/v1/shared/runners.go index 6c99630ae1..3c7557868a 100644 --- a/routers/api/v1/shared/runners.go +++ b/routers/api/v1/shared/runners.go @@ -143,6 +143,7 @@ func GetRunner(ctx *context.APIContext, ownerID, repoID, runnerID int64) { actionRunner, err := convert.ToActionRunner(runner) if err != nil { ctx.Error(http.StatusInternalServerError, "ToActionRunner", err) + return } ctx.JSON(http.StatusOK, actionRunner) } @@ -165,6 +166,7 @@ func RegisterRunner(ctx *context.APIContext, ownerID, repoID int64) { runner.GenerateToken() if err := actions_model.CreateRunner(ctx, runner); err != nil { ctx.Error(http.StatusInternalServerError, "CreateRunner", err) + return } response := &structs.RegisterRunnerResponse{ diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index 4455649f26..21a3d26078 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -237,6 +237,7 @@ func VerifyUserGPGKey(ctx *context.APIContext) { return } ctx.Error(http.StatusInternalServerError, "VerifyUserGPGKey", err) + return } keys, err := db.Find[asymkey_model.GPGKey](ctx, asymkey_model.FindGPGKeyOptions{ diff --git a/routers/api/v1/user/star.go b/routers/api/v1/user/star.go index ea50080804..967bb51a56 100644 --- a/routers/api/v1/user/star.go +++ b/routers/api/v1/user/star.go @@ -107,6 +107,7 @@ func GetMyStarredRepos(ctx *context.APIContext) { repos, err := getStarredRepos(ctx, ctx.Doer, true, utils.GetListOptions(ctx)) if err != nil { ctx.Error(http.StatusInternalServerError, "getStarredRepos", err) + return } ctx.SetTotalCountHeader(int64(ctx.Doer.NumStars)) diff --git a/routers/api/v1/user/watch.go b/routers/api/v1/user/watch.go index c31744b280..42bff98143 100644 --- a/routers/api/v1/user/watch.go +++ b/routers/api/v1/user/watch.go @@ -69,6 +69,7 @@ func GetWatchedRepos(ctx *context.APIContext) { repos, total, err := getWatchedRepos(ctx, ctx.ContextUser, private, utils.GetListOptions(ctx)) if err != nil { ctx.Error(http.StatusInternalServerError, "getWatchedRepos", err) + return } ctx.SetTotalCountHeader(total) @@ -102,6 +103,7 @@ func GetMyWatchedRepos(ctx *context.APIContext) { repos, total, err := getWatchedRepos(ctx, ctx.Doer, true, utils.GetListOptions(ctx)) if err != nil { ctx.Error(http.StatusInternalServerError, "getWatchedRepos", err) + return } ctx.SetTotalCountHeader(total) diff --git a/routers/private/actions.go b/routers/private/actions.go index 2b22c0f964..a85c695752 100644 --- a/routers/private/actions.go +++ b/routers/private/actions.go @@ -41,6 +41,7 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) { ctx.JSON(http.StatusInternalServerError, private.Response{ Err: err.Error(), }) + return } ownerID := optional.None[int64]() diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index f7013f14c6..11ec5e9a9f 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -189,6 +189,7 @@ func HookPostReceive(ctx *app_context.PrivateContext) { ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), }) + return } } diff --git a/routers/private/manager.go b/routers/private/manager.go index 90b48256df..0c2e9c6948 100644 --- a/routers/private/manager.go +++ b/routers/private/manager.go @@ -53,6 +53,7 @@ func FlushQueues(ctx *context.PrivateContext) { ctx.JSON(http.StatusRequestTimeout, private.Response{ UserMsg: fmt.Sprintf("%v", err), }) + return } ctx.PlainText(http.StatusOK, "success") } diff --git a/routers/private/serv.go b/routers/private/serv.go index 26f7e288cd..a63e5ea86d 100644 --- a/routers/private/serv.go +++ b/routers/private/serv.go @@ -54,6 +54,7 @@ func ServNoCommand(ctx *context.PrivateContext) { ctx.JSON(http.StatusBadRequest, private.Response{ UserMsg: fmt.Sprintf("Bad key id: %d", keyID), }) + return } results := private.KeyAndOwner{} diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 2f502871fb..723edff290 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -246,11 +246,11 @@ func SignInPost(ctx *context.Context) { u, source, err := auth_method.UserSignIn(ctx, form.UserName, form.Password) if err != nil { if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) { + log.Warn("Failed authentication attempt for %s from %s: %v", form.UserName, ctx.RemoteAddr(), err) ctx.RenderWithErr(ctx.Tr("form.username_password_incorrect"), tplSignIn, &form) - log.Warn("Failed authentication attempt for %s from %s: %v", form.UserName, ctx.RemoteAddr(), err) } else if user_model.IsErrEmailAlreadyUsed(err) { - ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSignIn, &form) log.Warn("Failed authentication attempt for %s from %s: %v", form.UserName, ctx.RemoteAddr(), err) + ctx.RenderWithErr(ctx.Tr("form.email_been_used"), tplSignIn, &form) } else if user_model.IsErrUserProhibitLogin(err) { log.Warn("Failed authentication attempt for %s from %s: %v", form.UserName, ctx.RemoteAddr(), err) ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") @@ -673,11 +673,12 @@ func handleUserCreated(ctx *context.Context, u *user_model.User, gothUser *goth. ctx.Data["IsSendRegisterMail"] = true ctx.Data["Email"] = u.Email ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) - ctx.HTML(http.StatusOK, TplActivate) if err := ctx.Cache.Put("MailResendLimit_"+u.LowerName, u.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) } + + ctx.HTML(http.StatusOK, TplActivate) return false } diff --git a/routers/web/moderation/report.go b/routers/web/moderation/report.go index 39ca9e8824..2ce58f0e4f 100644 --- a/routers/web/moderation/report.go +++ b/routers/web/moderation/report.go @@ -25,8 +25,8 @@ func NewReport(ctx *context.Context) { contentID := ctx.FormInt64("id") if contentID <= 0 { setMinimalContextData(ctx) - ctx.RenderWithErr(ctx.Tr("moderation.report_abuse_form.invalid"), tplSubmitAbuseReport, nil) log.Warn("The content ID is expected to be an integer greater that 0; the provided value is %s.", ctx.FormString("id")) + ctx.RenderWithErr(ctx.Tr("moderation.report_abuse_form.invalid"), tplSubmitAbuseReport, nil) return } @@ -43,8 +43,8 @@ func NewReport(ctx *context.Context) { contentType = moderation.ReportedContentTypeComment default: setMinimalContextData(ctx) - ctx.RenderWithErr(ctx.Tr("moderation.report_abuse_form.invalid"), tplSubmitAbuseReport, nil) log.Warn("The provided content type `%s` is not among the expected values.", contentTypeString) + ctx.RenderWithErr(ctx.Tr("moderation.report_abuse_form.invalid"), tplSubmitAbuseReport, nil) return } diff --git a/routers/web/repo/badges/badges.go b/routers/web/repo/badges/badges.go index bf39064be0..f8c0635b82 100644 --- a/routers/web/repo/badges/badges.go +++ b/routers/web/repo/badges/badges.go @@ -153,6 +153,7 @@ func GetLatestReleaseBadge(ctx *app_context.Context) { return } ctx.ServerError("GetLatestReleaseByRepoID", err) + return } if err := release.LoadAttributes(ctx); err != nil { diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 5a6e99f8aa..40cb0b113e 100644 --- a/routers/web/repo/editor.go +++ b/routers/web/repo/editor.go @@ -417,6 +417,7 @@ func editFilePost(ctx *context.Context, form forms.EditRepoFileForm, isNewFile b } ctx.RenderWithErr(flashError, tplEditFile, &form) } + return } if ctx.Repo.Repository.IsEmpty { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index c05497dd5d..f0857e22ff 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2459,6 +2459,7 @@ func UpdateIssueMilestone(ctx *context.Context) { has, err := db.GetEngine(ctx).Where("issue_id = ? AND type = ?", issue.ID, issues_model.CommentTypeMilestone).OrderBy("id DESC").Limit(1).Get(comment) if !has || err != nil { ctx.ServerError("GetLatestMilestoneComment", err) + return } if err := comment.LoadMilestone(ctx); err != nil { ctx.ServerError("LoadMilestone", err) @@ -2942,6 +2943,7 @@ func ListIssues(ctx *context.Context) { continue } ctx.Error(http.StatusInternalServerError, err.Error()) + return } } diff --git a/routers/web/repo/issue_dependency.go b/routers/web/repo/issue_dependency.go index 0d9595f2a4..aac0e35093 100644 --- a/routers/web/repo/issue_dependency.go +++ b/routers/web/repo/issue_dependency.go @@ -34,13 +34,11 @@ func AddDependency(ctx *context.Context) { return } - // Redirect - defer ctx.Redirect(issue.Link()) - // Dependency dep, err := issues_model.GetIssueByID(ctx, depID) if err != nil { ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_dep_issue_not_exist")) + ctx.Redirect(issue.Link()) return } @@ -48,6 +46,7 @@ func AddDependency(ctx *context.Context) { if issue.RepoID != dep.RepoID { if !setting.Service.AllowCrossRepositoryDependencies { ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_dep_not_same_repo")) + ctx.Redirect(issue.Link()) return } if err := dep.LoadRepo(ctx); err != nil { @@ -62,6 +61,7 @@ func AddDependency(ctx *context.Context) { } if !depRepoPerm.CanReadIssuesOrPulls(dep.IsPull) { // you can't see this dependency + ctx.Redirect(issue.Link()) return } } @@ -69,6 +69,7 @@ func AddDependency(ctx *context.Context) { // Check if issue and dependency is the same if dep.ID == issue.ID { ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_same_issue")) + ctx.Redirect(issue.Link()) return } @@ -76,14 +77,18 @@ func AddDependency(ctx *context.Context) { if err != nil { if issues_model.IsErrDependencyExists(err) { ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_dep_exists")) + ctx.Redirect(issue.Link()) return } else if issues_model.IsErrCircularDependency(err) { ctx.Flash.Error(ctx.Tr("repo.issues.dependency.add_error_cannot_create_circular")) + ctx.Redirect(issue.Link()) return } ctx.ServerError("CreateOrUpdateIssueDependency", err) return } + + ctx.Redirect(issue.Link()) } // RemoveDependency removes the dependency diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go index 5631027c3c..3d9067f6bc 100644 --- a/routers/web/repo/projects.go +++ b/routers/web/repo/projects.go @@ -649,6 +649,7 @@ func MoveIssues(ctx *context.Context) { form := &movedIssuesForm{} if err = json.NewDecoder(ctx.Req.Body).Decode(&form); err != nil { ctx.ServerError("DecodeMovedIssuesForm", err) + return } issueIDs := make([]int64, 0, len(form.Issues)) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 6ba951ee5f..ec2d2e778b 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1527,6 +1527,7 @@ func MergePullRequest(ctx *context.Context) { ctx.Flash.Error(ctx.Tr("repo.pulls.delete_after_merge.head_branch.insufficient_branch")) default: ctx.ServerError("DeleteBranchAfterMerge", err) + return } ctx.JSONRedirect(issue.Link()) @@ -1676,7 +1677,6 @@ func CompareAndPullRequestPost(ctx *context.Context) { log.Error("Unexpected error of NewPullRequest: %T %s", err, err) ctx.ServerError("CompareAndPullRequest", err) } - ctx.ServerError("NewPullRequest", err) return } @@ -1988,6 +1988,7 @@ func PrepareViewPullInfoActionsTrust(ctx *context.Context, pull *issues_model.Pu someRunsNeedApproval, err := actions_model.HasRunThatNeedApproval(ctx, pull.Issue.RepoID, pull.ID) if err != nil { ctx.ServerError("HasRunThatNeedApproval", err) + return } ctx.Data["SomePullRequestRunsNeedApproval"] = someRunsNeedApproval diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index b001cf961e..1891497b19 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -396,8 +396,8 @@ func SettingsPost(ctx *context.Context) { case "federation": if !setting.Federation.Enabled { - ctx.NotFound("", nil) ctx.Flash.Info(ctx.Tr("repo.settings.federation_not_enabled")) + ctx.NotFound("", nil) return } followingRepos := strings.TrimSpace(form.FollowingRepos) diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index df0255279a..09adfae069 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -1065,6 +1065,7 @@ func renderHomeCode(ctx *context.Context) { return } ctx.Redirect(submodule.ResolveUpstreamURL(ctx.Repo.Repository.HTMLURL())) + return // technically ctx.Written() check below is fine, but this passes lint-single-response } else if entry.IsDir() { renderDirectory(ctx) } else { diff --git a/routers/web/repo/wiki.go b/routers/web/repo/wiki.go index 8153288312..51bffb6191 100644 --- a/routers/web/repo/wiki.go +++ b/routers/web/repo/wiki.go @@ -151,12 +151,15 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { return nil, nil } + defer func() { + if ctx.Written() { + wikiRepo.Close() + } + }() + // Get page list. entries, err := commit.ListEntries() if err != nil { - if wikiRepo != nil { - wikiRepo.Close() - } ctx.ServerError("ListEntries", err) return nil, nil } @@ -170,9 +173,6 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { if repo_model.IsErrWikiInvalidFileName(err) { continue } - if wikiRepo != nil { - wikiRepo.Close() - } ctx.ServerError("WikiFilenameToName", err) return nil, nil } else if wikiName == "_Sidebar" || wikiName == "_Footer" { @@ -206,8 +206,8 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { data, entry, pageFilename, noEntry := wikiContentsByName(ctx, commit, pageName) if noEntry { ctx.Redirect(ctx.Repo.RepoLink + "/wiki/?action=_pages") - } - if entry == nil || ctx.Written() { + return nil, nil + } else if entry == nil { if wikiRepo != nil { wikiRepo.Close() } @@ -218,9 +218,6 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { if !isSideBar { sidebarContent, _, _, _ = wikiContentsByName(ctx, commit, "_Sidebar") if ctx.Written() { - if wikiRepo != nil { - wikiRepo.Close() - } return nil, nil } } else { @@ -231,9 +228,6 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { if !isFooter { footerContent, _, _, _ = wikiContentsByName(ctx, commit, "_Footer") if ctx.Written() { - if wikiRepo != nil { - wikiRepo.Close() - } return nil, nil } } else { @@ -270,9 +264,6 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { ctx.Data["EscapeStatus"], ctx.Data["content"], err = renderFn(data) if err != nil { - if wikiRepo != nil { - wikiRepo.Close() - } ctx.ServerError("Render", err) return nil, nil } @@ -291,9 +282,6 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { buf.Reset() ctx.Data["sidebarEscapeStatus"], ctx.Data["sidebarContent"], err = renderFn(sidebarContent) if err != nil { - if wikiRepo != nil { - wikiRepo.Close() - } ctx.ServerError("Render", err) return nil, nil } @@ -306,9 +294,6 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { buf.Reset() ctx.Data["footerEscapeStatus"], ctx.Data["footerContent"], err = renderFn(footerContent) if err != nil { - if wikiRepo != nil { - wikiRepo.Close() - } ctx.ServerError("Render", err) return nil, nil } @@ -336,6 +321,12 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) return nil, nil } + defer func() { + if ctx.Written() { + wikiRepo.Close() + } + }() + // get requested pagename pageName := wiki_service.WebPathFromRequest(ctx.PathParamRaw("*")) if len(pageName) == 0 { @@ -355,8 +346,9 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) data, entry, pageFilename, noEntry := wikiContentsByName(ctx, commit, pageName) if noEntry { ctx.Redirect(ctx.Repo.RepoLink + "/wiki/?action=_pages") + return nil, nil } - if entry == nil || ctx.Written() { + if entry == nil { if wikiRepo != nil { wikiRepo.Close() } @@ -384,9 +376,6 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) Page: page, }) if err != nil { - if wikiRepo != nil { - wikiRepo.Close() - } ctx.ServerError("CommitsByFileAndRange", err) return nil, nil } @@ -433,8 +422,8 @@ func renderEditPage(ctx *context.Context) { data, entry, _, noEntry := wikiContentsByName(ctx, commit, pageName) if noEntry { ctx.Redirect(ctx.Repo.RepoLink + "/wiki/?action=_pages") - } - if entry == nil || ctx.Written() { + return + } else if entry == nil { return } diff --git a/routers/web/shared/storage_overview.go b/routers/web/shared/storage_overview.go index fac4aa99e5..a2726620c8 100644 --- a/routers/web/shared/storage_overview.go +++ b/routers/web/shared/storage_overview.go @@ -18,6 +18,7 @@ import ( func StorageOverview(ctx *context.Context, userID int64, tpl base.TplName) { if !setting.Quota.Enabled { ctx.NotFound("MustEnableQuota", nil) + return } ctx.Data["Title"] = ctx.Tr("settings.storage_overview") ctx.Data["PageIsStorageOverview"] = true diff --git a/routers/web/user/notification.go b/routers/web/user/notification.go index c79852f29e..f92f0d6b24 100644 --- a/routers/web/user/notification.go +++ b/routers/web/user/notification.go @@ -203,6 +203,7 @@ func NotificationStatusPost(ctx *context.Context) { if !ctx.FormBool("noredirect") { url := fmt.Sprintf("%s/notifications?page=%s", setting.AppSubURL, url.QueryEscape(ctx.FormString("page"))) ctx.Redirect(url, http.StatusSeeOther) + return } getNotifications(ctx) diff --git a/routers/web/user/setting/keys.go b/routers/web/user/setting/keys.go index 935efd7ba7..054b6a35ee 100644 --- a/routers/web/user/setting/keys.go +++ b/routers/web/user/setting/keys.go @@ -156,6 +156,7 @@ func KeysPost(ctx *context.Context) { default: ctx.ServerError("VerifyGPG", err) } + return } ctx.Flash.Success(ctx.Tr("settings.verify_gpg_key_success", keyID)) ctx.Redirect(setting.AppSubURL + "/user/settings/keys") @@ -227,6 +228,7 @@ func KeysPost(ctx *context.Context) { default: ctx.ServerError("VerifySSH", err) } + return } ctx.Flash.Success(ctx.Tr("settings.verify_ssh_key_success", fingerprint)) ctx.Redirect(setting.AppSubURL + "/user/settings/keys") @@ -281,6 +283,7 @@ func DeleteKey(ctx *context.Context) { default: ctx.Flash.Warning("Function not implemented") ctx.Redirect(setting.AppSubURL + "/user/settings/keys") + return } ctx.JSONRedirect(setting.AppSubURL + "/user/settings/keys") } diff --git a/routers/web/webfinger.go b/routers/web/webfinger.go index 5f67e436bf..fb11cd3f9e 100644 --- a/routers/web/webfinger.go +++ b/routers/web/webfinger.go @@ -73,12 +73,11 @@ func WebfingerQuery(ctx *context.Context) { } ctx.Resp.Header().Add("Access-Control-Allow-Origin", "*") - ctx.JSON(http.StatusOK, &webfingerJRD{ + ctx.JSONWithContentType(http.StatusOK, "application/jrd+json", &webfingerJRD{ Subject: fmt.Sprintf("acct:%s@%s", "ghost", appURL.Host), Aliases: aliases, Links: links, }) - ctx.Resp.Header().Set("Content-Type", "application/jrd+json") return } @@ -184,10 +183,9 @@ func WebfingerQuery(ctx *context.Context) { } ctx.Resp.Header().Add("Access-Control-Allow-Origin", "*") - ctx.JSON(http.StatusOK, &webfingerJRD{ + ctx.JSONWithContentType(http.StatusOK, "application/jrd+json", &webfingerJRD{ Subject: fmt.Sprintf("acct:%s@%s", url.QueryEscape(u.Name), appURL.Host), Aliases: aliases, Links: links, }) - ctx.Resp.Header().Set("Content-Type", "application/jrd+json") } diff --git a/services/context/base.go b/services/context/base.go index dc3d226bb0..5e71f83576 100644 --- a/services/context/base.go +++ b/services/context/base.go @@ -130,7 +130,12 @@ func (b *Base) Error(status int, contents ...string) { // JSON render content as JSON func (b *Base) JSON(status int, content any) { - b.Resp.Header().Set("Content-Type", "application/json;charset=utf-8") + b.JSONWithContentType(status, "application/json;charset=utf-8", content) +} + +// JSON render content as JSON with a customizable Content-Type header +func (b *Base) JSONWithContentType(status int, contentType string, content any) { + b.Resp.Header().Set("Content-Type", contentType) b.Resp.WriteHeader(status) if err := json.NewEncoder(b.Resp).Encode(content); err != nil { log.Error("Render JSON failed: %v", err)