🐛 bug: guard session logger tag against released middleware

[gaby]

Motivation

• The auto-registered ${session-id} logger tag could call m.ID() after the session middleware had cleared m.Session and returned the *Middleware to the pool, which can cause a nil-pointer panic when logger runs after downstream middleware returns.
• Make a minimal change to prevent dereferencing a released session object while preserving existing behavior and redaction for live sessions.

Description

• In middleware/session/middleware.go the logger context tag was changed to safely read m.Session under m.mu.RLock() and return an empty string when the session has already been released instead of calling m.ID() directly.
• Added a regression test Test_SessionLoggerTagWithOuterLoggerDoesNotPanic in middleware/session/middleware_test.go that mounts logger outside session and asserts a request completes without panic when Format: "${session-id}" is used.
• No public API changes or behavior changes for live sessions; only guards against the stale/released-session crash.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a305da1c-bd88-4035-bff3-2e6b5af3633e

📥 Commits

Reviewing files that changed from the base of the PR and between c8e8a77 and e2bb09a.

📒 Files selected for processing (1)

• middleware/session/middleware.go

🚧 Files skipped from review as they are similar to previous changes (1)

• middleware/session/middleware.go

---

Walkthrough

Logger tag ${session-id} now reads the session ID from request locals and returns "" if missing, redacting non-empty IDs. Middleware initialize stores the session ID and middleware pointer in request locals; release clears those locals and nils the middleware pointer. A regression test verifies an outer logger preceding the session middleware does not panic or produce output.

Changes

Session logger + lifecycle

Layer / File(s)	Summary
Logger tag middleware/session/middleware.go	logger.RegisterContextTag("session-id", ...) now reads sessionIDContextKey from the request context, returns "" when absent/empty, and returns redact.Prefix(id) when present.
Init: call store helper middleware/session/middleware.go	(*Middleware).initialize(...) now calls storeMiddlewareContext(...) during setup.
Store session ID middleware/session/middleware.go	Adds storeMiddlewareContext(...) to persist session.ID() under sessionIDContextKey and the middleware pointer under middlewareContextKey in Fiber locals.
Release: clear context middleware/session/middleware.go	Adds clearMiddlewareContext(...) to clear sessionIDContextKey and middlewareContextKey from Fibers locals and overwrite request context values; releaseMiddleware calls it only when m.ctx != nil.
Regression Test middleware/session/middleware_test.go	Added Test_SessionLoggerTagWithOuterLoggerDoesNotPanic which installs an outer logger.New using ${session-id}, registers session middleware afterward, issues a GET, asserts 200 OK, and verifies the outer logger buffer is empty.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• gofiber/fiber#3016: Also modifies session middleware behavior (context keys and initialization/release patterns).

Suggested labels

SessionMW

Suggested reviewers

• sixcolors
• efectn
• ReneWerner87

Poem

🐰 I hopped through contexts, soft and spry,
I shelved the ID where request locals lie,
When logs ask gently, I whisper redacted,
On release I tidy — no pointer distracted. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies a bug fix for guarding session logger tags against released middleware, which directly matches the changeset's core purpose.
Description check	✅ Passed	The PR description clearly explains the motivation, describes the changes made in both files, and confirms no breaking API changes—fulfilling the core template requirements.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-session-logger-tag-panic-issue

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.1)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request improves the session middleware by implementing thread-safe access to the session object during log tag registration, which prevents panics when a session is nil or has been released. It also includes a new regression test to ensure stability when using the session logger tag. The review feedback recommends returning an empty string if the session ID is empty to prevent misleading redacted output and suggests adding an assertion to the regression test to verify that the log output is empty.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.19%. Comparing base (437fef9) to head (e2bb09a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4265      +/-   ##
==========================================
+ Coverage   91.17%   91.19%   +0.02%     
==========================================
  Files         129      129              
  Lines       12757    12767      +10     
==========================================
+ Hits        11631    11643      +12     
+ Misses        710      709       -1     
+ Partials      416      415       -1     

Flag	Coverage Δ
unittests	91.19% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR hardens the session middleware’s auto-registered ${session-id} logger tag to avoid dereferencing a released session/middleware instance, and adds a regression test for the logger-outside-session middleware ordering.

Changes:

• Updated ${session-id} tag extraction to read m.Session under m.mu.RLock() and return "" if the session is already released.
• Added Test_SessionLoggerTagWithOuterLoggerDoesNotPanic to cover the outer-logger ordering that previously could panic.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
middleware/session/middleware.go	Adjusts ${session-id} tag renderer to avoid calling through a released session/middleware instance.
middleware/session/middleware_test.go	Adds a regression test for logger mounted outside session middleware.

[gaby]

Addressing comments

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[gaby]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied the requested changes in e2bb09a. The session logger tag now reads an immutable request-scoped session ID instead of a pooled middleware pointer, the session/middleware context values are cleared on release so an outer logger renders an empty ${session-id}, and the regression test now creates the session handler before the logger while still mounting logger outside session.