Security hardening: path containment, staged deploys, limits, token, OAuth #18

Open
Leon Schmidt wants to merge 1 commit from chore/security-hardening into main
Owner

A set of security and robustness fixes to the deploy pipeline and page serving, each covered by tests.

  1. Path containment. New pathsafe package: SafeLabel (single DNS-style
    label) 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.

  2. 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.

  3. 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.

  4. Token handling and log hygiene. The deploy/delete token is taken from
    an Authorization: Bearer header; a token presented as a query
    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.
    (I decided to allow both behaviours to remain non-breaking, and to give users choice).

  5. 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.

A set of security and robustness fixes to the deploy pipeline and page serving, each covered by tests. 1. Path containment. New `pathsafe` package: SafeLabel (single DNS-style label) 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. 2. 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. 3. 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. 4. Token handling and log hygiene. The deploy/delete token is taken from an Authorization: Bearer header; ~~a token presented as a query 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.~~ _(I decided to allow both behaviours to remain non-breaking, and to give users choice)_. 5. 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.
Security hardening: path containment, staged deploys, limits, token, OAuth
All checks were successful
Test / test (pull_request) Successful in 54s
c30b1a2aa5
A set of security and robustness fixes to the deploy pipeline and page
serving, each covered by tests.

1. Path containment. New `pathsafe` package: SafeLabel (single DNS-style
   label) 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.

2. 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.

3. 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.

4. Token handling and log hygiene. The deploy/delete token is taken from
   an Authorization: Bearer header; a token presented as a query
   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.

5. 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.

The Dockerfile copies the whole module so the pathsafe subpackage builds.
README documents the header-based auth and the 410 policy.
Leon Schmidt left a comment

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.

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 permissions
Author
Owner

This clarification is important! It's one of the main features of Forge Pages to be able to use a CI token here.

This 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 repo
The `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`.
Author
Owner

Delete the last sentence. I will continue to accept the token in the query.

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).
Author
Owner

"The delete endoint also requires the repo parameter and an access token"

"The delete endoint also requires the `repo` parameter 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).
Author
Owner

Bro...

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")
Author
Owner

There errors seem completely overengineered...

There errors seem completely overengineered...
@ -68,0 +206,4 @@
return "", fmt.Errorf("computing %s sibling path: %w", kind, err)
}
return candidate, nil
}
Author
Owner

Bro this whole function is absolutely unnessesairy. I'm going to factor it out.

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)
}
Author
Owner

Same here. Why the hell would you need "attempts" for this?

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 = t
Author
Owner

Need a take a look at this but it seems smart.

Need 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 MiB
Author
Owner

I 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)

I 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 pathsafe
Author
Owner

We really don't need a package for TWO (!) helper functions.

We really don't need a package for TWO (!) helper functions.
Leon Schmidt canceled time tracking 2026-07-28 21:45:36 +02:00
Leon Schmidt added spent time 2026-07-28 21:45:41 +02:00
22 minutes
All checks were successful
Test / test (pull_request) Successful in 54s
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin chore/security-hardening:chore/security-hardening
git switch chore/security-hardening
Sign in to join this conversation.
No reviewers
No milestone
No assignees
1 participant
Notifications
Total time spent: 22 minutes
Leon Schmidt
22 minutes
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
leon/Forge-Pages!18
No description provided.