**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13066
A partial backport of the `tests/forgery` changes from `feat: make delete repository transcation READ COMMITTED (#12856)` is included and makes it a clean cherry-pick.
- Move the permissions and consistency logic to services/context/project.go
- When an invalid `project=id` is provided in the query string to display the form to create a new issue, it will now fail instead of silently discard the invalid id
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13504
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13495
Currently the dropdown will overflow on narrow screens, because it is
aligned on the left side to the button. To prevent this, align the right
side to the button.
| Before | After |
|--------|------|
| |  |
|||
Co-authored-by: Beowulf <beowulf@beocode.eu>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13497
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
Reviewed-by: Beowulf <beowulf@beocode.eu>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13179
With this settings:
```
[attachment]
ENABLED = false
```
it's not possible to edit a comment
```
repo-legacy.js:378 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'dropzone')
at HTMLDivElement.onEditContent (repo-legacy.js:378:19)
at HTMLDocument.dispatch (jquery.js:5145:1)
at elemData.handle (jquery.js:4949:1)
```
The dropzone is not displayed when attachment is disabled
## Testing
1. Disable attachments via
```
[attachment]
ENABLED = false
```
2. Check that you can still edit a comment you authored on any issue, any repository.
Co-authored-by: steven.guiheux <steven.guiheux@ovhcloud.com>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13473
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13424
Forgejo only added the legacy `gitea_runtime_token`.
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
(can be removed for JavaScript changes)
- I added test coverage for Go changes...
- [x] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [x] `make pr-go` before pushing
### Tests for JavaScript changes
(can be removed for Go changes)
- I added test coverage for JavaScript changes...
- [ ] in `web_src/js/*.test.js` if it can be unit tested.
- [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [ ] 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.
*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*
The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.
Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13458
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13443Fixes#13440.
## Testing
Manual testing was performed with an automated reproduction. Attached `main.go` program was used to test go-git interaction with a server using `START_SSH_SERVER = true`. This test app uses go-git to perform an ssh-based push, which replicates the behaviour described in #13440. After dropping main.go into an empty directory, the test app can be run with:
```
go mod init repro
go mod tidy
REPRO_REPO_URL=ssh://mfenniak@localhost:2222/mfenniak/test.git go run main.go
```
When the fix is **not** present, the database/repository desync warning is shown after running the test command, which indicates that the hooks were interrupted and did not update the database state of the `main` branch:
When this fix **is** present, the database/repository desync warning does not appear, which indicates that the hooks were not interrupted. Repeated test in both modes 3x times to ensure there were no race conditions involved.
While it would be possible to incorporate this test into Forgejo's integration testing suite, it would require adding a dependency to `go-git`. As Go doesn't have a concept of test-only dependencies, that would make `go-git` a dependency... which we just recently removed by removing it from Forgejo Runner's jobparser library. It would make `go-git` available for accidental incorporation into Forgejo's code, where it does not behave the same as Forgejo's internal git CLI libraries.
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13451
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/12415
The admin system status template wraps the server start time in
<relative-time format="duration"> intending to display elapsed time as
a bare duration ("two months"), but the custom element ignored the
format attribute and always rendered "two months ago". Implement the
duration mode using Intl.NumberFormat with style: "unit" so the same
calendar-aware diffing produces a localized, suffix-free string.
Resolves#12078
Co-authored-by: Burak Emir <buraqml@proton.me>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13401
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13406
Forgejo only added the legacy `gitea_default_actions_url`.
### 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
### 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.
Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13411
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13331
- Fix migrating https://github.com/charlesrocket/freebsd-collection. It
has 2 issues and 43 pull requests.
- This creates the interesting situation: len(issues) = 45. perPage = 45
thus no stop condition. But! resp.After is empty, so on the next request
the same list of issues is requested and then runs into unique
constraint failure.
- Make the stop condition based if resp.After is empty, thus indicating
its the last page.
Co-authored-by: Gusted <postmaster@gusted.xyz>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13397
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/12917
I noticed that when generating a repo from a template repo, if a file in the template repo contains an unrecognized variable reference (not part of the set of variables that Forgejo replaces), it attempts to expand it anyway, and ultimately replaces it with just the variable name, stripping off the `$` or `${}`. For example, if a file contains `Authorization: Bearer ${ACCESS_TOKEN}`, in the generated repo this will be changed to `Authorization: Bearer ACCESS_TOKEN` which is unexpected and undesired.
I considered whether it would be possible to fix this by simply making the function passed to `os.Expand()` return `${VARIABLE_NAME}` instead of just `VARIABLE_NAME` for unrecognized keys, but this wouldn't work because it's not possible to distinguish between `$VARIABLE_NAME` and `${VARIABLE_NAME}` syntax, so some variable references would get mangled anyway. Instead, I fixed it by adding a simple regex and using that to perform the replacements instead of `os.Expand()`. I added unit tests and updated one of the generated repo integration tests to validate the functionality.
Co-authored-by: Brandon Rothweiler <bdr9@noreply.codeberg.org>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13388
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13367
- adjusted the starting for the acme HTTP handler to use `net.JoinHostPort` to join host and port
- Resolves#12274
- I tested on my server which only had ipv6 enabled.
- Log message before fix: `...s/graceful/server.go:76:ListenAndServe() [E] Unable to GetListener: address [[::]:443]:443: too many colons in address`
- Log message after fix: `...s/graceful/server.go:50:NewServer() [I] Starting new Let's Encrypt HTTP Challenge server: tcp:[::]:80 on PID: 31302`
Co-authored-by: Florian Lang <florianlang@noreply.codeberg.org>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13380
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/10927
Continuation of #10598Closes#7254
Implement repo granular watch selection
I adopted a lot of the frontend from #10598 while redoing the entire back-end. I propose this new model:
- Remove the WatchMode enum.
- Add the WatchSelection struct, which represents the granular selection. Notice that there is no `watching` bool. I tried very hard to keep the structure as simple and redundancy-free as possible. Therefore, people not watching a repo at all either don't have a watch record or one with an entirely unselected WatchSelection struct.
- Add the WatchSource enum. It replaces the WatchMode enum and has a single purpose: determine whether a watch was explicitly or automatically initiated.
Notice that replacing this
```go
And("`watch`.mode<>?", WatchModeDont).
```
with this is correct:
```go
And(
builder.Or(
builder.Eq{"`watch`.watch_selection_issues": true},
builder.Eq{"`watch`.watch_selection_pull_requests": true},
builder.Eq{"`watch`.watch_selection_releases": true},
),
).
```
That's because there are four modes: dont, none, auto and normal. When `<>` with dont, we look for auto and normal, because there are no records with none. Therefore, the old code looks for records that indicate watching. The code I replaced this with does so, too, just more granular.
Also notice that I've prepared a future `user preset` in a few places. See below for a little more info on that.
## Next PR
I plan to continue working on this. I want to implement a `user preset` option. The user sets that `user preset` in her settings and may use them in any repo.
<details>
- rename account settings to account and notifications
- user preset (always use this preset for newly accessible repos (according to AutoWatchOnChanges and AutoWatchNewRepos))
</details>
## Further PRs
- make api able to granular watch
- move (email) notifications to new notifications tab
Co-authored-by: 0ko <0ko@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-authored-by: pat-s <patrick.schratz@gmail.com>
Reviewed-on: #10927
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
(cherry picked from commit 8770ffc848)
Forgejo's git pre-receive hook validates whether the authenticated user can write to the target branch during a push operation. A caching performance optimization in the hook caused the pre-receive hook to validate only the first branch reference in the push. While typically a push operation has the same rights for all branches on a repository, some exceptions exist which could allow a user to push to a branch, pass the validation, and then bypass checks on additional references due to the cache. The cache has been removed, forcing all references to be validated for code write permissions.
Thanks to Gitea for identifying this issue in https://github.com/go-gitea/gitea/pull/38103/changes/f25811942cea35233299efdbdfeec40663d4807a. This effort has been re-engineered for Forgejo, consistent with Forgejo's AI Agreement.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13371
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/11795
Adds an extra check to ensure the `keyId` and `actorId` included in signed requests and actor records point back to the originating host. This check prevents server-side request forgery (SSRF) attacks where a carefully crafted request could be used to trick a federation server into making requests to arbitrary hosts and ports.
### Tests for Go changes
- I added test coverage for Go changes...
- [x] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [x] `make pr-go` before pushing
Co-authored-by: elle <0xllx0@noreply.codeberg.org>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13351
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13338
Resolves https://codeberg.org/forgejo/forgejo/issues/13319.
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] 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
- [x] 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.
- [ ] 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.
Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13344
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13085
Job logs now render OSC 8 hyperlink escape sequences as clickable links in the Actions log viewer, so the Forgejo log has clickable links. A workflow step opts in by emitting the OSC 8 sequence (`ESC ] 8 ; ; <url> ESC \ <text> ESC ] 8 ; ; ESC \`), which is the same escape code terminals already use for links.
Hardens the anchors `ansi_up` emits: adds `rel="noopener noreferrer nofollow"` and `target="_blank"`, and drops any link whose scheme is not http or https.
## Checklist
### Tests for JavaScript changes
- I added test coverage for JavaScript changes...
- [x] in `web_src/js/*.test.js` if it can be unit tested.
- [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server.
### 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
- [x] 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.
- [ ] 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.
Co-authored-by: Dax Kelson <daxkelson@gmail.com>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13324
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13271
- separate test cases from fixtures
- testcase.go: A `testCase` describes a test using `testData`
- fixture.go: `fixture*` are function that creates the conditions for this test case to run
- move the testData closer to the beginning
- update the README.md
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13271
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
(cherry picked from commit c1351e62d4)
```
Conflicts:
routers/api/v1/permissions/tests/README.md
one line because coverage targets are not the same in v15, keep the
non coverage documentation
routers/api/v1/permissions/tests/check_fork_destination.go
ignore as it only exists in the main branch
```
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13312
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13299fixes#13295
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] 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
- [x] 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.
- [ ] 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.
*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*
The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.
Co-authored-by: dawe <dawedawe@posteo.de>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13310
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13304
Regression in #12776 -- `[service].REQUIRE_SIGNIN_VIEW` and `[service.explore].REQUIRE_SIGNIN_VIEW` values were being cached into a constructed middleware during module initialization, before the `settings` object had been initialized by reading settings from the config. As a result, neither setting applied to the middleware. Apparently zero automated tests cover this capability.
Fixed by changing the bool into a closure for realtime access to the setting. Added automated tests. Verified that the tests failed when broken.
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] 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.
- Prerelease regression.
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13307
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13258Close#13250
ExtraLinesCount was not send to reply template
### Tests for Go changes
(can be removed for JavaScript changes)
- I added test coverage for Go changes...
- [X] 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 did not document these changes and I do not expect someone else to do it.
Co-authored-by: steven.guiheux <steven.guiheux@ovhcloud.com>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13303
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13293
Test failure: https://codeberg.org/forgejo/forgejo/actions/runs/174007/jobs/2/attempt/1 Have only observed once and can't reproduce locally, but likely caused by #13263 / #13289.
```
--- FAIL: TestGetDiffPreview (0.20s)
--- FAIL: TestGetDiffPreview/empty_branch,_same_results (0.02s)
diff_test.go:132:
Error Trace: /workspace/forgejo/forgejo/services/repository/files/diff_test.go:132
Error: Received unexpected error:
unable to run diff-index pipeline in temporary repo: exec: canceling Cmd: invalid argument
Test: TestGetDiffPreview/empty_branch,_same_results
FAIL
```
pidfd_open is [documented](https://www.man7.org/linux/man-pages/man2/pidfd_open.2.html) to return the EINVAL (invalid argument) error if the pid is not valid, which could happen if the process terminates itself before Forgejo sends `SIGTERM`. This PR adds a check for that error and stops `Cancel`, with a minor refactor to avoid code duplication.
### Tests for Go changes
- I added test coverage for Go changes...
- [x] in their respective `*_test.go` for unit tests.
- [x] 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.
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13302
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13289#13263 unintentionally prevents Forgejo from building on non-Linux UNIX OSes. This PR adds a second graceful shutdown implementation which uses repeated signalling of the subprocess to detect if it is still running. This additional implementation is used for non-Linux UNIX platforms. It is also a fallback when Linux returns `ENOSYS` which should be the case in Linux kernels older than 5.3.
## Testing
- Validated build fails on a FreeBSD VM w/ head `forgejo`
- `FreeBSD freebsd-test 15.1-RELEASE FreeBSD 15.1-RELEASE releng/15.1-n283562-96841ea08dcf GENERIC amd64`
- Validate build succeeds and process tests pass on FreeBSD VM w/ this branch:
```
mfenniak@freebsd-test:~/Dev/forgejo $ GO_TEST_PACKAGES=forgejo.org/modules/process make test-backend
Running go test with -tags 'sqlite sqlite_unlock_notify'...
ok forgejo.org/modules/process 5.690s
```
- Note: other some other test modules do not pass like `forgejo.org/modules/log`, but this seems consistent with `forgejo` head. As we don't have a baseline for expectations here, this isn't an issue that can be addressed in this PR.
- Fallback for pre-Linux 5.3 has been tested on a Debian Buster install, kernel 4.19.0, and confirmed `forgejo.org/modules/process` test success, and test failure without the fallback path.
### 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.
- Adjustment to unreleased bugfix.
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13292
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13263
By default, when you create an `exec.CommandContext` in Go, when the context is cancelled or timed out, Go will SIGKILL the subprocess which does not allow the subprocess to perform any cleanup work. As some git operations in Forgejo will be linked to the current HTTP context, and the current HTTP context will be closed when the HTTP client disconnects, this can theoretically cause SIGKILLs when locks are being held.
This PR contains two fixes:
- Change the behaviour of all subprocesses that Forgejo creates to use a graceful termination, SIGTERM'ing the process, and then SIGKILL'ing it after a timeout.
- Fixes a bug where previously all subprocesses are setup in a process group (by 1d04e8641d), but they were never *killed* in a process group, preventing child processes from receiving these termination signals. Both SIGTERM and SIGKILL are sent to the process group.
Introduces new configuration setting which controls subcommand graceful timeouts. If a subprocess does not shutdown from SIGTERM within `[server].SUBPROCESS_TERMINATE_GRACE` (default 5 seconds), then an error will be logged, and the process will be SIGKILL'd. The error is:
```
2026/07/01 08:31:20 modules/log/init.go:38:func2() [E] Subprocess pid=3790963 (/nix/store/c0277k5giric1mn9dklllavbzvxl6hzb-git-2.53.0/bin/git blame --porcelain 8ee7e7c32b -- options/locale/locale_en-US.ini) failed to terminate from SIGTERM after grace period 0s, and was sent SIGKILL. The subprocess termination may leave incomplete state on the server, such as lock files. `[server].SUBPROCESS_TERMINATE_GRACE` can be changed to give subprocesses more time to cleanly exit.
```
(This was produced on a live server by setting SUBPROCESS_TERMINATE_GRACE to zero seconds, loading a page that does a `git blame`, and hitting escape in the browser quickly.)
### 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.
- Will create a documentation PR with new config settings.
- [ ] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [x] 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.
- [ ] 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.
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13274
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13262
A runner can either belong to an owner (user, organization) or a repository, not both. While `CreateRunner()` enforces that, `UpdateRunner()` does not, which leads to bugs. With this change, `UpdateRunner()` rejects runners that have _both_ fields set to prevent unexpected ownership changes.
Resolves https://codeberg.org/forgejo/forgejo/issues/12106 and makes https://codeberg.org/forgejo/forgejo/pulls/12117 obsolete.
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
(can be removed for JavaScript changes)
- I added test coverage for Go changes...
- [x] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [x] `make pr-go` before pushing
### Tests for JavaScript changes
(can be removed for Go changes)
- I added test coverage for JavaScript changes...
- [ ] in `web_src/js/*.test.js` if it can be unit tested.
- [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).
### 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
- [x] 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.
- [ ] 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.
*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*
The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.
Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13266
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/12963
When downloading archives forgejo uses git cat-file to resolve a ref name (e.g. branch or tag) to a commit id. There were a check if the input already was a valid full commit id, sadly without a check if the commit exists. Therefore, I removed this "early return".
I noticed this while trying to download archives from codeberg and the there was just a timeout after 10min. Upon trying this on a self-hosted instance I saw the error in the logs, which led me to his fix.
Co-authored-by: Marcel <me@m4rc3l.de>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13242
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/12976Fixesforgejo/forgejo#12951
The changes are made to the following template files:
- on the repo star/unstar button
- on the repo watch/unwatch button
- on the issue notification button
Tests have been added to ensure that the URL of the page doesn't change when the user clicks those buttons.
Co-authored-by: birdgoose <admin@shithub.cc>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13229
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13215
Followup of the refactor adding setter/getter to APIContext https://codeberg.org/forgejo/forgejo/pulls/13143
- Only has an impact on the getters used by `routers/api/v1/permissions`
- When GetX and X both exist, remove GetX
- When GetX exists and X does not, rename GetX to X
- GetOrg is an exception as it would conflict with Org and is renamed Organization. This is due to APIContext having its own Org structure that includes an Organization.
Co-authored-by: limiting-factor <limiting-factor@posteo.com>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13217
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13143
The details of the refactor are in [a separate branch](https://codeberg.org/limiting-factor/forgejo/commits/branch/wip-api-context-accessors-save) which details how it was done. This pull request contains a squash of those commits rebased on top of the current Forgejo branch, which involved conflict resolution.
Given the magnitude of the change, there is a high chance that a large number of future pull requests merged in Forgejo will create conflicts. They will be dealt with in the same way as chore: refactor orgAssignment to two separate middlewares (#13155).
## Review
The only risk I can think of is a getter/setter implementation being incorrectly implemented. This is probably where a detailed review is most needed, in the `services/context/api.go` file.
To not increase the size of the pull request, the `GetX()` accessors were preserved although they are redundant with `X()`. They will be removed in a followup pull request.
## Backporting strategy.
Cherry-picking the commit to v15 has a significant amount of conflicts in 8 files out of 111.
```
unmerged routers/api/v1/activitypub/repository.go
unmerged routers/api/v1/admin/user.go
unmerged routers/api/v1/api.go
unmerged routers/api/v1/org/team.go
unmerged routers/api/v1/repo/action.go
unmerged routers/api/v1/repo/pull.go
unmerged routers/api/v1/user/follower.go
unmerged services/context/api.go
```
Instead of resolving them, I think it is easier and safer to repeat the [refactor steps](https://codeberg.org/limiting-factor/forgejo/commits/branch/wip-api-context-accessors-save) in v15.
I also think it is worth the effort since it will avoid trivial backport conflicts on over 100 files.
## Context
This is a followup pull request discussed originally at https://codeberg.org/forgejo/forgejo/pulls/12512
Co-authored-by: limiting-factor <limiting-factor@posteo.com>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13213
Reviewed-by: limiting-factor <limiting-factor@noreply.codeberg.org>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13204#12768 added a test `TestCreateTaskForRunnerNoJobUpdated` which has been failing on MariaDB 11.8 in integration-testing. As a fix here, the error occurring (which xorm [recognizes as an `ErrDeadlock`](9eb644730c/dialects/mysql.go (L961-L972))) is treated the same as having zero records updated, and the test case then passes in MariaDB 11.8.
There is no real-world report of "server error when multiple runners fetch same job on MariaDB 11.8+", but it is the expected failure that would occur in production, rarely, that this test failure highlights.
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- **Already present and failing**
- I ran...
- [x] `make pr-go` before pushing
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13207
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/13184
Similar to #13129, fixing a few places where `[migrations].ALLOWED_DOMAINS`, `...BLOCKED_DOMAINS`, and `...ALLOW_LOCALNETWORKS` are not implemented in external migrations, creating SSRF risks.
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### 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
- [x] 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.
- [ ] 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.
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13196
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
This PR fixes a regression noticed by @0ko caused by changes from PR !12491
The switch to using logical css properties had a regression as the `text-align: start`wasn't propagating to the th element like `text-align: left` was.
This PR attempts to fix this issue by switching to a grid
Co-authored-by: 0ko <0ko@noreply.codeberg.org>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13171
Reviewed-by: 0ko <0ko@noreply.codeberg.org>
System administrators can determine which hosts can be accessed by Forgejo for git mirroring via the config settings `[migrations].ALLOWED_DOMAINS`, `...BLOCKED_DOMAINS`, and `....ALLOW_LOCALNETWORKS`. However, there were edge cases where these settings were ineffective:
- Config entries were only checked when mirrors were initially configured, and not later when a mirror pull/push occurred. DNS changes, or server configuration changes, could cause a previously safe URL to no longer be safe. These settings are now re-checked on every mirror pull/push.
- When accessing a git repository over http, git would follow HTTP redirects, allowing a violation of the `[migrations]` config entries. The git option `http.followRedirects=false` is now set to prevent git from following these redirects.
- When performing Git LFS synchronization, it was assumed that the git URL passing migration URL checks was sufficient protection. Specifically for pull mirrors which allow the usage of "Advanced settings" -> "LFS endpoint" to configure a separate LFS endpoint, these settings were never enforced, and for all mirrors they would have been subject to not being rechecked if the DNS had changed since configured. LFS mirroring now uses an HTTP client that enforces the `[migrations]` config settings.
**Breaking:** If a repository mirror that depends on HTTP redirects will no longer work.
For example, if you've mirrored a repository from a Forgejo instance at `https://example.com/OWNER/REPO.git` and the repository or owner are renamed, Forgejo will provide an HTTP redirect that continues to allow git operations to work at the old URL. Due to this redirect prohibition, this mirror will no longer work.
### Tests for Go changes
- I added test coverage for Go changes...
- [x] in their respective `*_test.go` for unit tests.
- [x] 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.
- It is not currently documented that the `[migrations]` settings are used to control access to mirrors. Forgejo advises it when an error occurs, but it could be added: https://codeberg.org/forgejo/docs/pulls/2023
- [ ] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [x] 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.
- [ ] 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/13129
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
Any job with `permissions:`, and all jobs in a workflow with `permissions:`, will display a warning at the top of the Actions screen indicating that this field is not supported by Forgejo.
### Tests for Go changes
- I added test coverage for Go changes...
- [x] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
- [x] `make pr-go` before pushing
### Tests for JavaScript changes
- I added test coverage for JavaScript changes...
- [x] in `web_src/js/*.test.js` if it can be unit tested.
- [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).
### 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
- [x] 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.
- [ ] 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/13164
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
In cases where a job references `${{ needs.x.[...]}}` in a critical section relevant for dispatching a job -- `strategy.matrix`, `runs-on`, or `with` -- referencing a skipped job as `x` would cause an error that the job is missing the required output. With this change, the referencing job is now automatically skipped.
Fixes#13165.
I've performed manual testing on my dev instance and verified that the end-to-end multiarch test, identified as the problem in #13165, now successfully gets skipped as would be expected (and, not in the screenshot, that the matrix expands correctly when the `if` condition is true and real jobs are run).
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] 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.
- Fixes regression from #13030 which is unreleased.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13174
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
Closesforgejo/forgejo#13139
## Checklist
The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests for Go changes
(can be removed for JavaScript changes)
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] 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
- [x] 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.
- [ ] 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.
*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*
The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.
<!--start release-notes-assistant-->
## Release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Bug fixes
- [PR](https://codeberg.org/forgejo/forgejo/pulls/13140): <!--number 13140 --><!--line 0 --><!--description R0VUIC9yZXBvcy97b3duZXJ9L3tyZXBvfS9pc3N1ZXMvY29tbWVudHMve2lkfSBkb2VzIG5vdCBmYWlsIGlmIHRoZSBwb3N0ZXIgaXMgYSBkZWxldGVkIHVzZXI=-->GET /repos/{owner}/{repo}/issues/comments/{id} does not fail if the poster is a deleted user<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13140
Reviewed-by: Robert Wolff <mahlzahn@posteo.de>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Move from storing in the untyped Base.Data disctionary to a typed field of APIContext.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13138
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
