fix: trim whitespace in parseIpFromHeaders to prevent IP banlist bypass - #29857
fix: trim whitespace in parseIpFromHeaders to prevent IP banlist bypass#29857Pranjulchaurasiya wants to merge 3 commits into
Conversation
Fixes calcom#29851 parseIpFromHeaders did not trim whitespace after splitting on comma (or from array elements), so a padded x-forwarded-for header like ' 1.2.3.4 , 5.6.7.8' would fail an exact-match lookup against IP_BANLIST, allowing a banned IP to bypass the check. Added regression tests covering padded string, array, and an end-to-end isIpInBanlist test reproducing the reported bypass.
|
Welcome to Cal.diy, @Pranjulchaurasiya! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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 `@packages/lib/getIP.test.ts`:
- Around line 89-102: Update the afterEach cleanup in the “banlist bypass
regression (`#29851`)” test suite to delete process.env.IP_BANLIST when
originalBanlist is undefined; otherwise restore the saved value normally,
preventing an unset variable from becoming the string "undefined".
In `@packages/lib/getIP.ts`:
- Around line 7-8: Update the IP extraction logic in getIP so empty or sparse
array inputs do not pass undefined to trim; normalize a missing first element to
a safe string before trimming and preserve existing string-header behavior. Add
a regression test covering an empty array and asserting the safe string result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b50c994f-348a-4c3d-9dba-f33766bf60da
📒 Files selected for processing (2)
packages/lib/getIP.test.tspackages/lib/getIP.ts
- Guard against undefined rawIp when parsing an empty header array - Delete IP_BANLIST env var in test cleanup instead of assigning undefined - Add regression test for empty array input to parseIpFromHeaders
What does this PR do?
Fixes #29851
parseIpFromHeaderssplitx-forwarded-foron commas but never trimmedwhitespace, so a padded header (or padded array element) would return an
IP with leading/trailing spaces. This caused an exact-match lookup against
IP_BANLISTto fail silently, letting a banned IP bypass the block.Fix
Added
.trim()to the returned value in both the array and string branches.Tests
isIpInBanlistreproducing thereported bypass scenario
getIP.test.tspass locallyNote
I'm aware of #29852, an earlier PR from the issue reporter with a similar
fix — it was withdrawn by the author for unrelated reasons (not a rejection
of the approach), and the issue remains open, so submitting this fix here.
Note: this closes the specific padding-based bypass of the exact-match
IP_BANLISTcheck. It does not make the banlist airtight against a moregeneral case of an attacker directly controlling the
x-forwarded-forheader value — that's a separate, upstream concern outside this fix's scope.