16793050ce
#13087 added a new lint, `lint-single-response`, which guarantees that web handlers don't proceed after making a terminating call (eg. `ctx.Error(...)`). #12512 introduced a new type with these terminating-style methods on the interface `forgejo.org/routers/api/v1/permissions.Context`. This PR adds the new methods to the list of terminating calls.
## Testing
In addition to automated tests which check the usage, I've manually altered codepaths in `routers/api/v1` and removed `return` statements (for example, removing [this line](1ab4c8d915/routers/api/v1/permissions/req_admin.go (L18))), and then running `make lint-single-response`:
```
$ make lint-single-response
.../go/pkg/mod/golang.org/toolchain@v0.0.1-go1.26.4.linux-amd64/bin/go run ./build/lint-single-response/cmd ./...
.../Dev/forgejo/routers/api/v1/permissions/req_admin.go:17:3: Invocation of forgejo.org/routers/api/v1/permissions.Context / NotFound, and control flow continues afterwards.
exit status 3
make: *** [Makefile:535: lint-single-response] Error 1
```
This testing was important because `permissions.Context` is the first interface to be involved in this new lint, where the other usages are all structs.
### 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
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] 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/13234
Reviewed-by: limiting-factor <limiting-factor@noreply.codeberg.org>
3 KiB
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:
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, which we'll call terminating functions. The current list of such functions is in the singleresponse.go file, in the terminatingFuncs constant.
When a terminating function is used, the control flow of the calling function must not perform any work after the terminating function is invoked -- the control flow can only exit, via return or via reaching the end of the calling 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 a terminating function, 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 terminating functions 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. 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.