-
Notifications
You must be signed in to change notification settings - Fork 18
fix: exclude body from GET/HEAD requests in tokenExfiltrationDefense for Firefox compatibility #136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.