-
Notifications
You must be signed in to change notification settings - Fork 963
Remove referrerPolicy
from Cloudflare Worker fetch
requests
#8393
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
Changes from 11 commits
3407e2b
cc8114c
936f3f2
a0c9570
4b4a66f
d2b604f
c63a5c7
357fd9e
c2a5286
48c3940
73b6eca
9d220d6
1629b93
b1f3860
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@firebase/auth': patch | ||
'@firebase/util': minor | ||
--- | ||
|
||
Suppress the use of the `fetch` parameter `referrerPolicy` within Auth for `fetch` requests originating from Cloudflare Workers. Clouldflare Worker environments do not support this parameter and throw when it's used. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import { useFakeTimers } from 'sinon'; | |
import sinonChai from 'sinon-chai'; | ||
|
||
import { FirebaseError, getUA } from '@firebase/util'; | ||
import * as utils from '@firebase/util'; | ||
|
||
import { mockEndpoint } from '../../test/helpers/api/helper'; | ||
import { testAuth, TestAuth } from '../../test/helpers/mock_auth'; | ||
|
@@ -308,6 +309,66 @@ describe('api/_performApiRequest', () => { | |
}); | ||
}); | ||
|
||
context('referer policy exists on fetch request', () => { | ||
afterEach(mockFetch.tearDown); | ||
|
||
it('should have referrerPolicy set', async () => { | ||
/* eslint-disable no-var */ | ||
var referrerPolicySet: boolean = false; | ||
/* eslint-enable no-var */ | ||
mockFetch.setUpWithOverride( | ||
(input: RequestInfo | URL, request?: RequestInit) => { | ||
if (request !== undefined && request.referrerPolicy !== undefined) { | ||
referrerPolicySet = true; | ||
} | ||
return new Promise<never>((_, reject) => | ||
reject(new Error('network error')) | ||
); | ||
} | ||
); | ||
const promise = _performApiRequest( | ||
auth, | ||
HttpMethod.POST, | ||
Endpoint.SIGN_UP, | ||
request | ||
); | ||
await expect(promise).to.be.rejectedWith( | ||
FirebaseError, | ||
'auth/network-request-failed' | ||
); | ||
expect(referrerPolicySet).to.be.true; | ||
}); | ||
|
||
it('should not have referrerPolicy set on Cloudflare workers', async () => { | ||
sinon.stub(utils, 'isCloudflareWorker').returns(true); | ||
/* eslint-disable no-var */ | ||
var referrerPolicySet: boolean = false; | ||
/* eslint-enable no-var */ | ||
mockFetch.setUpWithOverride( | ||
(input: RequestInfo | URL, request?: RequestInit) => { | ||
if (request !== undefined && request.referrerPolicy !== undefined) { | ||
referrerPolicySet = true; | ||
} | ||
return new Promise<never>((_, reject) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. optional: possible to resolve the promise instead of reject? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also curious why these tests have a rejected promise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! Updated tests so that these mocks resolve. |
||
reject(new Error('network error')) | ||
); | ||
} | ||
); | ||
const promise = _performApiRequest( | ||
auth, | ||
HttpMethod.POST, | ||
Endpoint.SIGN_UP, | ||
request | ||
); | ||
await expect(promise).to.be.rejectedWith( | ||
FirebaseError, | ||
'auth/network-request-failed' | ||
); | ||
expect(referrerPolicySet).to.be.false; | ||
sinon.restore(); | ||
}); | ||
}); | ||
|
||
context('with network issues', () => { | ||
afterEach(mockFetch.tearDown); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
import { FirebaseError, querystring } from '@firebase/util'; | ||
import { FirebaseError, isCloudflareWorker, querystring } from '@firebase/util'; | ||
|
||
import { AuthErrorCode, NamedErrorParams } from '../core/errors'; | ||
import { | ||
|
@@ -148,14 +148,23 @@ export async function _performApiRequest<T, V>( | |
headers[HttpHeader.X_FIREBASE_LOCALE] = auth.languageCode; | ||
} | ||
|
||
const fetchArgs: RequestInit = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should consider adding some unit tests on _performApiRequest since we introduced new logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
method, | ||
headers, | ||
...body | ||
}; | ||
|
||
/* Security-conscious server-side frameworks tend to have built in mitigations for referrer | ||
problems". See the Cloudflare GitHub issue #487: Error: The 'referrerPolicy' field on | ||
'RequestInitializerDict' is not implemented." | ||
https://github.com/cloudflare/next-on-pages/issues/487 */ | ||
if (!isCloudflareWorker()) { | ||
Xiaoshouzi-gh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fetchArgs.referrerPolicy = 'no-referrer'; | ||
} | ||
|
||
return FetchProvider.fetch()( | ||
_getFinalTarget(auth, auth.config.apiHost, path, query), | ||
{ | ||
method, | ||
headers, | ||
referrerPolicy: 'no-referrer', | ||
...body | ||
} | ||
fetchArgs | ||
); | ||
}); | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.