Skip to content

fix(panelSessionGate): correct the Lua user-path match and execute the gate in tests (Refs #963) - #967

Open
Shabbir7890 wants to merge 1 commit into
MagnaCapax:mainfrom
Shabbir7890:fix/963-panel-session-gate-lua-pattern
Open

Shabbir7890 wants to merge 1 commit into
MagnaCapax:mainfrom
Shabbir7890:fix/963-panel-session-gate-lua-pattern

Conversation

@Shabbir7890

Copy link
Copy Markdown
Contributor

Refs #963.

This takes the pattern fix from #963 and adds the executing test the issue asks for.

The fix. panel_user() matched "^/user%-([a-z][a-z0-9]*)($|/)". In a Lua pattern that trailing group is the literal text $|/ (Lua has no alternation, and $ anchors only as the final character), so it matched no real path, panel_user() returned nil for every request, and the gate delegated straight to Basic auth. I replaced it with the two shapes that actually occur:

local user = path:match("^/user%-([a-z][a-z0-9]*)/") or path:match("^/user%-([a-z][a-z0-9]*)$")

The test. PanelSessionGateSourceTest only matches source strings, which is exactly how this shipped green. PanelSessionGateExecutionTest runs the real panelSessionGate.lua under a stubbed lighty global and asserts the decision it reaches:

  • the public login handler on /user-<name>/panelSessionLogin.php resolves the user and sets REMOTE_USER
  • an HTML visitor with no session gets a 302 to the login page (and the bare /user-<name> form matches too)
  • a request already carrying Basic credentials is delegated (returns nil)
  • a non-panel path never sets REMOTE_USER

I checked it is a real guard: with the old ($|/) pattern the first two of those fail; with the fix all four pass.

Running the test. It needs a Lua interpreter (any of lua5.4/lua5.3/lua5.1/lua) and skips cleanly when none is present, so it never turns a Lua-less host red. I added lua5.4 to the dev Dockerfile and a line to check-tools.sh. I couldn't push the workflow file myself (no workflow scope on my token), so the CI build job needs the same one-liner to actually run the test there rather than skip it:

      - name: Install bash tools
        run: |
          sudo apt-get update
-          sudo apt-get install -y shellcheck shfmt
+          sudo apt-get install -y shellcheck shfmt lua5.4

Validation. Built the dev image and ran the full development suite against a clean clone of the branch: 3169 tests, same 25 environmental failures as clean main (the php-xml/curl/uid-0 set from #936), no new failures. php -l clean on the changed PHP.

Scope note: this is defect 1 only. Defect 2 (the REMOTE_USER / extern-authn handoff) is written up in #963 as a lead that still needs a real-vhost check — I'd rather confirm that on an actual per-user vhost before proposing the method => "extern" change, so it can go on top of this once verified.

…e gate in tests (Refs MagnaCapax#963)

panel_user() used the PCRE idiom "($|/)" inside a Lua string.match, where it
is the literal text "$|/" and matches no real path. The gate therefore returned
nil on every request and delegated to Basic auth, so the opt-in cookie login was
inert wherever enabled. Match the two real shapes explicitly: a trailing-slash
prefix or the bare /user-<name>.

PanelSessionGateSourceTest only asserts source strings, which is how this shipped
green. Add PanelSessionGateExecutionTest, which runs the real .lua under a stubbed
lighty global and asserts the decision reached (login handler resolves the user,
html visitor without a session gets 302, a request with Basic creds delegates, a
non-panel path never sets REMOTE_USER). The dev container gets lua5.4 so the test
runs; it skips cleanly where no Lua is present.
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