Skip to content
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

Change cloudflare issue request flow #292

Merged

Conversation

ppopth
Copy link
Member

@ppopth ppopth commented Feb 2, 2022

Since we don't have any query param in the issue request any more, we remove the code which uses the query params to identify the issue request.

Currently we have a Referer header. We also need to use that header in the issue request as well. However such change requires a change in the code structure, as follows:

Previously we used axios to send an issuance request in handleBeforeRequest, but now we will send the request in
handleBeforeSendHeaders because we need to read the Referer header first.

After reading the Referer header, we extract the __cf_chl_tk query param in the Referer url and add the __cf_chl_f_tk query param with that token in the issuance request URL.

We need a new "issueInfo" property to remember the request body we can read in handleBeforeRequest and then it will be read in handleBeforeSendHeaders so that in handleBeforeSendHeaders we will have both the body and the Referer header.

Since there is no more query param, we will use only the body param to
identify when the user want to issue tokens.
@ppopth ppopth force-pushed the change-cloudflare-issue-request-flow branch from 8dd58b9 to 4f6af15 Compare February 3, 2022 08:06
if (typeof chrome !== 'undefined') {
if (typeof browser !== 'undefined') {
return 'Firefox';
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no else after return

} else {
return 'Chrome';
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no else after return

@@ -57,10 +71,14 @@ chrome.webRequest.onBeforeRequest.addListener(handleBeforeRequest, { urls: ['<al
'blocking',
]);

const extraInfos = ['requestHeaders', 'blocking'];
if (getBrowser() === 'Chrome') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can have a const dicvtionary with browser
something like

const BROWSERS = {
  CHROME: 'Chrome',
  FIREFOX: 'Firefox',
  EDGE: 'Edge',
} as const;
type BROWSERS = typeof BROWSERS[keyof typeof BROWSERS];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to enum instead then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum is not well built in typescript, and can rapidly makes code hard to read. Using a const map works better.

@ppopth ppopth force-pushed the change-cloudflare-issue-request-flow branch from 33f8db0 to ca5c2aa Compare February 3, 2022 12:36
@ppopth ppopth requested a review from thibmeu February 3, 2022 13:00
Chrome needs the extraHeaders flag in extraInfos to read the Referer
header.
Previously we used axios to send an issuance request in
handleBeforeRequest, but now we will send the request in
handleBeforeSendHeaders because we need to read the Referer header
first.

After reading the Referer header, we extract the __cf_chl_tk query param
in the Referer url and add the __cf_chl_f_tk query param with that token
in the issuance request URL.

We need a new "issueInfo" property to remember the request body we can
read in handleBeforeRequest and then it will be read in
handleBeforeSendHeaders so that in handleBeforeSendHeaders we will have
both the body and the Referer header.
@ppopth ppopth force-pushed the change-cloudflare-issue-request-flow branch from ca5c2aa to ac60db3 Compare February 3, 2022 14:16
expect(newIssueInfo).toBeNull();

expect(issue.mock.calls.length).toBe(1);
expect(issue).toHaveBeenCalledWith('https://captcha.website/?__cf_chl_f_tk=token', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check that this request's Referer header will either not be present or contain the URL without the parameter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what request do you mean? issue is a method that will send a request using axios which cannot have a Referer header at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant for the challenge solve request, and the Referer not being present is perfect. Thanks!

@RealDolos
Copy link

fwiw, with this PR things seem to work in Firefox Nightly. Thanks

@mendelsphotography
Copy link

Does this fix the issue #291 or no? Just wondering since just happened to me the other day and I tught it was me since I reinstalled chrome.
Because the lastest release is from a long time ago so does that mean this need to be compiled on my PC now?

@ppopth ppopth merged commit 013485d into privacypass:master Feb 4, 2022
@ppopth ppopth deleted the change-cloudflare-issue-request-flow branch February 4, 2022 06:06
@warren-bank
Copy link

fwiw, you might want to compare the differences in implementation between this PR and PR #283 in the 2x commits made on 01/29/2022. The code here uses some very loose browser detection, whereas the code in the other PR uses feature detection.. which will support older versions of all supported browsers. For example, the code here will break in versions of Chrome < 72.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants