Skip to content

Auth and logging hardening: token safeguards, query strings in logs, signed upload URLs #294

Closed

Description

@chenrui333

Follow-up to #292 and #293. Three hardening changes for authentication and logging that I have been running on a patched 9.8.0, rebased onto dev (6ae06c2). As before they are branches in my fork, each with tests, fork CI and a Codex review. They apply to the TypeScript server; as far as I can tell #288 behaves the same way today on each point.

1. Token validation and direct-download safeguards

Branch: fix/token-validation-safeguards (review copy)

Direct downloads hand the runner a presigned storage URL whose only gate is the Actions runtime token. The branch:

  • refuses to start with ENABLE_DIRECT_DOWNLOADS=true and SKIP_TOKEN_VALIDATION=true (presigned URLs would be issued on unverified claims) unless an explicit UNSAFE_… override is set;
  • requires exp (jose only checks it when present; GitHub runtime tokens always carry it);
  • adds an optional ACTIONS_TOKEN_AUDIENCE;
  • schema-checks the ac claim and treats Permission as read/write bit flags, so a write-only scope is not used for reads;
  • logs only the jose error code and claim name instead of attaching the error, which carries the decoded claims;
  • makes the presigned URL lifetime configurable with DIRECT_DOWNLOAD_URL_TTL_SECONDS (default 600, today's fixed 10 minutes), with the reader lease outliving the URL.

Tests cover expired, not-yet-valid, wrong issuer/audience, untrusted key, alg: none and missing claims on lookup, create and finalize, plus the startup guard and the URL lifetime. sqlite/filesystem 97 passed; postgres and mysql 47 passed each.

2. Keep query strings out of logs

Branch: fix/redact-query-strings-in-logs (review copy)

The request, response and error hooks and the Results Passthrough log event.path, which includes the query string. Upload URLs are used by Azure Blob-style clients that put SAS-style parameters in the query, so the query can carry credentials. The branch logs paths without their query, and the upload route logs storage failures itself, because Nitro's default handler logs unhandled errors with the full URL.

3. Signed upload URLs

Branch: feat/signed-upload-urls, stacked on 2 (review copy)

Today the upload id in signed_upload_url is the only thing the block endpoint checks. This makes the URL actually signed, like the SAS on an Azure Blob URL: CreateCacheEntry adds a random 256-bit signature to the URL's query, only its SHA-256 is stored (nullable uploads.capabilityHash, added by $9_upload_capability_hash, which skips the column if it already exists), and single-shot, block and blocklist requests must carry it (constant-time compare, 403 otherwise). Upload URLs accept only PUT; other methods get 405 instead of being forwarded to the Results service. @actions/cache, BuildKit and other Azure-style clients keep the URL's query, so they need no change. Uploads in flight during the upgrade lose their URLs and the client retries the save, which is worth a release note. sqlite/filesystem 75 passed; postgres and mysql 20 passed each.

If you would rather discuss 3 privately first, enabling private vulnerability reporting on the repo would give a channel; I'm happy to move it there.

The ADR numbers in 1 and 3 (0010, 0011) will need renumbering against #288's ADR-0010.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions