fix: exclude body from GET/HEAD requests in tokenExfiltrationDefense for Firefox compatibility#136
Conversation
📝 WalkthroughWalkthroughThis change modifies request body handling in the tokenExfiltrationDefense module. It now conditionally includes the request body only for non-GET/HEAD HTTP methods, preventing invalid body transmission with GET/HEAD requests while maintaining existing behavior for other methods. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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 |
|
Hello @fab-uleuh, Thank you very much for the PR and bringing this to my attention. @coderabbitai I can't reproduce the error that is describing here. GET requests works in my test. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: Short answer Details and pointers
If you want, tell me the exact code that triggers the error and I’ll show the minimal fix. 🌐 Web query: 💡 Result: Short answer Why it happens
How to fix
References
🌐 Web query: 💡 Result: Short answer: Firefox is enforcing the Fetch/Request rule that GET and HEAD must not have a body. If your init object includes body: undefined (or any body) for a GET/HEAD request Firefox will show "body: undefined" and throw the TypeError — remove the body field (or set method to POST) to fix it. [1][2] Why this happens
Fixes (choose one)
Minimal example: References 🏁 Script executed: Length of output: 4005 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 25572 @garronej You're absolutely right about your suspicion! Firefox is doing something equivalent to The Key DistinctionThe critical difference is:
According to the web search results, the Fetch spec and Firefox strictly enforce that GET/HEAD requests must not have a body property at all. Even though Evidence Supporting the Fix
let request_cloneForReplay = (() => {
const method = request.method.toUpperCase();
if (method !== "GET" && method !== "HEAD") {
return undefined;
}
try {
return request.clone();
} catch {
return undefined;
}
})();
...(method !== "GET" && method !== "HEAD" && body !== undefined ? { body } : {})When This MattersThis issue surfaces when:
The fix aligns with Firefox's strict RFC 7231 compliance and matches the existing patterns in your codebase. |
|
@coderabbitai ok I guess then. But this does not explain why I can't reproduce the issue. I've only tested with GET request though, not with HEAD... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 35479 🏁 Script executed: Length of output: 10562 🏁 Script executed: Length of output: 1188 @garronej I think I found why you can't reproduce it! You likely don't have Looking at the codebase, all your examples have it disabled:
Why this mattersThe The actual bug flow (when enabled)When // User code:
fetch('https://example.com')
// Inside the patched fetch:
let body: BodyInit | undefined; // starts as undefined
handle_body: {
from_init: {
if (!init) { break from_init; } // ✓ breaks (no init)
}
if (request.body === null) {
body = undefined; // ✓ explicitly set to undefined
break handle_body;
}
}
// Old code constructed:
const nextInit = {
method: request.method,
headers,
body, // ← This is body: undefined - Firefox error!
...
}The difference:
To reproduceTry testing with: oidcEarlyInit({
enableTokenExfiltrationDefense: true,
resourceServersAllowedHostnames: ["your-api.example.com"]
});Then make a simple GET request on Firefox - it should fail with the TypeError that the PR addresses. |
|
@coderabbitai of course I did enable it.... And now I tested making a HEAD request and no type error.
@fab-uleuh There might be something wrong with the interceptor but at the moment I can't reproduce the bug you're describing. Could you share a bit more context so I can understand what's going on here? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@garronej You're absolutely correct, and I apologize for the confusion. Your test proves that This raises an important question: what specific scenario is causing the error for @fab-uleuh? Let me suggest some investigation paths: Could @fab-uleuh provide:
It's possible the error occurs with:
Until we can reproduce the exact error, I'm hesitant about this change since it modifies behavior that currently works in your tests. 🏁 Script executed: Length of output: 662 @garronej Wait - there's already a comment in the codebase at line 251 of This suggests the issue was already known. Let me check the context: 🏁 Script executed: Length of output: 2629 @garronej Ah! That comment is part of the PR itself - it's being added by @fab-uleuh, not existing code. This means:
This is puzzling. Your test directly contradicts the premise of the fix. Here are the possible explanations: Possible scenarios where an error COULD occur:
What to ask @fab-uleuh:Could you provide:
Decision point:Even if we can't reproduce it, the fix:
You could merge it as defensive programming, or wait for more evidence. Your call! |

Problem
When
enableTokenExfiltrationDefenseis enabled, all fetch requests fail on Firefox with:TypeError: Window.fetch: HEAD or GET Request cannot have a body.
Chrome works fine because it silently ignores the body on GET/HEAD requests, but Firefox strictly enforces the HTTP specification (RFC 7231).
Root Cause
In
patchFetchApiToSubstituteTokenPlaceholder, thenextInitobject unconditionally includes thebodyproperty, even for GET/HEAD requests where it should be omitted.Solution
Conditionally include
bodyonly for methods that support request bodies (not GET/HEAD). This follows the same pattern already used indpop.ts(line 346-348).Testing
src/core/dpop.ts)Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.