🐛 bug: prevent session fixation by preserving successful extractor in chains - #4469
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesExtractor tracking through getSessionID → Session
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.2)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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/session/session.go`:
- Around line 380-399: The extractor selection logic is still falling through
from a read-only winner into writable extractors, which lets `setSession`
persist the same session ID into cookies/headers. Update the extractor
resolution in this block so that once `s.extractor` has won with a non-writable
source (for example query/form/param), you do not continue to
`s.config.Extractor` or its `Chain`; only return cookie/header extractors when
the winning extractor itself is cookie/header, and otherwise stop with no
writable fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 62773ff8-151b-41b4-8fb6-2c9407588a7e
📒 Files selected for processing (2)
middleware/session/session.gomiddleware/session/store.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/session/session.go`:
- Line 64: The Session field rename was only partially applied: update all
remaining references in Session methods to use s.fresh instead of s.isFresh. Fix
the assignments/checks in the Session-related code paths that currently
reference the old name (including the places in the Session type’s methods) so
they match the renamed struct field and the existing sess.fresh usage in
store.go.
In `@middleware/session/store.go`:
- Line 318: The session store code still references the removed `sess.isFresh`
field, causing a compile error. Update the `store.go` logic to use the renamed
`sess.fresh` field wherever this freshness flag is reset, matching the existing
assignment in the session handling code so the store and session structs stay
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 883b943b-ba59-4626-92ac-6ddd6b07d849
📒 Files selected for processing (2)
middleware/session/session.gomiddleware/session/store.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/session/session.go`:
- Line 64: The Session field rename was only partially applied: update all
remaining references in Session methods to use s.fresh instead of s.isFresh. Fix
the assignments/checks in the Session-related code paths that currently
reference the old name (including the places in the Session type’s methods) so
they match the renamed struct field and the existing sess.fresh usage in
store.go.
In `@middleware/session/store.go`:
- Line 318: The session store code still references the removed `sess.isFresh`
field, causing a compile error. Update the `store.go` logic to use the renamed
`sess.fresh` field wherever this freshness flag is reset, matching the existing
assignment in the session handling code so the store and session structs stay
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 883b943b-ba59-4626-92ac-6ddd6b07d849
📒 Files selected for processing (2)
middleware/session/session.gomiddleware/session/store.go
🛑 Comments failed to post (2)
middleware/session/session.go (1)
64-64: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Build break:
s.isFreshno longer exists — rename tos.fresh.The struct field was renamed from
isFreshtofresh(andstore.goLine 165 already assignssess.fresh), but these references were not updated. The compiler reportss.isFresh undefined (type *Session has no field or method isFresh)at Lines 64, 117, and 326, failing lint, govulncheck, and all unit jobs.🐛 Proposed fix
@@ acquireSession (Line 64) - s.isFresh = true + s.fresh = true@@ Fresh (Line 117) - return s.isFresh + return s.fresh@@ refresh (Line 326) - s.isFresh = true + s.fresh = trueAlso applies to: 117-117, 326-326
🧰 Tools
🪛 GitHub Actions: Linter / 0_lint _ lint.txt
[error] 64-64: golangci-lint: undefined method/field 'isFresh' (s.isFresh undefined). Error: (type *Session has no field or method isFresh).
🪛 GitHub Actions: Linter / lint _ lint
[error] 64-64: golangci-lint reported compile error: s.isFresh undefined (type *Session has no field or method isFresh)
🪛 GitHub Actions: PR `#4469` / 0_Analyze (go).txt
[warning] 64-64: Type extraction warning while processing package
github.com/gofiber/fiber/v3/middleware/session: /home/runner/work/fiber/fiber/middleware/session/session.go:64:4: s.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Actions: PR `#4469` / Analyze (go)
[error] 64-64: Type extraction failed while processing package
github.com/gofiber/fiber/v3/middleware/session: /home/runner/work/fiber/fiber/middleware/session/session.go:64:4: s.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Actions: Run govulncheck / 0_govulncheck-check.txt
[error] 64-64: govulncheck failed due to Go compilation issue: s.isFresh undefined (type *Session has no field or method isFresh)
🪛 GitHub Actions: Run govulncheck / govulncheck-check
[error] 64-64: govulncheck failed due to package loading error: /home/runner/work/fiber/fiber/middleware/session/session.go:64:4: s.isFresh undefined (type *Session has no field or method isFresh)
🪛 GitHub Check: govulncheck-check
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: lint / lint
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: unit (1.25.x, macos-latest)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: unit (1.25.x, windows-latest)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: unit (1.26.x, macos-latest)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)
[failure] 64-64:
s.isFresh undefined (type *Session has no field or method isFresh)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/session/session.go` at line 64, The Session field rename was only partially applied: update all remaining references in Session methods to use s.fresh instead of s.isFresh. Fix the assignments/checks in the Session-related code paths that currently reference the old name (including the places in the Session type’s methods) so they match the renamed struct field and the existing sess.fresh usage in store.go.Source: Linters/SAST tools
middleware/session/store.go (1)
318-318: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Build break:
sess.isFreshno longer exists — rename tosess.fresh.Same root cause as the
session.gorename: the field is nowfresh(assigned assess.freshat Line 165), so this reference fails to compile (sess.isFresh undefined).🐛 Proposed fix
- sess.isFresh = false + sess.fresh = false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.sess.fresh = false🧰 Tools
🪛 GitHub Check: govulncheck-check
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: lint / lint
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh) (typecheck)🪛 GitHub Check: unit (1.25.x, macos-latest)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: unit (1.25.x, windows-latest)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)🪛 GitHub Check: unit (1.26.x, macos-latest)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)
[failure] 318-318:
sess.isFresh undefined (type *Session has no field or method isFresh)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/session/store.go` at line 318, The session store code still references the removed `sess.isFresh` field, causing a compile error. Update the `store.go` logic to use the renamed `sess.fresh` field wherever this freshness flag is reset, matching the existing assignment in the session handling code so the store and session structs stay consistent.Source: Linters/SAST tools
The merge of main introduced the isFresh -> fresh struct field rename, but the session-extractor changes still referenced the old isFresh name, leaving the package uncompilable. Update the remaining references in session.go and store.go to use the new fresh field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSaJqSGCPXEHywa3bhWJnT
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4469 +/- ##
==========================================
+ Coverage 92.89% 92.92% +0.02%
==========================================
Files 138 138
Lines 13486 13509 +23
==========================================
+ Hits 12528 12553 +25
+ Misses 591 590 -1
+ Partials 367 366 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Address the review feedback on the session-fixation fix: - getExtractorInfo no longer falls through to writable cookie/header sinks when the winning extractor was a read-only source (query/form/param) for an existing session, which closed the session-fixation path the PR targets. Freshly generated sessions still persist to the configured sinks. - Use the comma-ok form when reading the selected extractor from context locals so it satisfies the repo's strict errcheck config (check-type-assertions + check-blank), fixing the lint workflow failure. - Add tests covering extractor preservation, the no-promotion path, fresh fallthrough, and getExtractorInfo branches (package coverage 92.8% -> 96.4%). - Document the errcheck type-assertion/blank pitfalls in AGENTS.md so future changes run `make lint` and avoid the recurring failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSaJqSGCPXEHywa3bhWJnT
|
Pending @sixcolors approval |
PR #4469 renamed the unexported boolean field Session.isFresh (and the local isFresh in Store.getSession) to fresh, dropping the predicate prefix that signals the value is a boolean. Restore isFresh everywhere it was shortened; the public Fresh() method is unchanged, so there is no API change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
Motivation
Description
sessionExtractorContextKey) and capturing it during extraction inStore.getSessionID. (modifiedmiddleware/session/store.go)Session(sess.extractor) when a session is created so later save/delete operations can make write-back decisions based on the actual source. (modifiedmiddleware/session/store.go,middleware/session/session.go)Session.getExtractorInfo()to prefer the stored successful extractor when it is a cookie/header sink and otherwise fall back to the configured cookie/header extractors for fresh sessions. (modifiedmiddleware/session/session.go)Session.Release()to avoid cross-request contamination from the session pool. (modifiedmiddleware/session/session.go)Testing
go test ./middleware/session -run 'Test_Session_Save|Test_Session_ChainedExtractors|Test_Store_getSessionID'which passed for the modified behavior (example output:ok github.com/gofiber/fiber/v3/middleware/session 5.129s).go test ./middleware/session -run Test_Store_getSessionID(passed).make generatesucceeded,make modernizesucceeded, andmake formatsucceeded, whilemake auditfailed due to environment-reported standard-library vuln findings fromgovulncheck(pre-existing toolchain/stdlib issues unrelated to this patch) andmake betteralignreported a non-functional struct-alignment suggestion; these failures are environmental/pre-existing and did not affect the session-targeted tests.make test(full test suite); the session-targeted tests relevant to this fix passed, though unrelated integration tests in the full shuffled/race run surfaced unrelated failures in this environment.Codex Task