Security hardening: path containment, staged deploys, limits, token, OAuth #18
No reviewers
Labels
No labels
bug
ci
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No assignees
1 participant
Notifications
Total time spent: 22 minutes
Due date
Leon Schmidt
22 minutes
No due date set.
Dependencies
No dependencies set
Reference
leon/Forge-Pages!18
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/security-hardening"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A set of security and robustness fixes to the deploy pipeline and page serving, each covered by tests.
Path containment. New
pathsafepackage: SafeLabel (single DNS-stylelabel) and SecureJoin (join + filepath.Rel containment; rejects
absolute, empty, separator-bearing, and "."/".." components). owner,
repo, and additional_base_path are validated and every constructed
storage path and tar-entry path is contained beneath serve_path, so a
crafted additional_base_path (e.g. ../../other/repo/ROOT) or a tar
member with ../ can no longer reach or delete another repo's pages or
the serve tree. Non-regular tar entries (symlinks/devices) stay
discarded, closing the symlink-pivot variant.
Staged, serialized deployments. Uploads extract into a hidden staging
sibling and are swapped into the live target by atomic rename only on
success, so a malformed or oversized upload never destroys the last
good deployment (previously the live directory was RemoveAll'd before
extraction). Deploy/delete for a given target are serialized by a
refcounted keyed mutex (entries freed at refcount 0). Stale work
siblings are swept only after a successful promotion and use a
dot-prefixed reserved namespace, so a user-chosen additional_base_path
cannot collide with or delete cleanup state or a recovery copy.
Cumulative extraction limit. A new max_total_extracted_bytes config
(default 256 MiB) bounds total uncompressed output per deployment,
enforced via a capped copy so no more than the ceiling reaches disk;
complements the existing per-file and file-count limits. Tar-limit
errors are typed sentinels matched with errors.Is.
Token handling and log hygiene. The deploy/delete token is taken from
an Authorization: Bearer header;
a token presented as a query(I decided to allow both behaviours to remain non-breaking, and to give users choice).parameter is rejected with 410 (even alongside a valid header), since
URLs leak into server, proxy, and CI logs. Request-URL logging redacts
access_token/token values and fails closed (whole-query redaction) on
any query it cannot parse.
Proxy-aware OAuth return URLs. External URLs are built from the
configured pages_url scheme/host instead of r.TLS (which is nil behind
a TLS-terminating reverse proxy, yielding http:// return URLs). The
request Host is validated against the base domain or a single-label
owner subdomain and the redirect host is reconstructed from validated
components, preventing Host-header redirect injection.
Contributed by Michal Bielicki, but looks AI-generated. I will review the changes thoroughly.
Ok there is not a single chance I am going to merge this AI-generated and overengineered code into main.
I like the feature proposals though, so I'm going to cherry pick SOME of it, once I find the time for it.
@ -22,3 +21,2 @@The Forge Pages Server is a Go application that provides a `POST /deploy` endpoint. Pages must be TAR'ed and GZIP'ed and posted to this endpoint, authenticated with an `Authorization: Bearer <token>` header, together with the following query parameters:- `repo`: Repository slug. Used to construct the URL where the page will be deployed to. **To prevent confusion, the repo name is always lowercased!**- `access_token`: The workflow token (e.g. `${{ forgejo.token }}`) to verify permissions to deploy a page to the target specified by `repo`.- This can also be a PAT, as long as it has the appropriate permissionsThis clarification is important! It's one of the main features of Forge Pages to be able to use a CI token here.
@ -26,3 +24,4 @@- Alternativly, you can add an empty file called `.protect` to the root of the page to enable protection- `additional_base_path`: Can be used to set an additional base path suffix to allow for multiple deployments per repoThe `Authorization: Bearer <token>` header carries the workflow token (e.g. `${{ forgejo.token }}`) used to verify permissions to deploy a page to the target specified by `repo`. This can also be a PAT, as long as it has the appropriate permissions. **The token is never accepted as a query parameter** (e.g. `?access_token=...`) — URLs end up in server logs, reverse-proxy access logs, and browser/CI history, so a request presenting the token only in the query string is rejected with `410 Gone`.Delete the last sentence. I will continue to accept the token in the query.
@ -29,3 +29,3 @@When visiting a protected page, you will get redirected to the configured OAuth2 provider, where you must log in. If you have the correct permission, you will get redirected to the page.Deployments can be deleted using the `DELETE /deploy` endpoint or by uploading an empty page to the same location. The delete endpoint also requires the `repo` and `access_token` parameters (and `additional_base_path` if set during deployment).Deployments can be deleted using the `DELETE /deploy` endpoint or by uploading an empty page to the same location. The delete endpoint also requires the `repo` query parameter and the `Authorization: Bearer <token>` header (and `additional_base_path` if set during deployment)."The delete endoint also requires the
repoparameter and an access token"@ -18,0 +22,4 @@// falls back to defaultMaxTotalExtractedBytes (256 MiB) -- see// maxTotalExtractedBytesLimit in deploy_ops.go, which applies that// fallback directly so the guard stays active even for a Config built// without calling setDefaults (as several tests do).Bro...
@ -17,0 +41,4 @@// more than maxNumOfTarEntries entries, guarding against archives that try// to exhaust resources via sheer file count rather than any single file's// size.var ErrTooManyEntries = errors.New("tar archive has too many entries")There errors seem completely overengineered...
@ -68,0 +206,4 @@return "", fmt.Errorf("computing %s sibling path: %w", kind, err)}return candidate, nil}Bro this whole function is absolutely unnessesairy. I'm going to factor it out.
@ -68,0 +225,4 @@}}return "", fmt.Errorf("could not allocate unique %s sibling path for %s after %d attempts", kind, target, maxAttempts)}Same here. Why the hell would you need "attempts" for this?
@ -111,0 +441,4 @@log.Printf("Possible path traversal detected while unpacking tar.gz entry %s, stopping deployment: %s", header.Name, err)return fmt.Errorf("%w: tar entry %s: %s", ErrPathTraversal, header.Name, err)}target = tNeed a take a look at this but it seems smart.
@ -27,0 +32,4 @@// per-file (maxTarEntrySizeBytes) and per-archive file-count// (maxNumOfTarEntries) limits -- those alone still allow up to// 2500 * 5 MiB (~12 GiB) to be written per request.defaultMaxTotalExtractedBytes = 268435456 // 256 MiBI don't this it is required to have both size limits, especially since the setting of them is inconsistent (one has a default and is configurable, while the other is not)
@ -0,0 +2,4 @@// user-supplied path components (repo/owner labels, tar entry segments)// before they are used to build filesystem paths, preventing directory// traversal and cross-tenant path collisions.package pathsafeWe really don't need a package for TWO (!) helper functions.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.