Skip to content

fix: enforce GitHub auth consistently across locations (PTFE-3620) - #227

Merged
matthiasL-scality merged 1 commit into
mainfrom
PTFE-3620/harden-auth
Oct 20, 2026
Merged

matthiasL-scality merged 1 commit into
mainfrom
PTFE-3620/harden-auth

Conversation

@matthiasL-scality

Copy link
Copy Markdown
Contributor

Summary

Hardens how GitHub authentication and upload restrictions apply across nginx locations. Tracked in PTFE-3620; details are in the ticket.

  • Lua handlers that end the request themselves (presign/redirect, latest / last_success / last_failure) now run in the content phase, after github_access.lua.
  • Upload-restricted paths are matched on the normalized URI.
  • The /github/scality/<repo>/ alias is limited to GET/HEAD reads under /download/.

Review focus

  1. conf/nginx.conf.template: the rewrite_by_lua_file → content_by_lua_file switch for the six affected locations, and the reworked /github/scality/ alias.
  2. lua/github_access.lua: restriction_check now uses ngx.var.uri.

Testing

  • New tests/end2end/test_auth_bypass.py.
  • The full end-to-end suite passes locally (115 tests), including the existing presign, redirect, find_build and alias tests.

@matthiasL-scality
matthiasL-scality requested a review from a team as a code owner October 20, 2026 07:56
Comment thread conf/nginx.conf.template
@claude

claude Bot commented Oct 20, 2026

Copy link
Copy Markdown
  • conf/nginx.conf.template:274: the GET/HEAD if regex ^(GET|HEAD)$ overwrites $1, so $github_repo becomes GET and Script-Name becomes /github/scality/GET/. That breaks build-prefix filtering for the alias (empty root listing).
    - Use ^(?:GET|HEAD)$, or move set $github_repo $1; above the if.
    - Add a test that checks the alias passes the right repo in Script-Name, e.g. a root listing through /github/scality/<repo>/builds/.

    Review by Claude Code

- Run presign/redirect and find_build handlers in the content phase so
  github_access.lua (access phase) runs before they end the request.
- Match upload-restricted paths on the decoded URI instead of the raw
  request_uri.
- Keep the /github/scality/<repo>/ alias confined to GET/HEAD reads
  under /download/: let nginx re-escape the rewritten URI, capture the
  repo name before the method check, and restrict it to repo name
  characters since it is copied into the Script-Name header.
- Add end-to-end regression tests.
@claude

claude Bot commented Oct 20, 2026

Copy link
Copy Markdown

LGTM

Review by Claude Code

@matthiasL-scality
matthiasL-scality merged commit df3a36b into main Oct 20, 2026
5 checks passed
@matthiasL-scality
matthiasL-scality deleted the PTFE-3620/harden-auth branch October 20, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant